FindBrowserWithWebContents now recognises DevTool's embedded WebContent

FindBrowserWithWebContents now recognises DevTool's embedded WebContent

If the DevTools window is invoked in-tab, the task manager refused to add it to the list of displayed processes, because there was no tab owning this WebContent. This CL improves the check in FindBrowserWithWebContents to recognise the DevTool's WebContent as valid in this case. To avoid crashing in TabContentsResource::GetIcon(), a FaviconTabHelper::CreateForWebContents() call had to be added.

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

[vabr (Chromium), 2013-08-29 10:38:46]

Hi all,

pkasting@chromium.org: Please review the change in
chrome/browser/ui/browser_finder.cc.

yoshiki@chromium.org: Please review the fix and the test in
chrome/browser/task_manager/*. Any hint for a better test welcome (see my
comment).

Thanks!
Vaclav

https://codereview.chromium.org/23576007/diff/8001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

https://codereview.chromium.org/23576007/diff/8001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_browsertest.cc:507:
TaskManagerBrowserTestUtil::WaitForWebResourceChange(2);
I have an issue with this browser test. If the DevTools window does not show up
in the task manager (which was the case before my fix), the test times out. Do
we really need to wait the whole time-out period, or is there any reasonable way
to cut it off sooner?

[pfeldman, 2013-08-29 11:57:36]

https://codereview.chromium.org/23576007/diff/8001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_finder.cc (right):

https://codereview.chromium.org/23576007/diff/8001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_finder.cc:169: Browser*
FindBrowserWithWebContents(const WebContents* web_contents) {
This method is used in many places that do various operations on the browser
returned. It used to return NULL for devtools, but now this is being changed. I
wonder if you could move this to the specific call site in order to minimize the
impact of this change.

[vabr (Chromium), 2013-08-29 12:51:09]

@pfeldman: Does this look better now?
@pkasting: Please feel free to ignore this, the file I wanted you to review was
dropped from the CL. (Moved you to Cc.)
@yoshiki: Please review the current patchset.

Thanks all!

Vaclav

https://codereview.chromium.org/23576007/diff/8001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_finder.cc (right):

https://codereview.chromium.org/23576007/diff/8001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_finder.cc:169: Browser*
FindBrowserWithWebContents(const WebContents* web_contents) {
On 2013/08/29 11:57:36, pfeldman wrote:
> This method is used in many places that do various operations on the browser
> returned. It used to return NULL for devtools, but now this is being changed.
I
> wonder if you could move this to the specific call site in order to minimize
the
> impact of this change.

That's a good point, I was also a bit scared, although the tests passed.

As a bonus, one less file to modify, and freeing pkasting from the burden of
this review. :)

[pfeldman, 2013-08-29 13:15:51]

> @pfeldman: Does this look better now?

looks good! thanks!

[yoshiki, 2013-08-30 01:30:21]

LGTM

[commit-bot: I haz the power, 2013-08-30 06:25:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/23576007/18001

[commit-bot: I haz the power, 2013-08-30 10:30:12]

Change committed as 220549