webapps: allow callers of icon downloader/selector to specify a minimum size

webapps: allow callers of icon downloader/selector to specify a minimum size

Since we now want to allow splash screen images down to 48dp but still want
to keep the one density bucket requirement below for home screen icons, the
callers should be able to specify the minimum icon size that they want
selected/downloaded.

Since it was going to be a ton of boilerplate to add this in the same way as
ideal_splash_icon_size_in_dp was added, also refactor the way icon sizes are
stored to reduce the boiler plate.

As a plus, AppBanner on desktop also gets minimum icon sizes correctly now
which was present as a TODO.

BUG=508627
Committed: https://crrev.com/45a03c70176668cceedede5bd203b4c80b7c95cc
Cr-Commit-Position: refs/heads/master@{#349106}

[Lalit Maganti, 2015-09-03 16:57:14]

lalitm@google.com changed reviewers:
+ dfalcantara@chromium.org, mlamouri@chromium.org

[Lalit Maganti, 2015-09-03 16:57:14]

Mounir and Dan: just FYI - no need to review until icon downloading for splash
screen lands.

[Lalit Maganti, 2015-09-03 22:03:46]

Patchset #4 (id:60001) has been deleted

[Lalit Maganti, 2015-09-08 20:04:45]

Dan: rebased this on master.

[gone, 2015-09-08 21:01:58]

https://chromiumcodereview.appspot.com/1308533006/diff/140001/chrome/android/...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://chromiumcodereview.appspot.com/1308533006/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:340:
float idealIconSizeInDp = idealIconSizeInPx / density;
does it make sense to reuse part of getIdealSizeFromResource()?

https://chromiumcodereview.appspot.com/1308533006/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:356:
private static int getIdealSizeFromResource(Context context, int resource) {
say that this returns in Dp somewhere.

https://chromiumcodereview.appspot.com/1308533006/diff/140001/chrome/browser/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://chromiumcodereview.appspot.com/1308533006/diff/140001/chrome/browser/...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
Doing it like this means that doing 4 JNI calls when you could have just passed
them in from the Java side.  That's terrible from a performance standpoint.  Do
these values ever change?  Does it make sense to initialize them once from Java,
then send and cache them on the native side?

[gone, 2015-09-08 21:02:47]

https://chromiumcodereview.appspot.com/1308533006/diff/140001/chrome/browser/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://chromiumcodereview.appspot.com/1308533006/diff/140001/chrome/browser/...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
On 2015/09/08 21:01:57, dfalcantara wrote:
> Doing it like this means that doing 4 JNI calls when you could have just
passed
> them in from the Java side.  That's terrible from a performance standpoint. 
Do
> these values ever change?  Does it make sense to initialize them once from
Java,
> then send and cache them on the native side?

Cache as native-side statics, I mean.

[mlamouri (slow - plz ping), 2015-09-10 11:19:18]

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

[mlamouri (slow - plz ping), 2015-09-10 11:19:19]

+dominickn@ for App Banner Desktop.

[dominickn, 2015-09-11 00:29:59]

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:30:
minimum_splash_image_size_in_dp_(minimum_splash_image_size_in_dp) {
Nit: reorder the parameters so that the base class constructor arguments come
first, and then the additional splash screen arguments.

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/banners...
File chrome/browser/banners/app_banner_data_fetcher.h (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher.h:175: const
base::WeakPtr<Delegate> weak_delegate_;
Nit: reorder these to put the int members after the weak delegate so the order
of arguments in the constructor matches the order of members in the class.

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_desktop.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_desktop.cc:14: // user experience.
Nit: remove this TODO as this CL addresses it. Alternatively, change the TODO to
be "identify the best minimum icon size to guarantee the best user experience",
as it is now being enforced.

[Lalit Maganti, 2015-09-11 13:06:01]

Between the changes because of the suggestions and the rebase I had to do on top
of the rounding fix, this probably needs a full re-review :/ Sorry about that.

https://codereview.chromium.org/1308533006/diff/140001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:340:
float idealIconSizeInDp = idealIconSizeInPx / density;
On 2015/09/08 21:01:57, dfalcantara wrote:
> does it make sense to reuse part of getIdealSizeFromResource()?

It would have but not any more since I want to keep the unrounded value for as
long as possible.

https://codereview.chromium.org/1308533006/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:356:
private static int getIdealSizeFromResource(Context context, int resource) {
On 2015/09/08 21:01:57, dfalcantara wrote:
> say that this returns in Dp somewhere.

Added to function name.

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:30:
minimum_splash_image_size_in_dp_(minimum_splash_image_size_in_dp) {
On 2015/09/11 00:29:58, dominickn wrote:
> Nit: reorder the parameters so that the base class constructor arguments come
> first, and then the additional splash screen arguments.

Done.

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
On 2015/09/08 21:02:47, dfalcantara wrote:
> On 2015/09/08 21:01:57, dfalcantara wrote:
> > Doing it like this means that doing 4 JNI calls when you could have just
> passed
> > them in from the Java side.  That's terrible from a performance standpoint. 
> Do
> > these values ever change?  Does it make sense to initialize them once from
> Java,
> > then send and cache them on the native side?
> 
> Cache as native-side statics, I mean.

The problem would be where this initialization would be done. There is no sane
place for that to happen. Because of that, I've switched back to storing the
variables in Android so we don't do JNI calls.

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/banners...
File chrome/browser/banners/app_banner_data_fetcher.h (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher.h:175: const
base::WeakPtr<Delegate> weak_delegate_;
On 2015/09/11 00:29:58, dominickn wrote:
> Nit: reorder these to put the int members after the weak delegate so the order
> of arguments in the constructor matches the order of members in the class.

Done.

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_desktop.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_desktop.cc:14: // user experience.
On 2015/09/11 00:29:58, dominickn wrote:
> Nit: remove this TODO as this CL addresses it. Alternatively, change the TODO
to
> be "identify the best minimum icon size to guarantee the best user
experience",
> as it is now being enforced.

Done.

[gone, 2015-09-11 16:48:48]

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
On 2015/09/11 13:06:00, Lalit Maganti wrote:
> On 2015/09/08 21:02:47, dfalcantara wrote:
> > On 2015/09/08 21:01:57, dfalcantara wrote:
> > > Doing it like this means that doing 4 JNI calls when you could have just
> > passed
> > > them in from the Java side.  That's terrible from a performance
standpoint. 
> > Do
> > > these values ever change?  Does it make sense to initialize them once from
> > Java,
> > > then send and cache them on the native side?
> > 
> > Cache as native-side statics, I mean.
> 
> The problem would be where this initialization would be done. There is no sane
> place for that to happen. Because of that, I've switched back to storing the
> variables in Android so we don't do JNI calls.

You should really turn down the hyperbole and investigate potential options,
instead.  What's stopping you, for example, from checking if the static
variables I suggested have been initialized at all from C++, making a single JNI
call to tell Java code that it needs to be initialized, then making a Java->C++
call with the four values?

[Lalit Maganti, 2015-09-11 17:09:09]

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
On 2015/09/11 16:48:48, dfalcantara wrote:
> On 2015/09/11 13:06:00, Lalit Maganti wrote:
> > On 2015/09/08 21:02:47, dfalcantara wrote:
> > > On 2015/09/08 21:01:57, dfalcantara wrote:
> > > > Doing it like this means that doing 4 JNI calls when you could have just
> > > passed
> > > > them in from the Java side.  That's terrible from a performance
> standpoint. 
> > > Do
> > > > these values ever change?  Does it make sense to initialize them once
from
> > > Java,
> > > > then send and cache them on the native side?
> > > 
> > > Cache as native-side statics, I mean.
> > 
> > The problem would be where this initialization would be done. There is no
sane
> > place for that to happen. Because of that, I've switched back to storing the
> > variables in Android so we don't do JNI calls.
> 
> You should really turn down the hyperbole and investigate potential options,
> instead.  What's stopping you, for example, from checking if the static
> variables I suggested have been initialized at all from C++, making a single
JNI
> call to tell Java code that it needs to be initialized, then making a
Java->C++
> call with the four values?

I did consider that but I really didn't like the boilerplate of that approach -
it seems to be more boilerplaty than simply passing the variables in when the
C++ objects are fist created.

[gone, 2015-09-11 17:17:54]

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
On 2015/09/11 17:09:09, Lalit Maganti wrote:
> On 2015/09/11 16:48:48, dfalcantara wrote:
> > On 2015/09/11 13:06:00, Lalit Maganti wrote:
> > > On 2015/09/08 21:02:47, dfalcantara wrote:
> > > > On 2015/09/08 21:01:57, dfalcantara wrote:
> > > > > Doing it like this means that doing 4 JNI calls when you could have
just
> > > > passed
> > > > > them in from the Java side.  That's terrible from a performance
> > standpoint. 
> > > > Do
> > > > > these values ever change?  Does it make sense to initialize them once
> from
> > > > Java,
> > > > > then send and cache them on the native side?
> > > > 
> > > > Cache as native-side statics, I mean.
> > > 
> > > The problem would be where this initialization would be done. There is no
> sane
> > > place for that to happen. Because of that, I've switched back to storing
the
> > > variables in Android so we don't do JNI calls.
> > 
> > You should really turn down the hyperbole and investigate potential options,
> > instead.  What's stopping you, for example, from checking if the static
> > variables I suggested have been initialized at all from C++, making a single
> JNI
> > call to tell Java code that it needs to be initialized, then making a
> Java->C++
> > call with the four values?
> 
> I did consider that but I really didn't like the boilerplate of that approach
-
> it seems to be more boilerplaty than simply passing the variables in when the
> C++ objects are fist created.

How about caching the values when the first calls are to
ShortcutHelper::Get*SizeInDp()?

[Lalit Maganti, 2015-09-11 17:26:03]

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
On 2015/09/11 17:17:54, dfalcantara wrote:
> On 2015/09/11 17:09:09, Lalit Maganti wrote:
> > On 2015/09/11 16:48:48, dfalcantara wrote:
> > > On 2015/09/11 13:06:00, Lalit Maganti wrote:
> > > > On 2015/09/08 21:02:47, dfalcantara wrote:
> > > > > On 2015/09/08 21:01:57, dfalcantara wrote:
> > > > > > Doing it like this means that doing 4 JNI calls when you could have
> just
> > > > > passed
> > > > > > them in from the Java side.  That's terrible from a performance
> > > standpoint. 
> > > > > Do
> > > > > > these values ever change?  Does it make sense to initialize them
once
> > from
> > > > > Java,
> > > > > > then send and cache them on the native side?
> > > > > 
> > > > > Cache as native-side statics, I mean.
> > > > 
> > > > The problem would be where this initialization would be done. There is
no
> > sane
> > > > place for that to happen. Because of that, I've switched back to storing
> the
> > > > variables in Android so we don't do JNI calls.
> > > 
> > > You should really turn down the hyperbole and investigate potential
options,
> > > instead.  What's stopping you, for example, from checking if the static
> > > variables I suggested have been initialized at all from C++, making a
single
> > JNI
> > > call to tell Java code that it needs to be initialized, then making a
> > Java->C++
> > > call with the four values?
> > 
> > I did consider that but I really didn't like the boilerplate of that
approach
> -
> > it seems to be more boilerplaty than simply passing the variables in when
the
> > C++ objects are fist created.
> 
> How about caching the values when the first calls are to
> ShortcutHelper::Get*SizeInDp()?

That is another possibility yes but would still have the performance penalty of
4 JNI calls on the first try. If you think that's acceptable, I would be happy
enough to proceed with that. Another option is we have a single method which
retrieves all 4 and caches it. Would involve the creation of a Java object and
passing it to C++ but would save 4 JNI calls. We could even cache that for
greater efficiency.

[gone, 2015-09-11 17:33:03]

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
On 2015/09/11 17:26:03, Lalit Maganti wrote:
> On 2015/09/11 17:17:54, dfalcantara wrote:
> > On 2015/09/11 17:09:09, Lalit Maganti wrote:
> > > On 2015/09/11 16:48:48, dfalcantara wrote:
> > > > On 2015/09/11 13:06:00, Lalit Maganti wrote:
> > > > > On 2015/09/08 21:02:47, dfalcantara wrote:
> > > > > > On 2015/09/08 21:01:57, dfalcantara wrote:
> > > > > > > Doing it like this means that doing 4 JNI calls when you could
have
> > just
> > > > > > passed
> > > > > > > them in from the Java side.  That's terrible from a performance
> > > > standpoint. 
> > > > > > Do
> > > > > > > these values ever change?  Does it make sense to initialize them
> once
> > > from
> > > > > > Java,
> > > > > > > then send and cache them on the native side?
> > > > > > 
> > > > > > Cache as native-side statics, I mean.
> > > > > 
> > > > > The problem would be where this initialization would be done. There is
> no
> > > sane
> > > > > place for that to happen. Because of that, I've switched back to
storing
> > the
> > > > > variables in Android so we don't do JNI calls.
> > > > 
> > > > You should really turn down the hyperbole and investigate potential
> options,
> > > > instead.  What's stopping you, for example, from checking if the static
> > > > variables I suggested have been initialized at all from C++, making a
> single
> > > JNI
> > > > call to tell Java code that it needs to be initialized, then making a
> > > Java->C++
> > > > call with the four values?
> > > 
> > > I did consider that but I really didn't like the boilerplate of that
> approach
> > -
> > > it seems to be more boilerplaty than simply passing the variables in when
> the
> > > C++ objects are fist created.
> > 
> > How about caching the values when the first calls are to
> > ShortcutHelper::Get*SizeInDp()?
> 
> That is another possibility yes but would still have the performance penalty
of
> 4 JNI calls on the first try. If you think that's acceptable, I would be happy
> enough to proceed with that. Another option is we have a single method which
> retrieves all 4 and caches it. Would involve the creation of a Java object and
> passing it to C++ but would save 4 JNI calls. We could even cache that for
> greater efficiency.

A single 4 JNI call hit where you cache the values is not as good as the 2 JNI
calls I was suggesting, but I'd still find it acceptable over making these four
JNI calls every single time the user navigates to a new page (which triggers the
creation of a new AppBannerDataFetcher).

I'm not sure why you need to create a Java object, though.  You can make calls
to static Java methods without creating objects; look at
AwContents::DocumentHasImagesCallback() and
AwContents#onDocumentHasImagesResponse().

[Lalit Maganti, 2015-09-11 17:38:49]

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
On 2015/09/11 17:33:03, dfalcantara wrote:
> On 2015/09/11 17:26:03, Lalit Maganti wrote:
> > On 2015/09/11 17:17:54, dfalcantara wrote:
> > > On 2015/09/11 17:09:09, Lalit Maganti wrote:
> > > > On 2015/09/11 16:48:48, dfalcantara wrote:
> > > > > On 2015/09/11 13:06:00, Lalit Maganti wrote:
> > > > > > On 2015/09/08 21:02:47, dfalcantara wrote:
> > > > > > > On 2015/09/08 21:01:57, dfalcantara wrote:
> > > > > > > > Doing it like this means that doing 4 JNI calls when you could
> have
> > > just
> > > > > > > passed
> > > > > > > > them in from the Java side.  That's terrible from a performance
> > > > > standpoint. 
> > > > > > > Do
> > > > > > > > these values ever change?  Does it make sense to initialize them
> > once
> > > > from
> > > > > > > Java,
> > > > > > > > then send and cache them on the native side?
> > > > > > > 
> > > > > > > Cache as native-side statics, I mean.
> > > > > > 
> > > > > > The problem would be where this initialization would be done. There
is
> > no
> > > > sane
> > > > > > place for that to happen. Because of that, I've switched back to
> storing
> > > the
> > > > > > variables in Android so we don't do JNI calls.
> > > > > 
> > > > > You should really turn down the hyperbole and investigate potential
> > options,
> > > > > instead.  What's stopping you, for example, from checking if the
static
> > > > > variables I suggested have been initialized at all from C++, making a
> > single
> > > > JNI
> > > > > call to tell Java code that it needs to be initialized, then making a
> > > > Java->C++
> > > > > call with the four values?
> > > > 
> > > > I did consider that but I really didn't like the boilerplate of that
> > approach
> > > -
> > > > it seems to be more boilerplaty than simply passing the variables in
when
> > the
> > > > C++ objects are fist created.
> > > 
> > > How about caching the values when the first calls are to
> > > ShortcutHelper::Get*SizeInDp()?
> > 
> > That is another possibility yes but would still have the performance penalty
> of
> > 4 JNI calls on the first try. If you think that's acceptable, I would be
happy
> > enough to proceed with that. Another option is we have a single method which
> > retrieves all 4 and caches it. Would involve the creation of a Java object
and
> > passing it to C++ but would save 4 JNI calls. We could even cache that for
> > greater efficiency.
> 
> A single 4 JNI call hit where you cache the values is not as good as the 2 JNI
> calls I was suggesting, but I'd still find it acceptable over making these
four
> JNI calls every single time the user navigates to a new page (which triggers
the
> creation of a new AppBannerDataFetcher).
> 
> I'm not sure why you need to create a Java object, though.  You can make calls
> to static Java methods without creating objects; look at
> AwContents::DocumentHasImagesCallback() and
> AwContents#onDocumentHasImagesResponse().

No I don't mean create an object to call it. I meant to do 1 JNI call where the
Java side returned an object containing all 4 ints. This could even be an array
I guess (although that would be ugly and have questions about which position
holds which value)

Ahhhhh now I understand why you were so concerned about the performance
implication of this - this happens on every page. Me and Mounir were unsure as
we thought that this wasn't a performance sensitive codepath but if it happens
every page load we definitely don't want too many JNI calls.

In summary, I think either caching with 1 JNI call retrieving all 4 ints at the
same time or the caching of 4 JNI calls is fine by me. Your call which one to
pick.

[gone, 2015-09-11 17:42:10]

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
On 2015/09/11 17:38:49, Lalit Maganti wrote:
> On 2015/09/11 17:33:03, dfalcantara wrote:
> > On 2015/09/11 17:26:03, Lalit Maganti wrote:
> > > On 2015/09/11 17:17:54, dfalcantara wrote:
> > > > On 2015/09/11 17:09:09, Lalit Maganti wrote:
> > > > > On 2015/09/11 16:48:48, dfalcantara wrote:
> > > > > > On 2015/09/11 13:06:00, Lalit Maganti wrote:
> > > > > > > On 2015/09/08 21:02:47, dfalcantara wrote:
> > > > > > > > On 2015/09/08 21:01:57, dfalcantara wrote:
> > > > > > > > > Doing it like this means that doing 4 JNI calls when you could
> > have
> > > > just
> > > > > > > > passed
> > > > > > > > > them in from the Java side.  That's terrible from a
performance
> > > > > > standpoint. 
> > > > > > > > Do
> > > > > > > > > these values ever change?  Does it make sense to initialize
them
> > > once
> > > > > from
> > > > > > > > Java,
> > > > > > > > > then send and cache them on the native side?
> > > > > > > > 
> > > > > > > > Cache as native-side statics, I mean.
> > > > > > > 
> > > > > > > The problem would be where this initialization would be done.
There
> is
> > > no
> > > > > sane
> > > > > > > place for that to happen. Because of that, I've switched back to
> > storing
> > > > the
> > > > > > > variables in Android so we don't do JNI calls.
> > > > > > 
> > > > > > You should really turn down the hyperbole and investigate potential
> > > options,
> > > > > > instead.  What's stopping you, for example, from checking if the
> static
> > > > > > variables I suggested have been initialized at all from C++, making
a
> > > single
> > > > > JNI
> > > > > > call to tell Java code that it needs to be initialized, then making
a
> > > > > Java->C++
> > > > > > call with the four values?
> > > > > 
> > > > > I did consider that but I really didn't like the boilerplate of that
> > > approach
> > > > -
> > > > > it seems to be more boilerplaty than simply passing the variables in
> when
> > > the
> > > > > C++ objects are fist created.
> > > > 
> > > > How about caching the values when the first calls are to
> > > > ShortcutHelper::Get*SizeInDp()?
> > > 
> > > That is another possibility yes but would still have the performance
penalty
> > of
> > > 4 JNI calls on the first try. If you think that's acceptable, I would be
> happy
> > > enough to proceed with that. Another option is we have a single method
which
> > > retrieves all 4 and caches it. Would involve the creation of a Java object
> and
> > > passing it to C++ but would save 4 JNI calls. We could even cache that for
> > > greater efficiency.
> > 
> > A single 4 JNI call hit where you cache the values is not as good as the 2
JNI
> > calls I was suggesting, but I'd still find it acceptable over making these
> four
> > JNI calls every single time the user navigates to a new page (which triggers
> the
> > creation of a new AppBannerDataFetcher).
> > 
> > I'm not sure why you need to create a Java object, though.  You can make
calls
> > to static Java methods without creating objects; look at
> > AwContents::DocumentHasImagesCallback() and
> > AwContents#onDocumentHasImagesResponse().
> 
> No I don't mean create an object to call it. I meant to do 1 JNI call where
the
> Java side returned an object containing all 4 ints. This could even be an
array
> I guess (although that would be ugly and have questions about which position
> holds which value)
> 
> Ahhhhh now I understand why you were so concerned about the performance
> implication of this - this happens on every page. Me and Mounir were unsure as
> we thought that this wasn't a performance sensitive codepath but if it happens
> every page load we definitely don't want too many JNI calls.
> 
> In summary, I think either caching with 1 JNI call retrieving all 4 ints at
the
> same time or the caching of 4 JNI calls is fine by me. Your call which one to
> pick.

Sorry :/  Should have explained why this was less than ideal more clearly.

If you can find a way to return a four int array, go for it.  Otherwise, the
one-time 4 JNI call hit is fine by me.

I get the feeling that the array path should be possible, but I don't know how
to do it off the top of my head.

[dominickn, 2015-09-14 00:07:10]

AppBannerDesktop lgtm with nit

https://codereview.chromium.org/1308533006/diff/200001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_desktop.cc (right):

https://codereview.chromium.org/1308533006/diff/200001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_desktop.cc:13: // TODO(dominickn)
Ientify the best minimum icon size to guarantee the best
Nit: "Identify"

[Lalit Maganti, 2015-09-14 11:35:19]

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/1308533006/diff/140001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:152:
ShortcutHelper::GetMinimumIconSizeInDp());
On 2015/09/11 17:42:10, dfalcantara wrote:
> On 2015/09/11 17:38:49, Lalit Maganti wrote:
> > On 2015/09/11 17:33:03, dfalcantara wrote:
> > > On 2015/09/11 17:26:03, Lalit Maganti wrote:
> > > > On 2015/09/11 17:17:54, dfalcantara wrote:
> > > > > On 2015/09/11 17:09:09, Lalit Maganti wrote:
> > > > > > On 2015/09/11 16:48:48, dfalcantara wrote:
> > > > > > > On 2015/09/11 13:06:00, Lalit Maganti wrote:
> > > > > > > > On 2015/09/08 21:02:47, dfalcantara wrote:
> > > > > > > > > On 2015/09/08 21:01:57, dfalcantara wrote:
> > > > > > > > > > Doing it like this means that doing 4 JNI calls when you
could
> > > have
> > > > > just
> > > > > > > > > passed
> > > > > > > > > > them in from the Java side.  That's terrible from a
> performance
> > > > > > > standpoint. 
> > > > > > > > > Do
> > > > > > > > > > these values ever change?  Does it make sense to initialize
> them
> > > > once
> > > > > > from
> > > > > > > > > Java,
> > > > > > > > > > then send and cache them on the native side?
> > > > > > > > > 
> > > > > > > > > Cache as native-side statics, I mean.
> > > > > > > > 
> > > > > > > > The problem would be where this initialization would be done.
> There
> > is
> > > > no
> > > > > > sane
> > > > > > > > place for that to happen. Because of that, I've switched back to
> > > storing
> > > > > the
> > > > > > > > variables in Android so we don't do JNI calls.
> > > > > > > 
> > > > > > > You should really turn down the hyperbole and investigate
potential
> > > > options,
> > > > > > > instead.  What's stopping you, for example, from checking if the
> > static
> > > > > > > variables I suggested have been initialized at all from C++,
making
> a
> > > > single
> > > > > > JNI
> > > > > > > call to tell Java code that it needs to be initialized, then
making
> a
> > > > > > Java->C++
> > > > > > > call with the four values?
> > > > > > 
> > > > > > I did consider that but I really didn't like the boilerplate of that
> > > > approach
> > > > > -
> > > > > > it seems to be more boilerplaty than simply passing the variables in
> > when
> > > > the
> > > > > > C++ objects are fist created.
> > > > > 
> > > > > How about caching the values when the first calls are to
> > > > > ShortcutHelper::Get*SizeInDp()?
> > > > 
> > > > That is another possibility yes but would still have the performance
> penalty
> > > of
> > > > 4 JNI calls on the first try. If you think that's acceptable, I would be
> > happy
> > > > enough to proceed with that. Another option is we have a single method
> which
> > > > retrieves all 4 and caches it. Would involve the creation of a Java
object
> > and
> > > > passing it to C++ but would save 4 JNI calls. We could even cache that
for
> > > > greater efficiency.
> > > 
> > > A single 4 JNI call hit where you cache the values is not as good as the 2
> JNI
> > > calls I was suggesting, but I'd still find it acceptable over making these
> > four
> > > JNI calls every single time the user navigates to a new page (which
triggers
> > the
> > > creation of a new AppBannerDataFetcher).
> > > 
> > > I'm not sure why you need to create a Java object, though.  You can make
> calls
> > > to static Java methods without creating objects; look at
> > > AwContents::DocumentHasImagesCallback() and
> > > AwContents#onDocumentHasImagesResponse().
> > 
> > No I don't mean create an object to call it. I meant to do 1 JNI call where
> the
> > Java side returned an object containing all 4 ints. This could even be an
> array
> > I guess (although that would be ugly and have questions about which position
> > holds which value)
> > 
> > Ahhhhh now I understand why you were so concerned about the performance
> > implication of this - this happens on every page. Me and Mounir were unsure
as
> > we thought that this wasn't a performance sensitive codepath but if it
happens
> > every page load we definitely don't want too many JNI calls.
> > 
> > In summary, I think either caching with 1 JNI call retrieving all 4 ints at
> the
> > same time or the caching of 4 JNI calls is fine by me. Your call which one
to
> > pick.
> 
> Sorry :/  Should have explained why this was less than ideal more clearly.
> 
> If you can find a way to return a four int array, go for it.  Otherwise, the
> one-time 4 JNI call hit is fine by me.
> 
> I get the feeling that the array path should be possible, but I don't know how
> to do it off the top of my head.

Implemented the array solution in latest PS.

https://codereview.chromium.org/1308533006/diff/200001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_desktop.cc (right):

https://codereview.chromium.org/1308533006/diff/200001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_desktop.cc:13: // TODO(dominickn)
Ientify the best minimum icon size to guarantee the best
On 2015/09/14 00:07:09, dominickn wrote:
> Nit: "Identify"

Done.

[gone, 2015-09-14 23:03:32]

https://chromiumcodereview.appspot.com/1308533006/diff/220001/chrome/android/...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://chromiumcodereview.appspot.com/1308533006/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:356:
static int[] getIconAndSplashImageSizes(Context context) {
private static int[] getIconAndSplashImageSizes(etc)

JNI is weird.

https://chromiumcodereview.appspot.com/1308533006/diff/220001/chrome/browser/...
File chrome/browser/android/shortcut_helper.cc (right):

https://chromiumcodereview.appspot.com/1308533006/diff/220001/chrome/browser/...
chrome/browser/android/shortcut_helper.cc:30: // Retrieves and caches the ideal
and minimum sizes of the home screen icon
"launcher" or "Home screen"

https://chromiumcodereview.appspot.com/1308533006/diff/220001/chrome/browser/...
chrome/browser/android/shortcut_helper.cc:41: // This ordering must be kept up
to date with the Java ShortcutHelper.
Assert that the array size is correct to loosely enforce this requirement.

https://chromiumcodereview.appspot.com/1308533006/diff/220001/chrome/browser/...
File chrome/browser/android/shortcut_helper.h (right):

https://chromiumcodereview.appspot.com/1308533006/diff/220001/chrome/browser/...
chrome/browser/android/shortcut_helper.h:31: // should be.
Doubly redundant.  Doubly doubly redundant because you copy-pasted it without
changing icon -> Image.

// Gets the ideal size for an icon displayed on the splash screen.
// Gets the minimum size for an icon displayed on the splash screen.

Fix the other ones, too.

https://chromiumcodereview.appspot.com/1308533006/diff/220001/chrome/browser/...
File chrome/browser/manifest/manifest_icon_selector.cc (right):

https://chromiumcodereview.appspot.com/1308533006/diff/220001/chrome/browser/...
chrome/browser/manifest/manifest_icon_selector.cc:172: ideal_icon_size_in_dp *
device_scale_factor;
Why did you stop rounding?

[mlamouri (slow - plz ping), 2015-09-15 12:32:12]

After a quick look, I'm really surprised to see arguments being "ideal" then
"minimum". I would expect the opposite. It might just be my taste so I will not
insist on that. Though, you should probably have a DCHECK() verifying that ideal
>= minimum.

[Lalit Maganti, 2015-09-15 16:59:04]

Mounir: I did flip back and forth between minimum then ideal and ideal then
minimum. Settled on this way because the ideal felt more "important" if it came
first. Pretty much the only reason I did that way :P

https://codereview.chromium.org/1308533006/diff/220001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1308533006/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:356:
static int[] getIconAndSplashImageSizes(Context context) {
On 2015/09/14 23:03:32, dfalcantara wrote:
> private static int[] getIconAndSplashImageSizes(etc)
> 
> JNI is weird.

Done. And agreed.

https://codereview.chromium.org/1308533006/diff/220001/chrome/browser/android...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/1308533006/diff/220001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:30: // Retrieves and caches the ideal
and minimum sizes of the home screen icon
On 2015/09/14 23:03:32, dfalcantara wrote:
> "launcher" or "Home screen"

Done.

https://codereview.chromium.org/1308533006/diff/220001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:41: // This ordering must be kept up
to date with the Java ShortcutHelper.
On 2015/09/14 23:03:32, dfalcantara wrote:
> Assert that the array size is correct to loosely enforce this requirement.

Done.

https://codereview.chromium.org/1308533006/diff/220001/chrome/browser/android...
File chrome/browser/android/shortcut_helper.h (right):

https://codereview.chromium.org/1308533006/diff/220001/chrome/browser/android...
chrome/browser/android/shortcut_helper.h:31: // should be.
On 2015/09/14 23:03:32, dfalcantara wrote:
> Doubly redundant.  Doubly doubly redundant because you copy-pasted it without
> changing icon -> Image.
> 
> // Gets the ideal size for an icon displayed on the splash screen.
> // Gets the minimum size for an icon displayed on the splash screen.
> 
> Fix the other ones, too.

Changed all of these to be consistent with the Javadocs.

https://codereview.chromium.org/1308533006/diff/220001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector.cc (right):

https://codereview.chromium.org/1308533006/diff/220001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector.cc:172: ideal_icon_size_in_dp *
device_scale_factor;
On 2015/09/14 23:03:32, dfalcantara wrote:
> Why did you stop rounding?

Tiredness when resolving merge conflicts = silly stuff like this.

Fixed the same issue in downloader as well.

[gone, 2015-09-15 18:46:07]

lgtm

https://chromiumcodereview.appspot.com/1308533006/diff/240001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java
(right):

https://chromiumcodereview.appspot.com/1308533006/diff/240001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:50:
mTab.getWebContents());
does this not fit on one line anymore?  seemed fine in patch set 7.

https://chromiumcodereview.appspot.com/1308533006/diff/240001/chrome/browser/...
File chrome/browser/android/banners/app_banner_manager_android.h (right):

https://chromiumcodereview.appspot.com/1308533006/diff/240001/chrome/browser/...
chrome/browser/android/banners/app_banner_manager_android.h:29: jobject obj);
nit: put back on one line?

[Lalit Maganti, 2015-09-15 18:48:33]

https://codereview.chromium.org/1308533006/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java
(right):

https://codereview.chromium.org/1308533006/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:50:
mTab.getWebContents());
On 2015/09/15 18:46:07, dfalcantara wrote:
> does this not fit on one line anymore?  seemed fine in patch set 7.

Done.

https://codereview.chromium.org/1308533006/diff/240001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.h (right):

https://codereview.chromium.org/1308533006/diff/240001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.h:29: jobject obj);
On 2015/09/15 18:46:07, dfalcantara wrote:
> nit: put back on one line?

Done.

[Lalit Maganti, 2015-09-15 18:48:37]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-09-15 18:48:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1308533006/#ps260001
(title: "Address review comments")

[commit-bot: I haz the power, 2015-09-15 18:49:51]

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

[commit-bot: I haz the power, 2015-09-15 21:08:36]

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

[commit-bot: I haz the power, 2015-09-15 21:08:37]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Lalit Maganti, 2015-09-16 09:10:45]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-09-16 09:10:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1308533006/#ps280001
(title: "Fix ordering of methods")

[commit-bot: I haz the power, 2015-09-16 09:11:02]

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

[commit-bot: I haz the power, 2015-09-16 09:12:54]

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

[commit-bot: I haz the power, 2015-09-16 09:12:55]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Lalit Maganti, 2015-09-16 09:19:02]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-09-16 09:19:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1308533006/#ps300001
(title: "Rebase to fix merge")

[commit-bot: I haz the power, 2015-09-16 09:19:11]

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

[commit-bot: I haz the power, 2015-09-16 09:27:27]

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

[commit-bot: I haz the power, 2015-09-16 09:27:28]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[mlamouri (slow - plz ping), 2015-09-16 09:58:29]

lgtm with comments applied

https://codereview.chromium.org/1308533006/diff/300001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:303:
public static int getIdealIconSizeInDp(Context context) {
The method name is confusing because it doesn't mention homescreen but the
comments and code does. Why not name it getIdealHomescreenIconSizeInDp() ?

https://codereview.chromium.org/1308533006/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:313:
public static int getMinimumIconSizeInDp(Context context) {
ditto

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:25: static int kIdealIconSize = -1;
nit: leave empty line before

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:37: std::vector<int> size_vector;
nit: empty line before and rename to sizes.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:48: DCHECK(size_vector.size() == 4);
Could you do this check before doing random access in 0..3 range?

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:52: } // namespace
nit: empty line before, also I prefer "anonymous namespace" but up to you

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:54: // Send a
message to the renderer to retrieve information about the page.
Could you add a DCHECK for ideal/minimum?

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/banners...
File chrome/browser/banners/app_banner_data_fetcher.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher.cc:87: }
Could you add a DCHECK for ideal/minimum?

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_downloader.cc:21: if (!web_contents ||
!icon_url.is_valid())
Please add a DCHECK() checking that ideal >= minimum.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector.cc:169: const float
device_scale_factor =
Add DCHECK() for minimum/ideal

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector.h (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector.h:37: //
|minimum_icon_size_in_dp|.
nit: I find the comment confusing. Maybe it could say that the returned icon
will be as close as possible to ideal_icon_size_in_dp and will not be smaller
than minumimu_...?

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector_unittest.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:31: GURL
FindBestMatchingIcon(const std::vector<content::Manifest::Icon>& icons,
Could you name this FindBestMatchingIconWithMinimum()?

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:341: // (very_big,
big) => big
Could you add a test where the minimum is 1 and have an icon size of 2?

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:341: // (very_big,
big) => big
Could you add a test where the minimum is 1, ideal is 200 and have an icon size
of 2?

[Lalit Maganti, 2015-09-16 11:45:26]

https://codereview.chromium.org/1308533006/diff/300001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:303:
public static int getIdealIconSizeInDp(Context context) {
On 2015/09/16 09:58:28, Mounir Lamouri wrote:
> The method name is confusing because it doesn't mention homescreen but the
> comments and code does. Why not name it getIdealHomescreenIconSizeInDp() ?

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:313:
public static int getMinimumIconSizeInDp(Context context) {
On 2015/09/16 09:58:28, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:25: static int kIdealIconSize = -1;
On 2015/09/16 09:58:28, Mounir Lamouri wrote:
> nit: leave empty line before

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:37: std::vector<int> size_vector;
On 2015/09/16 09:58:28, Mounir Lamouri wrote:
> nit: empty line before and rename to sizes.

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:48: DCHECK(size_vector.size() == 4);
On 2015/09/16 09:58:28, Mounir Lamouri wrote:
> Could you do this check before doing random access in 0..3 range?

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:52: } // namespace
On 2015/09/16 09:58:28, Mounir Lamouri wrote:
> nit: empty line before, also I prefer "anonymous namespace" but up to you

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:54: // Send a
message to the renderer to retrieve information about the page.
On 2015/09/16 09:58:28, Mounir Lamouri wrote:
> Could you add a DCHECK for ideal/minimum?

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/banners...
File chrome/browser/banners/app_banner_data_fetcher.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher.cc:87: }
On 2015/09/16 09:58:29, Mounir Lamouri wrote:
> Could you add a DCHECK for ideal/minimum?

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_downloader.cc:21: if (!web_contents ||
!icon_url.is_valid())
On 2015/09/16 09:58:29, Mounir Lamouri wrote:
> Please add a DCHECK() checking that ideal >= minimum.

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector.cc:169: const float
device_scale_factor =
On 2015/09/16 09:58:29, Mounir Lamouri wrote:
> Add DCHECK() for minimum/ideal

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector.h (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector.h:37: //
|minimum_icon_size_in_dp|.
On 2015/09/16 09:58:29, Mounir Lamouri wrote:
> nit: I find the comment confusing. Maybe it could say that the returned icon
> will be as close as possible to ideal_icon_size_in_dp and will not be smaller
> than minumimu_...?

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector_unittest.cc (right):

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:31: GURL
FindBestMatchingIcon(const std::vector<content::Manifest::Icon>& icons,
On 2015/09/16 09:58:29, Mounir Lamouri wrote:
> Could you name this FindBestMatchingIconWithMinimum()?

Done.

https://codereview.chromium.org/1308533006/diff/300001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:341: // (very_big,
big) => big
On 2015/09/16 09:58:29, Mounir Lamouri wrote:
> Could you add a test where the minimum is 1 and have an icon size of 2?

Done.

[Lalit Maganti, 2015-09-16 12:18:45]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-09-16 12:18:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, dfalcantara@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1308533006/#ps340001
(title: "Fix test failure")

[commit-bot: I haz the power, 2015-09-16 12:18:55]

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

[commit-bot: I haz the power, 2015-09-16 13:00:49]

Committed patchset #17 (id:340001)

[commit-bot: I haz the power, 2015-09-16 13:01:45]

Patchset 17 (id:??) landed as
https://crrev.com/45a03c70176668cceedede5bd203b4c80b7c95cc
Cr-Commit-Position: refs/heads/master@{#349106}

[commit-bot: I haz the power, 2015-09-23 12:54:58]

Patchset 17 (id:??) landed as
https://crrev.com/45a03c70176668cceedede5bd203b4c80b7c95cc
Cr-Commit-Position: refs/heads/master@{#349106}