Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1206323003:
Use SINGLE_DISABLE_VALUE_TYPE for disable flags and update flag names. (Closed)
DescriptionUse SINGLE_DISABLE_VALUE_TYPE for disable flags and update flag names.
Uses disable type flags to make the link to turn off default enabled experiments
say "Disable". Also updates enable flag titles to be consistent and not imply
enable or disable.
See screenshot in bug for the difference.
BUG=245319
TEST=Visual, see flags on chrome://flags
Committed: https://crrev.com/88fd51293c02b73560b00c36fc0c8b49a60939ef
Cr-Commit-Position: refs/heads/master@{#361573}
Patch Set 1 #Patch Set 2 : Fix hyperlink auditing flag and merge with master. #Patch Set 3 : Merge with master. #Patch Set 4 : Merge with master #Patch Set 5 : Merge with master. #
Total comments: 24
Patch Set 6 : Review comments. #Patch Set 7 : Merge #
Messages
Total messages: 30 (12 generated)
flackr@chromium.org changed reviewers: + thakis@chromium.org
Hey, Sorry for the large patch, this updates our flags to use the disable flag type and naming convention which should resolve the issues pushing developers to use tristate Default/Enable/Disable flags for non finch experiments. I wasn't sure whether it was worth changing all of the current flags, but it seems important to raise awareness for this style of flags (no current usage of disable value type) for devs adding new flags in light of lack of awareness in chromium-dev discussions: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/7pTJJGew_eI Please take a look when you have time - this should only change the appearance of the chrome://flags page and I have before / after screenshots on the bug. To resolve the default state confusion on the bug I plan to follow this up with removing tristate flags for non finch experiments where the default is known at compile time by using SINGLE_VALUE_TYPE and SINGLE_DISABLE_VALUE_TYPE.
The CQ bit was checked by flackr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206323003/1
On 2015/06/30 17:00:49, flackr wrote: > Hey, > > Sorry for the large patch, this updates our flags to use the disable flag type > and naming convention which should resolve the issues pushing developers to use > tristate Default/Enable/Disable flags for non finch experiments. > > I wasn't sure whether it was worth changing all of the current flags, but it > seems important to raise awareness for this style of flags (no current usage of > disable value type) for devs adding new flags in light of lack of awareness in > chromium-dev discussions: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/7pTJJGew_eI > > Please take a look when you have time - this should only change the appearance > of the chrome://flags page and I have before / after screenshots on the bug. To > resolve the default state confusion on the bug I plan to follow this up with > removing tristate flags for non finch experiments where the default is known at > compile time by using SINGLE_VALUE_TYPE and SINGLE_DISABLE_VALUE_TYPE. P.S. with regards to the comment on the previous CL that most of these flags should be removed, I initially planned to do a removal pass but found that we have a spreadsheet tracking all of these flags on which each flag owner had determined which ones were obsolete and it seems everything remaining was determined to still be "useful". We could revisit this, but I didn't feel like I could make the decision over those who had recently reviewed their flags.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was unchecked by flackr@chromium.org
I haven't done a proper review but the idea of this lgtm.
(Oops, I didn't realise my l g t m would actually count as an OWNERS review in all files; please wait for a proper review also.)
This never landed :( It's probably rotten by now. Any chance of doing it?
On 2015/07/28 07:31:06, Matt Giuca wrote: > This never landed :( > > It's probably rotten by now. Any chance of doing it? I've updated the patch, just need a review. Perhaps you wouldn't mind doing a full review?
flackr@chromium.org changed reviewers: + pkasting@chromium.org - thakis@chromium.org
Peter, would you have time / be willing to stamp this? It just changes the title strings (and some cases descriptions) in chrome://flags to not reflect whether it is enabling or disabling, and updates all of about_flags.cc to use the new macro for flags which disable features to present the disable link when the flag has not been passed for disable flags. I started the thread on chromium-dev to ensure that if there were any objections people had a chance to speak out without having to watch reviews. WDYT?
My eyes glazed over, but LGTM in general. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:74: MTP support in File Manager. Nit: No trailing period (also several other flag names). We should use periods on sentences, but mere names shouldn't get them. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:86: The new ZIP unpacker. Nit: Just "New ZIP unpacker" https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:1222: Allow calibrating the color of the display if the display supports the feature. Nit: "Allow color calibration of the display..." to match name of the flag. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:6296: Automatic timezone update by the current geolocation. This flag's name and description are almost identical. It seems like the name should be briefer, the description more detailed, or both. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromium_str... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromium_str... chrome/app/chromium_strings.grd:559: iframe-based Chromium sign-in flows. Nit: If we just remove the word "Chromium", it seems like this description is just as meaningful, and we can move the copies from here and google_chrome_strings.grd to the common strings file. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5276: Enable support for Native Client. Nit: Our descriptions are all over the board in terms of whether they're complete sentences or just "longer names", whether they use imperative or descriptive verb forms, etc... it seems like we should make these consistent, although perhaps that's beyond the scope of your change. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5281: Enable Native Client for all web applications, even those that were not installed from the Chrome Web Store. Nit: Similarly, "Support Native Client for..."? https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5409: + Touch adjustment. Nit: No period on flag names (many places) https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5412: + Touch adjustment is the process of refining the position of a touch gesture in order to compensate for touches having poor resolution compared to a mouse. Nit: How about just: "Refines the position of..." https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5488: + <message name="IDS_FLAGS_FAST_UNLOAD_NAME" desc="Name of the 'Extensions on chrome:// URLs' lab"> Nit: Shouldn't this "NAME" string come before the "DESCRIPTION" string? https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5498: + 'window-controls' element Nit: Capitalize "window" https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5610: + Support for WebRTC hardware video decoding. Nit: Seems like "Support for" can be dropped from all flag names and just left in the descriptions if necessary (many places)
Agreed, it's a large fairly mechanical change. Thank you so much for reviewing! https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:74: MTP support in File Manager. On 2015/11/24 21:17:04, Peter Kasting wrote: > Nit: No trailing period (also several other flag names). > > We should use periods on sentences, but mere names shouldn't get them. Done. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:86: The new ZIP unpacker. On 2015/11/24 21:17:04, Peter Kasting wrote: > Nit: Just "New ZIP unpacker" Done. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:1222: Allow calibrating the color of the display if the display supports the feature. On 2015/11/24 21:17:04, Peter Kasting wrote: > Nit: "Allow color calibration of the display..." to match name of the flag. Done. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:6296: Automatic timezone update by the current geolocation. On 2015/11/24 21:17:04, Peter Kasting wrote: > This flag's name and description are almost identical. It seems like the name > should be briefer, the description more detailed, or both. Done. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromium_str... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/1206323003/diff/80001/chrome/app/chromium_str... chrome/app/chromium_strings.grd:559: iframe-based Chromium sign-in flows. On 2015/11/24 21:17:04, Peter Kasting wrote: > Nit: If we just remove the word "Chromium", it seems like this description is > just as meaningful, and we can move the copies from here and > google_chrome_strings.grd to the common strings file. Done. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5276: Enable support for Native Client. On 2015/11/24 21:17:05, Peter Kasting wrote: > Nit: Our descriptions are all over the board in terms of whether they're > complete sentences or just "longer names", whether they use imperative or > descriptive verb forms, etc... it seems like we should make these consistent, > although perhaps that's beyond the scope of your change. Agree, though it would be really nice to defer this. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5281: Enable Native Client for all web applications, even those that were not installed from the Chrome Web Store. On 2015/11/24 21:17:05, Peter Kasting wrote: > Nit: Similarly, "Support Native Client for..."? Done. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5409: + Touch adjustment. On 2015/11/24 21:17:05, Peter Kasting wrote: > Nit: No period on flag names (many places) Done. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5412: + Touch adjustment is the process of refining the position of a touch gesture in order to compensate for touches having poor resolution compared to a mouse. On 2015/11/24 21:17:04, Peter Kasting wrote: > Nit: How about just: "Refines the position of..." Agreed, sounds simpler. Done. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5488: + <message name="IDS_FLAGS_FAST_UNLOAD_NAME" desc="Name of the 'Extensions on chrome:// URLs' lab"> On 2015/11/24 21:17:05, Peter Kasting wrote: > Nit: Shouldn't this "NAME" string come before the "DESCRIPTION" string? Done. Also looks like the description for this one was copied from IDS_FLAGS_EXTENSIONS_ON_CHROME_URLS_NAME, fixed that as well. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5498: + 'window-controls' element On 2015/11/24 21:17:05, Peter Kasting wrote: > Nit: Capitalize "window" Done. https://codereview.chromium.org/1206323003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5610: + Support for WebRTC hardware video decoding. On 2015/11/24 21:17:05, Peter Kasting wrote: > Nit: Seems like "Support for" can be dropped from all flag names and just left > in the descriptions if necessary (many places) Done.
The CQ bit was checked by flackr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1206323003/#ps100001 (title: "Review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206323003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1206323003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by flackr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/1206323003/#ps120001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206323003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1206323003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by flackr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206323003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1206323003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/88fd51293c02b73560b00c36fc0c8b49a60939ef Cr-Commit-Position: refs/heads/master@{#361573} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
