Make devtools_window.cc multi-desktop aware.

Make devtools_window.cc multi-desktop aware.

Getting rid of BrowserList:: methods which only know about the native-desktop.

Note that CreateBrowserForDevTools is still native desktop only so that devtools will not fully work yet in a multi-desktop environment, but this is making progress in the refactoring goal of getting rid of all native desktop only methods.

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

[gab, 2013-02-07 18:01:36]

Pavel, please take a look.

This is part of a generic effort to remove non-multi-desktop aware BrowserList::
methods, for more example CLs: take a look recent commits for this CL's tracking
BUG.

Cheers!
gab

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/de...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:811: browser_ ?
browser_->host_desktop_type() : chrome::GetActiveDesktop();
@pfeldman: Looks like you introduced OpenInNewTab in http://crrev.com/118666 a
year ago or so. Few questions for you:
1) Is OpenInNewTab() still used? Even if I remove all instances of it Chrome
still compiles, but I'm thinking perhaps some IPC calls from webkit depend on
it..?
Grep'ing suggests that this perhaps true, but "OpenInNewTab" is kind of a
generic name and unrelated messages could use the same name...

2) Is |browser_| guaranteed to be non-NULL here (i.e. from the code it seems
like it is if Show() is always called first... and many other methods make this
assumption and skip the NULL check it seems)?
If so, that would be preferable as chrome::GetActiveDesktop() is a very last
resort call for when we have absolutely no context...

[pfeldman, 2013-02-07 20:12:49]

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/br...
File chrome/browser/devtools/browser_list_tabcontents_provider.cc (right):

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/br...
chrome/browser/devtools/browser_list_tabcontents_provider.cc:97:
browser_list->get(0),
Will it tolerate 0 in case of empty list?

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/de...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:419: for (chrome::BrowserIterator it;
!it.done(); it.Next()) {
You seem to be reverting https://codereview.chromium.org/11519012, is that
intentional?

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:811: browser_ ?
browser_->host_desktop_type() : chrome::GetActiveDesktop();
On 2013/02/07 18:01:36, gab wrote:
> @pfeldman: Looks like you introduced OpenInNewTab in http://crrev.com/118666 a
> year ago or so. Few questions for you:
> 1) Is OpenInNewTab() still used? Even if I remove all instances of it Chrome
> still compiles, but I'm thinking perhaps some IPC calls from webkit depend on
> it..?

It is used, see DevToolsHostMsg_OpenInNewTab issued in devtools_client.cc. It is
called from the WebDevToolsFrontendClient::openInNewTab override. That override
has default implementation and hence it compiles for you.

> Grep'ing suggests that this perhaps true, but "OpenInNewTab" is kind of a
> generic name and unrelated messages could use the same name...
> 
> 2) Is |browser_| guaranteed to be non-NULL here (i.e. from the code it seems
> like it is if Show() is always called first... and many other methods make
this
> assumption and skip the NULL check it seems)?
> If so, that would be preferable as chrome::GetActiveDesktop() is a very last
> resort call for when we have absolutely no context...

browser_ is a browser we create for showing undocked devtools window. So it is
NULL when you see devtools docked and non-NULL when it is undocked.

[gab, 2013-02-07 20:31:45]

Thanks for the quick review, see replies below.

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/br...
File chrome/browser/devtools/browser_list_tabcontents_provider.cc (right):

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/br...
chrome/browser/devtools/browser_list_tabcontents_provider.cc:97:
browser_list->get(0),
On 2013/02/07 20:12:49, pfeldman wrote:
> Will it tolerate 0 in case of empty list?

