webapps: initial addition of splash screen icon downloading

webapps: initial addition of splash screen icon downloading

This CL adds the downloading of the splash screen icon in
both the AppBanner and AddToHomescreenDialog code paths.

On both the AddToHomescreenDialog and AppBanner paths, the
splash screen icon is not compulsory. The task to download
the icon is sent after the user accepts the addition to
homescreen as a fire and forget task with the data storage
updated if the icon is downloaded at some point. This makes
the icon available for consumption by the splash screen.

This is a part of a group of CLs:
(1) https://codereview.chromium.org/1286973003
(2) https://codereview.chromium.org/1310223002 (this)
(3) https://codereview.chromium.org/1239923002

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

[Lalit Maganti, 2015-08-24 14:37:00]

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

[Lalit Maganti, 2015-08-24 14:37:00]

Mounir and Dan: This is still a WIP as there are still personal TODOs and other
things I want to clean up but just wanted to put this up before our meeting. 

Also we need to discuss how we should deal with failure to get the splashscreen
icon when we are about to show app banner.

[Lalit Maganti, 2015-08-24 14:37:02]

Mounir and Dan: This is still a WIP as there are still personal TODOs and other
things I want to clean up but just wanted to put this up before our meeting. 

Also we need to discuss how we should deal with failure to get the splashscreen
icon when we are about to show app banner.

[mlamouri (slow - plz ping), 2015-08-24 16:43:55]

The gist of the change sgtm. I left a few comments, see below.

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

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:71: Cancel();
see below.

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:85: Cancel();
You are making the splashscreen icon a requirement in order to show the app
banner. Is that on purpose? I don't think we should do that for now. We can
check with app banners stackholders whether they are fine with that. At least,
we should require background_color if we require a splashscreen icon.

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:92:
AppBannerDataFetcher::OnHasServiceWorker(web_contents);
Maybe you could do something like:

void AppBannerDataFetcherAndroid::OnHasServiceWorker(WebContents* web_contents)
{
  AppBannerDataFetcher::OnHasServiceWorker(web_contents);

  // The base class already cancelled the process.
  if (!is_active)
    return;

  // Find the splashscreen if any the download it and set it.
}

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

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:15: class ShortcutHelper {
Maybe make it final?

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:23: const std::string& id,
nit: id -> webapp_id

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:27: static void
AddSplashscreenIconToWebappData(const std::string& id,
ditto

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

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:174: id));
nit: "id));" doesn't fit in the previous line?

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:105:
content::Manifest manifest_;
You could probably save some bits by only keep the icons_ in there.
Alternatively, you can find the best splash icon early on so you only save that
GURL.

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right):

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:154:
data_fetcher_->FetchSplashscreenIcon(uid);
This is slightly different from what I had in mind: I thought of downloading the
splash icon when the user opens the "Add to Homescreen" menu. I guess the
behaviour you have will save some data in case of the user changes their mind
and cancel the operation.

[gone, 2015-08-24 17:15:55]

Can you start fleshing out CL descriptions before you send it out (even for
initial review)?  Makes it miles easier to understand what I'm supposed to be
comparing the code against.

[Lalit Maganti, 2015-08-24 17:19:54]

On 2015/08/24 17:15:55, dfalcantara wrote:
> Can you start fleshing out CL descriptions before you send it out (even for
> initial review)?  Makes it miles easier to understand what I'm supposed to be
> comparing the code against.

Yup will do from now on. 

Have updated this CLs description as well.

[gone, 2015-08-24 18:43:13]

Guessing this CL will need to be tweaked given the conversation earlier?

https://chromiumcodereview.appspot.com/1310223002/diff/40001/chrome/android/j...
File chrome/android/java/res/values/dimens.xml (right):

https://chromiumcodereview.appspot.com/1310223002/diff/40001/chrome/android/j...
chrome/android/java/res/values/dimens.xml:199: <!-- WebApp dimensions -->
nit: webapp or web app or Web app are all used everywhere.  Camel-casing is
right out.

[Lalit Maganti, 2015-08-25 11:38:44]

https://codereview.chromium.org/1310223002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/1310223002/diff/40001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:199: <!-- WebApp dimensions -->
On 2015/08/24 18:43:13, dfalcantara wrote:
> nit: webapp or web app or Web app are all used everywhere.  Camel-casing is
> right out.

This was one of my first edits to the Chrome codebase and I've been
stashing/stash popping it since then :P

Fixed.

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

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:71: Cancel();
On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> see below.

Acknowledged.

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:85: Cancel();
On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> You are making the splashscreen icon a requirement in order to show the app
> banner. Is that on purpose? I don't think we should do that for now. We can
> check with app banners stackholders whether they are fine with that. At least,
> we should require background_color if we require a splashscreen icon.

Changed according to our discussions.

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:92:
AppBannerDataFetcher::OnHasServiceWorker(web_contents);
On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> Maybe you could do something like:
> 
> void AppBannerDataFetcherAndroid::OnHasServiceWorker(WebContents*
web_contents)
> {
>   AppBannerDataFetcher::OnHasServiceWorker(web_contents);
> 
>   // The base class already cancelled the process.
>   if (!is_active)
>     return;
> 
>   // Find the splashscreen if any the download it and set it.
> }

This code has been redone completely.

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

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:15: class ShortcutHelper {
On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> Maybe make it final?

Done.

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:23: const std::string& id,
On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> nit: id -> webapp_id

Done.

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:27: static void
AddSplashscreenIconToWebappData(const std::string& id,
On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> ditto

Done.

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

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:174: id));
On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> nit: "id));" doesn't fit in the previous line?

Changed to webapp_id which doesn't fit :/

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:105:
content::Manifest manifest_;
On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> You could probably save some bits by only keep the icons_ in there.
> Alternatively, you can find the best splash icon early on so you only save
that
> GURL.

I like the sound of just saving the URL TBH so I've changed it to be like so.

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right):

https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:154:
data_fetcher_->FetchSplashscreenIcon(uid);
On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> This is slightly different from what I had in mind: I thought of downloading
the
> splash icon when the user opens the "Add to Homescreen" menu. I guess the
> behaviour you have will save some data in case of the user changes their mind
> and cancel the operation.

Retained this behavior as discussed.

[Lalit Maganti, 2015-08-25 11:42:19]

On 2015/08/25 11:38:44, Lalit Maganti wrote:
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/android/java/res...
> File chrome/android/java/res/values/dimens.xml (right):
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/android/java/res...
> chrome/android/java/res/values/dimens.xml:199: <!-- WebApp dimensions -->
> On 2015/08/24 18:43:13, dfalcantara wrote:
> > nit: webapp or web app or Web app are all used everywhere.  Camel-casing is
> > right out.
> 
> This was one of my first edits to the Chrome codebase and I've been
> stashing/stash popping it since then :P
> 
> Fixed.
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/banners/app_banner_data_fetcher_android.cc
(right):
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> chrome/browser/android/banners/app_banner_data_fetcher_android.cc:71:
Cancel();
> On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > see below.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> chrome/browser/android/banners/app_banner_data_fetcher_android.cc:85:
Cancel();
> On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > You are making the splashscreen icon a requirement in order to show the app
> > banner. Is that on purpose? I don't think we should do that for now. We can
> > check with app banners stackholders whether they are fine with that. At
least,
> > we should require background_color if we require a splashscreen icon.
> 
> Changed according to our discussions.
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> chrome/browser/android/banners/app_banner_data_fetcher_android.cc:92:
> AppBannerDataFetcher::OnHasServiceWorker(web_contents);
> On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > Maybe you could do something like:
> > 
> > void AppBannerDataFetcherAndroid::OnHasServiceWorker(WebContents*
> web_contents)
> > {
> >   AppBannerDataFetcher::OnHasServiceWorker(web_contents);
> > 
> >   // The base class already cancelled the process.
> >   if (!is_active)
> >     return;
> > 
> >   // Find the splashscreen if any the download it and set it.
> > }
> 
> This code has been redone completely.
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/shortcut_helper.h (right):
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> chrome/browser/android/shortcut_helper.h:15: class ShortcutHelper {
> On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > Maybe make it final?
> 
> Done.
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> chrome/browser/android/shortcut_helper.h:23: const std::string& id,
> On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > nit: id -> webapp_id
> 
> Done.
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> chrome/browser/android/shortcut_helper.h:27: static void
> AddSplashscreenIconToWebappData(const std::string& id,
> On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:174: id));
> On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > nit: "id));" doesn't fit in the previous line?
> 
> Changed to webapp_id which doesn't fit :/
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:105:
> content::Manifest manifest_;
> On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > You could probably save some bits by only keep the icons_ in there.
> > Alternatively, you can find the best splash icon early on so you only save
> that
> > GURL.
> 
> I like the sound of just saving the URL TBH so I've changed it to be like so.
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc
(right):
> 
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:154:
> data_fetcher_->FetchSplashscreenIcon(uid);
> On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > This is slightly different from what I had in mind: I thought of downloading
> the
> > splash icon when the user opens the "Add to Homescreen" menu. I guess the
> > behaviour you have will save some data in case of the user changes their
mind
> > and cancel the operation.
> 
> Retained this behavior as discussed.

