Break out manifest icon logic from ShortcutHelper

Break out manifest icon logic from ShortcutHelper

* Parcels out logic from the ShortcutHelper that deals with
  Manifest icons and puts it into the ManifestIconSelector.

* Renames the ShortcutHelperTest to ManifestIconSelectorTest
  since that class entirely tested Manifest icon support,
  anyway.

BUG=453170
Committed: https://crrev.com/44329fdcf31d412ee6a202a5e287acd9d41f17f9
Cr-Commit-Position: refs/heads/master@{#314394}

[gone, 2015-01-29 01:13:28]

dfalcantara@chromium.org changed reviewers:
+ mlamouri@chromium.org

[gone, 2015-01-29 01:13:29]

The total diff looks strange, but the patch SEEMS to have uploaded correctly. 
Happens when you rename things, I guess.

[gone, 2015-01-30 01:32:25]

Poke.

[gone, 2015-01-30 18:00:45]

dfalcantara@chromium.org changed reviewers:
+ aurimas@chromium.org

[aurimas (slooooooooow), 2015-01-30 18:46:27]

lgtm

[mlamouri (slow - plz ping), 2015-01-30 19:25:43]

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
File chrome/browser/android/manifest_icon_selector.cc (right):

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.cc:11: #include
"content/public/common/manifest.h"
nit: already included in the header, fwiw.

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.cc:14: #include "url/gurl.h"
nit: ditto

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
File chrome/browser/android/manifest_icon_selector.h (right):

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.h:9: #include
"content/public/common/manifest.h"
nit: you might want to include gurl.h here.

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.h:17: }
nit: "// namespace IPC"

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.h:21: // Selects the icon most
closely matching the size constraints.
nit: I would specify that this is following very basic heuristics and
improvements are welcome.

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.h:24:
ManifestIconSelector(content::WebContents* web_contents,
Instead of having a constructor that takes a WebContents and an int, could you
pass that to |FindBestMatchingIcon| and make it static? (Actually, you would get
only the preferred icon size in pixel, right?)

If you need the preferred_icon_size_in_px_ too much, you could have the ctor
private and create an instance instance inside the static method.

I think that would make it less heavy to use for callers, instead of having a
class member, they could just use ManifestIconSelector as a helper.

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
File chrome/browser/android/manifest_icon_selector_unittest.cc (right):

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector_unittest.cc:63: virtual
~ManifestIconSelectorTest() {}
nit: shouldn't that be override (instead of virtual)?

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector_unittest.cc:79:
manifest_icon_selector_.reset(NULL);
nit: nullptr? feel free to update the entire file to nullptr as long as you are
modifying it.

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector_unittest.cc:95: return 48;
Could you make it a const for the test class?

[gone, 2015-01-30 20:08:11]

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
File chrome/browser/android/manifest_icon_selector.cc (right):

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.cc:11: #include
"content/public/common/manifest.h"
On 2015/01/30 19:25:42, Mounir Lamouri wrote:
> nit: already included in the header, fwiw.

Done.

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.cc:14: #include "url/gurl.h"
On 2015/01/30 19:25:42, Mounir Lamouri wrote:
> nit: ditto

Done.

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
File chrome/browser/android/manifest_icon_selector.h (right):

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.h:9: #include
"content/public/common/manifest.h"
On 2015/01/30 19:25:42, Mounir Lamouri wrote:
> nit: you might want to include gurl.h here.

Done.

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.h:17: }
On 2015/01/30 19:25:43, Mounir Lamouri wrote:
> nit: "// namespace IPC"

Done.

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.h:21: // Selects the icon most
closely matching the size constraints.
On 2015/01/30 19:25:42, Mounir Lamouri wrote:
> nit: I would specify that this is following very basic heuristics and
> improvements are welcome.

Done.

https://chromiumcodereview.appspot.com/880203004/diff/60001/chrome/browser/an...
chrome/browser/android/manifest_icon_selector.h:24:
ManifestIconSelector(content::WebContents* web_contents,
On 2015/01/30 19:25:42, Mounir Lamouri wrote:
> Instead of having a constructor that takes a WebContents and an int, could you
> pass that to |FindBestMatchingIcon| and make it static? (Actually, you would
get
> only the preferred icon size in pixel, right?)
> 
> If you need the preferred_icon_size_in_px_ too much, you could have the ctor
> private and create an instance instance inside the static method.
> 
> I think that would make it less heavy to use for callers, instead of having a
> class member, they could just use ManifestIconSelector as a helper.

Done.

[mlamouri (slow - plz ping), 2015-01-30 22:32:13]

It seems that you didn't apply the comments I made for the unittest file. I've
added them. Only nits though.

Also, I think you should move the private static methods outside of the header
of ManifestIconSelector (static private methods could as well live in an
anonymous namespace in that case). However, you could make
|preferred_icon_size_in_px| a class member, in which case, you could make those
methods no longer static. You might want to update the comments too which refer
to |preferred_icon_size_in_px| and |preferred_icon_size_in_px_|.

Finally, the implementations of ManifestIconSelector methods doesn't seem to
match the header file.

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
File chrome/browser/android/manifest_icon_selector.h (right):

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector.h:30: content::WebContents*
web_contents);
Wouldn't that be better to not depend on WebContents*? As suggested before, this
method could take the required arguments instead of computing them. I guess
device_scale_factor would be needed in addition of the size in dp or px, I'm not
sure which one would be better to understand for callers. Maybe px?

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
File chrome/browser/android/manifest_icon_selector_unittest.cc (right):

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector_unittest.cc:63: virtual
~ManifestIconSelectorTest() {}
I think you should use override instead of virtual.

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector_unittest.cc:68:
GetPreferredIconSizeInDp()));
I guess you should update this. It probably doesn't compile anymore.

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector_unittest.cc:79:
manifest_icon_selector_.reset(NULL);
Feel free to s/NULL/nullptr/ in this file.

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector_unittest.cc:95: return 48;
It might be better to use a const here even if you declare it in the test class.

