Don't ignore manifest icons for sites that don't have a service worker.

Don't ignore manifest icons for sites that don't have a service worker.

https://crrev.com/2942513002 changed the InstallableManager such that it
will wait indefinitely for a site to register a service worker. For add
to homescreen, this means that sites which have icons in a manifest, but
no service worker, InstallableManager::GetData will time out and the
icons in the manifest will be ignored.

This CL updates AddToHomeScreenDataFetcher to run InstallableManager in
two phases, like AppBannerManager does. The first phase fetches the
manifest and the most appropriate icons from it. The second phase
checks installability, including the presence of a service worker.

Note that sites which have icons in a manifest and otherwise meet the
PWA eligibility criteria *except* for not having a service worker will
now take up to 4 seconds (the timeout period) before the details are
populated.

BUG=721881, 697228
Review-Url: https://codereview.chromium.org/2949993002
Cr-Commit-Position: refs/heads/master@{#483626}
Committed: https://chromium.googlesource.com/chromium/src/+/5cc00e8e97d0dabc14e7dd4284d0244033caffd3

[dominickn, 2017-06-21 01:17:04]

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

[commit-bot: I haz the power, 2017-06-21 01:17:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-06-21 01:18:03]

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

[dominickn, 2017-06-21 01:18:04]

Hey Peter, WDYT about this?

https://codereview.chromium.org/2949993002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (left):

https://codereview.chromium.org/2949993002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:216: // WebAPKs
are wholly defined by the Web Manifest. Ignore the <meta> tag
I thought about this for a while and I don't think it's necessary. Curious to
hear what you think.

[commit-bot: I haz the power, 2017-06-21 03:24:00]

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

[commit-bot: I haz the power, 2017-06-21 03:24:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dominickn, 2017-06-21 03:45:12]

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

[commit-bot: I haz the power, 2017-06-21 03:45:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 05:01:18]

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

[commit-bot: I haz the power, 2017-06-21 05:01:19]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-06-21 17:49:32]

I tried your CL with
https://googlechrome.github.io/samples/web-application-manifest/index.html and
the add-to-homescreen-flow uses a gray icon with a letter instead of one of the
icons specified in the Web Manifest as it did before

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

https://codereview.chromium.org/2949993002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:42: //
|weak_observer| pointer becomes invalid, the pipeline should kill itself.
Nit: |weak_observer_|

https://codereview.chromium.org/2949993002/diff/20001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2949993002/diff/20001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:206:
InstallableManager::CreateForWebContents(web_contents);
We can probably remove calls to InstallableManager::CreateForWebContents()
elsewhere

WebApkUpdateDataFetcher::FetchInstallableData() for instance

[dominickn, 2017-06-22 02:54:15]

On 2017/06/21 17:49:32, pkotwicz wrote:
> I tried your CL with
> https://googlechrome.github.io/samples/web-application-manifest/index.html and
> the add-to-homescreen-flow uses a gray icon with a letter instead of one of
the
> icons specified in the Web Manifest as it did before

Good catch, thanks. Fixed up that logic.

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

https://codereview.chromium.org/2949993002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:42: //
|weak_observer| pointer becomes invalid, the pipeline should kill itself.
On 2017/06/21 17:49:32, pkotwicz wrote:
> Nit: |weak_observer_|

Done.

https://codereview.chromium.org/2949993002/diff/20001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2949993002/diff/20001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:206:
InstallableManager::CreateForWebContents(web_contents);
On 2017/06/21 17:49:32, pkotwicz wrote:
> We can probably remove calls to InstallableManager::CreateForWebContents()
> elsewhere
> 
> WebApkUpdateDataFetcher::FetchInstallableData() for instance

Good point, done. Though if it's needed in unit tests it might be necessary to
inject calls to CreateForWebContents like I did for the unit tests here.

[pkotwicz, 2017-06-22 17:49:27]

Looks good. Just one suggestion on how to make the code (in my opinion) cleaner

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

https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:201:
CreateLauncherIcon(raw_icon_);
We should probably rename |raw_icon_| to |primary_raw_icon_|

https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:214: if
(data.primary_icon && !data.primary_icon->drawsNothing()) {
Nit: Based on InstallableManager::OnIconFetched(), it looks like
data.primary_icon would be null if the icon draws nothing.

https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:252:
AreWebManifestUrlsWebApkCompatible(data.manifest));
I think that it might be more intuitive if ParamsToPerformInstallableCheck() was
instead:

InstallableParams ParamsToPerformManifestAndIconFetch(bool
check_webapk_compatibility) {
  InstallableParams params;
  params.check_installable = check_webapk_compatibility;
  return params;
}

I am suggesting that OnDidGetManifestAndIcon() would fetch the badge icon as
well

the webapk_compatibility determination on line 251 would become:

webapk_compatible =
    (data.error_code == NO_ERROR_DETECTED &&
     !raw_icon_.is_null() &&
     AreWebManifestUrlsWebApkCompatible(data.manifest));

[dominickn, 2017-06-23 01:19:06]

Description was changed from

==========
Don't ignore manifest icons for sites that don't have a service worker.

https://crrev.com/2942513002 changed the InstallableManager such that it
will wait indefinitely for a site to register a service worker. For add
to homescreen, this means that sites which have icons in a manifest, but
no service worker, InstallableManager::GetData will time out and the
icons in the manifest will be ignored.

This CL updates AddToHomeScreenDataFetcher to run InstallableManager in
two phases, like AppBannerManager does. The first phase fetches the
manifest and the most appropriate icons from it. The second phase
checks installability, including the presence of a service worker.

Note that sites which have icons in a manifest, but no service worker
will now take up to 4 seconds (the timeout period) before the details
are populated.

BUG=721881
==========

to

==========
Don't ignore manifest icons for sites that don't have a service worker.

https://crrev.com/2942513002 changed the InstallableManager such that it
will wait indefinitely for a site to register a service worker. For add
to homescreen, this means that sites which have icons in a manifest, but
no service worker, InstallableManager::GetData will time out and the
icons in the manifest will be ignored.

This CL updates AddToHomeScreenDataFetcher to run InstallableManager in
two phases, like AppBannerManager does. The first phase fetches the
manifest and the most appropriate icons from it. The second phase
checks installability, including the presence of a service worker.

Note that sites which have icons in a manifest and otherwise meet the
PWA eligibility criteria *except* for not having a service worker will
now take up to 4 seconds (the timeout period) before the details are
populated.

BUG=721881
==========

[dominickn, 2017-06-23 01:20:22]

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

[commit-bot: I haz the power, 2017-06-23 01:20:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-06-23 01:52:04]

Thanks! I did a bunch of reorganisation here as well that hopefully makes things
more clear.

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

https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:201:
CreateLauncherIcon(raw_icon_);
On 2017/06/22 17:49:27, pkotwicz wrote:
> We should probably rename |raw_icon_| to |primary_raw_icon_|

Done.

https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:214: if
(data.primary_icon && !data.primary_icon->drawsNothing()) {
On 2017/06/22 17:49:27, pkotwicz wrote:
> Nit: Based on InstallableManager::OnIconFetched(), it looks like
> data.primary_icon would be null if the icon draws nothing.

Good point, can't even remember code that I wrote.

https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:252:
AreWebManifestUrlsWebApkCompatible(data.manifest));
On 2017/06/22 17:49:27, pkotwicz wrote:
> I think that it might be more intuitive if ParamsToPerformInstallableCheck()
was
> instead:
> 
> InstallableParams ParamsToPerformManifestAndIconFetch(bool
> check_webapk_compatibility) {
>   InstallableParams params;
>   params.check_installable = check_webapk_compatibility;
>   return params;
> }
> 
> I am suggesting that OnDidGetManifestAndIcon() would fetch the badge icon as
> well
> 
> the webapk_compatibility determination on line 251 would become:
> 
> webapk_compatible =
>     (data.error_code == NO_ERROR_DETECTED &&
>      !raw_icon_.is_null() &&
>      AreWebManifestUrlsWebApkCompatible(data.manifest));

That won't work: setting check_installable to true will make us wait for the
service worker. But I can fetch the badge icon based on the
check_webapk_compatibility value if that's what you mean. That means this piece
code will never be reached if we don't have valid icons.

[commit-bot: I haz the power, 2017-06-23 02:30:43]

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

[commit-bot: I haz the power, 2017-06-23 02:30:44]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-06-23 19:23:19]

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

https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:252:
AreWebManifestUrlsWebApkCompatible(data.manifest));
Sorry for the confusion. I meant it would be more intuitive if
ParamsToPerformInstallableCheck() was instead:

InstallableParams ParamsToPerformInstallableCheck(bool
check_webapk_compatibility) {
  InstallableParams params;
  params.check_installable = check_webapk_compatibility;
  return params;
}

It would be nice if we did not make the second call to
InstallableManager::GetData() unless |check_webapk_compatibility_| == true

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

https://codereview.chromium.org/2949993002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:238:
badge_icon_.reset();
Shouldn't this be removed?

https://codereview.chromium.org/2949993002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:289:
data_timeout_timer_.Stop();
Won't this cause OnUserTitleAvailable() never to be called?

[pkotwicz, 2017-06-23 19:30:46]

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

https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:252:
AreWebManifestUrlsWebApkCompatible(data.manifest));
Oh, I see you have in fact made this change :)

[dominickn, 2017-06-26 00:50:52]

Description was changed from

==========
Don't ignore manifest icons for sites that don't have a service worker.

https://crrev.com/2942513002 changed the InstallableManager such that it
will wait indefinitely for a site to register a service worker. For add
to homescreen, this means that sites which have icons in a manifest, but
no service worker, InstallableManager::GetData will time out and the
icons in the manifest will be ignored.

This CL updates AddToHomeScreenDataFetcher to run InstallableManager in
two phases, like AppBannerManager does. The first phase fetches the
manifest and the most appropriate icons from it. The second phase
checks installability, including the presence of a service worker.

Note that sites which have icons in a manifest and otherwise meet the
PWA eligibility criteria *except* for not having a service worker will
now take up to 4 seconds (the timeout period) before the details are
populated.

BUG=721881
==========

to

==========
Don't ignore manifest icons for sites that don't have a service worker.

https://crrev.com/2942513002 changed the InstallableManager such that it
will wait indefinitely for a site to register a service worker. For add
to homescreen, this means that sites which have icons in a manifest, but
no service worker, InstallableManager::GetData will time out and the
icons in the manifest will be ignored.

This CL updates AddToHomeScreenDataFetcher to run InstallableManager in
two phases, like AppBannerManager does. The first phase fetches the
manifest and the most appropriate icons from it. The second phase
checks installability, including the presence of a service worker.

Note that sites which have icons in a manifest and otherwise meet the
PWA eligibility criteria *except* for not having a service worker will
now take up to 4 seconds (the timeout period) before the details are
populated.

BUG=721881,697228.
==========

[dominickn, 2017-06-26 01:55:35]

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

[commit-bot: I haz the power, 2017-06-26 01:55:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-06-26 01:55:46]

Description was changed from

==========
Don't ignore manifest icons for sites that don't have a service worker.

https://crrev.com/2942513002 changed the InstallableManager such that it
will wait indefinitely for a site to register a service worker. For add
to homescreen, this means that sites which have icons in a manifest, but
no service worker, InstallableManager::GetData will time out and the
icons in the manifest will be ignored.

This CL updates AddToHomeScreenDataFetcher to run InstallableManager in
two phases, like AppBannerManager does. The first phase fetches the
manifest and the most appropriate icons from it. The second phase
checks installability, including the presence of a service worker.

Note that sites which have icons in a manifest and otherwise meet the
PWA eligibility criteria *except* for not having a service worker will
now take up to 4 seconds (the timeout period) before the details are
populated.

BUG=721881,697228.
==========

to

==========
Don't ignore manifest icons for sites that don't have a service worker.

https://crrev.com/2942513002 changed the InstallableManager such that it
will wait indefinitely for a site to register a service worker. For add
to homescreen, this means that sites which have icons in a manifest, but
no service worker, InstallableManager::GetData will time out and the
icons in the manifest will be ignored.

This CL updates AddToHomeScreenDataFetcher to run InstallableManager in
two phases, like AppBannerManager does. The first phase fetches the
manifest and the most appropriate icons from it. The second phase
checks installability, including the presence of a service worker.

Note that sites which have icons in a manifest and otherwise meet the
PWA eligibility criteria *except* for not having a service worker will
now take up to 4 seconds (the timeout period) before the details are
populated.

BUG=721881,697228
==========

[dominickn, 2017-06-26 02:39:17]

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

https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:252:
AreWebManifestUrlsWebApkCompatible(data.manifest));
On 2017/06/23 19:30:45, pkotwicz wrote:
> Oh, I see you have in fact made this change :)