Dan: you said that adding logging may be a good idea for debugging future
issues. What would you suggest using considering that it needs to be common to
both AddToHomescreenDialog and AppBanner?

[gone, 2015-08-26 01:32:03]

On 2015/08/25 11:42:19, Lalit Maganti wrote:
> On 2015/08/25 11:38:44, Lalit Maganti wrote:
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/android/java/res...
> > File chrome/android/java/res/values/dimens.xml (right):
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/android/java/res...
> > chrome/android/java/res/values/dimens.xml:199: <!-- WebApp dimensions -->
> > On 2015/08/24 18:43:13, dfalcantara wrote:
> > > nit: webapp or web app or Web app are all used everywhere.  Camel-casing
is
> > > right out.
> > 
> > This was one of my first edits to the Chrome codebase and I've been
> > stashing/stash popping it since then :P
> > 
> > Fixed.
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > File chrome/browser/android/banners/app_banner_data_fetcher_android.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/banners/app_banner_data_fetcher_android.cc:71:
> Cancel();
> > On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > > see below.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/banners/app_banner_data_fetcher_android.cc:85:
> Cancel();
> > On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > > You are making the splashscreen icon a requirement in order to show the
app
> > > banner. Is that on purpose? I don't think we should do that for now. We
can
> > > check with app banners stackholders whether they are fine with that. At
> least,
> > > we should require background_color if we require a splashscreen icon.
> > 
> > Changed according to our discussions.
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/banners/app_banner_data_fetcher_android.cc:92:
> > AppBannerDataFetcher::OnHasServiceWorker(web_contents);
> > On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > > Maybe you could do something like:
> > > 
> > > void AppBannerDataFetcherAndroid::OnHasServiceWorker(WebContents*
> > web_contents)
> > > {
> > >   AppBannerDataFetcher::OnHasServiceWorker(web_contents);
> > > 
> > >   // The base class already cancelled the process.
> > >   if (!is_active)
> > >     return;
> > > 
> > >   // Find the splashscreen if any the download it and set it.
> > > }
> > 
> > This code has been redone completely.
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > File chrome/browser/android/shortcut_helper.h (right):
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/shortcut_helper.h:15: class ShortcutHelper {
> > On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > > Maybe make it final?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/shortcut_helper.h:23: const std::string& id,
> > On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > > nit: id -> webapp_id
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/shortcut_helper.h:27: static void
> > AddSplashscreenIconToWebappData(const std::string& id,
> > On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > > ditto
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc
(right):
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:174: id));
> > On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > > nit: "id));" doesn't fit in the previous line?
> > 
> > Changed to webapp_id which doesn't fit :/
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h
(right):
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:105:
> > content::Manifest manifest_;
> > On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > > You could probably save some bits by only keep the icons_ in there.
> > > Alternatively, you can find the best splash icon early on so you only save
> > that
> > > GURL.
> > 
> > I like the sound of just saving the URL TBH so I've changed it to be like
so.
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:154:
> > data_fetcher_->FetchSplashscreenIcon(uid);
> > On 2015/08/24 16:43:54, Mounir Lamouri wrote:
> > > This is slightly different from what I had in mind: I thought of
downloading
> > the
> > > splash icon when the user opens the "Add to Homescreen" menu. I guess the
> > > behaviour you have will save some data in case of the user changes their
> mind
> > > and cancel the operation.
> > 
> > Retained this behavior as discussed.
> 
> Dan: you said that adding logging may be a good idea for debugging future
> issues. What would you suggest using considering that it needs to be common to
> both AddToHomescreenDialog and AppBanner?

Nothing about it would really be specific to either of those.  I was just
suggesting adding a logging message at the point where WebappActivity tries to
access its splash screen image.  If it's not available, output a Log.d() message
or something indicating that one isn't available at that point.  If you want to
get more specific, you could also indicate whether we failed to download it
(404) or whether it's in the process of being downloaded.