[gone, 2015-01-30 22:43:31]

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
File chrome/browser/android/manifest_icon_selector.h (right):

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector.h:30: content::WebContents*
web_contents);
On 2015/01/30 22:32:13, Mounir Lamouri wrote:
> Wouldn't that be better to not depend on WebContents*? As suggested before,
this
> method could take the required arguments instead of computing them. I guess
> device_scale_factor would be needed in addition of the size in dp or px, I'm
not
> sure which one would be better to understand for callers. Maybe px?

The logic is used in three different places, though: once in ShortcutHelper,
once in the new app banners, and once in the unit tests.  I'm not sure if it
makes sense to triplicate the same logic, especially if it's for a image that
will eventually be displayed in Android's UI in dp.

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
File chrome/browser/android/manifest_icon_selector_unittest.cc (right):

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector_unittest.cc:63: virtual
~ManifestIconSelectorTest() {}
On 2015/01/30 22:32:13, Mounir Lamouri wrote:
> I think you should use override instead of virtual.

Fixed your usage of virtual ~FakeScreen too.

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector_unittest.cc:68:
GetPreferredIconSizeInDp()));
On 2015/01/30 22:32:13, Mounir Lamouri wrote:
> I guess you should update this. It probably doesn't compile anymore.

Yeah, the joys of juggling multiple CLs.

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector_unittest.cc:79:
manifest_icon_selector_.reset(NULL);
On 2015/01/30 22:32:13, Mounir Lamouri wrote:
> Feel free to s/NULL/nullptr/ in this file.

Done.

https://chromiumcodereview.appspot.com/880203004/diff/100001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector_unittest.cc:95: return 48;
On 2015/01/30 22:32:13, Mounir Lamouri wrote:
> It might be better to use a const here even if you declare it in the test
class.

Done.

[gone, 2015-02-03 00:21:08]

Ping.

[mlamouri (slow - plz ping), 2015-02-03 17:14:14]

lgtm with comments addressed.

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
File chrome/browser/android/manifest_icon_selector.h (right):

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector.h:26: // Returns the icon url if a
suitable icon is found. An empty URL otherwise.
Could you point that the preferred size is in dp, not in px. It will hopefully
reduce the chances of someone passing the wrong value there.

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector.h:30: content::WebContents*
web_contents);
It seems unfortunate to pass a WebContents when we actually want to know the
density. I understand that we usually know about the WebContents and it allows
that code to do the logic but maybe we can make it pass a screen instead?
Reducing the dependency would be good. Typically, it would allow testing it
without a web context.

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector.h:48: int
preferred_icon_size_in_px);
If you want to pass |preferred_icon_size_in_px| to all the private static
method, you should move them to the .cc file and make them anonymous. Otherwise,
make |preferred_icon_size_in_px| a member of the class and make those method not
static, you can create an instance of the class in |FindBestMatchingIcon|.

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
File chrome/browser/android/manifest_icon_selector_unittest.cc (right):

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector_unittest.cc:66: class
ManifestIconSelectorTest : public ChromeRenderViewHostTestHarness  {
If you remove the WebContents dependency on the helper class, I think you can
make this a regular unit test.

[gone, 2015-02-03 19:25:06]

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
File chrome/browser/android/manifest_icon_selector.h (right):

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector.h:26: // Returns the icon url if a
suitable icon is found. An empty URL otherwise.
On 2015/02/03 17:14:14, Mounir Lamouri wrote:
> Could you point that the preferred size is in dp, not in px. It will hopefully
> reduce the chances of someone passing the wrong value there.

Done.

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector.h:30: content::WebContents*
web_contents);
On 2015/02/03 17:14:14, Mounir Lamouri wrote:
> It seems unfortunate to pass a WebContents when we actually want to know the
> density. I understand that we usually know about the WebContents and it allows
> that code to do the logic but maybe we can make it pass a screen instead?
> Reducing the dependency would be good. Typically, it would allow testing it
> without a web context.

Passing in just the screen now.

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector.h:48: int
preferred_icon_size_in_px);
On 2015/02/03 17:14:14, Mounir Lamouri wrote:
> If you want to pass |preferred_icon_size_in_px| to all the private static
> method, you should move them to the .cc file and make them anonymous.
Otherwise,
> make |preferred_icon_size_in_px| a member of the class and make those method
not
> static, you can create an instance of the class in |FindBestMatchingIcon|.

Done.

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
File chrome/browser/android/manifest_icon_selector_unittest.cc (right):

https://chromiumcodereview.appspot.com/880203004/diff/140001/chrome/browser/a...
chrome/browser/android/manifest_icon_selector_unittest.cc:66: class
ManifestIconSelectorTest : public ChromeRenderViewHostTestHarness  {
On 2015/02/03 17:14:14, Mounir Lamouri wrote:
> If you remove the WebContents dependency on the helper class, I think you can
> make this a regular unit test.

Done.

[gone, 2015-02-03 19:25:40]

dfalcantara@chromium.org changed reviewers:
+ newt@chromium.org

[gone, 2015-02-03 19:25:41]

newt@ for OWNERs

[newt (away), 2015-02-03 19:44:27]

lgtm

[gone, 2015-02-03 19:55:07]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2015-02-03 19:56:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/880203004/180001

[commit-bot: I haz the power, 2015-02-03 20:13:00]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-03 20:15:10]

Patchset 10 (id:??) landed as
https://crrev.com/44329fdcf31d412ee6a202a5e287acd9d41f17f9
Cr-Commit-Position: refs/heads/master@{#314394}