Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(351)

Issue 1407753002: Clean up terminology used in about_flags. (Closed)

Created:
5 years, 2 months ago by Alexei Svitkine (slow)
Modified:
5 years, 2 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Clean up terminology used in about_flags. Renames about_flags::Experiment to about_flags::FeatureEntry and updates associated function names and comments. The goal is to reduce confusion with the other meaning of "Experiment" in Chromium - i.e. FieldTrial. In about_ui code, tends to use "Experimental feature" terminology, since it is more natural there, compared to FeatureEntry, which is used internally by about_flags code. Also: - Adds g_ prefix to global variables in about_flags.cc, so it's clear where they are coming from. - Some comment clean ups beyond just the simple terminology change. - Remove an extra include of data_reduction_proxy_switches.h, which was already included earlier. - Fixes AboutFlagsTest.NoSeparators, which previously only ever checked the first entry. - Removes IDS_FLAGS_UI_NO_EXPERIMENTS_AVAILABLE and IDS_FLAGS_UI_NO_UNSUPPORTED_EXPERIMENTS UI strings. In practice, these were never shown (there are always experiments and unsupported experiments), so the associated code/strings was simply unnecessary binary bloat. BUG=490840 TBR=blundell@chromium.org Committed: https://crrev.com/e9ed35e42e3e0ceadf7fe4c7e323ba9983c86e69 Cr-Commit-Position: refs/heads/master@{#354135}

Patch Set 1 : #

Total comments: 2

Patch Set 2 : Rebase. #

Patch Set 3 : Update comment on FeatureEntry. #

Total comments: 3

Patch Set 4 : Remove unneccessary divs. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+442 lines, -480 lines) Patch
M chrome/browser/about_flags.h View 1 2 5 chunks +44 lines, -40 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 58 chunks +200 lines, -200 lines 0 comments Download
M chrome/browser/about_flags_unittest.cc View 23 chunks +108 lines, -108 lines 0 comments Download
M chrome/browser/profiles/profile_window.cc View 1 3 chunks +5 lines, -7 lines 0 comments Download
M chrome/browser/ui/webui/flags_ui.cc View 7 chunks +30 lines, -36 lines 0 comments Download
M components/flags_ui/flags_ui_constants.h View 3 chunks +5 lines, -7 lines 0 comments Download
M components/flags_ui/flags_ui_constants.cc View 3 chunks +5 lines, -7 lines 0 comments Download
M components/flags_ui/resources/flags.css View 1 chunk +0 lines, -6 lines 0 comments Download
M components/flags_ui/resources/flags.html View 1 2 3 5 chunks +3 lines, -19 lines 0 comments Download
M components/flags_ui/resources/flags.js View 9 chunks +42 lines, -44 lines 0 comments Download
M components/flags_ui_strings.grdp View 1 chunk +0 lines, -6 lines 0 comments Download

Messages

Total messages: 28 (11 generated)
Alexei Svitkine (slow)
Nico, PTAL. This supersedes the previous CL (https://codereview.chromium.org/1159433002/).
5 years, 2 months ago (2015-10-14 19:41:56 UTC) #7
Nico
lgtm https://codereview.chromium.org/1407753002/diff/100001/chrome/browser/about_flags.h File chrome/browser/about_flags.h (right): https://codereview.chromium.org/1407753002/diff/100001/chrome/browser/about_flags.h#newcode31 chrome/browser/about_flags.h:31: // feature (and for testing). nit: Since this ...
5 years, 2 months ago (2015-10-14 19:59:08 UTC) #8
Nico
(maybe say in the CL description why you're renaming it ("to reduce confusability with finch ...
5 years, 2 months ago (2015-10-14 19:59:51 UTC) #9
Alexei Svitkine (slow)
"(maybe say in the CL description why you're renaming it ("to reduce confusability with finch ...
5 years, 2 months ago (2015-10-14 20:14:29 UTC) #11
Alexei Svitkine (slow)
+jhawkins for components/flags_ui/OWNERS
5 years, 2 months ago (2015-10-14 20:15:13 UTC) #13
James Hawkins
https://codereview.chromium.org/1407753002/diff/160001/components/flags_ui/resources/flags.html File components/flags_ui/resources/flags.html (right): https://codereview.chromium.org/1407753002/diff/160001/components/flags_ui/resources/flags.html#newcode59 components/flags_ui/resources/flags.html:59: <div jsdisplay="supportedFeatures.length > 0"> What is the point of ...
5 years, 2 months ago (2015-10-14 20:34:40 UTC) #14
Nico
https://codereview.chromium.org/1407753002/diff/160001/components/flags_ui/resources/flags.html File components/flags_ui/resources/flags.html (right): https://codereview.chromium.org/1407753002/diff/160001/components/flags_ui/resources/flags.html#newcode114 components/flags_ui/resources/flags.html:114: </div> closing tag is here
5 years, 2 months ago (2015-10-14 20:40:27 UTC) #15
Alexei Svitkine (slow)
https://codereview.chromium.org/1407753002/diff/160001/components/flags_ui/resources/flags.html File components/flags_ui/resources/flags.html (right): https://codereview.chromium.org/1407753002/diff/160001/components/flags_ui/resources/flags.html#newcode59 components/flags_ui/resources/flags.html:59: <div jsdisplay="supportedFeatures.length > 0"> On 2015/10/14 20:34:40, James Hawkins ...
5 years, 2 months ago (2015-10-14 20:58:43 UTC) #16
Alexei Svitkine (slow)
+blundell for components/OWNERS for components/flags_ui_strings.grdp
5 years, 2 months ago (2015-10-14 21:05:43 UTC) #18
James Hawkins
lgtm
5 years, 2 months ago (2015-10-14 21:20:10 UTC) #19
Alexei Svitkine (slow)
TBR=blundell for components/ OWNERS
5 years, 2 months ago (2015-10-14 21:21:04 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407753002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407753002/180001
5 years, 2 months ago (2015-10-14 21:22:08 UTC) #23
commit-bot: I haz the power
Committed patchset #4 (id:180001)
5 years, 2 months ago (2015-10-14 22:34:06 UTC) #24
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/e9ed35e42e3e0ceadf7fe4c7e323ba9983c86e69 Cr-Commit-Position: refs/heads/master@{#354135}
5 years, 2 months ago (2015-10-14 22:35:02 UTC) #25
blundell
At least on the iOS port, I'm pretty sure that there are times when there ...
5 years, 2 months ago (2015-10-15 07:30:10 UTC) #26
Alexei Svitkine (slow)
On 2015/10/15 07:30:10, blundell wrote: > At least on the iOS port, I'm pretty sure ...
5 years, 2 months ago (2015-10-15 15:34:53 UTC) #27
blundell
5 years, 2 months ago (2015-10-16 07:24:55 UTC) #28
Message was sent while issue was closed.
On 2015/10/15 15:34:53, Alexei Svitkine wrote:
> On 2015/10/15 07:30:10, blundell wrote:
> > At least on the iOS port, I'm pretty sure that there are times when there
are
> no
> > experiments available. What will happen now if there are no experiments
> > available? I'm much less worried about the "no unsupported experiments"
> removal
> > from the iOS POV, as I imagine there's always an experiment that doesn't
make
> > sense on iOS.
> 
> So, the unsupported section just doesn't get shown on iOS at all. So that one,
> there's no change at all for iOS pov.
> 
> In the other case, it will just show an empty list (i.e. blank under the
> "Experiments" header).

LGTM

OK, that doesn't seem problematic to me. Thanks!

Powered by Google App Engine
This is Rietveld 408576698