[gone, 2015-08-26 02:01:20]

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

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:169: /**
TODO what?

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:76:
int splashIconPx =
resources.getDimensionPixelSize(R.dimen.webapp_splash_icon_size);
We should consider calling these splash images instead of splash icons; it'll
get confusing with the launcher/recents icon.  If these images get defined as a
separate manifest tag, devs might want to use a different image than their
launcher icon.  I think "splash images" are the more common nomenclature,
anyway.

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:178:
private native long nativeInit(int splashIconSize, int iconSize);
splashImageDp, iconDp

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

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:52:
int splashSizePx =
resources.getDimensionPixelSize(R.dimen.webapp_splash_icon_size);
This logic is duplicated.  Is there anywhere common you can pull it out to?

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:61:
mTab.getWebContents(), splashIconDp, iconDp);
nit: indentation is wrong.

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:113:
int idealSplashIconSizeInDp, int idealIconSizeInDp);
nit: use consistent naming for these function definitions and the parameters you
pass in above.

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/a...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/a...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:67: // is
there any fallback option.
You explicitly check if the icon was downloaded in the implementation, and your
fallback is to set an empty bitmap.  Can you clarify what you mean here?

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/a...
File chrome/browser/android/banners/app_banner_data_fetcher_android.h (right):

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/a...
chrome/browser/android/banners/app_banner_data_fetcher_android.h:30: // Fetches
the splashscreen icon and stores it in the webapp's data storage.
nit: use WebappDataStorage to be explicit.

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/a...
File chrome/browser/android/shortcut_helper.h (left):

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/a...
chrome/browser/android/shortcut_helper.h:15: // from there via a JNI
(Initialize) call and MUST BE DESTROYED via Destroy().
I asked you to update this comment when your original split happened.  Looks
like you landed it without addressing any of my comments from that patch set. 
Sloppy for me not checking the difference between your patch sets, and sloppy
for you missing them.  Fun times all around.  Please address all of my comments
from there on this CL :P

https://chromiumcodereview.appspot.com/1288903002/#ps40001

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/a...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/a...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:166: // is
there any fallback option.
Same comment as above, since you just seem to have duplicated this code.  You
might as well put it in the ShortcutHelper.

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/b...
File chrome/browser/banners/app_banner_debug_log.cc (right):

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/b...
chrome/browser/banners/app_banner_debug_log.cc:22: "could not determine the best
splashscreen icon to use";
This one isn't used anywhere.

https://chromiumcodereview.appspot.com/1310223002/diff/60001/chrome/browser/b...
chrome/browser/banners/app_banner_debug_log.cc:28: const char
kNoSplashIconAvailable[] =
Neither is this.

[Lalit Maganti, 2015-08-26 13:11:56]

Dan: please take a look at my comments as there are responses to your comments
as well as pointers of other changes I've made since the patch you reviewed.

https://codereview.chromium.org/1310223002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1310223002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:169: /**
On 2015/08/26 02:01:19, dfalcantara wrote:
> TODO what?

Add javadoc which I have now done.

https://codereview.chromium.org/1310223002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:174:
private static void addSplashscreenIconToWebappData(Context context,
I've changed the name of this method to be more future-proof now that we have a
single update method rather than using the builder pattern. Also I've changed
splashIcon -> splashImage here to be more consistent.

https://codereview.chromium.org/1310223002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/1310223002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:76:
int splashIconPx =
resources.getDimensionPixelSize(R.dimen.webapp_splash_icon_size);
On 2015/08/26 02:01:19, dfalcantara wrote:
> We should consider calling these splash images instead of splash icons; it'll
> get confusing with the launcher/recents icon.  If these images get defined as
a
> separate manifest tag, devs might want to use a different image than their
> launcher icon.  I think "splash images" are the more common nomenclature,
> anyway.  

I like the name so I have globally (C++ and Java in this patch) renamed it to be
splashImage.

https://codereview.chromium.org/1310223002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:178:
private native long nativeInit(int splashIconSize, int iconSize);
On 2015/08/26 02:01:19, dfalcantara wrote:
> splashImageDp, iconDp

Added ideal as a prefix and the word "in" as well to make it consistent and
clear across C++ and Java.

https://codereview.chromium.org/1310223002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java
(right):

https://codereview.chromium.org/1310223002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:52:
int splashSizePx =
resources.getDimensionPixelSize(R.dimen.webapp_splash_icon_size);
On 2015/08/26 02:01:19, dfalcantara wrote:
> This logic is duplicated.  Is there anywhere common you can pull it out to?

The best place would probably be ShortcutHelper but I'm worried it's turning
into a bit a of a dumping ground for this code. At least the refactor I did
earlier makes that a bit more bearable I guess.

I have moved it to ShortcutHelper - if you have a better idea let me know.

https://codereview.chromium.org/1310223002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:61:
mTab.getWebContents(), splashIconDp, iconDp);
On 2015/08/26 02:01:19, dfalcantara wrote:
> nit: indentation is wrong.

Done.

https://codereview.chromium.org/1310223002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:113:
int idealSplashIconSizeInDp, int idealIconSizeInDp);
On 2015/08/26 02:01:19, dfalcantara wrote:
> nit: use consistent naming for these function definitions and the parameters
you
> pass in above.

Done.

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

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:67: // is
there any fallback option.
On 2015/08/26 02:01:19, dfalcantara wrote:
> You explicitly check if the icon was downloaded in the implementation, and
your
> fallback is to set an empty bitmap.  Can you clarify what you mean here?

Moved to shortcuthelper and clarified there.

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_data_fetcher_android.h (right):

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.h:30: // Fetches
the splashscreen icon and stores it in the webapp's data storage.
On 2015/08/26 02:01:19, dfalcantara wrote:
> nit: use WebappDataStorage to be explicit.

Done.

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_data_fetcher_android.h:47: int
ideal_splash_icon_size_;
Changed to image and added dp qualifier.

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

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:38:
ideal_splash_icon_size_(splash_icon_size),
Changed this whole file have "ideal" and "in dp"

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

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.h:30: int
splash_icon_size,
Changed to have "ideal" and "in dp"

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.h:31: int icon_size);
Changed to have "ideal" and "in dp"

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.h:73: const int
ideal_splash_icon_size_;
Changed to have "ideal" and "in dp"

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.h (left):

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:15: // from there via a JNI
(Initialize) call and MUST BE DESTROYED via Destroy().
On 2015/08/26 02:01:19, dfalcantara wrote:
> I asked you to update this comment when your original split happened.  Looks
> like you landed it without addressing any of my comments from that patch set. 
> Sloppy for me not checking the difference between your patch sets, and sloppy
> for you missing them.  Fun times all around.  Please address all of my
comments
> from there on this CL :P
> 
> https://chromiumcodereview.appspot.com/1288903002/#ps40001

Yeah I missed them all because I basically redid the CL from scratch so thought
that most of the comments from the old CL would not be relevant. Clearly that
was a mistake.

Sorry about that. Being careless with review comments is something I have
noticed is a big problem and I'm actively working on being more careful about
these things.

I will put all those cleanups in a separate CL as the would just pollute this
one.

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

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:166: // is
there any fallback option.
On 2015/08/26 02:01:19, dfalcantara wrote:
> Same comment as above, since you just seem to have duplicated this code.  You
> might as well put it in the ShortcutHelper.

Put into shortcut helper and fixed the comment.

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

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:124: virtual void
OnHasServiceWorker(content::WebContents* web_contents);
Reverted this change as it is not necessary and also made this method private.

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:142: bool
CheckFetcherIsStillAlive(content::WebContents* web_contents);
Reverted as no longer necessary.

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_debug_log.cc (right):

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.cc:22: "could not determine the best
splashscreen icon to use";
On 2015/08/26 02:01:20, dfalcantara wrote:
> This one isn't used anywhere.

Removed. Was used earlier for logging but forgot to remove declaration.

https://codereview.chromium.org/1310223002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_debug_log.cc:28: const char
kNoSplashIconAvailable[] =
On 2015/08/26 02:01:20, dfalcantara wrote:
> Neither is this.

As above.

[Lalit Maganti, 2015-08-26 13:19:31]

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

https://codereview.chromium.org/1310223002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:298:
public static String getEncodedMac(Context context, String url) {
This was moved because it was actually below the private static methods.

[Lalit Maganti, 2015-08-26 21:18:21]

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:57: int
ideal_splash_icon_size_in_dp,
Eh I missed the change from icon -> image here.

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right):

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:31: jint
ideal_splash_icon_size_in_dp,
And here.

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:45: int
ideal_splash_icon_size_in_dp,
And here.

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h (right):

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h:35: int
ideal_splash_icon_size_in_dp,
And here.

[Lalit Maganti, 2015-08-26 21:22:29]

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.h (right):

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.h:20: int
ideal_splash_icon_size_in_dp,
And here.

[gone, 2015-08-26 21:39:25]

https://chromiumcodereview.appspot.com/1310223002/diff/120001/chrome/android/...
File chrome/android/java/res/values/dimens.xml (right):

https://chromiumcodereview.appspot.com/1310223002/diff/120001/chrome/android/...
chrome/android/java/res/values/dimens.xml:163: 
nit: useless whitespace

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

https://chromiumcodereview.appspot.com/1310223002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:48: *
shortcuts (both webapps and non webapps) to homescreen.
If you're going to change this class comment anyway:

This class contains functions related to adding shortcuts to the Android Home
screen.  These shortcuts are used to either open a page in the main browser or
open a web app.

https://chromiumcodereview.appspot.com/1310223002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:172: *
Stores the webpp data inside WebappDataStorage for the specified webapp.
Can you please do a typo pass before sending any CLs out for re-review?

webpp -> webapp

https://chromiumcodereview.appspot.com/1310223002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:277: *
should have.
"ideal" and "should have" are redundant.  The comment doesn't make sense if you
read it straight through.

Maybe:
Returns the ideal size for an image displayed on a web app's splash screen.

https://chromiumcodereview.appspot.com/1310223002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:287: *
and app banners should have.
Same as above.  Doesn't make sense.

Returns the ideal size for an icon representing a web app.  This size is used on
app banners, the Android Home screen, and in Android's recent tasks list, among
other places.

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

https://chromiumcodereview.appspot.com/1310223002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:50:
mTab.getWebContents(),
Indentation continues to be wrong.  This is java code.  We indent by 8 for
parameters that don't fit on the same line.

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

https://chromiumcodereview.appspot.com/1310223002/diff/120001/chrome/browser/...
chrome/browser/android/banners/app_banner_manager_android.cc:34: JNIEnv* env,
Indentation is wrong.  You indent by 4 in C++ for long lines.

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

https://chromiumcodereview.appspot.com/1310223002/diff/120001/chrome/browser/...
chrome/browser/android/shortcut_helper.cc:68: // to be downloaded so if the
downloader returns false there is no fallback.
The fallback is still you setting the splash image to null and storing it.  I'm
not sure why you bother with storing null, BTW.  Are you planning on checking if
the splash image key exists in the SharedPreference for some reason?

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

https://chromiumcodereview.appspot.com/1310223002/diff/120001/chrome/browser/...
chrome/browser/android/shortcut_helper.h:18: // ShortcutHelper in Java. The
object is owned by the Java object. It is created
Nothing owns the native ShortcutHelper -- you don't even have a way to construct
one from Java since you split apart AddToHomescreenDialogHelper, which has the
Initialize()/nativeInitialize() calls.  This comment even references a Destroy()
function which doesn't exist anymore.

Revert back to the old class comment.

[Lalit Maganti, 2015-08-26 22:55:01]

https://codereview.chromium.org/1310223002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/1310223002/diff/120001/chrome/android/java/re...
chrome/android/java/res/values/dimens.xml:163: 
On 2015/08/26 at 21:39:25, dfalcantara wrote:
> nit: useless whitespace

I removed the removal from the previous patch because it was noise but removed
again.

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

https://codereview.chromium.org/1310223002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:48: *
shortcuts (both webapps and non webapps) to homescreen.
On 2015/08/26 at 21:39:25, dfalcantara wrote:
> If you're going to change this class comment anyway:
> 
> This class contains functions related to adding shortcuts to the Android Home
screen.  These shortcuts are used to either open a page in the main browser or
open a web app.

Done.

https://codereview.chromium.org/1310223002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:172: *
Stores the webpp data inside WebappDataStorage for the specified webapp.
On 2015/08/26 at 21:39:25, dfalcantara wrote:
> Can you please do a typo pass before sending any CLs out for re-review?
> 
> webpp -> webapp

:/

Will do.

https://codereview.chromium.org/1310223002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:277: *
should have.
On 2015/08/26 at 21:39:25, dfalcantara wrote:
> "ideal" and "should have" are redundant.  The comment doesn't make sense if
you read it straight through.
> 
> Maybe:
> Returns the ideal size for an image displayed on a web app's splash screen.

Changed as suggested.

https://codereview.chromium.org/1310223002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:287: *
and app banners should have.
On 2015/08/26 at 21:39:25, dfalcantara wrote:
> Same as above.  Doesn't make sense.
> 
> Returns the ideal size for an icon representing a web app.  This size is used
on app banners, the Android Home screen, and in Android's recent tasks list,
among other places.

Changed as suggested.

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

https://codereview.chromium.org/1310223002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:50:
mTab.getWebContents(),
On 2015/08/26 at 21:39:25, dfalcantara wrote:
> Indentation continues to be wrong.  This is java code.  We indent by 8 for
parameters that don't fit on the same line.

:/

Another thing I need to look at before sending out the CL.

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.h (right):

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.h:20: int
ideal_splash_icon_size_in_dp,
On 2015/08/26 at 21:22:29, Lalit Maganti wrote:
> And here.

Done.

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

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:34: JNIEnv* env,
On 2015/08/26 at 21:39:25, dfalcantara wrote:
> Indentation is wrong.  You indent by 4 in C++ for long lines.

Done.

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

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:68: // to be downloaded so if the
downloader returns false there is no fallback.
On 2015/08/26 at 21:39:25, dfalcantara wrote:
> The fallback is still you setting the splash image to null and storing it. 
I'm not sure why you bother with storing null, BTW.  Are you planning on
checking if the splash image key exists in the SharedPreference for some reason?

What I mean by no fallback is that I don't check the return boolean of
ManifestIconDownloader::Downloader and do something else if it is false.

I've changed StoreWebappData to bail out if the image draws nothing.

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

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/shortcut_helper.h:18: // ShortcutHelper in Java. The
object is owned by the Java object. It is created
On 2015/08/26 at 21:39:25, dfalcantara wrote:
> Nothing owns the native ShortcutHelper -- you don't even have a way to
construct one from Java since you split apart AddToHomescreenDialogHelper, which
has the Initialize()/nativeInitialize() calls.  This comment even references a
Destroy() function which doesn't exist anymore.
> 
> Revert back to the old class comment.

This is fixed by a rebase. I hadn't based this off the cleanup CL yet.

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:57: int
ideal_splash_icon_size_in_dp,
On 2015/08/26 at 21:18:21, Lalit Maganti wrote:
> Eh I missed the change from icon -> image here.

Done.

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right):

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:31: jint
ideal_splash_icon_size_in_dp,
On 2015/08/26 at 21:18:21, Lalit Maganti wrote:
> And here.

Done.

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:45: int
ideal_splash_icon_size_in_dp,
On 2015/08/26 at 21:18:21, Lalit Maganti wrote:
> And here.

Done.

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h (right):

https://codereview.chromium.org/1310223002/diff/120001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h:35: int
ideal_splash_icon_size_in_dp,
On 2015/08/26 at 21:18:21, Lalit Maganti wrote:
> And here.

Done.

[gone, 2015-08-27 18:00:29]

Implementation l-g-t-m; you need tests for everything you've added.

https://chromiumcodereview.appspot.com/1310223002/diff/220001/chrome/browser/...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://chromiumcodereview.appspot.com/1310223002/diff/220001/chrome/browser/...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:60:
ManifestIconSelector::FindBestMatchingIcon(
nit: indentation is off

You could try using 'git cl format'.  It might fix this for you automatically
but it might also do weird things.  Also doesn't work on Java.

https://chromiumcodereview.appspot.com/1310223002/diff/220001/chrome/browser/...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.h
(right):

https://chromiumcodereview.appspot.com/1310223002/diff/220001/chrome/browser/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.h:79: int
event_request_id_;
why'd this get moved up?

[Lalit Maganti, 2015-08-27 18:57:15]

By tests do you mean Java tests which also test C++ stuff like those which exist
now?

I'll need to write and land the registry patch before this one though.

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

https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:60:
ManifestIconSelector::FindBestMatchingIcon(
On 2015/08/27 at 18:00:29, dfalcantara wrote:
> nit: indentation is off
> 
> You could try using 'git cl format'.  It might fix this for you automatically
but it might also do weird things.  Also doesn't work on Java.

That's annoying. I spent a long time trying to fix all indentation issues
yesterday :/

git cl format messed up stuff in quite a few places so decided to ignore it. May
use it in future patches though.

https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.h
(right):

https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.h:79: int
event_request_id_;
On 2015/08/27 at 18:00:29, dfalcantara wrote:
> why'd this get moved up?

To make it consistent with the ordering in the constructor/initializer list.
Otherwise the compiler complains that the variables are being initialized in the
wrong order.

[gone, 2015-08-27 19:06:05]

I'm not following; you're adding the downloading of splash screen images, right?
 Are there any tests for that that exist?  Can any of the existing tests that go
through those pathways be adjusted to check that your splash screen image
download gets triggered?

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

https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:60:
ManifestIconSelector::FindBestMatchingIcon(
On 2015/08/27 18:57:14, Lalit Maganti wrote:
> On 2015/08/27 at 18:00:29, dfalcantara wrote:
> > nit: indentation is off
> > 
> > You could try using 'git cl format'.  It might fix this for you
automatically
> but it might also do weird things.  Also doesn't work on Java.
> 
> That's annoying. I spent a long time trying to fix all indentation issues
> yesterday :/
> 
> git cl format messed up stuff in quite a few places so decided to ignore it.
May
> use it in future patches though.

Yeah, I generally avoid it because it's more unpredictable than I'd like, but
figured I'd let you know about the option.

https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.h
(right):

https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.h:79: int
event_request_id_;
On 2015/08/27 18:57:14, Lalit Maganti wrote:
> On 2015/08/27 at 18:00:29, dfalcantara wrote:
> > why'd this get moved up?
> 
> To make it consistent with the ordering in the constructor/initializer list.
> Otherwise the compiler complains that the variables are being initialized in
the
> wrong order.

Rephrasing then: why did put data_fetcher_ in the order you did in the
initializer list?  That forces this to be reordered.

[Lalit Maganti, 2015-08-27 19:16:26]

On 2015/08/27 at 19:06:05, dfalcantara wrote:
> I'm not following; you're adding the downloading of splash screen images,
right?  Are there any tests for that that exist?  Can any of the existing tests
that go through those pathways be adjusted to check that your splash screen
image download gets triggered?
> 
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> File chrome/browser/android/banners/app_banner_data_fetcher_android.cc
(right):
> 
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> chrome/browser/android/banners/app_banner_data_fetcher_android.cc:60:
ManifestIconSelector::FindBestMatchingIcon(
> On 2015/08/27 18:57:14, Lalit Maganti wrote:
> > On 2015/08/27 at 18:00:29, dfalcantara wrote:
> > > nit: indentation is off
> > > 
> > > You could try using 'git cl format'.  It might fix this for you
automatically
> > but it might also do weird things.  Also doesn't work on Java.
> > 
> > That's annoying. I spent a long time trying to fix all indentation issues
> > yesterday :/
> > 
> > git cl format messed up stuff in quite a few places so decided to ignore it.
May
> > use it in future patches though.
> 
> Yeah, I generally avoid it because it's more unpredictable than I'd like, but
figured I'd let you know about the option.
> 
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> File chrome/browser/android/banners/app_banner_infobar_delegate_android.h
(right):
> 
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> chrome/browser/android/banners/app_banner_infobar_delegate_android.h:79: int
event_request_id_;
> On 2015/08/27 18:57:14, Lalit Maganti wrote:
> > On 2015/08/27 at 18:00:29, dfalcantara wrote:
> > > why'd this get moved up?
> > 
> > To make it consistent with the ordering in the constructor/initializer list.
> > Otherwise the compiler complains that the variables are being initialized in
the
> > wrong order.
> 
> Rephrasing then: why did put data_fetcher_ in the order you did in the
initializer list?  That forces this to be reordered.

Right now there are no C++ tests for any of the files I've modified. For
AddToHomescreenDialogHelper and AppBannerManager there are integration tests in
Java which test the full AddToHomescreen/AppBanner flows. Do you want me to add
tests which check that the splash screen image is also downloaded correctly in
the integration tests?

[Lalit Maganti, 2015-08-27 19:16:40]

https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.h
(right):

https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.h:79: int
event_request_id_;
On 2015/08/27 at 19:06:05, dfalcantara wrote:
> On 2015/08/27 18:57:14, Lalit Maganti wrote:
> > On 2015/08/27 at 18:00:29, dfalcantara wrote:
> > > why'd this get moved up?
> > 
> > To make it consistent with the ordering in the constructor/initializer list.
> > Otherwise the compiler complains that the variables are being initialized in
the
> > wrong order.
> 
> Rephrasing then: why did put data_fetcher_ in the order you did in the
initializer list?  That forces this to be reordered.

Hmm that's a fair point. It's just adding noise to this commit I guess so I can
switch it back.

[gone, 2015-08-27 19:50:28]

On 2015/08/27 19:16:26, Lalit Maganti wrote:
> On 2015/08/27 at 19:06:05, dfalcantara wrote:
> > I'm not following; you're adding the downloading of splash screen images,
> right?  Are there any tests for that that exist?  Can any of the existing
tests
> that go through those pathways be adjusted to check that your splash screen
> image download gets triggered?
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> > File chrome/browser/android/banners/app_banner_data_fetcher_android.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> > chrome/browser/android/banners/app_banner_data_fetcher_android.cc:60:
> ManifestIconSelector::FindBestMatchingIcon(
> > On 2015/08/27 18:57:14, Lalit Maganti wrote:
> > > On 2015/08/27 at 18:00:29, dfalcantara wrote:
> > > > nit: indentation is off
> > > > 
> > > > You could try using 'git cl format'.  It might fix this for you
> automatically
> > > but it might also do weird things.  Also doesn't work on Java.
> > > 
> > > That's annoying. I spent a long time trying to fix all indentation issues
> > > yesterday :/
> > > 
> > > git cl format messed up stuff in quite a few places so decided to ignore
it.
> May
> > > use it in future patches though.
> > 
> > Yeah, I generally avoid it because it's more unpredictable than I'd like,
but
> figured I'd let you know about the option.
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> > File chrome/browser/android/banners/app_banner_infobar_delegate_android.h
> (right):
> > 
> >
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> > chrome/browser/android/banners/app_banner_infobar_delegate_android.h:79: int
> event_request_id_;
> > On 2015/08/27 18:57:14, Lalit Maganti wrote:
> > > On 2015/08/27 at 18:00:29, dfalcantara wrote:
> > > > why'd this get moved up?
> > > 
> > > To make it consistent with the ordering in the constructor/initializer
list.
> > > Otherwise the compiler complains that the variables are being initialized
in
> the
> > > wrong order.
> > 
> > Rephrasing then: why did put data_fetcher_ in the order you did in the
> initializer list?  That forces this to be reordered.
> 
> Right now there are no C++ tests for any of the files I've modified. For
> AddToHomescreenDialogHelper and AppBannerManager there are integration tests
in
> Java which test the full AddToHomescreen/AppBanner flows. Do you want me to
add
> tests which check that the splash screen image is also downloaded correctly in
> the integration tests?

Yeah, I was suggesting changing those.  If you add an Observer to the
WebappDataObserver, for example, you can detect when Java code is told to
download the icon.

[gone, 2015-08-27 19:50:48]

On 2015/08/27 19:50:28, dfalcantara wrote:
> On 2015/08/27 19:16:26, Lalit Maganti wrote:
> > On 2015/08/27 at 19:06:05, dfalcantara wrote:
> > > I'm not following; you're adding the downloading of splash screen images,
> > right?  Are there any tests for that that exist?  Can any of the existing
> tests
> > that go through those pathways be adjusted to check that your splash screen
> > image download gets triggered?
> > > 
> > >
> >
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> > > File chrome/browser/android/banners/app_banner_data_fetcher_android.cc
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> > > chrome/browser/android/banners/app_banner_data_fetcher_android.cc:60:
> > ManifestIconSelector::FindBestMatchingIcon(
> > > On 2015/08/27 18:57:14, Lalit Maganti wrote:
> > > > On 2015/08/27 at 18:00:29, dfalcantara wrote:
> > > > > nit: indentation is off
> > > > > 
> > > > > You could try using 'git cl format'.  It might fix this for you
> > automatically
> > > > but it might also do weird things.  Also doesn't work on Java.
> > > > 
> > > > That's annoying. I spent a long time trying to fix all indentation
issues
> > > > yesterday :/
> > > > 
> > > > git cl format messed up stuff in quite a few places so decided to ignore
> it.
> > May
> > > > use it in future patches though.
> > > 
> > > Yeah, I generally avoid it because it's more unpredictable than I'd like,
> but
> > figured I'd let you know about the option.
> > > 
> > >
> >
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> > > File chrome/browser/android/banners/app_banner_infobar_delegate_android.h
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
> > > chrome/browser/android/banners/app_banner_infobar_delegate_android.h:79:
int
> > event_request_id_;
> > > On 2015/08/27 18:57:14, Lalit Maganti wrote:
> > > > On 2015/08/27 at 18:00:29, dfalcantara wrote:
> > > > > why'd this get moved up?
> > > > 
> > > > To make it consistent with the ordering in the constructor/initializer
> list.
> > > > Otherwise the compiler complains that the variables are being
initialized
> in
> > the
> > > > wrong order.
> > > 
> > > Rephrasing then: why did put data_fetcher_ in the order you did in the
> > initializer list?  That forces this to be reordered.
> > 
> > Right now there are no C++ tests for any of the files I've modified. For
> > AddToHomescreenDialogHelper and AppBannerManager there are integration tests
> in
> > Java which test the full AddToHomescreen/AppBanner flows. Do you want me to
> add
> > tests which check that the splash screen image is also downloaded correctly
in
> > the integration tests?
> 
> Yeah, I was suggesting changing those.  If you add an Observer to the
> WebappDataObserver, for example, you can detect when Java code is told to
> download the icon.

Er, WebappDataStorage.

[Lalit Maganti, 2015-08-27 19:52:12]

OK that sounds good to me. I'll add those after the registry patch.

[Lalit Maganti, 2015-08-27 19:52:12]

OK that sounds good to me. I'll add those after the registry patch.

[Lalit Maganti, 2015-09-01 15:33:08]

Added tests.

https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.h
(right):

https://codereview.chromium.org/1310223002/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.h:79: int
event_request_id_;
On 2015/08/27 19:16:40, Lalit Maganti wrote:
> On 2015/08/27 at 19:06:05, dfalcantara wrote:
> > On 2015/08/27 18:57:14, Lalit Maganti wrote:
> > > On 2015/08/27 at 18:00:29, dfalcantara wrote:
> > > > why'd this get moved up?
> > > 
> > > To make it consistent with the ordering in the constructor/initializer
list.
> > > Otherwise the compiler complains that the variables are being initialized
in
> the
> > > wrong order.
> > 
> > Rephrasing then: why did put data_fetcher_ in the order you did in the
> initializer list?  That forces this to be reordered.
> 
> Hmm that's a fair point. It's just adding noise to this commit I guess so I
can
> switch it back.

Done.

[gone, 2015-09-01 18:28:29]

The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-01 18:29:19]

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

[gone, 2015-09-01 18:33:52]

Running a dry run because I'm confused about how the test can pass.

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

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:169: *
Stores the webapp data inside WebappDataStorage for the specified webapp.
nit: Stores data for a web app inside its {@link WebappDataStorage}.

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:307: *
Returns the ideal size for an icon representing a web app.  This size is
nit: indent at 100

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:84:
sObserver.onUpdateSplashScreenImage(splashScreenImage);
Why not set sObserver to null and check if it's null here?  The standard thing
to do for observer classes (especially ones added only for unit tests) is to
make the Observer an Interface and null check the Observer.

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:125:
*/
* Don't bother saying it's for tests; it might be expanded in the future:
  "Observer used to monitor splash screen downloads."

