Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562643003:
Enable RL by default (Closed)
DescriptionEnable RL by default
BUG=577659
Committed: https://crrev.com/7cfbed3def4f21028e9e3295874c68a522dd63ac
Cr-Commit-Position: refs/heads/master@{#437765}
Patch Set 1 #Patch Set 2 : use gn flag #
Total comments: 6
Patch Set 3 : comments #
Total comments: 6
Patch Set 4 : feedback #
Total comments: 3
Patch Set 5 : feedback #Patch Set 6 : add unittest datatype #
Messages
Total messages: 39 (14 generated)
olivierrobin@chromium.org changed reviewers: + mardini@chromium.org, noyau@chromium.org
Please note that until setting is reversed in ExperimentalSettings, it will not be possible to disable it.
On 2016/12/08 10:30:53, Olivier Robin wrote: > Please note that until setting is reversed in ExperimentalSettings, it will not > be possible to disable it. This should probably not be a flag in code anymore, but a build variable so we can turn off everything if needed.
On 2016/12/08 11:32:54, noyau wrote: > On 2016/12/08 10:30:53, Olivier Robin wrote: > > Please note that until setting is reversed in ExperimentalSettings, it will > not > > be possible to disable it. > > This should probably not be a flag in code anymore, but a build variable so we > can turn off everything if needed. We can turn off everything by having |IsReadingListEnabled()| return false. For the record, next time I recommend using PSMultiValueSpecifier in the plist so that you don't have to update it separately. Still, lgtm.
Not for this feature, we need to turn off the compilation and inclusion of the extension as well. On 8 December 2016 at 12:42, <jif@google.com> wrote: > On 2016/12/08 11:32:54, noyau wrote: > > On 2016/12/08 10:30:53, Olivier Robin wrote: > > > Please note that until setting is reversed in ExperimentalSettings, it > will > > not > > > be possible to disable it. > > > > This should probably not be a flag in code anymore, but a build variable > so we > > can turn off everything if needed. > > We can turn off everything by having |IsReadingListEnabled()| return false. > > For the record, next time I recommend using PSMultiValueSpecifier in the > plist > so that you don't have to update it separately. > Still, lgtm. > > https://codereview.chromium.org/2562643003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
olivierrobin@chromium.org changed reviewers: + sdefresne@chromium.org
PTAL
On 2016/12/08 11:51:57, noyau wrote: > Not for this feature, we need to turn off the compilation and inclusion of > the extension as well. > > On 8 December 2016 at 12:42, <mailto:jif@google.com> wrote: > > > On 2016/12/08 11:32:54, noyau wrote: > > > On 2016/12/08 10:30:53, Olivier Robin wrote: > > > > Please note that until setting is reversed in ExperimentalSettings, it > > will > > > not > > > > be possible to disable it. > > > > > > This should probably not be a flag in code anymore, but a build variable > > so we > > > can turn off everything if needed. > > > > We can turn off everything by having |IsReadingListEnabled()| return false. > > > > For the record, next time I recommend using PSMultiValueSpecifier in the > > plist > > so that you don't have to update it separately. > > Still, lgtm. > > > > https://codereview.chromium.org/2562643003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Oh, right!
lgtm Don't forget to remove the experimental flags downstream.
sdefresne, can you take a look at the gn?
https://codereview.chromium.org/2562643003/diff/20001/components/reading_list... File components/reading_list/core/BUILD.gn (right): https://codereview.chromium.org/2562643003/diff/20001/components/reading_list... components/reading_list/core/BUILD.gn:11: enabled = is_ios && enable_reading_list nit: please name this "_enabled" (variable starting with an underscore are private, thus it won't be passed to the template and won't break if the template ever add a parameter named "enabled"). https://codereview.chromium.org/2562643003/diff/20001/components/reading_list... File components/reading_list/core/reading_list.gni (right): https://codereview.chromium.org/2562643003/diff/20001/components/reading_list... components/reading_list/core/reading_list.gni:5: # This file declares build flags for Reading List. Remove, this is obvious. https://codereview.chromium.org/2562643003/diff/20001/components/reading_list... components/reading_list/core/reading_list.gni:8: enable_reading_list = true Can you add a comment to document this variable? Maybe something like this. # Controls whether reading list support is active or not. Currently only # supported on iOS (on other platforms, the variable is ignored). enable_reading_list = true The reason is that you can get a list of all variables with documentation by using "gn args --list out/xxx". For variables defined in declare_args, the documentation is the comment above the variable definition.
Thanks. PTAL https://codereview.chromium.org/2562643003/diff/20001/components/reading_list... File components/reading_list/core/BUILD.gn (right): https://codereview.chromium.org/2562643003/diff/20001/components/reading_list... components/reading_list/core/BUILD.gn:11: enabled = is_ios && enable_reading_list On 2016/12/09 15:10:59, sdefresne wrote: > nit: please name this "_enabled" (variable starting with an underscore are > private, thus it won't be passed to the template and won't break if the template > ever add a parameter named "enabled"). Done. https://codereview.chromium.org/2562643003/diff/20001/components/reading_list... File components/reading_list/core/reading_list.gni (right): https://codereview.chromium.org/2562643003/diff/20001/components/reading_list... components/reading_list/core/reading_list.gni:5: # This file declares build flags for Reading List. On 2016/12/09 15:10:59, sdefresne wrote: > Remove, this is obvious. Done. https://codereview.chromium.org/2562643003/diff/20001/components/reading_list... components/reading_list/core/reading_list.gni:8: enable_reading_list = true On 2016/12/09 15:10:59, sdefresne wrote: > Can you add a comment to document this variable? Maybe something like this. > > # Controls whether reading list support is active or not. Currently only > # supported on iOS (on other platforms, the variable is ignored). > enable_reading_list = true > > The reason is that you can get a list of all variables with documentation by > using "gn args --list out/xxx". For variables defined in declare_args, the > documentation is the comment above the variable definition. Done.
https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... File components/reading_list/core/BUILD.gn (right): https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... components/reading_list/core/BUILD.gn:10: header_dir = "components/reading_list/core" nit: I think you can remove this as it is the default. https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... File components/reading_list/core/reading_list.gni (right): https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... components/reading_list/core/reading_list.gni:7: # supported on iOS (on other platforms, the variable is ignored). nit: /the variable is ignored/the feature is always disabled/ https://codereview.chromium.org/2562643003/diff/40001/ios/build/chrome_build.gni File ios/build/chrome_build.gni (right): https://codereview.chromium.org/2562643003/diff/40001/ios/build/chrome_build.... ios/build/chrome_build.gni:15: ios_enable_share_extension = enable_reading_list This won't do what you expect it to do (it will take the default value, not the override from args.gn). You need to leave it to true, fix the downstream bot to override both variable, then remove use of ios_enable_share_extension from the code in favor of enable_reading_list, then update the downstream bot to not override ios_enable_share_extension, then remove the variable. In general, you cannot have variable in a declare_args() depend on another variable defined in another declare_args() (it does something, but not what you would expect).
https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... File components/reading_list/core/BUILD.gn (right): https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... components/reading_list/core/BUILD.gn:10: header_dir = "components/reading_list/core" On 2016/12/09 15:37:15, sdefresne wrote: > nit: I think you can remove this as it is the default. Done. https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... File components/reading_list/core/reading_list.gni (right): https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... components/reading_list/core/reading_list.gni:7: # supported on iOS (on other platforms, the variable is ignored). On 2016/12/09 15:37:15, sdefresne wrote: > nit: /the variable is ignored/the feature is always disabled/ Done. https://codereview.chromium.org/2562643003/diff/40001/ios/build/chrome_build.gni File ios/build/chrome_build.gni (right): https://codereview.chromium.org/2562643003/diff/40001/ios/build/chrome_build.... ios/build/chrome_build.gni:15: ios_enable_share_extension = enable_reading_list On 2016/12/09 15:37:15, sdefresne wrote: > This won't do what you expect it to do (it will take the default value, not the > override from args.gn). You need to leave it to true, fix the downstream bot to > override both variable, then remove use of ios_enable_share_extension from the > code in favor of enable_reading_list, then update the downstream bot to not > override ios_enable_share_extension, then remove the variable. > > In general, you cannot have variable in a declare_args() depend on another > variable defined in another declare_args() (it does something, but not what you > would expect). I would like to be able to disable share_extension without disabling reading list (e.g. if there is a signature issue) as Reading list can work without ShareExtension. Can I just let the default to true and ensure manually this stays consistent?
On 2016/12/09 15:47:19, Olivier Robin wrote: > https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... > File components/reading_list/core/BUILD.gn (right): > > https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... > components/reading_list/core/BUILD.gn:10: header_dir = > "components/reading_list/core" > On 2016/12/09 15:37:15, sdefresne wrote: > > nit: I think you can remove this as it is the default. > > Done. > > https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... > File components/reading_list/core/reading_list.gni (right): > > https://codereview.chromium.org/2562643003/diff/40001/components/reading_list... > components/reading_list/core/reading_list.gni:7: # supported on iOS (on other > platforms, the variable is ignored). > On 2016/12/09 15:37:15, sdefresne wrote: > > nit: /the variable is ignored/the feature is always disabled/ > > Done. > > https://codereview.chromium.org/2562643003/diff/40001/ios/build/chrome_build.gni > File ios/build/chrome_build.gni (right): > > https://codereview.chromium.org/2562643003/diff/40001/ios/build/chrome_build.... > ios/build/chrome_build.gni:15: ios_enable_share_extension = enable_reading_list > On 2016/12/09 15:37:15, sdefresne wrote: > > This won't do what you expect it to do (it will take the default value, not > the > > override from args.gn). You need to leave it to true, fix the downstream bot > to > > override both variable, then remove use of ios_enable_share_extension from the > > code in favor of enable_reading_list, then update the downstream bot to not > > override ios_enable_share_extension, then remove the variable. > > > > In general, you cannot have variable in a declare_args() depend on another > > variable defined in another declare_args() (it does something, but not what > you > > would expect). > > I would like to be able to disable share_extension without disabling reading > list (e.g. if there is a signature issue) as Reading list can work without > ShareExtension. > Can I just let the default to true and ensure manually this stays consistent? Yes, this is what I was recommending :-)
lgtm https://codereview.chromium.org/2562643003/diff/60001/components/reading_list... File components/reading_list/core/BUILD.gn (right): https://codereview.chromium.org/2562643003/diff/60001/components/reading_list... components/reading_list/core/BUILD.gn:8: buildflag_header("enable_flags") { nit: please name the target after the header file ("reading_list_enable_flags"), and move it below the "core" target (that is the main target for this BUILD.gn file). https://codereview.chromium.org/2562643003/diff/60001/ios/build/chrome_build.gni File ios/build/chrome_build.gni (right): https://codereview.chromium.org/2562643003/diff/60001/ios/build/chrome_build.... ios/build/chrome_build.gni:8: import("//components/reading_list/core/reading_list.gni") Remove.
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@google.com, noyau@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2562643003/#ps80001 (title: "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: 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 olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org, jif@google.com, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2562643003/#ps100001 (title: "add unittest datatype")
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by olivierrobin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481362572578100,
"parent_rev": "022b7fecf0e0d5fc5abbdc7fcaa15b4482454646", "commit_rev":
"b51b29c90f4b93e9a989e3ee6a2505494bb5f007"}
Message was sent while issue was closed.
Description was changed from ========== Enable RL by default BUG=577659 ========== to ========== Enable RL by default BUG=577659 Review-Url: https://codereview.chromium.org/2562643003 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2564293002/ by sdefresne@chromium.org. The reason for reverting is: Speculative revert as I think this CL broke the downstream autoroller. It is a bit hard to debug as the autoroller was unable to land some CL as the downstream tree was closed, but the last successful roll would have happened at 39e086ae3 [1], and the first failing roll is at a229e828a [2]. This CL is the only one having an impact on the iOS build, so I think it is causing the failures: $ git log --oneline 39e086ae3...a229e828a a229e828a692 Strength-reduce the "scale-factor changed" condition in LayoutSVGRoot d1693364766c Update V8 to version 5.7.202. 7cfbed3def4f Enable RL by default 314c528dc177 Revert of Improve EAT_STREAM_PARAMETERS for Windows x86 (patchset #10 id:240001 of https://codereview.chromium.org/2559323007/ ) [1]: https://uberchromegw.corp.google.com/i/internal.bling.main/builders/trunk-aut... [2]: https://uberchromegw.corp.google.com/i/internal.bling.main/builders/trunk-aut....
Message was sent while issue was closed.
Description was changed from ========== Enable RL by default BUG=577659 Review-Url: https://codereview.chromium.org/2562643003 ========== to ========== Enable RL by default BUG=577659 Review-Url: https://codereview.chromium.org/2562643003 ==========
The problem was fixed. I tested locally (twosided fails to apply issue). https://codereview.chromium.org/2562643003/diff/60001/components/reading_list... File components/reading_list/core/BUILD.gn (right): https://codereview.chromium.org/2562643003/diff/60001/components/reading_list... components/reading_list/core/BUILD.gn:8: buildflag_header("enable_flags") { On 2016/12/09 17:42:10, sdefresne wrote: > nit: please name the target after the header file ("reading_list_enable_flags"), > and move it below the "core" target (that is the main target for this BUILD.gn > file). Done.
Relanding as issue should be fixed.
The CQ bit was checked by olivierrobin@chromium.org
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 ========== Enable RL by default BUG=577659 Review-Url: https://codereview.chromium.org/2562643003 ========== to ========== Enable RL by default BUG=577659 Committed: https://crrev.com/7cfbed3def4f21028e9e3295874c68a522dd63ac Cr-Commit-Position: refs/heads/master@{#437765} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7cfbed3def4f21028e9e3295874c68a522dd63ac Cr-Commit-Position: refs/heads/master@{#437765} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
