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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by dominickn
Modified:
2 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+281 lines, -164 lines) Patch
M chrome/browser/android/webapk/webapk_update_data_fetcher.cc View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h View 1 2 3 4 5 6 7 8 9 6 chunks +15 lines, -8 lines 0 comments Download
M chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc View 1 2 3 4 5 6 7 8 9 12 chunks +72 lines, -68 lines 0 comments Download
M chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc View 1 2 3 4 5 6 7 14 chunks +182 lines, -82 lines 0 comments Download
M chrome/browser/android/webapps/add_to_homescreen_manager.cc View 1 2 3 4 2 chunks +9 lines, -1 line 0 comments Download
M chrome/browser/banners/app_banner_manager.cc View 1 2 2 chunks +1 line, -4 lines 0 comments Download
M chrome/browser/ui/tab_helpers.cc View 2 chunks +2 lines, -0 lines 0 comments Download
Trybot results:  linux_chromium_chromeos_ozone_rel_ng   ios-device   chromeos_amd64-generic_chromium_compile_only_ng   linux_chromium_tsan_rel_ng   ios-simulator   linux_chromium_rel_ng   cast_shell_linux   win_chromium_rel_ng   mac_chromium_rel_ng   android_n5x_swarming_rel   android_clang_dbg_recipe   win_clang   mac_chromium_compile_dbg_ng   chromium_presubmit   linux_chromium_compile_dbg_ng   android_arm64_dbg_recipe   cast_shell_android   linux_android_rel_ng   win_chromium_compile_dbg_ng   linux_chromium_headless_rel   linux_chromium_chromeos_rel_ng   android_compile_dbg   ios-simulator-xcode-clang   chromeos_daisy_chromium_compile_only_ng   linux_chromium_asan_rel_ng   ios-device-xcode-clang   android_cronet   win_chromium_x64_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   ios-device   chromeos_amd64-generic_chromium_compile_only_ng   linux_chromium_tsan_rel_ng   ios-simulator   cast_shell_linux   linux_chromium_rel_ng   win_chromium_rel_ng   android_n5x_swarming_rel   win_clang   mac_chromium_rel_ng   android_clang_dbg_recipe   mac_chromium_compile_dbg_ng   chromium_presubmit   linux_chromium_compile_dbg_ng   android_arm64_dbg_recipe   cast_shell_android   linux_android_rel_ng   linux_chromium_headless_rel   win_chromium_compile_dbg_ng   android_compile_dbg   ios-simulator-xcode-clang   linux_chromium_chromeos_rel_ng   chromeos_daisy_chromium_compile_only_ng   ios-device-xcode-clang   win_chromium_x64_rel_ng   linux_chromium_asan_rel_ng   android_cronet 
Commit queue not available (can’t edit this change).

Dependent Patchsets:

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 ...
3 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 ...
3 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 > ...
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 months ago (2017-06-23 19:30:46 UTC) #21
pkotwicz
3 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 ...
2 months, 4 weeks 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 ...
2 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 ...
2 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 ...
2 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 ...
2 months, 3 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 ...
2 months, 3 weeks ago (2017-06-29 04:39:57 UTC) #54
Avi (ping after 24h)
lgtm
2 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
2 months, 3 weeks ago (2017-06-30 03:19:19 UTC) #58
commit-bot: I haz the power
2 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b