* Make this an Interface.

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:128:
/**
nit: indentation is off.

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:436:
public void testWebAppSplashscreen() throws Exception {
testWebAppSplashImageIsDownloaded

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:441:
InfoBarContainer container =
getActivity().getActivityTab().getInfoBarContainer();
This is broken.  The InfoBarContainer listener needs to be set before the page
is loaded, which is what the removed lines involving TabLoadObserver are
supposed to be doing.  You may as well do what the whole
testWebAppBannerAppears() function used to do, and then tack on your splash
screen image download test.  If you go down that route, you need to set the
InfoBarListener right before the "Indicate a day has passed...", then put
everything after your file's line 444 after that code.

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:466:
// Test the bitmap sizes
nit: comments are sentences.  add a period

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:467:
int ideal = getActivity().getResources().getDimensionPixelSize(
nit: idealSize

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java
(right):

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java:114:
WebappDataStorage.setObserverForTests(mDataStorageObserver);
These are only used in one test.  Just move it to the test itself.

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java:190:
// Make sure that the splash screen icon was downloaded.
icon -> image

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java:199:
int ideal = mActivity.getResources().getDimensionPixelSize(
idealSize

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/test/dat...
File chrome/test/data/webapps/manifest_test_page.html (right):

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/test/dat...
chrome/test/data/webapps/manifest_test_page.html:6: <body>
You took out the service worker, right?  Update the page text.

[commit-bot: I haz the power, 2015-09-01 19:35:13]

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

[commit-bot: I haz the power, 2015-09-01 19:35:14]

Dry run: This issue passed the CQ dry run.

[Lalit Maganti, 2015-09-01 19:45:52]

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

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:169: *
Stores the webapp data inside WebappDataStorage for the specified webapp.
On 2015/09/01 18:33:51, dfalcantara wrote:
> nit: Stores data for a web app inside its {@link WebappDataStorage}.

Done.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:307: *
Returns the ideal size for an icon representing a web app.  This size is
On 2015/09/01 18:33:51, dfalcantara wrote:
> nit: indent at 100

Done.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:84:
sObserver.onUpdateSplashScreenImage(splashScreenImage);
On 2015/09/01 18:33:51, dfalcantara wrote:
> Why not set sObserver to null and check if it's null here?  The standard thing
> to do for observer classes (especially ones added only for unit tests) is to
> make the Observer an Interface and null check the Observer.

That's what I did initially when I wrote this CL but I decided to follow what
had been done in app banner. Guess I should have stuck with my intuition.

Done.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:125:
*/
On 2015/09/01 18:33:51, dfalcantara wrote:
> * Don't bother saying it's for tests; it might be expanded in the future:
>   "Observer used to monitor splash screen downloads."
> 
> * Make this an Interface.