When WebAPKs are enabled by default we'll always want to make the second call
unless the manifest is empty. :)

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

https://codereview.chromium.org/2949993002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:238:
badge_icon_.reset();
On 2017/06/23 19:23:19, pkotwicz wrote:
> Shouldn't this be removed?

Good catch, thanks.

https://codereview.chromium.org/2949993002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:289:
data_timeout_timer_.Stop();
On 2017/06/23 19:23:19, pkotwicz wrote:
> Won't this cause OnUserTitleAvailable() never to be called?

Added an explicit call in OnDidFetchManifestAndIcons. That should be the one
call path where we don't already call OnUserDataAvailable.

Also fixed up the tests so we can test the time out case properly

[commit-bot: I haz the power, 2017-06-26 02:59:17]

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

[commit-bot: I haz the power, 2017-06-26 02:59:18]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-06-26 07:31:12]

Patchset #6 (id:100001) has been deleted

[dominickn, 2017-06-26 10:39:35]

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

[commit-bot: I haz the power, 2017-06-26 10:39:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 11:31:40]

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

[commit-bot: I haz the power, 2017-06-26 11:31:41]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-06-26 14:10:03]

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

[commit-bot: I haz the power, 2017-06-26 14:10:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 15:16:10]

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

[commit-bot: I haz the power, 2017-06-26 15:16:11]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-06-27 00:58:39]

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

[commit-bot: I haz the power, 2017-06-27 00:58:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkotwicz, 2017-06-27 01:09:32]

Just two comments

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

https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:189:
badge_icon_.reset();
We should probably delete line 189

https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:206: if
(data.manifest.IsEmpty() || !data.primary_icon) {
Nit: It would be clearer if you stopped the timer here. Favicon fetches are
fast. (A few milliseconds). I measured how long they took a while ago but don't
remember the exact results

With the current code it is possible for
AddToHomescreenManager::OnDidDetermineWebApkCompatibility() to be called twice
in the following scenario

Web Manifest is empty
1) AddToHomescreenManager::OnDidDetermineWebApkCompatibility() is called from
line 207
2) AddToHomescreenDataFetcher::OnDataTimedout() is called prior to
AddToHomescreenDataFetcher::OnFaviconFetched()
3) AddToHomescreenManager::OnDidDetermineWebApkCompatibility() is called from
line 184

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

https://codereview.chromium.org/2949993002/diff/160001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:250:
weak_observer_->OnDidDetermineWebApkCompatibility(webapk_compatible);
As discussed offline, the order of OnDidDetermineWebApkCompatibility() and
OnUserTitleAvailable() is important

[dominickn, 2017-06-27 01:37:21]

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

[dominickn, 2017-06-27 01:37:24]

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

https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:189:
badge_icon_.reset();
On 2017/06/27 01:09:32, pkotwicz wrote:
> We should probably delete line 189

Well, we're always non-WebAPK compatible in here, so it can probably stay.

https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:206: if
(data.manifest.IsEmpty() || !data.primary_icon) {
On 2017/06/27 01:09:32, pkotwicz wrote:
> Nit: It would be clearer if you stopped the timer here. Favicon fetches are
> fast. (A few milliseconds). I measured how long they took a while ago but
don't
> remember the exact results
> 
> With the current code it is possible for
> AddToHomescreenManager::OnDidDetermineWebApkCompatibility() to be called twice
> in the following scenario
> 
> Web Manifest is empty
> 1) AddToHomescreenManager::OnDidDetermineWebApkCompatibility() is called from
> line 207
> 2) AddToHomescreenDataFetcher::OnDataTimedout() is called prior to
> AddToHomescreenDataFetcher::OnFaviconFetched()
> 3) AddToHomescreenManager::OnDidDetermineWebApkCompatibility() is called from
> line 184

Done.

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

https://codereview.chromium.org/2949993002/diff/160001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:250:
weak_observer_->OnDidDetermineWebApkCompatibility(webapk_compatible);
On 2017/06/27 01:09:32, pkotwicz wrote:
> As discussed offline, the order of OnDidDetermineWebApkCompatibility() and
> OnUserTitleAvailable() is important

This is fixed.

[commit-bot: I haz the power, 2017-06-27 01:37:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 03:16:35]

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

[commit-bot: I haz the power, 2017-06-27 03:16:36]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-06-27 15:48:20]

LGTM with a few comments

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

