Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2483463002:
[Offline Pages] Feature flag to allow concurrent background loading on svelte (Closed)
|
Created:
4 years, 1 month ago by dougarnett Modified:
4 years, 1 month ago CC:
chromium-reviews, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, asvitkine+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Feature flag to allow concurrent background loading on svelte
Adds flag (default off) to allow immediate processing of SavePageLater requests
on low-end devices. This will allow us to evaluate concurrent loading on our
development devices and allows for prospect of a future Finch trial.
BUG=655341
Committed: https://crrev.com/fa0aebf37854698a45180ac8886b78d22f8b5de4
Cr-Commit-Position: refs/heads/master@{#431094}
Patch Set 1 #Patch Set 2 : Added dependency on :switches #Patch Set 3 : Unit test deps on switches #Patch Set 4 : Added flag to LoginCustomFlags histogram #
Total comments: 5
Patch Set 5 : Address petewil@ comment #
Total comments: 2
Patch Set 6 : Reverted change to expose flag to java per tedchoc@ feedback #Patch Set 7 : Merge #Messages
Total messages: 53 (36 generated)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dougarnett@chromium.org changed reviewers: + dimich@chromium.org, petewil@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit. https://codereview.chromium.org/2483463002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2483463002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14944: + <message name="IDS_FLAGS_OFFLINE_PAGES_SVELTE_CONCURRENT_LOADING_NAME" desc="Name for the flag to enable concurrent background loading on svelte devices."> Folks might not know what "svelte" means to us - perhaps saying devices with 512MB memory or less would make more sense to the person reading the flag list. The flag name is fine, we only need to change the description that is shown to the user in chrome://flags.
https://codereview.chromium.org/2483463002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2483463002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14944: + <message name="IDS_FLAGS_OFFLINE_PAGES_SVELTE_CONCURRENT_LOADING_NAME" desc="Name for the flag to enable concurrent background loading on svelte devices."> On 2016/11/08 00:42:43, Pete Williamson wrote: > Folks might not know what "svelte" means to us - perhaps saying devices with > 512MB memory or less would make more sense to the person reading the flag list. > The flag name is fine, we only need to change the description that is shown to > the user in chrome://flags. Are you saying mention this in this NAME desc (this line) or that the below DESCRIPTION desc with "Android svelte (512MB RAM)" is not clear enough)?
Yay! lgtm https://codereview.chromium.org/2483463002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2483463002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14944: + <message name="IDS_FLAGS_OFFLINE_PAGES_SVELTE_CONCURRENT_LOADING_NAME" desc="Name for the flag to enable concurrent background loading on svelte devices."> On 2016/11/08 00:42:43, Pete Williamson wrote: > Folks might not know what "svelte" means to us - perhaps saying devices with > 512MB memory or less would make more sense to the person reading the flag list. > The flag name is fine, we only need to change the description that is shown to > the user in chrome://flags. +1 to avoid 'svelte' in all names. "512Mb devices"?
https://codereview.chromium.org/2483463002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2483463002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14944: + <message name="IDS_FLAGS_OFFLINE_PAGES_SVELTE_CONCURRENT_LOADING_NAME" desc="Name for the flag to enable concurrent background loading on svelte devices."> On 2016/11/08 00:48:11, dougarnett wrote: > On 2016/11/08 00:42:43, Pete Williamson wrote: > > Folks might not know what "svelte" means to us - perhaps saying devices with > > 512MB memory or less would make more sense to the person reading the flag > list. > > The flag name is fine, we only need to change the description that is shown to > > the user in chrome://flags. > > Are you saying mention this in this NAME desc (this line) or that the below > DESCRIPTION desc with "Android svelte (512MB RAM)" is not clear enough)? I was thinking that the desc field for the name should explain what svelte is.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2483463002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2483463002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14944: + <message name="IDS_FLAGS_OFFLINE_PAGES_SVELTE_CONCURRENT_LOADING_NAME" desc="Name for the flag to enable concurrent background loading on svelte devices."> On 2016/11/08 00:53:39, Pete Williamson wrote: > On 2016/11/08 00:48:11, dougarnett wrote: > > On 2016/11/08 00:42:43, Pete Williamson wrote: > > > Folks might not know what "svelte" means to us - perhaps saying devices with > > > 512MB memory or less would make more sense to the person reading the flag > > list. > > > The flag name is fine, we only need to change the description that is shown > to > > > the user in chrome://flags. > > > > Are you saying mention this in this NAME desc (this line) or that the below > > DESCRIPTION desc with "Android svelte (512MB RAM)" is not clear enough)? > I was thinking that the desc field for the name should explain what svelte is. Done.
dougarnett@chromium.org changed reviewers: + asvitkine@chromium.org, tedchoc@chromium.org
tedchoc@ please give owners review of chrome_feature_list.cc asvitkine@ please give owners review of histograms.xml
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2483463002/diff/80001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2483463002/diff/80001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:64: &offline_pages::kOfflinePagesSvelteConcurrentLoadingFeature, This array says these are the features exposed to java but I don't see this used anywhere in the java code. Do you actually need this here? Are any of these offline entries accessed from java? I'd vote for not adding things to this list until it is needed.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2483463002/diff/80001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2483463002/diff/80001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:64: &offline_pages::kOfflinePagesSvelteConcurrentLoadingFeature, On 2016/11/08 17:42:13, Ted C wrote: > This array says these are the features exposed to java but I don't see this used > anywhere in the java code. Do you actually need this here? > > Are any of these offline entries accessed from java? > > I'd vote for not adding things to this list until it is needed. Done, thanks
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dougarnett@chromium.org changed reviewers: + jwd@chromium.org - asvitkine@chromium.org, tedchoc@chromium.org
jwd@ can you review histograms.xml?
lgtm
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, petewil@chromium.org Link to the patchset: https://codereview.chromium.org/2483463002/#ps100001 (title: "Reverted change to expose flag to java per tedchoc@ feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dougarnett@chromium.org
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, dimich@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2483463002/#ps120001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Feature flag to allow concurrent background loading on svelte Adds flag (default off) to allow immediate processing of SavePageLater requests on low-end devices. This will allow us to evaluate concurrent loading on our development devices and allows for prospect of a future Finch trial. BUG=655341 ========== to ========== [Offline Pages] Feature flag to allow concurrent background loading on svelte Adds flag (default off) to allow immediate processing of SavePageLater requests on low-end devices. This will allow us to evaluate concurrent loading on our development devices and allows for prospect of a future Finch trial. BUG=655341 Committed: https://crrev.com/fa0aebf37854698a45180ac8886b78d22f8b5de4 Cr-Commit-Position: refs/heads/master@{#431094} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fa0aebf37854698a45180ac8886b78d22f8b5de4 Cr-Commit-Position: refs/heads/master@{#431094} |
