Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2068393002:
Flag for Offline Page Sharing (Closed)
|
Created:
4 years, 6 months ago by Vivian Modified:
4 years, 5 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, asvitkine+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFlag for Offline Page Sharing
This is a flag for the experimental offline page sharing feature. It defalts to off.
Will update the bug number once get approval from dimich to issue a bug for the project.
BUG=622139
Committed: https://crrev.com/4357adacf634f51c2a885316e38288beb2fda99a
Cr-Commit-Position: refs/heads/master@{#406980}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Edit according to Jian's comments and formatting #
Total comments: 5
Patch Set 3 : Resolve merge conflict #Patch Set 4 : Fix a merge error #Patch Set 5 : Merge flag check statement #
Total comments: 6
Patch Set 6 : Nit changes. #Patch Set 7 : Resolve merge conflict #Dependent Patchsets: Messages
Total messages: 61 (43 generated)
weiran@google.com changed reviewers: + fgorski@chromium.org, jianli@chromium.org, thakis@chromium.org
plal
lgtm mod nits. Please format the code. Please don't commit until jianli@ approves. https://codereview.chromium.org/2068393002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/2068393002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_bridge.cc:150: const JavaParamRef<jclass>& clazz) { nit: alignment, please run git cl format https://codereview.chromium.org/2068393002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model_impl_unittest.cc (right): https://codereview.chromium.org/2068393002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_impl_unittest.cc:1098: ""); nit: I think git cl format might disagree with this formatting
https://codereview.chromium.org/2068393002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2068393002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14687: + Enables the saved offline pages to be shared as a file via other applications. I think you probably don't need to mention "as a file". https://codereview.chromium.org/2068393002/diff/1/chrome/browser/android/chro... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2068393002/diff/1/chrome/browser/android/chro... chrome/browser/android/chrome_feature_list.cc:42: &offline_pages::kOfflinePagesSharingFeature, I don't think you need to add this unless you really need to check if the feature is enabled on Java code. Most of time we just delegate the check to C++ side. Indeed kOfflinePagesBackgroundLoadingFeature should also be removed from this list. I will ask someone to take care of this in another patch.
https://codereview.chromium.org/2068393002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_feature.cc (right): https://codereview.chromium.org/2068393002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_feature.cc:31: IsOfflinePagesBackgroundLoadingEnabled(); Checking this flag should also be added here.
Thanks for the comments! Will make sure run git cl format next time. Made some change according to comments. ptal https://codereview.chromium.org/2068393002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2068393002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14687: + Enables the saved offline pages to be shared as a file via other applications. On 2016/06/15 23:19:30, jianli wrote: > I think you probably don't need to mention "as a file". Done. Deleted "as a file". https://codereview.chromium.org/2068393002/diff/1/chrome/browser/android/chro... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2068393002/diff/1/chrome/browser/android/chro... chrome/browser/android/chrome_feature_list.cc:42: &offline_pages::kOfflinePagesSharingFeature, On 2016/06/15 23:19:31, jianli wrote: > I don't think you need to add this unless you really need to check if the > feature is enabled on Java code. Most of time we just delegate the check to C++ > side. > > Indeed kOfflinePagesBackgroundLoadingFeature should also be removed from this > list. I will ask someone to take care of this in another patch. Done. https://codereview.chromium.org/2068393002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/2068393002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_bridge.cc:150: const JavaParamRef<jclass>& clazz) { On 2016/06/15 22:54:28, fgorski wrote: > nit: alignment, please run git cl format Done. https://codereview.chromium.org/2068393002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_feature.cc (right): https://codereview.chromium.org/2068393002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_feature.cc:31: IsOfflinePagesBackgroundLoadingEnabled(); On 2016/06/15 23:27:52, jianli wrote: > Checking this flag should also be added here. Done. https://codereview.chromium.org/2068393002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model_impl_unittest.cc (right): https://codereview.chromium.org/2068393002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_impl_unittest.cc:1098: ""); On 2016/06/15 22:54:28, fgorski wrote: > nit: I think git cl format might disagree with this formatting Seems that git cl format didn't change the format of this part.
lgtm https://codereview.chromium.org/2068393002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2068393002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1889: IDS_FLAGS_ENABLE_AUTOPLAY_MUTED_VIDEOS_DESCRIPTION, kOsAndroid, Please try not to touch other unrelated parts. I know these are caused by cl format.
https://codereview.chromium.org/2068393002/diff/20001/components/offline_page... File components/offline_pages/offline_page_feature.cc (right): https://codereview.chromium.org/2068393002/diff/20001/components/offline_page... components/offline_pages/offline_page_feature.cc:31: IsOfflinePagesSharingEnabled(); this part bothers me a little bit. I mean: if the sharing of offline pages is enabled, but nothing else, can we really claim that offline pages are enabled? I think it is ok to stay like that for now, but we should discuss the meaning of the flag in that shape.
https://codereview.chromium.org/2068393002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2068393002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1889: IDS_FLAGS_ENABLE_AUTOPLAY_MUTED_VIDEOS_DESCRIPTION, kOsAndroid, On 2016/06/16 01:10:14, jianli wrote: > Please try not to touch other unrelated parts. I know these are caused by cl > format. Acknowledged. https://codereview.chromium.org/2068393002/diff/20001/components/offline_page... File components/offline_pages/offline_page_feature.cc (right): https://codereview.chromium.org/2068393002/diff/20001/components/offline_page... components/offline_pages/offline_page_feature.cc:31: IsOfflinePagesSharingEnabled(); On 2016/06/16 15:09:39, fgorski wrote: > this part bothers me a little bit. I mean: if the sharing of offline pages is > enabled, but nothing else, can we really claim that offline pages are enabled? > > I think it is ok to stay like that for now, but we should discuss the meaning of > the flag in that shape. I agree with Filip. Even if offline pages is not enabled, the pages downloaded before should be able to be shared. Enabling page sharing does not necessarily means that offline pages are enabled as well. But Jian is the expert of flag. Waiting for Jian's thought on this.
https://codereview.chromium.org/2068393002/diff/20001/components/offline_page... File components/offline_pages/offline_page_feature.cc (right): https://codereview.chromium.org/2068393002/diff/20001/components/offline_page... components/offline_pages/offline_page_feature.cc:31: IsOfflinePagesSharingEnabled(); On 2016/06/16 17:02:58, Vivian wrote: > On 2016/06/16 15:09:39, fgorski wrote: > > this part bothers me a little bit. I mean: if the sharing of offline pages is > > enabled, but nothing else, can we really claim that offline pages are enabled? > > > > I think it is ok to stay like that for now, but we should discuss the meaning > of > > the flag in that shape. > > I agree with Filip. Even if offline pages is not enabled, the pages downloaded > before should be able to be shared. Enabling page sharing does not necessarily > means that offline pages are enabled as well. > > But Jian is the expert of flag. Waiting for Jian's thought on this. If we enable only sharing feature but not any other offline sub-features, we need to add sharing check in IsOfflinePagesEnabled because otherwise some offline API might not work. We do need to refactor this piece of logic since it is kind of confusing.
Description was changed from ========== Flag for Offline Page Sharing This is a flag for the experimental offline page sharing feature. It defalts to off. Will update the bug number once get approval from dimich to issue a bug for the project. BUG= ========== to ========== Flag for Offline Page Sharing This is a flag for the experimental offline page sharing feature. It defalts to off. Will update the bug number once get approval from dimich to issue a bug for the project. BUG=622139 ==========
fgorski@chromium.org changed reviewers: - thakis@chromium.org
The CQ bit was checked by weiran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2068393002/#ps20001 (title: "Edit according to Jian's comments and formatting")
The CQ bit was checked by weiran@google.com 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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by weiran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2068393002/#ps40001 (title: "Resolve merge conflict")
The CQ bit was checked by weiran@google.com 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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by weiran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2068393002/#ps60001 (title: "Fix a merge error")
The CQ bit was checked by weiran@google.com 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 weiran@google.com
The CQ bit was checked by weiran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2068393002/#ps80001 (title: "Merge flag check statement")
The CQ bit was checked by weiran@google.com 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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
3 more comments to address. https://codereview.chromium.org/2068393002/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2068393002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:14698: + Enables offline pages to be shared. Remove period from the end. https://codereview.chromium.org/2068393002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:14701: + Enables the saved offline pages to be shared via other applications. Enables sharing offline pages with other applications via intent. https://codereview.chromium.org/2068393002/diff/80001/components/offline_page... File components/offline_pages/offline_page_feature.cc (right): https://codereview.chromium.org/2068393002/diff/80001/components/offline_page... components/offline_pages/offline_page_feature.cc:34: IsOfflinePagesBackgroundLoadingEnabled() || IsOfflinePagesCTEnabled() || can you run git cl format?
The CQ bit was checked by weiran@google.com 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 checked by weiran@google.com 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...
Changes made according to comments. Submitted for dry run. https://codereview.chromium.org/2068393002/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2068393002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:14698: + Enables offline pages to be shared. On 2016/07/21 17:08:05, fgorski wrote: > Remove period from the end. Done. https://codereview.chromium.org/2068393002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:14701: + Enables the saved offline pages to be shared via other applications. On 2016/07/21 17:08:05, fgorski wrote: > Enables sharing offline pages with other applications via intent. Done. https://codereview.chromium.org/2068393002/diff/80001/components/offline_page... File components/offline_pages/offline_page_feature.cc (right): https://codereview.chromium.org/2068393002/diff/80001/components/offline_page... components/offline_pages/offline_page_feature.cc:34: IsOfflinePagesBackgroundLoadingEnabled() || IsOfflinePagesCTEnabled() || On 2016/07/21 17:08:05, fgorski wrote: > can you run git cl format? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by weiran@google.com 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.
The CQ bit was checked by weiran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2068393002/#ps120001 (title: "Resolve merge conflict")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
weiran@google.com changed reviewers: + tedchoc@chromium.org
tedchoc@chromium.org: Please review changes in chrome_feature_list.cc Thanks
On 2016/07/21 20:59:35, Vivian wrote: > mailto:tedchoc@chromium.org: Please review changes in chrome_feature_list.cc > > Thanks chrome_feature_list.cc - lgtm
The CQ bit was checked by weiran@google.com
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.
Description was changed from ========== Flag for Offline Page Sharing This is a flag for the experimental offline page sharing feature. It defalts to off. Will update the bug number once get approval from dimich to issue a bug for the project. BUG=622139 ========== to ========== Flag for Offline Page Sharing This is a flag for the experimental offline page sharing feature. It defalts to off. Will update the bug number once get approval from dimich to issue a bug for the project. BUG=622139 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Flag for Offline Page Sharing This is a flag for the experimental offline page sharing feature. It defalts to off. Will update the bug number once get approval from dimich to issue a bug for the project. BUG=622139 ========== to ========== Flag for Offline Page Sharing This is a flag for the experimental offline page sharing feature. It defalts to off. Will update the bug number once get approval from dimich to issue a bug for the project. BUG=622139 Committed: https://crrev.com/4357adacf634f51c2a885316e38288beb2fda99a Cr-Commit-Position: refs/heads/master@{#406980} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4357adacf634f51c2a885316e38288beb2fda99a Cr-Commit-Position: refs/heads/master@{#406980} |
