Task manager's apps and extensions should show the correct favicon

Task manager's apps and extensions should show the correct favicon

We need to get the correct favicon of the app or extension so that it's
easier to identify in the table.

Before and after screenshots are available on the BUG thread.

BUG=525293
TEST=browser_tests --gtest_filter=ExtensionTagsTest.*

Committed: https://crrev.com/d91a61917396cc7cf77a44b26290758045d57ef3
Cr-Commit-Position: refs/heads/master@{#346155}

[afakhry, 2015-08-27 00:18:10]

afakhry@chromium.org changed reviewers:
+ nick@chromium.org, rdevlin.cronin@chromium.org

[afakhry, 2015-08-27 00:18:10]

rdevlin.cronin@chromium.org: Can you please review this CL?
I followed the same approach in AppWindow::UpdateExtensionAppIcon().

[Devlin, 2015-08-27 16:08:55]

Could we also add a small test?  And, for documentation, mind adding a
screenshot?  (Always nice for UI changes)

https://codereview.chromium.org/1302423005/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1302423005/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/extension_task.cc:69:
return;
nit: it'll depend on who you ask, but I generally prefer to only early-return if
it saves code/eliminates a big indent.  So in a case like this, I'd just do:
if (!image->image_skia().isNull())
  set_icon(image->image_skia());

https://codereview.chromium.org/1302423005/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/extension_task.cc:80:
base::string16 title = extension ?
I don't know why I didn't see this before, but if |extension| is null here,
extension->is_app() will crash.  So we should either DCHECK extension, or handle
it being null properly.  Can it be null here?

https://codereview.chromium.org/1302423005/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/extension_task.cc:95: void
ExtensionTask::LoadExtensionIcon(content::WebContents* web_contents,
instead of passing in web_contents, just use web_contents().

[afakhry, 2015-08-27 21:48:15]

Modified the test and added the screenshots on the bug comments. PTAL.

https://codereview.chromium.org/1302423005/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1302423005/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/extension_task.cc:69:
return;
On 2015/08/27 16:08:55, Devlin wrote:
> nit: it'll depend on who you ask, but I generally prefer to only early-return
if
> it saves code/eliminates a big indent.  So in a case like this, I'd just do:
> if (!image->image_skia().isNull())
>   set_icon(image->image_skia());

Done.

https://codereview.chromium.org/1302423005/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/extension_task.cc:80:
base::string16 title = extension ?
On 2015/08/27 16:08:55, Devlin wrote:
> I don't know why I didn't see this before, but if |extension| is null here,
> extension->is_app() will crash.  So we should either DCHECK extension, or
handle
> it being null properly.  Can it be null here?

Yes it can, in some tests if you remember, which they don't add themselves to
the enabled extensions lists. We're not crashing these tests however since the
creation of the Extension task doesn't happen until the task manager starts
updating.

Thank you for spotting this!

https://codereview.chromium.org/1302423005/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/extension_task.cc:95: void
ExtensionTask::LoadExtensionIcon(content::WebContents* web_contents,
On 2015/08/27 16:08:55, Devlin wrote:
> instead of passing in web_contents, just use web_contents().

Done.

https://codereview.chromium.org/1302423005/diff/20001/chrome/browser/task_man...
File
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc
(right):

https://codereview.chromium.org/1302423005/diff/20001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc:20:
class TestImageLoader {
I took this TestImageLoader from here:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
with few modifications.

[Devlin, 2015-08-27 22:08:48]

Nice! lgtm with nit.

https://codereview.chromium.org/1302423005/diff/20001/chrome/browser/task_man...
File
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc
(right):

https://codereview.chromium.org/1302423005/diff/20001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc:20:
class TestImageLoader {
On 2015/08/27 21:48:15, afakhry wrote:
> I took this TestImageLoader from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> with few modifications.

This class is big enough that it should go in its own file, and extracted from
the other unittest.

[afakhry, 2015-08-27 23:52:48]

Done. I extracted TestImageLoader into its own files and added the source files
to the extensions_test_support_sources.

[afakhry, 2015-08-28 15:47:35]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-08-28 15:47:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1302423005/#ps60001
(title: "Cleaning up the browser test")

[commit-bot: I haz the power, 2015-08-28 15:47:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1302423005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1302423005/60001

[commit-bot: I haz the power, 2015-08-28 15:52:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-28 15:53:16]

Patchset 4 (id:??) landed as
https://crrev.com/d91a61917396cc7cf77a44b26290758045d57ef3
Cr-Commit-Position: refs/heads/master@{#346155}