Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1613)

Issue 2949993002: Don't ignore manifest icons for sites that don't have a service worker. (Closed)

Created:
10 months ago by dominickn
Modified:
9 months, 3 weeks ago
CC:
chromium-reviews, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, agrieve+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 1

Patch Set 2 : Create InstallableManager in the unit test #

Total comments: 4

Patch Set 3 : Fix up icon handling #

Total comments: 9

Patch Set 4 : Comments, clean up #

Total comments: 4

Patch Set 5 : Comments, do timeout testing properly #

Patch Set 6 : Self nits #

Patch Set 7 : Test image and manifest timeout separately #

Total comments: 6

Patch Set 8 : Move OnUserTitleAvailable #

Total comments: 2

Patch Set 9 : Nits #

Total comments: 4

Patch Set 10 : Comments #

Messages

Total messages: 61 (44 generated)
dominickn
Hey Peter, WDYT about this? https://codereview.chromium.org/2949993002/diff/1/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (left): https://codereview.chromium.org/2949993002/diff/1/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc#oldcode216 chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:216: // WebAPKs are wholly ...
10 months ago (2017-06-21 01:18:04 UTC) #4
pkotwicz
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 ...
10 months ago (2017-06-21 17:49:32 UTC) #11
dominickn
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 > ...
10 months ago (2017-06-22 02:54:15 UTC) #12
pkotwicz
Looks good. Just one suggestion on how to make the code (in my opinion) cleaner ...
10 months ago (2017-06-22 17:49:27 UTC) #13
dominickn
Thanks! I did a bunch of reorganisation here as well that hopefully makes things more ...
10 months ago (2017-06-23 01:52:04 UTC) #17
pkotwicz
https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc#newcode252 chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:252: AreWebManifestUrlsWebApkCompatible(data.manifest)); Sorry for the confusion. I meant it would ...
10 months ago (2017-06-23 19:23:19 UTC) #20
pkotwicz
https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc#newcode252 chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:252: AreWebManifestUrlsWebApkCompatible(data.manifest)); Oh, I see you have in fact made ...
10 months ago (2017-06-23 19:30:46 UTC) #21
pkotwicz
10 months ago (2017-06-23 19:30:51 UTC) #22
dominickn
https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2949993002/diff/40001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc#newcode252 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 ...
10 months ago (2017-06-26 02:39:17 UTC) #27
pkotwicz
Just two comments https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc#newcode189 chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:189: badge_icon_.reset(); We should probably delete line ...
9 months, 4 weeks ago (2017-06-27 01:09:32 UTC) #41
dominickn
https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc#newcode189 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 ...
9 months, 4 weeks ago (2017-06-27 01:37:24 UTC) #43
pkotwicz
LGTM with a few comments https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc#newcode189 chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:189: badge_icon_.reset(); It doesn't do ...
9 months, 4 weeks ago (2017-06-27 15:48:20 UTC) #47
dominickn
Thanks! https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2949993002/diff/140001/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc#newcode189 chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:189: badge_icon_.reset(); On 2017/06/27 15:48:19, pkotwicz wrote: > It ...
9 months, 4 weeks ago (2017-06-28 01:38:05 UTC) #49
dominickn
+avi for tab_helpers.cc. This is moving a WebContentsUserData from being lazily created to being created ...
9 months, 3 weeks ago (2017-06-29 04:39:57 UTC) #54
Avi (use Gerrit)
lgtm
9 months, 3 weeks ago (2017-06-29 14:55:46 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2949993002/200001
9 months, 3 weeks ago (2017-06-30 03:19:19 UTC) #58
commit-bot: I haz the power
9 months, 3 weeks ago (2017-06-30 04:28:25 UTC) #61
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/5cc00e8e97d0dabc14e7dd4284d0...

Powered by Google App Engine
This is Rietveld 408576698