Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806263005:
Convert the overlay scrollbar flag to use FeatureList (Closed)
|
Created:
3 years, 8 months ago by chaopeng Modified:
3 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, vakh+watch_chromium.org, piman+watch_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, nasko+codewatch_chromium.org, jam, timvolodine, mathp+autofillwatch_chromium.org, blink-reviews, darin-cc_chromium.org, asvitkine+watch_chromium.org, grt+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert the overlay scrollbar flag to use FeatureList
Current implementation uses an old-styled field trial flag.
To make server side configurations and client side flag handling easy
and clear, convert the field trial flag to use FeatureList.
BUG=710060
Review-Url: https://codereview.chromium.org/2806263005
Cr-Original-Commit-Position: refs/heads/master@{#465867}
Committed: https://chromium.googlesource.com/chromium/src/+/f8b7dc953a79f8565fde8cd22642b000a236d5a5
Review-Url: https://codereview.chromium.org/2806263005
Cr-Commit-Position: refs/heads/master@{#466032}
Committed: https://chromium.googlesource.com/chromium/src/+/d3ca34febe2838703bfd71217be83fb26e372257
Patch Set 1 : Convert the overlay scrollbar flag to use FeatureList #Patch Set 2 : remove overlay scrollbar switch in SitePerProcessBrowserTest #Patch Set 3 : fix for android and SitePerProcessBrowserTest #Patch Set 4 : update histograms.xml #
Total comments: 7
Patch Set 5 : add disable overlay scrollbar back to site_per_process_browsertest.cc #
Total comments: 6
Patch Set 6 : dtapuska comment addressed and fix SitePerProcessBrowserTest #Patch Set 7 : rebase update #Patch Set 8 : dtapuska comment addressed #
Total comments: 6
Patch Set 9 : dgozman and pkasting comments addressed #Patch Set 10 : add test for scoped_feature_list #
Total comments: 9
Patch Set 11 : dcheng comment addressed #
Total comments: 3
Patch Set 12 : dcheng comment#58 addressed #Patch Set 13 : for try bot #Patch Set 14 : remove ScopedFeatureList change #
Total comments: 2
Patch Set 15 : Ilya comments addressed #Patch Set 16 : move feature_list_ change to SetUpCommandLine #Messages
Total messages: 103 (63 generated)
The CQ bit was checked by chaopeng@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...
chaopeng@chromium.org changed reviewers: + dtapuska@chromium.org
dtapuska@ Do I need to change Histograms.xml?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...) 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 chaopeng@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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_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 chaopeng@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...
Description was changed from ========== Convert the overlay scrollbar flag to use FeatureList Current implementation uses an old-styled field trial flag. To make server side configurations and client side flag handling easy and clear, convert the field trial flag to use FeatureList. BUG=710060 ========== to ========== Convert the overlay scrollbar flag to use FeatureList Current implementation uses an old-styled field trial flag. To make server side configurations and client side flag handling easy and clear, convert the field trial flag to use FeatureList. BUG=710060 ==========
chaopeng@chromium.org changed reviewers: + tdresser@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 chaopeng@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...
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
https://codereview.chromium.org/2806263005/diff/80001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2806263005/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:621: } I added that to disable overlays in a specific test. Don't you need to do that in another way now? https://codereview.chromium.org/2806263005/diff/80001/ui/native_theme/native_... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/80001/ui/native_theme/native_... ui/native_theme/native_theme_features.cc:12: #if defined(OS_ANDROID) Shouldn't ChromeOS also be enabled by default?
https://codereview.chromium.org/2806263005/diff/80001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2806263005/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:621: } On 2017/04/11 19:11:42, bokan wrote: > I added that to disable overlays in a specific test. Don't you need to do that > in another way now? I think I should add it back. https://codereview.chromium.org/2806263005/diff/80001/ui/native_theme/native_... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/80001/ui/native_theme/native_... ui/native_theme/native_theme_features.cc:12: #if defined(OS_ANDROID) On 2017/04/11 19:11:42, bokan wrote: > Shouldn't ChromeOS also be enabled by default? We can enable on finch configuration for now. Then change here when we land it.
https://codereview.chromium.org/2806263005/diff/80001/ui/native_theme/native_... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/80001/ui/native_theme/native_... ui/native_theme/native_theme_features.cc:12: #if defined(OS_ANDROID) On 2017/04/11 19:16:48, chaopeng wrote: > On 2017/04/11 19:11:42, bokan wrote: > > Shouldn't ChromeOS also be enabled by default? > > We can enable on finch configuration for now. Then change here when we land it. I think the idea is that we'd use finch to disable it if needed. In which case, why not just make it enabled by default here - then I don't think we need to do anything in finch to enable it...just if we want to disable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. Thank you. https://codereview.chromium.org/2806263005/diff/80001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2806263005/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:621: } On 2017/04/11 19:11:42, bokan wrote: > I added that to disable overlays in a specific test. Don't you need to do that > in another way now? Done. https://codereview.chromium.org/2806263005/diff/80001/ui/native_theme/native_... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/80001/ui/native_theme/native_... ui/native_theme/native_theme_features.cc:12: #if defined(OS_ANDROID) On 2017/04/11 19:26:31, bokan wrote: > On 2017/04/11 19:16:48, chaopeng wrote: > > On 2017/04/11 19:11:42, bokan wrote: > > > Shouldn't ChromeOS also be enabled by default? > > > > We can enable on finch configuration for now. Then change here when we land > it. > > I think the idea is that we'd use finch to disable it if needed. In which case, > why not just make it enabled by default here - then I don't think we need to do > anything in finch to enable it...just if we want to disable. Done.
The CQ bit was checked by chaopeng@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.
https://codereview.chromium.org/2806263005/diff/100001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2806263005/diff/100001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:635: disabled_features += "," + std::string(features::kOverlayScrollbar.name); How do you know disable_features actually has anything? Would it make more sense to follow code like: https://cs.chromium.org/chromium/src/components/subresource_filter/core/brows... Where it initializes the scoped feature list from a feature list? https://codereview.chromium.org/2806263005/diff/100001/ui/native_theme/native... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/100001/ui/native_theme/native... ui/native_theme/native_theme_features.cc:13: const base::Feature kOverlayScrollbar{"OverlayScrollbar", I think it is probably more consistent to define OVERLAY_SCROLLBAR_ENABLED_BY_DEFAULT but keep set it inside the define. But keey the definition of base::Feature kOverlayScrollbar out.. Understand? https://codereview.chromium.org/2806263005/diff/100001/ui/native_theme/native... File ui/native_theme/native_theme_features.h (left): https://codereview.chromium.org/2806263005/diff/100001/ui/native_theme/native... ui/native_theme/native_theme_features.h:7: #ifndef UI_NATIVE_THEME_NATIVE_THEME_SWITCHES_H_ You need to change the defines here to match the new filename.
Updated. PTAL. Thank you. https://codereview.chromium.org/2806263005/diff/100001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2806263005/diff/100001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:635: disabled_features += "," + std::string(features::kOverlayScrollbar.name); On 2017/04/12 17:21:02, dtapuska wrote: > How do you know disable_features actually has anything? > > Would it make more sense to follow code like: > https://cs.chromium.org/chromium/src/components/subresource_filter/core/brows... > > Where it initializes the scoped feature list from a feature list? Can not make this work as the link. https://codereview.chromium.org/2806263005/diff/100001/ui/native_theme/native... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/100001/ui/native_theme/native... ui/native_theme/native_theme_features.cc:13: const base::Feature kOverlayScrollbar{"OverlayScrollbar", On 2017/04/12 17:21:02, dtapuska wrote: > I think it is probably more consistent to define > OVERLAY_SCROLLBAR_ENABLED_BY_DEFAULT but keep set it inside the define. But keey > the definition of base::Feature kOverlayScrollbar out.. Understand? Done. https://codereview.chromium.org/2806263005/diff/100001/ui/native_theme/native... File ui/native_theme/native_theme_features.h (left): https://codereview.chromium.org/2806263005/diff/100001/ui/native_theme/native... ui/native_theme/native_theme_features.h:7: #ifndef UI_NATIVE_THEME_NATIVE_THEME_SWITCHES_H_ On 2017/04/12 17:21:02, dtapuska wrote: > You need to change the defines here to match the new filename. Done.
lgtm from me but please wait for Dave's review as I don't know about FeatureList.
The CQ bit was checked by chaopeng@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_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_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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 chaopeng@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.
On 2017/04/12 21:01:08, bokan wrote: > lgtm from me but please wait for Dave's review as I don't know about > FeatureList. lgtm
ericwilligers@chromium.org: Please review changes in histograms.xml LoginCustomFlags added. mathp@chromium.org: Please review changes in chrome/renderer/autofill/form_classifier_browsertest.cc It is a header file rename. mattm@chromium.org: Please review changes in chrome/renderer/safe_browsing/ It is a header file rename. dgozman@chromium.org: Please review changes in content/ pkasting@chromium.org: Please review changes in ui/native_theme/
chaopeng@chromium.org changed reviewers: + isherman@chromium.org - ericwilligers@chromium.org
isherman@ PTAL at histograms.xml
https://codereview.chromium.org/2806263005/diff/160001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2806263005/diff/160001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:644: feature_list_.InitFromCommandLine(enabled_features, disabled_features); Let's add a method similar to ScopedFeatureList::InitWithFeatures: InitWithFeatureListAndOverrides(list, enabled_features, disabled_features) https://codereview.chromium.org/2806263005/diff/160001/ui/native_theme/native... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/160001/ui/native_theme/native... ui/native_theme/native_theme_features.cc:9: #if defined(OS_ANDROID) || defined(OS_CHROMEOS) I didn't find where this feature was enabled by default on ChromeOS before. Am I missing something?
safe_browsing lgtm
LGTM https://codereview.chromium.org/2806263005/diff/160001/ui/native_theme/native... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/160001/ui/native_theme/native... ui/native_theme/native_theme_features.cc:10: const base::FeatureState kOverlayScrollbarFeatureState = Nit: Prefer constexpr to const for compile-time constants. https://codereview.chromium.org/2806263005/diff/160001/ui/native_theme/native... ui/native_theme/native_theme_features.cc:22: } // namespace features Nit: Blank line above?
dgozman@ Updated, PTAL. Thank you. https://codereview.chromium.org/2806263005/diff/160001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2806263005/diff/160001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:644: feature_list_.InitFromCommandLine(enabled_features, disabled_features); On 2017/04/13 17:33:28, dgozman wrote: > Let's add a method similar to ScopedFeatureList::InitWithFeatures: > > InitWithFeatureListAndOverrides(list, enabled_features, disabled_features) Done. https://codereview.chromium.org/2806263005/diff/160001/ui/native_theme/native... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/160001/ui/native_theme/native... ui/native_theme/native_theme_features.cc:9: #if defined(OS_ANDROID) || defined(OS_CHROMEOS) On 2017/04/13 17:33:28, dgozman wrote: > I didn't find where this feature was enabled by default on ChromeOS before. Am I > missing something? The CrOS enable by default is in here before. https://cs.chromium.org/chromium/src/ui/native_theme/native_theme_switches.cc...
Thank you. lgtm
histograms.xml lgtm
chaopeng@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@chromium.org: Please review changes in base/ Thank you.
I'm also trying to understand the overall purpose of the new test helper: why isn't something like ScopedFeatureList::InitAndDisableFeature sufficient here? https://codereview.chromium.org/2806263005/diff/200001/base/test/scoped_featu... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2806263005/diff/200001/base/test/scoped_featu... base/test/scoped_feature_list.cc:78: void override_feature(const std::string& feature, Nit: OverrideFeature https://codereview.chromium.org/2806263005/diff/200001/base/test/scoped_featu... base/test/scoped_feature_list.cc:79: std::vector<std::string>& check_list, Output parameters in Chromium should pass by pointer, not mutable reference. https://codereview.chromium.org/2806263005/diff/200001/base/test/scoped_featu... base/test/scoped_feature_list.cc:111: if (feature_list) { Why do we have to handle the null case? I'm also not sure why we plumb this in, if we never use |feature_list| anyway and just consult FeatureList::GetInstance() https://codereview.chromium.org/2806263005/diff/200001/base/test/scoped_featu... base/test/scoped_feature_list.cc:117: std::vector<std::string> enabled_features_list = SplitString( How about SplitStringPiece? https://codereview.chromium.org/2806263005/diff/200001/ui/native_theme/native... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/200001/ui/native_theme/native... ui/native_theme/native_theme_features.cc:20: constexpr base::Feature kOverlayScrollbar{"OverlayScrollbar", This is declared as extern const in the header, but constexpr here? Does that work? IIRC it's non-trivial to get an extern constexpr object... so what's actually happening here?
> I'm also trying to understand the overall purpose of the new test helper: why isn't something like ScopedFeatureList::InitAndDisableFeature sufficient here? Because ScopedFeatureList::InitAndDisableFeature will remove all feature setting from command line. It will hide the potential failure when we setup a new feature trybot. I don't think that is good for integration test. Updated. PTAL. Thank you. https://codereview.chromium.org/2806263005/diff/200001/base/test/scoped_featu... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2806263005/diff/200001/base/test/scoped_featu... base/test/scoped_feature_list.cc:78: void override_feature(const std::string& feature, On 2017/04/14 23:30:17, dcheng wrote: > Nit: OverrideFeature Done. https://codereview.chromium.org/2806263005/diff/200001/base/test/scoped_featu... base/test/scoped_feature_list.cc:79: std::vector<std::string>& check_list, On 2017/04/14 23:30:17, dcheng wrote: > Output parameters in Chromium should pass by pointer, not mutable reference. Done. https://codereview.chromium.org/2806263005/diff/200001/base/test/scoped_featu... base/test/scoped_feature_list.cc:111: if (feature_list) { On 2017/04/14 23:30:17, dcheng wrote: > Why do we have to handle the null case? I'm also not sure why we plumb this in, > if we never use |feature_list| anyway and just consult > FeatureList::GetInstance() For the null case, we just need to write the overrides to feature_list. https://codereview.chromium.org/2806263005/diff/200001/ui/native_theme/native... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/200001/ui/native_theme/native... ui/native_theme/native_theme_features.cc:20: constexpr base::Feature kOverlayScrollbar{"OverlayScrollbar", On 2017/04/14 23:30:17, dcheng wrote: > This is declared as extern const in the header, but constexpr here? Does that > work? IIRC it's non-trivial to get an extern constexpr object... so what's > actually happening here? It works. I just follow the declaration from this file and it seems we use this for every feature declaration.
autofill lgtm
On 2017/04/15 00:12:54, chaopeng wrote: > > I'm also trying to understand the overall purpose of the new test helper: why > isn't something like ScopedFeatureList::InitAndDisableFeature sufficient here? > > Because ScopedFeatureList::InitAndDisableFeature will remove all feature setting > from command line. It will hide the potential failure when we setup a new > feature trybot. I don't think that is good for integration test. > OK, but isn't that a general problem of this class? Why fix it only for this instance of the problem, instead of addressing it more generally? And if it's not a general problem, why do we need this new code? https://codereview.chromium.org/2806263005/diff/220001/base/test/scoped_featu... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2806263005/diff/220001/base/test/scoped_featu... base/test/scoped_feature_list.cc:111: if (feature_list) { I still don't understand why we pass in feature_list, it's completely unused other than this null check. https://codereview.chromium.org/2806263005/diff/220001/base/test/scoped_featu... base/test/scoped_feature_list.cc:131: } I think it just feels weird that: 1. GetFeatureOverrides() turns its internal override map into command-line strings. 2. which we need to crack and parse here. 3. which we then feed back to FeatureList 4. which has to parse the input and turn it back into a list of overrides I would suggest skipping steps 2 and 3 and just concatenating any input features with the command-line that already exists. WDYT?
isherman@ PTAL at feature_list
https://codereview.chromium.org/2806263005/diff/220001/base/test/scoped_featu... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2806263005/diff/220001/base/test/scoped_featu... base/test/scoped_feature_list.cc:131: } On 2017/04/18 00:18:05, dcheng (OOO through May 2) wrote: > I think it just feels weird that: > 1. GetFeatureOverrides() turns its internal override map into command-line > strings. > 2. which we need to crack and parse here. > 3. which we then feed back to FeatureList > 4. which has to parse the input and turn it back into a list of overrides > > I would suggest skipping steps 2 and 3 and just concatenating any input features > with the command-line that already exists. WDYT? I don't think this comment has been addressed. Basically, I'm proposing that the input is just additional_disabled_features and additional_enabled_features, and we just smash the the current disabled + additional disabled together with a , to separate them; same for the current enabled + additional enabled.
On 2017/04/18 15:54:57, dcheng (OOO through May 2) wrote: > https://codereview.chromium.org/2806263005/diff/220001/base/test/scoped_featu... > File base/test/scoped_feature_list.cc (right): > > https://codereview.chromium.org/2806263005/diff/220001/base/test/scoped_featu... > base/test/scoped_feature_list.cc:131: } > On 2017/04/18 00:18:05, dcheng (OOO through May 2) wrote: > > I think it just feels weird that: > > 1. GetFeatureOverrides() turns its internal override map into command-line > > strings. > > 2. which we need to crack and parse here. > > 3. which we then feed back to FeatureList > > 4. which has to parse the input and turn it back into a list of overrides > > > > I would suggest skipping steps 2 and 3 and just concatenating any input > features > > with the command-line that already exists. WDYT? > > I don't think this comment has been addressed. > > Basically, I'm proposing that the input is just additional_disabled_features and > additional_enabled_features, and we just smash the the current disabled + > additional disabled together with a , to separate them; same for the current > enabled + additional enabled. But the override_disabled_feature maybe already in current enabled.
On 2017/04/18 15:58:31, chaopeng wrote: > On 2017/04/18 15:54:57, dcheng (OOO through May 2) wrote: > > > https://codereview.chromium.org/2806263005/diff/220001/base/test/scoped_featu... > > File base/test/scoped_feature_list.cc (right): > > > > > https://codereview.chromium.org/2806263005/diff/220001/base/test/scoped_featu... > > base/test/scoped_feature_list.cc:131: } > > On 2017/04/18 00:18:05, dcheng (OOO through May 2) wrote: > > > I think it just feels weird that: > > > 1. GetFeatureOverrides() turns its internal override map into command-line > > > strings. > > > 2. which we need to crack and parse here. > > > 3. which we then feed back to FeatureList > > > 4. which has to parse the input and turn it back into a list of overrides > > > > > > I would suggest skipping steps 2 and 3 and just concatenating any input > > features > > > with the command-line that already exists. WDYT? > > > > I don't think this comment has been addressed. > > > > Basically, I'm proposing that the input is just additional_disabled_features > and > > additional_enabled_features, and we just smash the the current disabled + > > additional disabled together with a , to separate them; same for the current > > enabled + additional enabled. > > But the override_disabled_feature maybe already in current enabled. OK I guess I misread the code. To be honest, it seems like FeatureList should just expose its internals to ScopedFeatureList. It still doesn't make sense to me to parse and unparse command-lines multiple times like this, when we already have the information. But I'll defer to isherman@
On 2017/04/15 00:12:54, chaopeng wrote: > > I'm also trying to understand the overall purpose of the new test helper: why > isn't something like ScopedFeatureList::InitAndDisableFeature sufficient here? > > Because ScopedFeatureList::InitAndDisableFeature will remove all feature setting > from command line. It will hide the potential failure when we setup a new > feature trybot. I don't think that is good for integration test. Sorry, I'm also not following the motivation here. Why does InitWithFeatures() not suffice for your needs? I agree with Daniel that either all of the Init() methods should probably be updated to override, or none of them should. I'm having a hard time understanding why some tests might want this behavior and others not.
On 2017/04/18 22:56:38, Ilya Sherman wrote: > On 2017/04/15 00:12:54, chaopeng wrote: > > > I'm also trying to understand the overall purpose of the new test helper: > why > > isn't something like ScopedFeatureList::InitAndDisableFeature sufficient here? > > > > Because ScopedFeatureList::InitAndDisableFeature will remove all feature > setting > > from command line. It will hide the potential failure when we setup a new > > feature trybot. I don't think that is good for integration test. > > Sorry, I'm also not following the motivation here. Why does InitWithFeatures() > not suffice for your needs? I agree with Daniel that either all of the Init() > methods should probably be updated to override, or none of them should. I'm > having a hard time understanding why some tests might want this behavior and > others not. Because ScopedFeatureList::InitAndDisableFeature will remove all feature setting from command line. It will hide the potential failure when we setup a new feature trybot. I think it is ok for unittest but not for integration test.
On 2017/04/18 23:29:54, chaopeng wrote: > On 2017/04/18 22:56:38, Ilya Sherman wrote: > > On 2017/04/15 00:12:54, chaopeng wrote: > > > > I'm also trying to understand the overall purpose of the new test helper: > > why > > > isn't something like ScopedFeatureList::InitAndDisableFeature sufficient > here? > > > > > > Because ScopedFeatureList::InitAndDisableFeature will remove all feature > > setting > > > from command line. It will hide the potential failure when we setup a new > > > feature trybot. I don't think that is good for integration test. > > > > Sorry, I'm also not following the motivation here. Why does > InitWithFeatures() > > not suffice for your needs? I agree with Daniel that either all of the Init() > > methods should probably be updated to override, or none of them should. I'm > > having a hard time understanding why some tests might want this behavior and > > others not. > > Because ScopedFeatureList::InitAndDisableFeature will remove all feature setting > from command line. It will hide the potential failure when we setup a new > feature trybot. I think it is ok for unittest but not for integration test. (a) What is a feature trybot? (b) Would there be any harm to respecting the existing command line for unit tests? Assuming we do need the new functionality for your use case, it would be simpler to migrate /all/ users of the API to the new functionality -- that's less code to maintain. Is there any reason not to do so?
On 2017/04/19 00:21:39, Ilya Sherman wrote: > On 2017/04/18 23:29:54, chaopeng wrote: > > On 2017/04/18 22:56:38, Ilya Sherman wrote: > > > On 2017/04/15 00:12:54, chaopeng wrote: > > > > > I'm also trying to understand the overall purpose of the new test > helper: > > > why > > > > isn't something like ScopedFeatureList::InitAndDisableFeature sufficient > > here? > > > > > > > > Because ScopedFeatureList::InitAndDisableFeature will remove all feature > > > setting > > > > from command line. It will hide the potential failure when we setup a new > > > > feature trybot. I don't think that is good for integration test. > > > > > > Sorry, I'm also not following the motivation here. Why does > > InitWithFeatures() > > > not suffice for your needs? I agree with Daniel that either all of the > Init() > > > methods should probably be updated to override, or none of them should. I'm > > > having a hard time understanding why some tests might want this behavior and > > > others not. > > > > Because ScopedFeatureList::InitAndDisableFeature will remove all feature > setting > > from command line. It will hide the potential failure when we setup a new > > feature trybot. I think it is ok for unittest but not for integration test. > > (a) What is a feature trybot? > (b) Would there be any harm to respecting the existing command line for unit > tests? Assuming we do need the new functionality for your use case, it would be > simpler to migrate /all/ users of the API to the new functionality -- that's > less code to maintain. Is there any reason not to do so? a) From the feature api's design doc. "The Python code that launches perf bots based on the above config will be modified to generate --disable-features= and --enable-features= command-line flags based on the config." So ScopedFeatureList::InitAndDisableFeature will remove the config from trybot settings (testing/variations/fieldtrial_testing_config.json). b) Would there be any harm to respecting the existing command line for unit tests? It need more investigation. We can search all ScopedFeatureList::InitXXX() call and replace it.
On 2017/04/19 02:43:08, chaopeng wrote: > On 2017/04/19 00:21:39, Ilya Sherman wrote: > > On 2017/04/18 23:29:54, chaopeng wrote: > > > On 2017/04/18 22:56:38, Ilya Sherman wrote: > > > > On 2017/04/15 00:12:54, chaopeng wrote: > > > > > > I'm also trying to understand the overall purpose of the new test > > helper: > > > > why > > > > > isn't something like ScopedFeatureList::InitAndDisableFeature sufficient > > > here? > > > > > > > > > > Because ScopedFeatureList::InitAndDisableFeature will remove all feature > > > > setting > > > > > from command line. It will hide the potential failure when we setup a > new > > > > > feature trybot. I don't think that is good for integration test. > > > > > > > > Sorry, I'm also not following the motivation here. Why does > > > InitWithFeatures() > > > > not suffice for your needs? I agree with Daniel that either all of the > > Init() > > > > methods should probably be updated to override, or none of them should. > I'm > > > > having a hard time understanding why some tests might want this behavior > and > > > > others not. > > > > > > Because ScopedFeatureList::InitAndDisableFeature will remove all feature > > setting > > > from command line. It will hide the potential failure when we setup a new > > > feature trybot. I think it is ok for unittest but not for integration test. > > > > (a) What is a feature trybot? > > (b) Would there be any harm to respecting the existing command line for unit > > tests? Assuming we do need the new functionality for your use case, it would > be > > simpler to migrate /all/ users of the API to the new functionality -- that's > > less code to maintain. Is there any reason not to do so? > > a) From the feature api's design doc. "The Python code that launches perf bots > based on the above config will be modified to generate --disable-features= and > --enable-features= command-line flags based on the config." So > ScopedFeatureList::InitAndDisableFeature will remove the config from trybot > settings (testing/variations/fieldtrial_testing_config.json). I agree, and I'll reiterate: This is true for all of the code that's currently using the ScopedFeatureList. How is your use case different? It seems like we're talking past each other somewhat in these CL comments. Would it help to schedule a brief meeting to have a more interactive, and hopefully therefore clearer, conversation? > b) Would there be any harm to respecting the existing command line for unit > tests? It need more investigation. We can search all > ScopedFeatureList::InitXXX() call and replace it.
On 2017/04/19 05:55:02, Ilya Sherman wrote: > I agree, and I'll reiterate: This is true for all of the code that's currently > using the ScopedFeatureList. How is your use case different? > > It seems like we're talking past each other somewhat in these CL comments. > Would it help to schedule a brief meeting to have a more interactive, and > hopefully therefore clearer, conversation? Just getting caught up here. I just chatted with chaopeng@ in person and I think we're basically in agreement but there's been some miscommunication. Let me see if I can clarify: The current situation is that using ScopedFeatureList resets to an empty feature list and then enables/disables an explicit list of features. chaopeng's point is that's never what you want for browser tests since it effectively overrides higher-level test configurations (e.g. those in fieldtrial_testing_config.json, or a bot set up to specifically test a feature). I think what Ilya and Daniel are saying is that it's never what you want for *any* tests and we should fix it everywhere. So, if a test is running with some features enabled/disabled from the command line, we should always respect those features unless ScopedFeatureList explicitly turns one of them off or on. Otherwise, it should just append the enable/disable to the list of features passed from the command line. Does that match everyone's understanding/opinion? If so, I'd propose we don't change ScopedFeatureList in this CL and use InitAndDisableFeature for our needs here to keep this CL focused (and we'll need to merge this back to M59 so the smaller the better). After this lands, in a followup patch, we'll change ScopedFeatureList to work in the way described above so that all tests using ScopedFeatureList prevent clobbering features specified on the command line. WDYT? Ilya, if I still missed the mark feel free to setup a VC between the three of us so we can get a better understanding. Thanks all.
The CQ bit was checked by chaopeng@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #13 (id:260001) has been deleted
The CQ bit was checked by chaopeng@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #13 (id:280001) has been deleted
The CQ bit was checked by chaopeng@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...
On 2017/04/19 14:49:38, bokan wrote: > On 2017/04/19 05:55:02, Ilya Sherman wrote: > > I agree, and I'll reiterate: This is true for all of the code that's currently > > using the ScopedFeatureList. How is your use case different? > > > > It seems like we're talking past each other somewhat in these CL comments. > > Would it help to schedule a brief meeting to have a more interactive, and > > hopefully therefore clearer, conversation? > > Just getting caught up here. I just chatted with chaopeng@ in person and I think > we're basically in agreement but there's been some miscommunication. Let me see > if I can clarify: > > The current situation is that using ScopedFeatureList resets to an empty feature > list and then enables/disables an explicit list of features. > > chaopeng's point is that's never what you want for browser tests since it > effectively overrides higher-level test configurations (e.g. those in > fieldtrial_testing_config.json, or a bot set up to specifically test a feature). > > I think what Ilya and Daniel are saying is that it's never what you want for > *any* tests and we should fix it everywhere. > > So, if a test is running with some features enabled/disabled from the command > line, we should always respect those features unless ScopedFeatureList > explicitly turns one of them off or on. Otherwise, it should just append the > enable/disable to the list of features passed from the command line. > > Does that match everyone's understanding/opinion? If so, I'd propose we don't > change ScopedFeatureList in this CL and use InitAndDisableFeature for our needs > here to keep this CL focused (and we'll need to merge this back to M59 so the > smaller the better). After this lands, in a followup patch, we'll change > ScopedFeatureList to work in the way described above so that all tests using > ScopedFeatureList prevent clobbering features specified on the command line. > WDYT? > > Ilya, if I still missed the mark feel free to setup a VC between the three of us > so we can get a better understanding. > > Thanks all. That sounds spot-on, and splitting off the ScopedFeatureList changes into a separate CL sounds good to me. Thanks for chiming in!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by chaopeng@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...
native_theme_features.* lgtm https://codereview.chromium.org/2806263005/diff/320001/ui/native_theme/native... File ui/native_theme/native_theme_features.cc (right): https://codereview.chromium.org/2806263005/diff/320001/ui/native_theme/native... ui/native_theme/native_theme_features.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2806263005/diff/320001/ui/native_theme/native... ui/native_theme/native_theme_features.cc:18: // or Linux. The status of native UI overlay scrollbars are determined in nit: s/are/is
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 chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, dtapuska@chromium.org, pkasting@chromium.org, mattm@chromium.org, dgozman@chromium.org, mathp@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2806263005/#ps340001 (title: "Ilya comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/19 14:49:38, bokan wrote: > On 2017/04/19 05:55:02, Ilya Sherman wrote: > > I agree, and I'll reiterate: This is true for all of the code that's currently > > using the ScopedFeatureList. How is your use case different? > > > > It seems like we're talking past each other somewhat in these CL comments. > > Would it help to schedule a brief meeting to have a more interactive, and > > hopefully therefore clearer, conversation? > > Just getting caught up here. I just chatted with chaopeng@ in person and I think > we're basically in agreement but there's been some miscommunication. Let me see > if I can clarify: > > The current situation is that using ScopedFeatureList resets to an empty feature > list and then enables/disables an explicit list of features. > > chaopeng's point is that's never what you want for browser tests since it > effectively overrides higher-level test configurations (e.g. those in > fieldtrial_testing_config.json, or a bot set up to specifically test a feature). > > I think what Ilya and Daniel are saying is that it's never what you want for > *any* tests and we should fix it everywhere. > > So, if a test is running with some features enabled/disabled from the command > line, we should always respect those features unless ScopedFeatureList > explicitly turns one of them off or on. Otherwise, it should just append the > enable/disable to the list of features passed from the command line. > > Does that match everyone's understanding/opinion? If so, I'd propose we don't > change ScopedFeatureList in this CL and use InitAndDisableFeature for our needs > here to keep this CL focused (and we'll need to merge this back to M59 so the > smaller the better). After this lands, in a followup patch, we'll change > ScopedFeatureList to work in the way described above so that all tests using > ScopedFeatureList prevent clobbering features specified on the command line. > WDYT? > > Ilya, if I still missed the mark feel free to setup a VC between the three of us > so we can get a better understanding. > > Thanks all. Thanks for clarifying this. Makes sense to me as well.
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1492648746335820,
"parent_rev": "263834d72b249d94e0c60896d1568c3b30344821", "commit_rev":
"f8b7dc953a79f8565fde8cd22642b000a236d5a5"}
Message was sent while issue was closed.
Description was changed from ========== Convert the overlay scrollbar flag to use FeatureList Current implementation uses an old-styled field trial flag. To make server side configurations and client side flag handling easy and clear, convert the field trial flag to use FeatureList. BUG=710060 ========== to ========== Convert the overlay scrollbar flag to use FeatureList Current implementation uses an old-styled field trial flag. To make server side configurations and client side flag handling easy and clear, convert the field trial flag to use FeatureList. BUG=710060 Review-Url: https://codereview.chromium.org/2806263005 Cr-Commit-Position: refs/heads/master@{#465867} Committed: https://chromium.googlesource.com/chromium/src/+/f8b7dc953a79f8565fde8cd22642... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:340001) as https://chromium.googlesource.com/chromium/src/+/f8b7dc953a79f8565fde8cd22642...
Message was sent while issue was closed.
On 2017/04/20 02:23:59, commit-bot: I haz the power wrote: > Committed patchset #15 (id:340001) as > https://chromium.googlesource.com/chromium/src/+/f8b7dc953a79f8565fde8cd22642... Revert CL here: https://codereview.chromium.org/2828183002 Seems to have caused many test failures on Linux TSan: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20TSan%...
Message was sent while issue was closed.
Description was changed from ========== Convert the overlay scrollbar flag to use FeatureList Current implementation uses an old-styled field trial flag. To make server side configurations and client side flag handling easy and clear, convert the field trial flag to use FeatureList. BUG=710060 Review-Url: https://codereview.chromium.org/2806263005 Cr-Commit-Position: refs/heads/master@{#465867} Committed: https://chromium.googlesource.com/chromium/src/+/f8b7dc953a79f8565fde8cd22642... ========== to ========== Convert the overlay scrollbar flag to use FeatureList Current implementation uses an old-styled field trial flag. To make server side configurations and client side flag handling easy and clear, convert the field trial flag to use FeatureList. BUG=710060 Review-Url: https://codereview.chromium.org/2806263005 Cr-Commit-Position: refs/heads/master@{#465867} Committed: https://chromium.googlesource.com/chromium/src/+/f8b7dc953a79f8565fde8cd22642... ==========
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, mathp@chromium.org, dtapuska@chromium.org, mattm@chromium.org, bokan@chromium.org, dgozman@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2806263005/#ps360001 (title: "move feature_list_ change to SetUpCommandLine")
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": 360001, "attempt_start_ts": 1492698568854410,
"parent_rev": "723dbf022b3038d9241fbf7767b46b61c0085e39", "commit_rev":
"d3ca34febe2838703bfd71217be83fb26e372257"}
Message was sent while issue was closed.
Description was changed from ========== Convert the overlay scrollbar flag to use FeatureList Current implementation uses an old-styled field trial flag. To make server side configurations and client side flag handling easy and clear, convert the field trial flag to use FeatureList. BUG=710060 Review-Url: https://codereview.chromium.org/2806263005 Cr-Commit-Position: refs/heads/master@{#465867} Committed: https://chromium.googlesource.com/chromium/src/+/f8b7dc953a79f8565fde8cd22642... ========== to ========== Convert the overlay scrollbar flag to use FeatureList Current implementation uses an old-styled field trial flag. To make server side configurations and client side flag handling easy and clear, convert the field trial flag to use FeatureList. BUG=710060 Review-Url: https://codereview.chromium.org/2806263005 Cr-Original-Commit-Position: refs/heads/master@{#465867} Committed: https://chromium.googlesource.com/chromium/src/+/f8b7dc953a79f8565fde8cd22642... Review-Url: https://codereview.chromium.org/2806263005 Cr-Commit-Position: refs/heads/master@{#466032} Committed: https://chromium.googlesource.com/chromium/src/+/d3ca34febe2838703bfd71217be8... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:360001) as https://chromium.googlesource.com/chromium/src/+/d3ca34febe2838703bfd71217be8... |