This is no different than the previous code which was dereferencing
BrowserList::begin().

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/de...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:419: for (chrome::BrowserIterator it;
!it.done(); it.Next()) {
On 2013/02/07 20:12:49, pfeldman wrote:
> You seem to be reverting https://codereview.chromium.org/11519012, is that
> intentional?

Yes, BrowserIterator is a new class which is multi-desktop aware.
BrowserList::const_iterator was only going over the native desktop.

Ananta originally hardcoded a few places to be aware of both list, but this was
getting messy so I built BrowserIterator which now implicitly goes over both
lists.

So, although this looks like a revert, it's subtly not :)!

You made me realize I had forgot the call in the header though (not sure why it
was ever exposed publicly since it was only used here...) -- removed.

https://codereview.chromium.org/12210063/diff/2001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:811: browser_ ?
browser_->host_desktop_type() : chrome::GetActiveDesktop();
On 2013/02/07 20:12:49, pfeldman wrote:
> On 2013/02/07 18:01:36, gab wrote:
> > @pfeldman: Looks like you introduced OpenInNewTab in http://crrev.com/118666
a
> > year ago or so. Few questions for you:
> > 1) Is OpenInNewTab() still used? Even if I remove all instances of it Chrome
> > still compiles, but I'm thinking perhaps some IPC calls from webkit depend
on
> > it..?
> 
> It is used, see DevToolsHostMsg_OpenInNewTab issued in devtools_client.cc. It
is
> called from the WebDevToolsFrontendClient::openInNewTab override. That
override
> has default implementation and hence it compiles for you.
> 
> > Grep'ing suggests that this perhaps true, but "OpenInNewTab" is kind of a
> > generic name and unrelated messages could use the same name...
> > 
> > 2) Is |browser_| guaranteed to be non-NULL here (i.e. from the code it seems
> > like it is if Show() is always called first... and many other methods make
> this
> > assumption and skip the NULL check it seems)?
> > If so, that would be preferable as chrome::GetActiveDesktop() is a very last
> > resort call for when we have absolutely no context...
> 
> browser_ is a browser we create for showing undocked devtools window. So it is
> NULL when you see devtools docked and non-NULL when it is undocked.

I see so is it possible for both inspected_web_contents and browser_ to be
NULL?!

[pfeldman, 2013-02-08 08:04:13]

> I see so is it possible for both inspected_web_contents and browser_ to be
> NULL?!

inspected_web_contents_ points to WebContents under inspection (when there is
one). browser_ is there when we are undocked. We only allow docked mode when
inspecting WebContents, so both being NULL is impossible:

inspected_web_contents_ == NULL => browser_ != NULL.

I would not rely upon it though unless necessary.

[gab, 2013-02-08 16:57:52]

On 2013/02/08 08:04:13, pfeldman wrote:
> > I see so is it possible for both inspected_web_contents and browser_ to be
> > NULL?!
> 
> inspected_web_contents_ points to WebContents under inspection (when there is
> one). browser_ is there when we are undocked. We only allow docked mode when
> inspecting WebContents, so both being NULL is impossible:
> 
> inspected_web_contents_ == NULL => browser_ != NULL.
> 
> I would not rely upon it though unless necessary.

Sounds good, modified the code as per that fact, please take another look.

Thanks!
Gab

[pfeldman, 2013-02-09 10:15:53]

https://codereview.chromium.org/12210063/diff/4003/chrome/browser/devtools/de...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/12210063/diff/4003/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:810: chrome::HostDesktopType
host_desktop_type;
I am not sure I follow the code below.

Note that we never want to open given |url| in the browser_ (the one we created
for the devtools front-end).

[gab, 2013-02-09 17:38:05]

Reply below, thanks for shedding some light on this.

https://codereview.chromium.org/12210063/diff/4003/chrome/browser/devtools/de...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/12210063/diff/4003/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:810: chrome::HostDesktopType
host_desktop_type;
On 2013/02/09 10:15:53, pfeldman wrote:
> I am not sure I follow the code below.
> 
> Note that we never want to open given |url| in the browser_ (the one we
created
> for the devtools front-end).

Right, this code does the same thing as the previous code (i.e., find a tabbed
browser (thus not browser_ which I believe to not be tabbed)); the only
difference is it's trying to infer context (desktop type) from browser_ in order
to know on which desktop it should look for a tabbed browser (the previous code
would always look on the native desktop; thus if this was to be called on the
ash desktop, a tabbed browser on the native desktop (potentially not even
visible to the user at that point) would open the URL...

This code seems to assume that there is a tabbed browser available though, is
that always true? It also doesn't try to separate browsers per profile... should
that matter? Perhaps one of the methods in browser_finder.h (e.g.,
FindBrowserWithProfile():
https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/ui/br...)
is better suited than this iterator lookup?

[pfeldman, 2013-02-11 11:32:24]

devtools lgtm. thanks for your explanation.

[gab, 2013-02-11 15:11:54]

On 2013/02/11 11:32:24, pfeldman wrote:
> devtools lgtm. thanks for your explanation.

Thanks!

[commit-bot: I haz the power, 2013-02-11 15:12:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/12210063/4003

[commit-bot: I haz the power, 2013-02-11 16:15:50]

Change committed as 181697