Introduces DevToolsManagerDelegate.

Introduces DevToolsManagerDelegate. 

The DevToolsManagerDelegate will be used to open DevToolsWindow from ServiceWorkerInternalsUI(chrome://serviceworker-internals/). 

BUG=358657
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270047

[horo, 2014-04-25 02:56:58]

kinuko@
Could you please review?

[jam, 2014-04-25 05:02:08]

note: the whole reason we have webui implementations in content is so that
webuis for features in content could live in content, removing the need to add
extra stuff in content/public just for chrome's webui implementation.

does this really need to be on chrome://inspect, which is implemented in chrome?
i.e. could it be on chrome://service-worker? or even in chrome://inspect which
has an iframe to chrome://service-worker?

[horo, 2014-04-25 09:31:55]

On 2014/04/25 05:02:08, jam wrote:
> note: the whole reason we have webui implementations in content is so that
> webuis for features in content could live in content, removing the need to add
> extra stuff in content/public just for chrome's webui implementation.
> 
> does this really need to be on chrome://inspect, which is implemented in
chrome?
> i.e. could it be on chrome://service-worker? or even in chrome://inspect which
> has an iframe to chrome://service-worker?

I think to open the DevTools window for ServiceWorker we should use
DevToolsWindow::OpenDevToolsWindowForWorker() which is in chrome directory.
So we have to expose some methods to get the notification for the status change
and get the information about the running workers to content/public.

But service_worker_context_observer.h don't need to be moved tocontent/public.
So I create "Patch set 3".

jam@ what do you think about it?

[jam, 2014-04-25 21:26:03]

On 2014/04/25 09:31:55, horo wrote:
> On 2014/04/25 05:02:08, jam wrote:
> > note: the whole reason we have webui implementations in content is so that
> > webuis for features in content could live in content, removing the need to
add
> > extra stuff in content/public just for chrome's webui implementation.
> > 
> > does this really need to be on chrome://inspect, which is implemented in
> chrome?
> > i.e. could it be on chrome://service-worker? or even in chrome://inspect
which
> > has an iframe to chrome://service-worker?
> 
> I think to open the DevTools window for ServiceWorker we should use
> DevToolsWindow::OpenDevToolsWindowForWorker() which is in chrome directory.
> So we have to expose some methods to get the notification for the status
change
> and get the information about the running workers to content/public.
> 
> But service_worker_context_observer.h don't need to be moved tocontent/public.
> So I create "Patch set 3".
> 
> jam@ what do you think about it?

it would help me understand things better if I could see the usage of this from
chrome layer. often changes to the content api are hard to review without seeing
how they'll be used.

+pfeldman for his thoughts: is there a way for content to tell chrome to open a
window for debugging a service worker? then this webui could be implemented in
content and iframed by chrome://inspect?

[michaeln, 2014-04-25 22:09:14]

not a full review, a preliminary look

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:22:
ServiceWorkerContextWrapper::~ServiceWorkerContextWrapper() {
I think you have to observer_list_->RemoveObserver(xxx) here since the list
might outlive |this|.

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:160:
BrowserThread::PostTask(BrowserThread::UI,
If I understand ObserverListThreadSafe properly, since callbackproxy is added to
the observerlist on the UI thread, this method should be getting called on the
UI thread. So i think you can call status_change_callback_list_.Notify() right
here.

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.h:59: virtual
scoped_ptr<base::CallbackList<void(void)>::Subscription>
From callback_list.h

  // Add a callback to the list. The callback will remain registered until the
  // returned Subscription is destroyed, which must occur before the
  // CallbackList is destroyed.
  scoped_ptr<Subscription> Add(const CallbackType& cb) WARN_UNUSED_RESULT;

The part about guaranteeing that all Subscriptions are destroyed prior to the
list being destroyed seems problematic for this use case.

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.h:73: class
CallbackProxy : public ServiceWorkerContextObserver,
Alternatively, it looks like ServiceWorkerContextWrapper could derive from
ServiceWorkerContextObserver directly, fewer moving parts.

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.h:93: void
onStatusChanged();
is this used?

[horo, 2014-04-26 08:00:45]

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:22:
ServiceWorkerContextWrapper::~ServiceWorkerContextWrapper() {
On 2014/04/25 22:09:14, michaeln wrote:
> I think you have to observer_list_->RemoveObserver(xxx) here since the list
> might outlive |this|.

Done.

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:160:
BrowserThread::PostTask(BrowserThread::UI,
On 2014/04/25 22:09:14, michaeln wrote:
> If I understand ObserverListThreadSafe properly, since callbackproxy is added
to
> the observerlist on the UI thread, this method should be getting called on the
> UI thread. So i think you can call status_change_callback_list_.Notify() right
> here.
> 

Done.

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.h:59: virtual
scoped_ptr<base::CallbackList<void(void)>::Subscription>
On 2014/04/25 22:09:14, michaeln wrote:
> From callback_list.h
> 
>   // Add a callback to the list. The callback will remain registered until the
>   // returned Subscription is destroyed, which must occur before the
>   // CallbackList is destroyed.
>   scoped_ptr<Subscription> Add(const CallbackType& cb) WARN_UNUSED_RESULT;
> 
> The part about guaranteeing that all Subscriptions are destroyed prior to the
> list being destroyed seems problematic for this use case.

Done.
Oh, I didn't understand that. Thank you.

https://codereview.chromium.org/251653003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.h:73: class
CallbackProxy : public ServiceWorkerContextObserver,
On 2014/04/25 22:09:14, michaeln wrote:
> Alternatively, it looks like ServiceWorkerContextWrapper could derive from
> ServiceWorkerContextObserver directly, fewer moving parts.

Done.

[horo, 2014-04-26 08:01:43]

On 2014/04/25 21:26:03, jam wrote:
> On 2014/04/25 09:31:55, horo wrote:
> > On 2014/04/25 05:02:08, jam wrote:
> > > note: the whole reason we have webui implementations in content is so that
> > > webuis for features in content could live in content, removing the need to
> add
> > > extra stuff in content/public just for chrome's webui implementation.
> > > 
> > > does this really need to be on chrome://inspect, which is implemented in
> > chrome?
> > > i.e. could it be on chrome://service-worker? or even in chrome://inspect
> which
> > > has an iframe to chrome://service-worker?
> > 
> > I think to open the DevTools window for ServiceWorker we should use
> > DevToolsWindow::OpenDevToolsWindowForWorker() which is in chrome directory.
> > So we have to expose some methods to get the notification for the status
> change
> > and get the information about the running workers to content/public.
> > 
> > But service_worker_context_observer.h don't need to be moved
tocontent/public.
> > So I create "Patch set 3".
> > 
> > jam@ what do you think about it?
> 
> it would help me understand things better if I could see the usage of this
from
> chrome layer. often changes to the content api are hard to review without
seeing
> how they'll be used.
> 
> +pfeldman for his thoughts: is there a way for content to tell chrome to open
a
> window for debugging a service worker? then this webui could be implemented in
> content and iframed by chrome://inspect?

jam@
This is the patch which is what I want to do all.
https://codereview.chromium.org/246183003/ DevTools Support for ServiceWoker.

I want to use Add/RemoveStatusChangeCallback and GetRunningServiceWorkerInfo of
ServiceWorkerContext in chrome/browser/devtools/devtools_targets_ui.cc and
chrome/browser/devtools/devtools_target_impl.cc.

[horo, 2014-04-30 11:08:44]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-30 11:09:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/251653003/170001

[commit-bot: I haz the power, 2014-04-30 11:09:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 11:09:09]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[michaeln, 2014-05-01 02:52:06]

https://codereview.chromium.org/251653003/diff/130001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/251653003/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:182:
status_change_callbacks_[i].Run();
callbacks could be removed or added while running a callback, you might want to
make a copy of the array prior to invoking them

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:21:
observer_list_->AddObserver(this);
this should always be called on the UI thread, we're depending on that now
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:25:
observer_list_->RemoveObserver(this);
does ObserverListThreadSafe require that an observer be removed on the same
thread on which it was added? bah... it does.

  // Remove an observer from the list if it is in the list.
  // If there are pending notifications in-transit to the observer, they will
  // be aborted.
  // If the observer to be removed is in the list, RemoveObserver MUST
  // be called from the same thread which called AddObserver.
  void RemoveObserver(ObserverType* obs)

There's nothing really guaranteeing that the dtor happens on the UI thread right
now (that i can see). Maybe the AddObserver and RemoveObserver calls could be
moved to Init() and Shutdown() respectively. Those are guaranteed to be called
on the UI thread.

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.h:57: virtual void
UnregisterServiceWorker(const GURL& pattern,
while you're here, can you make the line wrapping for the Unreg method match
that of the Reg method for consistency

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.h:71: //
ServiceWorkerContextObserver
can these go in the private section since it's really an impl detail that this
class derives from that interface, also OVERRIDE keyword these too

https://codereview.chromium.org/251653003/diff/170001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/251653003/diff/170001/content/public/browser/...
content/public/browser/service_worker_context.h:18: class ServiceWorkerInfo {
naming nit: RunningServiceWorkerInfo? I can easily imagine a different kind of
'info' making it into the this api.

https://codereview.chromium.org/251653003/diff/170001/content/public/browser/...
content/public/browser/service_worker_context.h:76: const
base::Callback<void(void)>& callback) = 0;
Identifying what to remove by callback ref seems odd somehow? Two callbacks
produced with the same inputs to two different calls to base::Bind are not
equal. Also this could definitely use some more specificity about what changed.

These kind of notifications might be better served with Listener style
delegation. I think vincent's api work is also headed toward a
ServiceWorkerContextClient or Listener sort of class in the content api.

[horo, 2014-05-01 04:52:31]

https://codereview.chromium.org/251653003/diff/130001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/251653003/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:182:
status_change_callbacks_[i].Run();
On 2014/05/01 02:52:07, michaeln wrote:
> callbacks could be removed or added while running a callback, you might want
to
> make a copy of the array prior to invoking them
Done.
Changed to use FOR_EACH_OBSERVER.

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:21:
observer_list_->AddObserver(this);
On 2014/05/01 02:52:07, michaeln wrote:
> this should always be called on the UI thread, we're depending on that now
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.
Moved to Init().

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:25:
observer_list_->RemoveObserver(this);
On 2014/05/01 02:52:07, michaeln wrote:
> does ObserverListThreadSafe require that an observer be removed on the same
> thread on which it was added? bah... it does.
> 
>   // Remove an observer from the list if it is in the list.
>   // If there are pending notifications in-transit to the observer, they will
>   // be aborted.
>   // If the observer to be removed is in the list, RemoveObserver MUST
>   // be called from the same thread which called AddObserver.
>   void RemoveObserver(ObserverType* obs)
> 
> There's nothing really guaranteeing that the dtor happens on the UI thread
right
> now (that i can see). Maybe the AddObserver and RemoveObserver calls could be
> moved to Init() and Shutdown() respectively. Those are guaranteed to be called
> on the UI thread.

Done.
Moved to Shutdown().

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.h:57: virtual void
UnregisterServiceWorker(const GURL& pattern,
On 2014/05/01 02:52:07, michaeln wrote:
> while you're here, can you make the line wrapping for the Unreg method match
> that of the Reg method for consistency

Done.

https://codereview.chromium.org/251653003/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.h:71: //
ServiceWorkerContextObserver
On 2014/05/01 02:52:07, michaeln wrote:
> can these go in the private section since it's really an impl detail that this
> class derives from that interface, also OVERRIDE keyword these too

Done.

https://codereview.chromium.org/251653003/diff/170001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/251653003/diff/170001/content/public/browser/...
content/public/browser/service_worker_context.h:18: class ServiceWorkerInfo {
On 2014/05/01 02:52:07, michaeln wrote:
> naming nit: RunningServiceWorkerInfo? I can easily imagine a different kind of
> 'info' making it into the this api.

Done.

https://codereview.chromium.org/251653003/diff/170001/content/public/browser/...
content/public/browser/service_worker_context.h:76: const
base::Callback<void(void)>& callback) = 0;
On 2014/05/01 02:52:07, michaeln wrote:
> Identifying what to remove by callback ref seems odd somehow? Two callbacks
> produced with the same inputs to two different calls to base::Bind are not
> equal. Also this could definitely use some more specificity about what
changed.
> 
> These kind of notifications might be better served with Listener style
> delegation. I think vincent's api work is also headed toward a
> ServiceWorkerContextClient or Listener sort of class in the content api.

Changed to use StatusChangeObserver.

[michaeln, 2014-05-05 21:01:15]

https://codereview.chromium.org/251653003/diff/250001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/251653003/diff/250001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:172: void
ServiceWorkerContextWrapper::OnVersionStateChanged(int64 version_id) {
does the devtools use case care about this one?

https://codereview.chromium.org/251653003/diff/250001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/251653003/diff/250001/content/public/browser/...
content/public/browser/service_worker_context.h:34: };
nit: could use a blank line

https://codereview.chromium.org/251653003/diff/250001/content/public/browser/...
content/public/browser/service_worker_context.h:37: virtual void
OnStatusChanged() = 0;
This class and it's method are pretty vague in its meaning. In your usecase, you
really want to know when the set of running workers has changed, is that right?

class Observer {
  virtual void OnRunningWorkersChanged() = 0;
};

I think additional methods could be added for what vincent is working on.

[pfeldman, 2014-05-06 20:54:44]

> +pfeldman for his thoughts: is there a way for content to tell chrome to open
a
> window for debugging a service worker?

Not as of today, but it should be very easy to add. Something like
DevToolsManager::OpenDevToolsWindow(scoped_refptr<DevToolsAgentHost>) could be
declared in content and implemented in chrome.

>  then this webui could be implemented in content and iframed by
chrome://inspect?

I am not a huge fan of a touch point integration like this. With a little
effort, exposing std::vector<scoped_refptr<DevToolsAgentHost>>
ServiceWorkerTargets(); in content would be sufficient to surface service worker
targets in chrome://inspect. chrome://inspect will poll them using this method.

[horo, 2014-05-07 14:43:30]

I changed the goal.
I tried to show "Inspect" button of SherviceWorker in chrome://inspect.
But I think it is more suitable to show "Inspect" button in
chrome://serviceworker-internals/.

To achieve this goal, we don't need to add methods in ServiceWorkerContext.
But we need to add an method in DevToolsManager.

So I completely changed this cl.
Could you please review again?

[pfeldman, 2014-05-07 16:01:29]

https://codereview.chromium.org/251653003/diff/270001/content/public/browser/...
File content/public/browser/devtools_manager.h (right):

https://codereview.chromium.org/251653003/diff/270001/content/public/browser/...
content/public/browser/devtools_manager.h:66: virtual void
RegisterOpenDevToolsWindowForWorkerCallback(
It does not look too pretty. John, do you think we could have

void Inspect(DevToolsAgentHost*)

in content_browser_delegate instead?

Alternatively, we could convert DevToolsHttpHandlerDelegate into
DevToolsManagerDelegate and add Inspect(DTAH) there. It would also need to be
pushed into content though.

[horo, 2014-05-08 04:08:23]

On 2014/05/07 16:01:29, pfeldman wrote:
>
https://codereview.chromium.org/251653003/diff/270001/content/public/browser/...
> File content/public/browser/devtools_manager.h (right):
> 
>
https://codereview.chromium.org/251653003/diff/270001/content/public/browser/...
> content/public/browser/devtools_manager.h:66: virtual void
> RegisterOpenDevToolsWindowForWorkerCallback(
> It does not look too pretty. John, do you think we could have
> 
> void Inspect(DevToolsAgentHost*)
> 
> in content_browser_delegate instead?
> 
> Alternatively, we could convert DevToolsHttpHandlerDelegate into
> DevToolsManagerDelegate and add Inspect(DTAH) there. It would also need to be
> pushed into content though.

Does "in content_browser_delegate" mean in WebContents, WebContentsImpl,
WebContentsDelegate and Browser?
I changed this cl to add InspectWorker() to them.

How about the patch set 11?

[pfeldman, 2014-05-08 10:26:30]

https://codereview.chromium.org/251653003/diff/290001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/251653003/diff/290001/content/public/browser/...
content/public/browser/web_contents.h:597: virtual void
InspectWorker(DevToolsAgentHost* agent_host) = 0;
Please don't. I don't see how WebContents interface can be workers or
devtools-aware. Especially on an instance.

[horo, 2014-05-08 11:29:19]

https://codereview.chromium.org/251653003/diff/290001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/251653003/diff/290001/content/public/browser/...
content/public/browser/web_contents.h:597: virtual void
InspectWorker(DevToolsAgentHost* agent_host) = 0;
On 2014/05/08 10:26:30, pfeldman wrote:
> Please don't. I don't see how WebContents interface can be workers or
> devtools-aware. Especially on an instance.

Could you please let me know how I can open DevToolsWindow from content?

[jam, 2014-05-08 16:27:17]

I don't understand the latest patchset (11). There's a method in WebContents
(presumbly called by chrome) that just calls out to the WebContentsDelegate (in
chrome). Why not just have chrome call out to other code in chrome directly
instead (see http://www.chromium.org/developers/content-module/content-api)?

https://codereview.chromium.org/251653003/diff/270001/content/public/browser/...
File content/public/browser/devtools_manager.h (right):

https://codereview.chromium.org/251653003/diff/270001/content/public/browser/...
content/public/browser/devtools_manager.h:66: virtual void
RegisterOpenDevToolsWindowForWorkerCallback(
On 2014/05/07 16:01:30, pfeldman wrote:
> It does not look too pretty. John, do you think we could have
> 
> void Inspect(DevToolsAgentHost*)
> 
> in content_browser_delegate instead?

just to make sure I understand, do you mean content_browser_client?

> 
> Alternatively, we could convert DevToolsHttpHandlerDelegate into
> DevToolsManagerDelegate and add Inspect(DTAH) there. It would also need to be
> pushed into content though.

[pfeldman, 2014-05-08 17:21:12]

On 2014/05/08 16:27:17, jam wrote:
> I don't understand the latest patchset (11). There's a method in WebContents
> (presumbly called by chrome) that just calls out to the WebContentsDelegate
(in
> chrome). Why not just have chrome call out to other code in chrome directly
> instead (see http://www.chromium.org/developers/content-module/content-api)
> 
>
https://codereview.chromium.org/251653003/diff/270001/content/public/browser/...
> File content/public/browser/devtools_manager.h (right):
> 
>
https://codereview.chromium.org/251653003/diff/270001/content/public/browser/...
> content/public/browser/devtools_manager.h:66: virtual void
> RegisterOpenDevToolsWindowForWorkerCallback(
> On 2014/05/07 16:01:30, pfeldman wrote:
> > It does not look too pretty. John, do you think we could have
> > 
> > void Inspect(DevToolsAgentHost*)
> > 
> > in content_browser_delegate instead?
> 
> just to make sure I understand, do you mean content_browser_client?
> 

I did. I mean that's the minimum we need to show any DTAH* in DevToolsWindow in
the embedder. Other solutions involve registering callbacks or coming up with
devtools_manager_client alikes.

> > 
> > Alternatively, we could convert DevToolsHttpHandlerDelegate into
> > DevToolsManagerDelegate and add Inspect(DTAH) there. It would also need to
be
> > pushed into content though.

[horo, 2014-05-09 02:18:16]

Sorry. I didn't understand what "content_browser_delegate" means.
I changed this cl to introduce ContentBrowserClient::InspectWorker() [patchset
12].

Is this what you meant?

[jam, 2014-05-09 17:15:41]

On 2014/05/08 17:21:12, pfeldman wrote:
> On 2014/05/08 16:27:17, jam wrote:
> > I don't understand the latest patchset (11). There's a method in WebContents
> > (presumbly called by chrome) that just calls out to the WebContentsDelegate
> (in
> > chrome). Why not just have chrome call out to other code in chrome directly
> > instead (see http://www.chromium.org/developers/content-module/content-api)
> > 
> >
>
https://codereview.chromium.org/251653003/diff/270001/content/public/browser/...
> > File content/public/browser/devtools_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/251653003/diff/270001/content/public/browser/...
> > content/public/browser/devtools_manager.h:66: virtual void
> > RegisterOpenDevToolsWindowForWorkerCallback(
> > On 2014/05/07 16:01:30, pfeldman wrote:
> > > It does not look too pretty. John, do you think we could have
> > > 
> > > void Inspect(DevToolsAgentHost*)
> > > 
> > > in content_browser_delegate instead?
> > 
> > just to make sure I understand, do you mean content_browser_client?
> > 
> 
> I did. I mean that's the minimum we need to show any DTAH* in DevToolsWindow
in
> the embedder. Other solutions involve registering callbacks or coming up with
> devtools_manager_client alikes.

Are there no devtools interfaces in content/public that are implemented by
chrome that we can put this on?

sorry for the delay, just saw this reply (yesterday was a bit hectic with the
tree). feel free to IM me and we can do this faster.
> 
> > > 
> > > Alternatively, we could convert DevToolsHttpHandlerDelegate into
> > > DevToolsManagerDelegate and add Inspect(DTAH) there. It would also need to
> be
> > > pushed into content though.

[pfeldman, 2014-05-09 17:46:47]

Chatted with John about this. We agreed upon the following plan:

- Introduce DevToolsManagerDelegate in content API
- Return it from ContentBrowserClient (as with some other delegates)
- Add a new method from latest patch set into the delegate, I'd call it in a
more generic manner: "void Inspect(DTAH*)"
- (outside of the scope of this patch) migrate DevToolsHttpHandlerDelegate
methods into this new DevToolsManagerDelegate

[horo, 2014-05-12 06:22:59]

On 2014/05/09 17:46:47, pfeldman wrote:
> Chatted with John about this. We agreed upon the following plan:
> 
> - Introduce DevToolsManagerDelegate in content API
> - Return it from ContentBrowserClient (as with some other delegates)
> - Add a new method from latest patch set into the delegate, I'd call it in a
> more generic manner: "void Inspect(DTAH*)"
> - (outside of the scope of this patch) migrate DevToolsHttpHandlerDelegate
> methods into this new DevToolsManagerDelegate

Thank you very much for the suggestion.

I introduced DevToolsManagerDelegate and implemented DevToolsManagerDelegateImpl
which Inspect() method calls DevToolsWindow::OpenDevToolsWindowForWorker().
To check whether the DevToolsAgentHost is for worker or not, I added IsWorker()
to DevToolsAgentHost.
Please review the patch set 13.

[pfeldman, 2014-05-12 06:38:23]

This looks much better. A couple of nits below.

https://codereview.chromium.org/251653003/diff/330001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/251653003/diff/330001/content/public/browser/...
content/public/browser/content_browser_client.h:641: virtual
DevToolsManagerDelegate* CreateDevToolsManagerDelegate(
I'm looking at GetWebContentsViewDelegate and
GetSpeechRecognitionManagerDelegate and I think it should be called
GetDevToolsManagerDelegate. Returning ownership seems to be common to the
delegate getters.

Since DevToolsManager is singleton, so should be its delegate. And you should
retain this delegate in the devtools manager + expose Inspect in devtools
manager that wires into the delegate.

https://codereview.chromium.org/251653003/diff/330001/content/public/browser/...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/251653003/diff/330001/content/public/browser/...
content/public/browser/devtools_agent_host.h:70: virtual bool IsWorker() = 0;
That is fine for now, but we'll convert it into GetType() as a follow up.

https://codereview.chromium.org/251653003/diff/330001/content/public/browser/...
File content/public/browser/devtools_manager_delegate.h (right):

https://codereview.chromium.org/251653003/diff/330001/content/public/browser/...
content/public/browser/devtools_manager_delegate.h:17: virtual void
Inspect(DevToolsAgentHost* agent_host) = 0;
It might need to be Inspect(BrowserContext* context, DevToolsAgentHost*
agent_host) = 0;

[horo, 2014-05-12 07:51:37]

https://codereview.chromium.org/251653003/diff/330001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/251653003/diff/330001/content/public/browser/...
content/public/browser/content_browser_client.h:641: virtual
DevToolsManagerDelegate* CreateDevToolsManagerDelegate(
On 2014/05/12 06:38:24, pfeldman wrote:
> I'm looking at GetWebContentsViewDelegate and
> GetSpeechRecognitionManagerDelegate and I think it should be called
> GetDevToolsManagerDelegate. Returning ownership seems to be common to the
> delegate getters.
> 
> Since DevToolsManager is singleton, so should be its delegate. And you should
> retain this delegate in the devtools manager + expose Inspect in devtools
> manager that wires into the delegate.

Done.

https://codereview.chromium.org/251653003/diff/330001/content/public/browser/...
File content/public/browser/devtools_manager_delegate.h (right):

https://codereview.chromium.org/251653003/diff/330001/content/public/browser/...
content/public/browser/devtools_manager_delegate.h:17: virtual void
Inspect(DevToolsAgentHost* agent_host) = 0;
On 2014/05/12 06:38:24, pfeldman wrote:
> It might need to be Inspect(BrowserContext* context, DevToolsAgentHost*
> agent_host) = 0;

Done.

[pfeldman, 2014-05-12 08:30:54]

Two nits, otherwise devtools lgtm

https://codereview.chromium.org/251653003/diff/390001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_manager_delegate_impl.h (right):

https://codereview.chromium.org/251653003/diff/390001/chrome/browser/devtools...
chrome/browser/devtools/devtools_manager_delegate_impl.h:27: Profile* profile_;
- remove profile.
add DISALLOW_COPY_AND_ASSIGN

[horo, 2014-05-12 09:17:10]

https://codereview.chromium.org/251653003/diff/390001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_manager_delegate_impl.h (right):

https://codereview.chromium.org/251653003/diff/390001/chrome/browser/devtools...
chrome/browser/devtools/devtools_manager_delegate_impl.h:27: Profile* profile_;
On 2014/05/12 08:30:55, pfeldman wrote:
> - remove profile.
> add DISALLOW_COPY_AND_ASSIGN

Done.

[horo, 2014-05-12 09:17:35]

jam@
Could you please review content/public/browser/*?

[jam, 2014-05-12 15:01:22]

https://codereview.chromium.org/251653003/diff/490001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/251653003/diff/490001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:282: OVERRIDE;
nit: don't start a line with OVERRIDE

https://codereview.chromium.org/251653003/diff/490001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_manager_delegate_impl.h (right):

https://codereview.chromium.org/251653003/diff/490001/chrome/browser/devtools...
chrome/browser/devtools/devtools_manager_delegate_impl.h:16: class
DevToolsManagerDelegateImpl : public content::DevToolsManagerDelegate {
nit: the convention for chrome implementing an interface from content is to name
it ChromeDevToolsManagerDelegate

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
content/public/browser/content_browser_client.h:641: virtual
DevToolsManagerDelegate* GetDevToolsManagerDelegate();
nit: see per above, the ifdef's methods are at the end,  so put this at line 596

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
content/public/browser/devtools_agent_host.h:70: virtual bool IsWorker() = 0;
who is calling this in chrome, other than a test? if it's just a test, can that
test be moved to content?

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
File content/public/browser/devtools_manager.h (right):

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
content/public/browser/devtools_manager.h:64: virtual void
Inspect(BrowserContext* browser_context,
why is there a method on both DevToolsManager & DevToolsManagerDelegate?

please include the chrome code which calls this in this cl, otherwise it's hard
to review content api changes to know what is necessary and what is not

[horo, 2014-05-12 16:21:47]

https://codereview.chromium.org/251653003/diff/490001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/251653003/diff/490001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:282: OVERRIDE;
On 2014/05/12 15:01:23, jam wrote:
> nit: don't start a line with OVERRIDE

Done.

https://codereview.chromium.org/251653003/diff/490001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_manager_delegate_impl.h (right):

https://codereview.chromium.org/251653003/diff/490001/chrome/browser/devtools...
chrome/browser/devtools/devtools_manager_delegate_impl.h:16: class
DevToolsManagerDelegateImpl : public content::DevToolsManagerDelegate {
On 2014/05/12 15:01:23, jam wrote:
> nit: the convention for chrome implementing an interface from content is to
name
> it ChromeDevToolsManagerDelegate

Done.

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
content/public/browser/content_browser_client.h:641: virtual
DevToolsManagerDelegate* GetDevToolsManagerDelegate();
On 2014/05/12 15:01:23, jam wrote:
> nit: see per above, the ifdef's methods are at the end,  so put this at line
596

Done.

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
content/public/browser/devtools_agent_host.h:70: virtual bool IsWorker() = 0;
On 2014/05/12 15:01:23, jam wrote:
> who is calling this in chrome, other than a test? if it's just a test, can
that
> test be moved to content?

ChromeDevToolsManagerDelegate::Inspect() calls this method to determine which
DevToolsWindows should be opened.

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
File content/public/browser/devtools_manager.h (right):

https://codereview.chromium.org/251653003/diff/490001/content/public/browser/...
content/public/browser/devtools_manager.h:64: virtual void
Inspect(BrowserContext* browser_context,
On 2014/05/12 15:01:23, jam wrote:
> why is there a method on both DevToolsManager & DevToolsManagerDelegate?
> 
> please include the chrome code which calls this in this cl, otherwise it's
hard
> to review content api changes to know what is necessary and what is not

Oh, we don't need this.
Removed Inspect() from DevToolsManager.

[jam, 2014-05-13 02:46:41]

lgtm

https://codereview.chromium.org/251653003/diff/550001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.h (right):

https://codereview.chromium.org/251653003/diff/550001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.h:13: class
DevToolsAgentHost;
nit: not needed by definition since it's used in an overridden method

[horo, 2014-05-13 03:17:16]

https://codereview.chromium.org/251653003/diff/550001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.h (right):

https://codereview.chromium.org/251653003/diff/550001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.h:13: class
DevToolsAgentHost;
On 2014/05/13 02:46:42, jam wrote:
> nit: not needed by definition since it's used in an overridden method

Done.

[horo, 2014-05-13 03:17:25]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-13 03:18:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/251653003/570001

[horo, 2014-05-13 03:20:45]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-05-13 03:20:53]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-13 03:23:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/251653003/570001

[horo, 2014-05-13 03:25:20]

The CQ bit was unchecked by horo@chromium.org

[kinuko, 2014-05-13 03:48:55]

Looks like the model's drastically changed from the initial patch. looks good to
me too (given that the model / interface details look good to pfeldman/jam)

https://codereview.chromium.org/251653003/diff/570001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/251653003/diff/570001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.h:56: virtual bool IsWorker()
OVERRIDE;
nit: could probably be a const method?

[horo, 2014-05-13 03:49:26]

The CQ bit was checked by horo@chromium.org

[horo, 2014-05-13 03:49:34]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-05-13 03:51:35]

The CQ bit was checked by horo@chromium.org

[horo, 2014-05-13 03:51:51]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-05-13 03:55:36]

https://codereview.chromium.org/251653003/diff/570001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/251653003/diff/570001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.h:56: virtual bool IsWorker()
OVERRIDE;
On 2014/05/13 03:48:56, kinuko wrote:
> nit: could probably be a const method?

Done.

[horo, 2014-05-13 03:55:46]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-13 03:56:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/251653003/590001

[horo, 2014-05-13 05:38:15]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-13 05:39:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/251653003/590001

[horo, 2014-05-13 05:41:45]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-13 05:42:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/251653003/620001

[commit-bot: I haz the power, 2014-05-13 07:50:04]

Change committed as 270047