Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854403003:
MD Settings: Add feature flag for showing content settings in a per-origin view. (Closed)
DescriptionMD Settings: Add feature flag for showing content settings in a per-origin view.
Add feature flag #enable-site-details. Turning this flag on will (in future)
show UI in MD Settings to list all content settings for a specific origin.
BUG=656758
Review-Url: https://codereview.chromium.org/2854403003
Cr-Commit-Position: refs/heads/master@{#469917}
Committed: https://chromium.googlesource.com/chromium/src/+/325fa4c7d89214f89c3ff66432276130aeb414fb
Patch Set 1 #Patch Set 2 : Add histogram.xml entry. #Patch Set 3 : Fix histogram entry? #
Total comments: 4
Patch Set 4 : Change to a feature instead of a switch. #Patch Set 5 : Fix histograms again. #Patch Set 6 : Rebase? #
Messages
Total messages: 51 (41 generated)
The CQ bit was checked by patricialor@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: linux_chromium_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 patricialor@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...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by patricialor@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.
patricialor@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dom, PTAL for local review? Thanks!
https://codereview.chromium.org/2854403003/diff/40001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2854403003/diff/40001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:258: "Adds UI to view all content settings for a specific origin."; Nit: "Adds UI in MD Settings to view..." ? https://codereview.chromium.org/2854403003/diff/40001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2854403003/diff/40001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:133: extern const char kEnableSiteDetails[]; Instead of having a switch, can you add a feature instead? Features are the "right way" to do this now. For instance, you can have a look at features::kModalPermissionPrompts, which has a chrome://flags entry and can be enabled on the command line with --enable-features=ModalPermissionPrompts. It's not that much different from your CL now: - add the feature in chrome/common/chrome_features* - remove the switch - change the about:flags entry and histograms value to pick up the feature I looked at the CL where site settings added this flag and it looks like they were a bit behind the times when they added it (crrev.com/2409003002)
The CQ bit was checked by patricialor@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: linux_chromium_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 patricialor@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: 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 patricialor@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 checked by patricialor@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/2854403003/diff/40001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2854403003/diff/40001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:258: "Adds UI to view all content settings for a specific origin."; On 2017/05/05 02:58:53, dominickn wrote: > Nit: "Adds UI in MD Settings to view..." ? Oh, that's better-thanks! https://codereview.chromium.org/2854403003/diff/40001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2854403003/diff/40001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:133: extern const char kEnableSiteDetails[]; On 2017/05/05 02:58:53, dominickn wrote: > Instead of having a switch, can you add a feature instead? Features are the > "right way" to do this now. For instance, you can have a look at > features::kModalPermissionPrompts, which has a chrome://flags entry and can be > enabled on the command line with --enable-features=ModalPermissionPrompts. > > It's not that much different from your CL now: > - add the feature in chrome/common/chrome_features* > - remove the switch > - change the about:flags entry and histograms value to pick up the feature > > I looked at the CL where site settings added this flag and it looks like they > were a bit behind the times when they added it (crrev.com/2409003002) Done, thank you!
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
lgtm, thanks!
The CQ bit was checked by patricialor@chromium.org
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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 patricialor@chromium.org
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_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 patricialor@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.
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2854403003/#ps140001 (title: "Rebase?")
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": 140001, "attempt_start_ts": 1494229501201110,
"parent_rev": "4fa0cb2c01a36ea42448397b360dd3c39de36159", "commit_rev":
"325fa4c7d89214f89c3ff66432276130aeb414fb"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Add feature flag for showing content settings in a per-origin view. Add feature flag #enable-site-details. Turning this flag on will (in future) show UI in MD Settings to list all content settings for a specific origin. BUG=656758 ========== to ========== MD Settings: Add feature flag for showing content settings in a per-origin view. Add feature flag #enable-site-details. Turning this flag on will (in future) show UI in MD Settings to list all content settings for a specific origin. BUG=656758 Review-Url: https://codereview.chromium.org/2854403003 Cr-Commit-Position: refs/heads/master@{#469917} Committed: https://chromium.googlesource.com/chromium/src/+/325fa4c7d89214f89c3ff6643227... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/325fa4c7d89214f89c3ff6643227... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