https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:189:
badge_icon_.reset();
It doesn't do any harm, but clearing the badge here, in my opinion, is confusing

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

https://codereview.chromium.org/2949993002/diff/180001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:323: void
AddToHomescreenDataFetcher::CreateLauncherIcon(const SkBitmap& icon) {
|icon| looks unused

https://codereview.chromium.org/2949993002/diff/180001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:359: if
(!web_contents() || !weak_observer_ || is_icon_saved_)
Aside: |is_icon_saved_| should now always be false

[dominickn, 2017-06-28 01:37:43]

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

[dominickn, 2017-06-28 01:38:05]

Thanks!

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

https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:189:
badge_icon_.reset();
On 2017/06/27 15:48:19, pkotwicz wrote:
> It doesn't do any harm, but clearing the badge here, in my opinion, is
confusing

Done.

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

https://codereview.chromium.org/2949993002/diff/180001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:323: void
AddToHomescreenDataFetcher::CreateLauncherIcon(const SkBitmap& icon) {
On 2017/06/27 15:48:19, pkotwicz wrote:
> |icon| looks unused

Whoops fixed.

https://codereview.chromium.org/2949993002/diff/180001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:359: if
(!web_contents() || !weak_observer_ || is_icon_saved_)
On 2017/06/27 15:48:20, pkotwicz wrote:
> Aside: |is_icon_saved_| should now always be false

Indeed. Removed.

[commit-bot: I haz the power, 2017-06-28 01:38:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 03:09:59]

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

[commit-bot: I haz the power, 2017-06-28 03:10:04]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-06-29 04:39:56]

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

[dominickn, 2017-06-29 04:39:57]

+avi for tab_helpers.cc. This is moving a WebContentsUserData from being lazily
created to being created up front (it will be needed up front on all platforms
shortly as we front-end checks for PWAs)

[Avi (use Gerrit), 2017-06-29 14:55:46]

lgtm

[dominickn, 2017-06-30 03:19:08]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-06-30 03:19:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2949993002/#ps200001
(title: "Comments")

[commit-bot: I haz the power, 2017-06-30 03:19:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-30 04:28:00]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1498792748226070,
"parent_rev": "4313334bead373b139eee72f47d43fbe6162a8f4", "commit_rev":
"5cc00e8e97d0dabc14e7dd4284d0244033caffd3"}

[commit-bot: I haz the power, 2017-06-30 04:28:23]

Description was changed from

==========
Don't ignore manifest icons for sites that don't have a service worker.

https://crrev.com/2942513002 changed the InstallableManager such that it
will wait indefinitely for a site to register a service worker. For add
to homescreen, this means that sites which have icons in a manifest, but
no service worker, InstallableManager::GetData will time out and the
icons in the manifest will be ignored.

This CL updates AddToHomeScreenDataFetcher to run InstallableManager in
two phases, like AppBannerManager does. The first phase fetches the
manifest and the most appropriate icons from it. The second phase
checks installability, including the presence of a service worker.

Note that sites which have icons in a manifest and otherwise meet the
PWA eligibility criteria *except* for not having a service worker will
now take up to 4 seconds (the timeout period) before the details are
populated.

BUG=721881,697228
==========

to

==========
Don't ignore manifest icons for sites that don't have a service worker.

https://crrev.com/2942513002 changed the InstallableManager such that it
will wait indefinitely for a site to register a service worker. For add
to homescreen, this means that sites which have icons in a manifest, but
no service worker, InstallableManager::GetData will time out and the
icons in the manifest will be ignored.

This CL updates AddToHomeScreenDataFetcher to run InstallableManager in
two phases, like AppBannerManager does. The first phase fetches the
manifest and the most appropriate icons from it. The second phase
checks installability, including the presence of a service worker.

Note that sites which have icons in a manifest and otherwise meet the
PWA eligibility criteria *except* for not having a service worker will
now take up to 4 seconds (the timeout period) before the details are
populated.

BUG=721881,697228

Review-Url: https://codereview.chromium.org/2949993002
Cr-Commit-Position: refs/heads/master@{#483626}
Committed:
https://chromium.googlesource.com/chromium/src/+/5cc00e8e97d0dabc14e7dd4284d0...
==========

[commit-bot: I haz the power, 2017-06-30 04:28:25]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/5cc00e8e97d0dabc14e7dd4284d0...