Done.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:128:
/**
On 2015/09/01 18:33:51, dfalcantara wrote:
> nit: indentation is off.

Done.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:436:
public void testWebAppSplashscreen() throws Exception {
On 2015/09/01 18:33:51, dfalcantara wrote:
> testWebAppSplashImageIsDownloaded

Done. As well as this I've moved the observer from local to global following
what I did in AddToHomescreenDialogHelperTest.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:441:
InfoBarContainer container =
getActivity().getActivityTab().getInfoBarContainer();
On 2015/09/01 18:33:51, dfalcantara wrote:
> This is broken.  The InfoBarContainer listener needs to be set before the page
> is loaded, which is what the removed lines involving TabLoadObserver are
> supposed to be doing.  You may as well do what the whole
> testWebAppBannerAppears() function used to do, and then tack on your splash
> screen image download test.  If you go down that route, you need to set the
> InfoBarListener right before the "Indicate a day has passed...", then put
> everything after your file's line 444 after that code.

Theorized on chat that the reason this test was passing locally for me was due
to a race condition.

Gone with your suggestion.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:466:
// Test the bitmap sizes
On 2015/09/01 18:33:51, dfalcantara wrote:
> nit: comments are sentences.  add a period

Done.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:467:
int ideal = getActivity().getResources().getDimensionPixelSize(
On 2015/09/01 18:33:51, dfalcantara wrote:
> nit: idealSize

Done.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java
(right):

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java:114:
WebappDataStorage.setObserverForTests(mDataStorageObserver);
On 2015/09/01 18:33:51, dfalcantara wrote:
> These are only used in one test.  Just move it to the test itself.

Done.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java:190:
// Make sure that the splash screen icon was downloaded.
On 2015/09/01 18:33:51, dfalcantara wrote:
> icon -> image

Done.

https://codereview.chromium.org/1310223002/diff/260001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java:199:
int ideal = mActivity.getResources().getDimensionPixelSize(
On 2015/09/01 18:33:52, dfalcantara wrote:
> idealSize

Done as well as fixing the comment.

https://codereview.chromium.org/1310223002/diff/260001/chrome/test/data/webap...
File chrome/test/data/webapps/manifest_test_page.html (right):

https://codereview.chromium.org/1310223002/diff/260001/chrome/test/data/webap...
chrome/test/data/webapps/manifest_test_page.html:6: <body>
On 2015/09/01 18:33:52, dfalcantara wrote:
> You took out the service worker, right?  Update the page text.

Done.

[gone, 2015-09-01 20:50:53]

lgtm, but I'd like a pointer to the app banner code to see if it needs to be
cleaned up.

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:84:
sObserver.onUpdateSplashScreenImage(splashScreenImage);
On 2015/09/01 19:45:52, Lalit Maganti wrote:
> On 2015/09/01 18:33:51, dfalcantara wrote:
> > Why not set sObserver to null and check if it's null here?  The standard
thing
> > to do for observer classes (especially ones added only for unit tests) is to
> > make the Observer an Interface and null check the Observer.
> 
> That's what I did initially when I wrote this CL but I decided to follow what
> had been done in app banner. Guess I should have stuck with my intuition.
> 
> Done.

Where exactly does AppBanner* code set an empty observer?  Curious to see the
reasoning why it's done there and whether it would apply in this case.

[Lalit Maganti, 2015-09-01 21:11:24]

On 2015/09/01 20:50:53, dfalcantara wrote:
> lgtm, but I'd like a pointer to the app banner code to see if it needs to be
> cleaned up.
> 
>
https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
> (right):
> 
>
https://chromiumcodereview.appspot.com/1310223002/diff/260001/chrome/android/...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:84:
> sObserver.onUpdateSplashScreenImage(splashScreenImage);
> On 2015/09/01 19:45:52, Lalit Maganti wrote:
> > On 2015/09/01 18:33:51, dfalcantara wrote:
> > > Why not set sObserver to null and check if it's null here?  The standard
> thing
> > > to do for observer classes (especially ones added only for unit tests) is
to
> > > make the Observer an Interface and null check the Observer.
> > 
> > That's what I did initially when I wrote this CL but I decided to follow
what
> > had been done in app banner. Guess I should have stuck with my intuition.
> > 
> > Done.
> 
> Where exactly does AppBanner* code set an empty observer?  Curious to see the
> reasoning why it's done there and whether it would apply in this case.

Actually it's not Observers I was thinking about - it was delegates. Subtly
different semantics I guess.

There are no empty Observers so no cleanup needed :)

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

I will have a look after the registry CL is updated. This CL might require some
changes I guess.

[Lalit Maganti, 2015-09-02 13:14:05]

Updated to patch to reflect the new semantics of registry and to be based off
the registry patch.

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

Looks fine after a first pass but I really do not like the Observer being only
used for tests. Unless this is an okay thing to do on Chrome Android, I think
you should fix that.

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

https://codereview.chromium.org/1310223002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:172: *
@param id          ID of the webapp whose data is being updated.
I don't think the data is being updated.

https://codereview.chromium.org/1310223002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:330: int
splashSizePx = resources.getDimensionPixelSize(resource);
nit: "int sizeInPx = ";

https://codereview.chromium.org/1310223002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/1310223002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:144:
public static interface Observer {
You are adding this observer interface just for tests? That doesn't sound like a
good idea. Could you think of another way to run the test without the observer?

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

https://codereview.chromium.org/1310223002/diff/320001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:88:
Java_ShortcutHelper_storeWebappData(
Maybe you could rename that createWebappDataInfo?

https://codereview.chromium.org/1310223002/diff/320001/chrome/test/data/banne...
File chrome/test/data/banners/manifest.json (right):

https://codereview.chromium.org/1310223002/diff/320001/chrome/test/data/banne...
chrome/test/data/banners/manifest.json:30: "src": "splash-image.png",
nit: I really don't like these names because nothing forbids us from havin
"launcher-icon-*" to be used for the splashscreen or "splash-image.png" to be
used as a launcher icon (in a 10x device :)).

Could you just name it image-512px.png ?

https://codereview.chromium.org/1310223002/diff/320001/chrome/test/data/webap...
File chrome/test/data/webapps/manifest.json (right):

https://codereview.chromium.org/1310223002/diff/320001/chrome/test/data/webap...
chrome/test/data/webapps/manifest.json:6: "src": "splash-image.png",
ditto

[Lalit Maganti, 2015-09-02 17:15:42]

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

https://codereview.chromium.org/1310223002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:172: *
@param id          ID of the webapp whose data is being updated.
On 2015/09/02 16:56:57, Mounir Lamouri wrote:
> I don't think the data is being updated.

Changed to match what you suggest elsewhere.

https://codereview.chromium.org/1310223002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:330: int
splashSizePx = resources.getDimensionPixelSize(resource);
On 2015/09/02 16:56:57, Mounir Lamouri wrote:
> nit: "int sizeInPx = ";

Done.

https://codereview.chromium.org/1310223002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/1310223002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:144:
public static interface Observer {
On 2015/09/02 16:56:57, Mounir Lamouri wrote:
> You are adding this observer interface just for tests? That doesn't sound like
a
> good idea. Could you think of another way to run the test without the
observer?

It was on Dan's suggestion. The other way I can think of is to simply run the
tests and observe the changes in the SharedPreferences but since there are
instrumentation tests that would be quite brittle.

I think this is quite common as it is done in AppDetailsDelegate.Observer for
example. I'll add a VisibleForTesting annotation like is done there though.

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

https://codereview.chromium.org/1310223002/diff/320001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:88:
Java_ShortcutHelper_storeWebappData(
On 2015/09/02 16:56:57, Mounir Lamouri wrote:
> Maybe you could rename that createWebappDataInfo?

Done but with createWebappData - Info seems redundant to me.

https://codereview.chromium.org/1310223002/diff/320001/chrome/test/data/banne...
File chrome/test/data/banners/manifest.json (right):

https://codereview.chromium.org/1310223002/diff/320001/chrome/test/data/banne...
chrome/test/data/banners/manifest.json:30: "src": "splash-image.png",
On 2015/09/02 16:56:58, Mounir Lamouri wrote:
> nit: I really don't like these names because nothing forbids us from havin
> "launcher-icon-*" to be used for the splashscreen or "splash-image.png" to be
> used as a launcher icon (in a 10x device :)).
> 
> Could you just name it image-512px.png ?

Done.

https://codereview.chromium.org/1310223002/diff/320001/chrome/test/data/webap...
File chrome/test/data/webapps/manifest.json (right):

https://codereview.chromium.org/1310223002/diff/320001/chrome/test/data/webap...
chrome/test/data/webapps/manifest.json:6: "src": "splash-image.png",
On 2015/09/02 16:56:58, Mounir Lamouri wrote:
> ditto

Done.

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

Well, the constant rebasing on the other CLs explains why doing reviews on the
patch set diffs was such a pain.  If you ever go for full stack implementation
reviews like this, please let me know to focus on the CLs it depends on instead
of sending the CLs further down the pipe out for review again immediately.

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

https://chromiumcodereview.appspot.com/1310223002/diff/320001/chrome/browser/...
chrome/browser/android/shortcut_helper.cc:88:
Java_ShortcutHelper_storeWebappData(
On 2015/09/02 17:15:42, Lalit Maganti wrote:
> On 2015/09/02 16:56:57, Mounir Lamouri wrote:
> > Maybe you could rename that createWebappDataInfo?
> 
> Done but with createWebappData - Info seems redundant to me.

nit: you're not really creating web app data, you're just creating the storage
location and storing data we're getting from the page.  change it back to
storeWebappData, keep the rest the same?

[Lalit Maganti, 2015-09-02 21:18:57]

On 2015/09/02 20:47:21, dfalcantara wrote:
> Well, the constant rebasing on the other CLs explains why doing reviews on the
> patch set diffs was such a pain.  If you ever go for full stack implementation
> reviews like this, please let me know to focus on the CLs it depends on
instead
> of sending the CLs further down the pipe out for review again immediately.
> 
>
https://chromiumcodereview.appspot.com/1310223002/diff/320001/chrome/browser/...
> File chrome/browser/android/shortcut_helper.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1310223002/diff/320001/chrome/browser/...
> chrome/browser/android/shortcut_helper.cc:88:
> Java_ShortcutHelper_storeWebappData(
> On 2015/09/02 17:15:42, Lalit Maganti wrote:
> > On 2015/09/02 16:56:57, Mounir Lamouri wrote:
> > > Maybe you could rename that createWebappDataInfo?
> > 
> > Done but with createWebappData - Info seems redundant to me.
> 
> nit: you're not really creating web app data, you're just creating the storage
> location and storing data we're getting from the page.  change it back to
> storeWebappData, keep the rest the same?

Initially this patch didn't depend on anything else as data storage landed
before I pushed this out for review. I had to rebase on registry because I had
to put the .register call in. I'll make this completely clear in future.

[Lalit Maganti, 2015-09-03 11:12:32]

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

https://codereview.chromium.org/1310223002/diff/320001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:88:
Java_ShortcutHelper_storeWebappData(
On 2015/09/02 20:47:21, dfalcantara wrote:
> On 2015/09/02 17:15:42, Lalit Maganti wrote:
> > On 2015/09/02 16:56:57, Mounir Lamouri wrote:
> > > Maybe you could rename that createWebappDataInfo?
> > 
> > Done but with createWebappData - Info seems redundant to me.
> 
> nit: you're not really creating web app data, you're just creating the storage
> location and storing data we're getting from the page.  change it back to
> storeWebappData, keep the rest the same?

Switched back and haven't changed anything else.

[Lalit Maganti, 2015-09-03 20:47:44]

The CQ bit was checked by lalitm@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-03 20:48:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310223002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310223002/400001

[mlamouri (slow - plz ping), 2015-09-03 22:58:40]

Apart from the test issue, l-g-t-m.

I will review the tests when they will be updated.

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

https://codereview.chromium.org/1310223002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:315:
return getIdealSizeFromResource(resources, R.dimen.app_banner_icon_size);
nit: could we have a webapp_homescreen_icon_size? instead or in addition, don't
mind.

https://codereview.chromium.org/1310223002/diff/420001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/1310223002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:86:
public static void setObserverForTests(Observer observer) {
So, after talking with you and Dan, I think we should have a
WebappDataStorageWrapper in the tests that will overload
updateSplashScreenImage().

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

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:7: #include
"base/bind.h"
do you need that #include?

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:10: #include
"chrome/browser/banners/app_banner_debug_log.h"
and that one?

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:13: #include
"chrome/browser/manifest/manifest_icon_downloader.h"
what about that one?

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

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:62: void
ShortcutHelper::FetchSplashScreenImage(
nit: add "// static" above.

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.h:68: int ideal_icon_size_in_dp() {
return ideal_icon_size_in_dp_; }
nit: make the method const.

[Lalit Maganti, 2015-09-04 11:13:24]

Mounir: PTAL - I'm using the factory pattern + subclassing to get rid of
observers.

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

https://codereview.chromium.org/1310223002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:315:
return getIdealSizeFromResource(resources, R.dimen.app_banner_icon_size);
On 2015/09/03 22:58:40, Mounir Lamouri wrote:
> nit: could we have a webapp_homescreen_icon_size? instead or in addition,
don't
> mind.

Done.

https://codereview.chromium.org/1310223002/diff/420001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/1310223002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:86:
public static void setObserverForTests(Observer observer) {
On 2015/09/03 22:58:40, Mounir Lamouri wrote:
> So, after talking with you and Dan, I think we should have a
> WebappDataStorageWrapper in the tests that will overload
> updateSplashScreenImage().

Changed as discussed.

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

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:7: #include
"base/bind.h"
On 2015/09/03 22:58:40, Mounir Lamouri wrote:
> do you need that #include?

Not any more.

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:10: #include
"chrome/browser/banners/app_banner_debug_log.h"
On 2015/09/03 22:58:40, Mounir Lamouri wrote:
> and that one?

ditto

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:13: #include
"chrome/browser/manifest/manifest_icon_downloader.h"
On 2015/09/03 22:58:40, Mounir Lamouri wrote:
> what about that one?

ditto

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

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/android...
chrome/browser/android/shortcut_helper.cc:62: void
ShortcutHelper::FetchSplashScreenImage(
On 2015/09/03 22:58:40, Mounir Lamouri wrote:
> nit: add "// static" above.

Done. And below as well.

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/1310223002/diff/420001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.h:68: int ideal_icon_size_in_dp() {
return ideal_icon_size_in_dp_; }
On 2015/09/03 22:58:40, Mounir Lamouri wrote:
> nit: make the method const.

Done.

[mlamouri (slow - plz ping), 2015-09-04 13:08:44]

lgtm with comments applied.

Thanks for taking the time to fix the observer issue! :)

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:82:
* Sets the observer to use.
Comment doesn't seem quite right.

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:131:
* Factory used to generate WebappDataStorage objects.
Explain that it is used for tests.

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:138:
public WebappDataStorage createStorage(final Context context, final String
webappId) {
nit: I would call it "create"

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:480:
WebappDataStorage.setFactoryForTests(dataStorageFactory);
Can you do that in the SetUp() step?

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java
(right):

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java:198:
WebappDataStorage.setFactoryForTests(dataStorageFactory);
ditto

[Lalit Maganti, 2015-09-04 14:10:26]

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:82:
* Sets the observer to use.
On 2015/09/04 13:08:44, Mounir Lamouri wrote:
> Comment doesn't seem quite right.

Done.

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:131:
* Factory used to generate WebappDataStorage objects.
On 2015/09/04 13:08:44, Mounir Lamouri wrote:
> Explain that it is used for tests.

Done.

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:138:
public WebappDataStorage createStorage(final Context context, final String
webappId) {
On 2015/09/04 13:08:44, Mounir Lamouri wrote:
> nit: I would call it "create"

Done.

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:480:
WebappDataStorage.setFactoryForTests(dataStorageFactory);
On 2015/09/04 13:08:44, Mounir Lamouri wrote:
> Can you do that in the SetUp() step?

I moved it here on Dan's suggestion so that I didn't add stuff the the global
space.

Especially because the factory is only used for this method.

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java
(right):

https://codereview.chromium.org/1310223002/diff/440001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelperTest.java:198:
WebappDataStorage.setFactoryForTests(dataStorageFactory);
On 2015/09/04 13:08:44, Mounir Lamouri wrote:
> ditto

Reply as in other test.

[Lalit Maganti, 2015-09-04 14:10:34]

The CQ bit was checked by lalitm@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-04 14:10:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310223002/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310223002/460001

[commit-bot: I haz the power, 2015-09-04 14:12:34]

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

[commit-bot: I haz the power, 2015-09-04 14:12:35]

Dry run: 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...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Lalit Maganti, 2015-09-04 14:20:21]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-09-04 14:20:22]

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

[commit-bot: I haz the power, 2015-09-04 14:20:40]

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

[commit-bot: I haz the power, 2015-09-04 14:42:56]

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

[commit-bot: I haz the power, 2015-09-04 14:42:57]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Lalit Maganti, 2015-09-04 14:54:50]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-09-04 14:54:51]

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

[commit-bot: I haz the power, 2015-09-04 14:55:06]

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

[commit-bot: I haz the power, 2015-09-04 16:22:21]

Committed patchset #26 (id:470030)

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

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