Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15716012:
Add a Preference for Google Services to Use the User's Location (Closed)
|
Created:
7 years, 6 months ago by robliao Modified:
7 years, 6 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, jar (doing other things), sail+watch_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org, Ilya Sherman Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd a Preference for Google Services to Use the User's Location
Adds a new Chrome preference (googlegeolocationaccess.enabled) and
hides it behind the ENABLE_GOOGLE_NOW compiler define as well as the
kEnableGoogleNowIntegration switch
BUG=164227
TEST=
1) Go to chrome://flags
2) Enable Google Now
3) Restart Chrome
4) Go to chrome://settings
5) Click "Show Advanced Settings..."
6) Click "Content Settings..."
7) Observe the checkbox in the location section
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205841
Patch Set 1 #
Total comments: 30
Patch Set 2 : Sync to Latest #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 5
Patch Set 5 : #
Total comments: 22
Patch Set 6 : Nits Fixes #Patch Set 7 : #Messages
Total messages: 38 (0 generated)
https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... chrome/browser/profiles/profile.cc:85: registry->RegisterBooleanPref( #ifdef? https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... chrome/browser/profiles/profile.cc:86: prefs::kComponentGeolocationEnabled, Why do we have the 'Component' part of the name? https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/geolocation_options.js (right): https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:9: * GeolocationOptions class It would help to end sentences with '.'. https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:10: * Shows the component geolocation option when Google Now is enabled 'when Google Now is enabled' belongs to the logic of the calling code, so shouldn't be here. Ad addition, there will be other triggers in future. https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:15: OptionsPage.call(this, Curious: what does this call do? https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:29: if (geolocationCheckboxContainer) { No need in curlies. https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:34: return { GeolocationOptions: GeolocationOptions }; return {GeolocationOptions: GeolocationOptions}; https://codereview.chromium.org/15716012/diff/1/chrome/browser/ui/webui/optio... File chrome/browser/ui/webui/options/geolocation_options_handler.cc (right): https://codereview.chromium.org/15716012/diff/1/chrome/browser/ui/webui/optio... chrome/browser/ui/webui/options/geolocation_options_handler.cc:25: #if defined(ENABLE_GOOGLE_NOW) Creation of this object is already under "#if defined(ENABLE_GOOGLE_NOW)", so no need in another IFDEF here. https://codereview.chromium.org/15716012/diff/1/chrome/chrome_browser_ui.gypi File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/15716012/diff/1/chrome/chrome_browser_ui.gypi... chrome/chrome_browser_ui.gypi:2201: 'browser/ui/webui/options/geolocation_options_handler.cc', Please don't include these files on platforms where Google Now doesn't exist. Example: https://chromiumcodereview.appspot.com/11412291/diff/27001/chrome/chrome_brow... https://codereview.chromium.org/15716012/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/15716012/diff/1/chrome/common/pref_names.h#ne... chrome/common/pref_names.h:686: extern const char kComponentGeolocationEnabled[]; Should be under IFDEF?
Add an Option for Google Services to Use the User's Location Adds a new Chrome preference (componentgeolocation.enabled) and hides it behind the ENABLE_GOOGLE_NOW compiler define as well as the kEnableGoogleNowIntegration switch
https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... chrome/browser/profiles/profile.cc:85: registry->RegisterBooleanPref( On 2013/06/07 00:56:07, vadimt wrote: > #ifdef? Done. https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... chrome/browser/profiles/profile.cc:86: prefs::kComponentGeolocationEnabled, This is to distinguish it from kGeolocationEnabled, which applies to webpage content. On 2013/06/07 00:56:07, vadimt wrote: > Why do we have the 'Component' part of the name? https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... chrome/browser/profiles/profile.cc:86: prefs::kComponentGeolocationEnabled, On 2013/06/07 00:56:07, vadimt wrote: > Why do we have the 'Component' part of the name? Done. https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/geolocation_options.js (right): https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:9: * GeolocationOptions class On 2013/06/07 00:56:07, vadimt wrote: > It would help to end sentences with '.'. Done. https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:10: * Shows the component geolocation option when Google Now is enabled On 2013/06/07 00:56:07, vadimt wrote: > 'when Google Now is enabled' belongs to the logic of the calling code, so > shouldn't be here. Ad addition, there will be other triggers in future. Done. https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:15: OptionsPage.call(this, This initializes the our "parent class". The other handlers also seem to do this. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... On 2013/06/07 00:56:07, vadimt wrote: > Curious: what does this call do? https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:15: OptionsPage.call(this, On 2013/06/07 00:56:07, vadimt wrote: > Curious: what does this call do? Done. https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:29: if (geolocationCheckboxContainer) { The Google JS style guide is written with curlies for one-liners. I like them better as well because if future changes are required in this block, there's no need to add the curlies. On 2013/06/07 00:56:07, vadimt wrote: > No need in curlies. https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:29: if (geolocationCheckboxContainer) { On 2013/06/07 00:56:07, vadimt wrote: > No need in curlies. Done. https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:34: return { GeolocationOptions: GeolocationOptions }; I went ahead and matched the style in the other files: three separate lines. On 2013/06/07 00:56:07, vadimt wrote: > return {GeolocationOptions: GeolocationOptions}; https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:34: return { GeolocationOptions: GeolocationOptions }; On 2013/06/07 00:56:07, vadimt wrote: > return {GeolocationOptions: GeolocationOptions}; Done. https://codereview.chromium.org/15716012/diff/1/chrome/browser/ui/webui/optio... File chrome/browser/ui/webui/options/geolocation_options_handler.cc (right): https://codereview.chromium.org/15716012/diff/1/chrome/browser/ui/webui/optio... chrome/browser/ui/webui/options/geolocation_options_handler.cc:25: #if defined(ENABLE_GOOGLE_NOW) On 2013/06/07 00:56:07, vadimt wrote: > Creation of this object is already under "#if defined(ENABLE_GOOGLE_NOW)", so no > need in another IFDEF here. Done. https://codereview.chromium.org/15716012/diff/1/chrome/chrome_browser_ui.gypi File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/15716012/diff/1/chrome/chrome_browser_ui.gypi... chrome/chrome_browser_ui.gypi:2201: 'browser/ui/webui/options/geolocation_options_handler.cc', That's cool! On 2013/06/07 00:56:07, vadimt wrote: > Please don't include these files on platforms where Google Now doesn't exist. > Example: > https://chromiumcodereview.appspot.com/11412291/diff/27001/chrome/chrome_brow... https://codereview.chromium.org/15716012/diff/1/chrome/chrome_browser_ui.gypi... chrome/chrome_browser_ui.gypi:2201: 'browser/ui/webui/options/geolocation_options_handler.cc', On 2013/06/07 00:56:07, vadimt wrote: > Please don't include these files on platforms where Google Now doesn't exist. > Example: > https://chromiumcodereview.appspot.com/11412291/diff/27001/chrome/chrome_brow... Done. https://codereview.chromium.org/15716012/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/15716012/diff/1/chrome/common/pref_names.h#ne... chrome/common/pref_names.h:686: extern const char kComponentGeolocationEnabled[]; On 2013/06/07 00:56:07, vadimt wrote: > Should be under IFDEF? Done.
https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... chrome/browser/profiles/profile.cc:86: prefs::kComponentGeolocationEnabled, On 2013/06/07 07:34:36, Robert Liao wrote: > This is to distinguish it from kGeolocationEnabled, which applies to webpage > content. > On 2013/06/07 00:56:07, vadimt wrote: > > Why do we have the 'Component' part of the name? > I see. But 'Component' implies component extension, right? I future, other services, may be including C++ ones, may use this flag too. How about AccessGeolocation? https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/geolocation_options.js (right): https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:15: OptionsPage.call(this, On 2013/06/07 07:34:36, Robert Liao wrote: > On 2013/06/07 00:56:07, vadimt wrote: > > Curious: what does this call do? > > Done. No need to additionally add "done" comments when you already answered. https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:29: if (geolocationCheckboxContainer) { On 2013/06/07 07:34:36, Robert Liao wrote: > The Google JS style guide is written with curlies for one-liners. I like them > better as well because if future changes are required in this block, there's no > need to add the curlies. > On 2013/06/07 00:56:07, vadimt wrote: > > No need in curlies. > I'm OK with this; however reviewers used to require me to remove curlies from single-liners... https://codereview.chromium.org/15716012/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:34: return { GeolocationOptions: GeolocationOptions }; On 2013/06/07 07:34:36, Robert Liao wrote: > I went ahead and matched the style in the other files: three separate lines. > On 2013/06/07 00:56:07, vadimt wrote: > > return {GeolocationOptions: GeolocationOptions}; > Your choice, but if it fits in 1 line, you could also use the format I suggested. https://codereview.chromium.org/15716012/diff/30004/chrome/browser/resources/... File chrome/browser/resources/options/geolocation_options.js (right): https://codereview.chromium.org/15716012/diff/30004/chrome/browser/resources/... chrome/browser/resources/options/geolocation_options.js:9: * GeolocationOptions class Need '.' too.
https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/15716012/diff/1/chrome/browser/profiles/profi... chrome/browser/profiles/profile.cc:86: prefs::kComponentGeolocationEnabled, Changed to kGoogleGeolocationAccessEnabled On 2013/06/07 17:16:28, vadimt wrote: > On 2013/06/07 07:34:36, Robert Liao wrote: > > This is to distinguish it from kGeolocationEnabled, which applies to webpage > > content. > > On 2013/06/07 00:56:07, vadimt wrote: > > > Why do we have the 'Component' part of the name? > > > > I see. But 'Component' implies component extension, right? > I future, other services, may be including C++ ones, may use this flag too. How > about AccessGeolocation? https://codereview.chromium.org/15716012/diff/30004/chrome/browser/resources/... File chrome/browser/resources/options/geolocation_options.js (right): https://codereview.chromium.org/15716012/diff/30004/chrome/browser/resources/... chrome/browser/resources/options/geolocation_options.js:9: * GeolocationOptions class Adding a '.' would not match the style for the other js files. This is also a title, not a sentence. On 2013/06/07 17:16:28, vadimt wrote: > Need '.' too.
https://codereview.chromium.org/15716012/diff/65001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/15716012/diff/65001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/content_settings_handler.cc:303: IDS_GEOLOCATION_GOOGLE_ACCESS_ENABLE }, Should end with _CHKBOX like IDS_COOKIES_LSO_CLEAR_WHEN_CLOSE_CHKBOX?
https://codereview.chromium.org/15716012/diff/65001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/15716012/diff/65001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/content_settings_handler.cc:303: IDS_GEOLOCATION_GOOGLE_ACCESS_ENABLE }, A lot of the checks strings in the setting page do not end in _CHKBOX. Is there a style guideline for this? On 2013/06/07 19:13:44, vadimt wrote: > Should end with _CHKBOX like IDS_COOKIES_LSO_CLEAR_WHEN_CLOSE_CHKBOX?
https://codereview.chromium.org/15716012/diff/65001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/15716012/diff/65001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/content_settings_handler.cc:303: IDS_GEOLOCATION_GOOGLE_ACCESS_ENABLE }, *checkbox strings On 2013/06/07 20:03:59, Robert Liao wrote: > A lot of the checks strings in the setting page do not end in _CHKBOX. Is there > a style guideline for this? > On 2013/06/07 19:13:44, vadimt wrote: > > Should end with _CHKBOX like IDS_COOKIES_LSO_CLEAR_WHEN_CLOSE_CHKBOX? >
https://codereview.chromium.org/15716012/diff/65001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/15716012/diff/65001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/content_settings_handler.cc:303: IDS_GEOLOCATION_GOOGLE_ACCESS_ENABLE }, On 2013/06/07 20:03:59, Robert Liao wrote: > A lot of the checks strings in the setting page do not end in _CHKBOX. Is there > a style guideline for this? > On 2013/06/07 19:13:44, vadimt wrote: > > Should end with _CHKBOX like IDS_COOKIES_LSO_CLEAR_WHEN_CLOSE_CHKBOX? > In the name of consistency with the rest of this file?
https://codereview.chromium.org/15716012/diff/65001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/15716012/diff/65001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/content_settings_handler.cc:303: IDS_GEOLOCATION_GOOGLE_ACCESS_ENABLE }, Ah, I see. Going forward, it seems like we should strip out all of these annotations. If I change a checkbox to a radio button, I don't want to have to change all of the constant names. This changelist will have an appended annotation. On 2013/06/07 20:14:39, vadimt wrote: > On 2013/06/07 20:03:59, Robert Liao wrote: > > A lot of the checks strings in the setting page do not end in _CHKBOX. Is > there > > a style guideline for this? > > On 2013/06/07 19:13:44, vadimt wrote: > > > Should end with _CHKBOX like IDS_COOKIES_LSO_CLEAR_WHEN_CLOSE_CHKBOX? > > > > In the name of consistency with the rest of this file?
lgtm
estade: Please provide owner approval. Thanks!
Owners below, please provide approval. cpu: chrome/app/generated_resources.grd sail: chrome/browser/profiles/profile.cc estade: chrome/browser/resources/options/content_settings.html chrome/browser/resources/options/geolocation_options.js chrome/browser/resources/options/options.js chrome/browser/resources/options/options_bundle.js chrome/browser/ui/webui/options/content_settings_handler.cc chrome/browser/ui/webui/options/geolocation_options_handler.h chrome/browser/ui/webui/options/geolocation_options_handler.cc chrome/browser/ui/webui/options/options_ui.cc darin: chrome/chrome_browser_ui.gypi chrome/common/pref_names.h chrome/common/pref_names.cc isherman: tools/metrics/actions/chromeactions.txt
This CL needs: - a BUG= line - a TEST= line that describes, step by step, how to exercise the new functionality
Done
grd changes do not require owner's approval anymore.
Message was sent while issue was closed.
Update issue
Reopening
Removing cpu from the review list
profiles LGTM
Remaining reviewers: ptal! Thanks! estade: chrome/browser/resources/options/content_settings.html chrome/browser/resources/options/geolocation_options.js chrome/browser/resources/options/options.js chrome/browser/resources/options/options_bundle.js chrome/browser/ui/webui/options/content_settings_handler.cc chrome/browser/ui/webui/options/geolocation_options_handler.h chrome/browser/ui/webui/options/geolocation_options_handler.cc chrome/browser/ui/webui/options/options_ui.cc darin: chrome/chrome_browser_ui.gypi chrome/common/pref_names.h chrome/common/pref_names.cc isherman: tools/metrics/actions/chromeactions.txt
just nits, otherwise lgtm https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/content_settings.html (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/content_settings.html:288: <if expr="pp_ifdef('enable_google_now')"> if expr should not be indented at all (like #ifdef) https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/geolocation_options.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/geolocation_options.js:28: var geolocationCheckboxContainer = $('geolocationCheckbox'); indent is 2 too much https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/geolocation_options.js:29: if (geolocationCheckboxContainer) { nit: doesn't seem like you need this check --- the file should only be included when the checkbox exists https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/options.js:21: var GeolocationOptions = options.GeolocationOptions; nit: no indent https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/options_bundle.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/options_bundle.js:47: <include src="certificate_tree.js"></include> hmmm... IMO the indent is wrong in this file. <if expr> doesn't imply its contents should be indented. But I won't demand you change it right now. Would be nice though. https://codereview.chromium.org/15716012/diff/67002/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/geolocation_options_handler.cc (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/geolocation_options_handler.cc:15: } nit: } on same line as { https://codereview.chromium.org/15716012/diff/67002/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/geolocation_options_handler.h (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/geolocation_options_handler.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: i'm told you don't need (c) https://codereview.chromium.org/15716012/diff/67002/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/geolocation_options_handler.h:13: class GeolocationOptionsHandler : public OptionsPageUIHandler { class level comment.
Nits fixes https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/content_settings.html (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/content_settings.html:288: <if expr="pp_ifdef('enable_google_now')"> On 2013/06/10 18:06:10, Evan Stade wrote: > if expr should not be indented at all (like #ifdef) Done. https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/geolocation_options.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/geolocation_options.js:28: var geolocationCheckboxContainer = $('geolocationCheckbox'); On 2013/06/10 18:06:10, Evan Stade wrote: > indent is 2 too much Done. https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/geolocation_options.js:29: if (geolocationCheckboxContainer) { On 2013/06/10 18:06:10, Evan Stade wrote: > nit: doesn't seem like you need this check --- the file should only be included > when the checkbox exists Done. https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/options.js:21: var GeolocationOptions = options.GeolocationOptions; I'm keeping this one like this for consistency with the rest of the file. On 2013/06/10 18:06:10, Evan Stade wrote: > nit: no indent https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/options_bundle.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/options_bundle.js:47: <include src="certificate_tree.js"></include> On 2013/06/10 18:06:10, Evan Stade wrote: > hmmm... IMO the indent is wrong in this file. <if expr> doesn't imply its > contents should be indented. But I won't demand you change it right now. Would > be nice though. No changes for consistency. https://codereview.chromium.org/15716012/diff/67002/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/geolocation_options_handler.cc (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/geolocation_options_handler.cc:15: } On 2013/06/10 18:06:10, Evan Stade wrote: > nit: } on same line as { Done. https://codereview.chromium.org/15716012/diff/67002/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/geolocation_options_handler.h (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/geolocation_options_handler.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. Keeping the banner for consistency. All the other files have this. On 2013/06/10 18:06:10, Evan Stade wrote: > nit: i'm told you don't need (c) https://codereview.chromium.org/15716012/diff/67002/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/geolocation_options_handler.h:13: class GeolocationOptionsHandler : public OptionsPageUIHandler { On 2013/06/10 18:06:10, Evan Stade wrote: > class level comment. Done.
https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/options.js:21: var GeolocationOptions = options.GeolocationOptions; On 2013/06/10 18:58:49, Robert Liao wrote: > I'm keeping this one like this for consistency with the rest of the file. > On 2013/06/10 18:06:10, Evan Stade wrote: > > nit: no indent > where in the rest of the file is there an if expr?
https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/options.js:21: var GeolocationOptions = options.GeolocationOptions; In this directory, options_bundle.js follows the <if ...> with a space style. On 2013/06/10 20:35:03, Evan Stade wrote: > On 2013/06/10 18:58:49, Robert Liao wrote: > > I'm keeping this one like this for consistency with the rest of the file. > > On 2013/06/10 18:06:10, Evan Stade wrote: > > > nit: no indent > > > > where in the rest of the file is there an if expr?
https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/options.js:21: var GeolocationOptions = options.GeolocationOptions; On 2013/06/10 20:58:29, Robert Liao wrote: > In this directory, options_bundle.js follows the <if ...> with a space style. > On 2013/06/10 20:35:03, Evan Stade wrote: > > On 2013/06/10 18:58:49, Robert Liao wrote: > > > I'm keeping this one like this for consistency with the rest of the file. > > > On 2013/06/10 18:06:10, Evan Stade wrote: > > > > nit: no indent > > > > > > > where in the rest of the file is there an if expr? > doesn't mean you should propagate the bad style into this file.
https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/options.js:21: var GeolocationOptions = options.GeolocationOptions; I agree with you that the space is unnecessary. However, the goal of the style guide is to make the code readable and uniform. Performing this change would make the code less uniform. Given that this statement is readable, I'm inclined to follow the style that's established in the options JS and HTML files. If the space is unnecessary, this should be fixed in all files rather than one-offing the fix in files here and there. We can take this offline for further discussion if you wish. On 2013/06/10 21:15:19, Evan Stade wrote: > On 2013/06/10 20:58:29, Robert Liao wrote: > > In this directory, options_bundle.js follows the <if ...> with a space style. > > On 2013/06/10 20:35:03, Evan Stade wrote: > > > On 2013/06/10 18:58:49, Robert Liao wrote: > > > > I'm keeping this one like this for consistency with the rest of the file. > > > > On 2013/06/10 18:06:10, Evan Stade wrote: > > > > > nit: no indent > > > > > > > > > > where in the rest of the file is there an if expr? > > > > doesn't mean you should propagate the bad style into this file.
lgtm
https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/options.js:21: var GeolocationOptions = options.GeolocationOptions; On 2013/06/10 21:29:46, Robert Liao wrote: > I agree with you that the space is unnecessary. However, the goal of the style > guide is to make the code readable and uniform. Performing this change would > make the code less uniform. Given that this statement is readable, I'm inclined > to follow the style that's established in the options JS and HTML files. > > If the space is unnecessary, this should be fixed in all files rather than > one-offing the fix in files here and there. > > We can take this offline for further discussion if you wish. > On 2013/06/10 21:15:19, Evan Stade wrote: > > On 2013/06/10 20:58:29, Robert Liao wrote: > > > In this directory, options_bundle.js follows the <if ...> with a space > style. > > > On 2013/06/10 20:35:03, Evan Stade wrote: > > > > On 2013/06/10 18:58:49, Robert Liao wrote: > > > > > I'm keeping this one like this for consistency with the rest of the > file. > > > > > On 2013/06/10 18:06:10, Evan Stade wrote: > > > > > > nit: no indent > > > > > > > > > > > > > where in the rest of the file is there an if expr? > > > > > > > doesn't mean you should propagate the bad style into this file. > feel free to fix them all right now then. It's a matter of seconds to do it.
https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/15716012/diff/67002/chrome/browser/resources/... chrome/browser/resources/options/options.js:21: var GeolocationOptions = options.GeolocationOptions; Indeed it's a matter of seconds, but best put into another changelist so it doesn't clutter this one. I'll send one out after this commits. On 2013/06/10 21:51:39, Evan Stade wrote: > On 2013/06/10 21:29:46, Robert Liao wrote: > > I agree with you that the space is unnecessary. However, the goal of the style > > guide is to make the code readable and uniform. Performing this change would > > make the code less uniform. Given that this statement is readable, I'm > inclined > > to follow the style that's established in the options JS and HTML files. > > > > If the space is unnecessary, this should be fixed in all files rather than > > one-offing the fix in files here and there. > > > > We can take this offline for further discussion if you wish. > > On 2013/06/10 21:15:19, Evan Stade wrote: > > > On 2013/06/10 20:58:29, Robert Liao wrote: > > > > In this directory, options_bundle.js follows the <if ...> with a space > > style. > > > > On 2013/06/10 20:35:03, Evan Stade wrote: > > > > > On 2013/06/10 18:58:49, Robert Liao wrote: > > > > > > I'm keeping this one like this for consistency with the rest of the > > file. > > > > > > On 2013/06/10 18:06:10, Evan Stade wrote: > > > > > > > nit: no indent > > > > > > > > > > > > > > > > where in the rest of the file is there an if expr? > > > > > > > > > > doesn't mean you should propagate the bad style into this file. > > > > feel free to fix them all right now then. It's a matter of seconds to do it.
Sky: Can you provide owner approval for the files below? Thanks! sky: chrome/chrome_browser_ui.gypi chrome/common/pref_names.h chrome/common/pref_names.cc
Updating to reflect r205649 changes.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/15716012/133001
Retried try job too often on linux_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/15716012/133001
Message was sent while issue was closed.
Change committed as 205841 |
