Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18341016:
Add types.private.ChromeDirectSetting and Connect it to preferencesPrivate.googleGeolocationAccessE… (Closed)
|
Created:
7 years, 5 months ago by robliao Modified:
7 years, 5 months ago CC:
vadimt, asargent_no_longer_on_chrome, battre, chromium-reviews, chromium-apps-reviews_chromium.org, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd types.private.ChromeDirectSetting and Connect it to preferencesPrivate.googleGeolocationAccessEnabled
* Adds a component extension only API type
chrome.types.private.ChromeDirectSetting (mirrors ChromeSetting except that it
does not provide levelOfControl info given that this only applies to component
extensions).
* Switches preferencesPrivate.googleGeolocationAccessEnabled to use chrome.types.private.ChromeDirectSetting
* Implements preliminary get, set, and clear functionality (direct access to the preferences)
* Adds stubs for onChange to be done later.
* Removes existing component extension affordances in the existing extension
preferences codepath
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211300
Patch Set 1 #
Total comments: 1
Patch Set 2 : Sync to Latest #
Total comments: 14
Patch Set 3 : CR Feedback #Patch Set 4 : CR Feedback #
Total comments: 2
Patch Set 5 : #Patch Set 6 : Added Preference Whitelist #
Total comments: 4
Patch Set 7 : Added Lazy Initialized Whitelist #
Total comments: 6
Patch Set 8 : CR Feedback #Patch Set 9 : #
Total comments: 39
Patch Set 10 : CR Feedback #Patch Set 11 : Update types_private.json to Use the Strings from types.json #Patch Set 12 : Remove Inline Constructors and Destructors #Patch Set 13 : Sync to Latest #Messages
Total messages: 37 (0 generated)
Publishing new review to update to latest and add a few lines of code. https://codereview.chromium.org/18341016/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/types_private.json (left): https://codereview.chromium.org/18341016/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/types_private.json:52: "incognitoSpecific": { Does anyone foresee component extensions requiring incognito prefs? If not, I'd rather remove this.
I've briefly looked at this :) Please proceed with the file owners.
asargent: chrome/browser/extensions/api/preference/chrome_direct_setting.h chrome/browser/extensions/api/preference/chrome_direct_setting.cc chrome/browser/extensions/api/preference/preference_api.cc chrome/browser/extensions/api/preference/preference_helpers.cc chrome/browser/extensions/extension_function_histogram_value.h chrome/browser/extensions/extension_function_registry.cc chrome/common/extensions/api/_api_features.json chrome/common/extensions/api/extension_api.cc chrome/common/extensions/api/preferences_private.json chrome/common/extensions/api/types_private.json chrome/common/extensions_api_resources.grd chrome/renderer/extensions/dispatcher.cc chrome/renderer/resources/extensions/chrome_direct_setting.js sky: chrome/chrome_browser_extensions.gypi chrome/chrome_renderer.gypi chrome/renderer/resources/renderer_resources.grd mpearson: tools/metrics/histograms/histograms.xml
On 2013/07/09 00:42:24, Robert Liao wrote: > asargent: > chrome/browser/extensions/api/preference/chrome_direct_setting.h > chrome/browser/extensions/api/preference/chrome_direct_setting.cc > chrome/browser/extensions/api/preference/preference_api.cc > chrome/browser/extensions/api/preference/preference_helpers.cc > chrome/browser/extensions/extension_function_histogram_value.h > chrome/browser/extensions/extension_function_registry.cc > chrome/common/extensions/api/_api_features.json > chrome/common/extensions/api/extension_api.cc > chrome/common/extensions/api/preferences_private.json > chrome/common/extensions/api/types_private.json > chrome/common/extensions_api_resources.grd > chrome/renderer/extensions/dispatcher.cc > chrome/renderer/resources/extensions/chrome_direct_setting.js > > sky: > chrome/chrome_browser_extensions.gypi > chrome/chrome_renderer.gypi > chrome/renderer/resources/renderer_resources.grd > > mpearson: > tools/metrics/histograms/histograms.xml Owners listed on the previous message, please provide owner approval. Thanks!
Syncing to the latest...
https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/api/preference/chrome_direct_setting.cc (right): https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:20: // this means the API permissions system has failed. Is the API permissions system enforced on the browser side? That is, can this *only* happen due to a bug in Chrome, or also for example if a renderer is compromised? If the latter, I don't think crashing is right, as that would enable a denial-of-service attack. Seeing as we already have EXTENSION_FUNCTION_VALIDATE below, could you make this return a bool and wrap it in EXTENSION_FUNCTION_VALIDATE? https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:37: const base::Value* value = preference->GetValue(); This will crash if the pref_key isn't found. Can you wrap it in EXTENSION_FUNCTION_VALIDATE? https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:59: PrefService* prefService = GetPrefService(); Local variables are named unix_hacker_style.
histograms.xml lgtm
LGTM
https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/api/preference/chrome_direct_setting.cc (right): https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:20: // this means the API permissions system has failed. The renderer performs the API check, so compromised renderers would send the function call to the frame process. I've relaxed this check. On 2013/07/09 07:34:32, Bernhard Bauer wrote: > Is the API permissions system enforced on the browser side? That is, can this > *only* happen due to a bug in Chrome, or also for example if a renderer is > compromised? If the latter, I don't think crashing is right, as that would > enable a denial-of-service attack. Seeing as we already have > EXTENSION_FUNCTION_VALIDATE below, could you make this return a bool and wrap it > in EXTENSION_FUNCTION_VALIDATE? https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:37: const base::Value* value = preference->GetValue(); This was a deliberate decision for the following reasons * Pref Keys are specified by Chrome Developers the API JSON definition file. * The Extension Prefs similarly fail fast (crash) in missing extension pref situations. * Pref Keys tend not to be removed once they are checked in. Crashing fast here allows Chrome Developers to easily see their error and fix it. On 2013/07/09 07:34:32, Bernhard Bauer wrote: > This will crash if the pref_key isn't found. Can you wrap it in > EXTENSION_FUNCTION_VALIDATE? https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:59: PrefService* prefService = GetPrefService(); Habits from a previous life. Fixed. On 2013/07/09 07:34:32, Bernhard Bauer wrote: > Local variables are named unix_hacker_style.
https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/api/preference/chrome_direct_setting.cc (right): https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:36: GetPrefService()->FindPreference(pref_key.c_str()); I'm also worried about directly using the passed in pref key instead of going through the mapping like for the regular preferences API. That means that the component extension could access any other (registered) preference. In addition, the API is available to any component extension, so *any* compromised component extension could change *any* preference. I would feel much more at ease if we had a mapping or a whitelist of accessible preferences here. https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:37: const base::Value* value = preference->GetValue(); On 2013/07/09 17:27:45, Robert Liao wrote: > This was a deliberate decision for the following reasons > * Pref Keys are specified by Chrome Developers the API JSON definition file. > * The Extension Prefs similarly fail fast (crash) in missing extension pref > situations. The preference API checks with a EXTENSION_FUNCTION_VALIDATE whether the extension pref exists (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...). > * Pref Keys tend not to be removed once they are checked in. > > Crashing fast here allows Chrome Developers to easily see their error and fix > it. EXTENSION_FUNCTION_VALIDATE will still crash in a Debug build, but in a Release build it will just kill the renderer process, not the browser. Both of these should be visible to Chrome developers. > On 2013/07/09 07:34:32, Bernhard Bauer wrote: > > This will crash if the pref_key isn't found. Can you wrap it in > > EXTENSION_FUNCTION_VALIDATE? >
https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/api/preference/chrome_direct_setting.cc (right): https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:36: GetPrefService()->FindPreference(pref_key.c_str()); Isn't the JSON API declaration sufficient for limiting prefs that can be accessed? What I wanted to avoid was a declaration in the JSON API and a redundant list that listed the allowed prefs. On 2013/07/09 17:58:27, Bernhard Bauer wrote: > I'm also worried about directly using the passed in pref key instead of going > through the mapping like for the regular preferences API. That means that the > component extension could access any other (registered) preference. In addition, > the API is available to any component extension, so *any* compromised component > extension could change *any* preference. I would feel much more at ease if we > had a mapping or a whitelist of accessible preferences here. https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:37: const base::Value* value = preference->GetValue(); Take a look at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... The set pref path will make it here and crash for a pref that's not in the extension pref value map (in our case, the google geolocation preference) I'll go ahead and make the check here. On 2013/07/09 17:58:27, Bernhard Bauer wrote: > On 2013/07/09 17:27:45, Robert Liao wrote: > > This was a deliberate decision for the following reasons > > * Pref Keys are specified by Chrome Developers the API JSON definition file. > > * The Extension Prefs similarly fail fast (crash) in missing extension pref > > situations. > > The preference API checks with a EXTENSION_FUNCTION_VALIDATE whether the > extension pref exists > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...). > > > * Pref Keys tend not to be removed once they are checked in. > > > > Crashing fast here allows Chrome Developers to easily see their error and fix > > it. > > EXTENSION_FUNCTION_VALIDATE will still crash in a Debug build, but in a Release > build it will just kill the renderer process, not the browser. Both of these > should be visible to Chrome developers. > > > On 2013/07/09 07:34:32, Bernhard Bauer wrote: > > > This will crash if the pref_key isn't found. Can you wrap it in > > > EXTENSION_FUNCTION_VALIDATE? > > >
https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/api/preference/chrome_direct_setting.cc (right): https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:36: GetPrefService()->FindPreference(pref_key.c_str()); On 2013/07/09 18:15:53, Robert Liao wrote: > Isn't the JSON API declaration sufficient for limiting prefs that can be > accessed? Sorry, no :-( The JSON API here is really just used to construct a convenience object that will call the right API method with the right parameter. All of this happens on the renderer side though, so the browser can't trust it. (The browser does check that the extension is allowed to call the API itself, but that mechanism doesn't know about the pref key parameter). > What I wanted to avoid was a declaration in the JSON API and a redundant list > that listed the allowed prefs. We could parse the JSON API in the browser too to extract the preference whitelist, but that would be additional work. If you're willing to implement that, I'm happy to review it :) https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:37: const base::Value* value = preference->GetValue(); On 2013/07/09 18:15:53, Robert Liao wrote: > Take a look at > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > The set pref path will make it here and crash for a pref that's not in the > extension pref value map (in our case, the google geolocation preference) > > I'll go ahead and make the check here. Thanks! This API function won't touch ExtensionPrefValueMap though, because we treat it as a user preference. https://codereview.chromium.org/18341016/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.cc (right): https://codereview.chromium.org/18341016/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:32: EXTENSION_FUNCTION_VALIDATE(preference); Move this one line up please? Otherwise you'll crash right before :)
https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/api/preference/chrome_direct_setting.cc (right): https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:36: GetPrefService()->FindPreference(pref_key.c_str()); I'm going to think about the best way for getting this through. On 2013/07/09 18:45:41, Bernhard Bauer wrote: > On 2013/07/09 18:15:53, Robert Liao wrote: > > Isn't the JSON API declaration sufficient for limiting prefs that can be > > accessed? > > Sorry, no :-( > > The JSON API here is really just used to construct a convenience object that > will call the right API method with the right parameter. All of this happens on > the renderer side though, so the browser can't trust it. (The browser does check > that the extension is allowed to call the API itself, but that mechanism doesn't > know about the pref key parameter). > > > What I wanted to avoid was a declaration in the JSON API and a redundant list > > that listed the allowed prefs. > > We could parse the JSON API in the browser too to extract the preference > whitelist, but that would be additional work. If you're willing to implement > that, I'm happy to review it :) https://codereview.chromium.org/18341016/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:37: const base::Value* value = preference->GetValue(); That's correct. Part of the impetus of this change is due to the issues we encountered previously with ExtensionPrefValueMap (which incidentally, did cause crashes) On 2013/07/09 18:45:41, Bernhard Bauer wrote: > On 2013/07/09 18:15:53, Robert Liao wrote: > > Take a look at > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > The set pref path will make it here and crash for a pref that's not in the > > extension pref value map (in our case, the google geolocation preference) > > > > I'll go ahead and make the check here. > > Thanks! > > This API function won't touch ExtensionPrefValueMap though, because we treat it > as a user preference. > https://codereview.chromium.org/18341016/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.cc (right): https://codereview.chromium.org/18341016/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:32: EXTENSION_FUNCTION_VALIDATE(preference); Oops! Fixed. On 2013/07/09 18:45:41, Bernhard Bauer wrote: > Move this one line up please? Otherwise you'll crash right before :)
I ended up landing on a whitelist * We can't run Javascript in the browser process! * A custom build rule may be interesting if we have more preferences. It's overkill for one. * A hash_set is simple enough to use and initialize, keeping our searches quick.
https://codereview.chromium.org/18341016/diff/34018/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.cc (right): https://codereview.chromium.org/18341016/diff/34018/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:20: const std::string &pref_key) { Nit: the ampersand comes after the type, not before the variable. https://codereview.chromium.org/18341016/diff/34018/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:30: const PreferenceWhitelist DirectSettingFunctionBase::preference_whitelist_ = This creates a static initializer, which isn't allowed in Chrome code. Common alternatives are using a LazyInstance, a custom Singleton class, or a plain old data list of const char[] that you iterate over.
https://codereview.chromium.org/18341016/diff/34018/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.cc (right): https://codereview.chromium.org/18341016/diff/34018/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:20: const std::string &pref_key) { On 2013/07/09 22:27:22, Bernhard Bauer wrote: > Nit: the ampersand comes after the type, not before the variable. Done. https://codereview.chromium.org/18341016/diff/34018/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.cc:30: const PreferenceWhitelist DirectSettingFunctionBase::preference_whitelist_ = I was afraid of that. Changed to use LazyInstance. On 2013/07/09 22:27:22, Bernhard Bauer wrote: > This creates a static initializer, which isn't allowed in Chrome code. > > Common alternatives are using a LazyInstance, a custom Singleton class, or a > plain old data list of const char[] that you iterate over.
https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:32: class PreferenceWhitelist { You can move this class (and the variable) to the implementation file, in an anomymous namespace. https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:34: explicit PreferenceWhitelist(); Explicit is only necessary for single-argument constructors.
https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:32: class PreferenceWhitelist { I'll move the class, but keep it in the namespace. I've found dealing with anonymous namespaces a nightmare in cdb/ntsd/windbg. You can't identify them since they are... anonymous! On 2013/07/09 23:23:44, Bernhard Bauer wrote: > You can move this class (and the variable) to the implementation file, in an > anomymous namespace. https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:34: explicit PreferenceWhitelist(); On 2013/07/09 23:23:44, Bernhard Bauer wrote: > Explicit is only necessary for single-argument constructors. Done.
https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:32: class PreferenceWhitelist { On 2013/07/09 23:37:41, Robert Liao wrote: > I'll move the class, but keep it in the namespace. > > I've found dealing with anonymous namespaces a nightmare in cdb/ntsd/windbg. You > can't identify them since they are... anonymous! I'm sorry, but that's not the customary way in Chromium code: see http://dev.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces. If we really need to reference an internal class somewhere else, we usually put it into an ...::internal namespace, but ease of debugging alone doesn't justify it. > On 2013/07/09 23:23:44, Bernhard Bauer wrote: > > You can move this class (and the variable) to the implementation file, in an > > anomymous namespace. >
https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:32: class PreferenceWhitelist { Where can I provide feedback on this rule? I've burned through a lot of time just waiting on anonymous namespaces to resolve in the debugger. In the meantime, I've wrapped the object. On 2013/07/10 10:52:25, Bernhard Bauer wrote: > On 2013/07/09 23:37:41, Robert Liao wrote: > > I'll move the class, but keep it in the namespace. > > > > I've found dealing with anonymous namespaces a nightmare in cdb/ntsd/windbg. > You > > can't identify them since they are... anonymous! > > I'm sorry, but that's not the customary way in Chromium code: see > http://dev.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces. If we > really need to reference an internal class somewhere else, we usually put it > into an ...::internal namespace, but ease of debugging alone doesn't justify it. > > > On 2013/07/09 23:23:44, Bernhard Bauer wrote: > > > You can move this class (and the variable) to the implementation file, in an > > > anomymous namespace. > > >
LGTM! On 2013/07/10 16:52:39, Robert Liao wrote: > https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... > File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): > > https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... > chrome/browser/extensions/api/preference/chrome_direct_setting.h:32: class > PreferenceWhitelist { > Where can I provide feedback on this rule? I've burned through a lot of time > just waiting on anonymous namespaces to resolve in the debugger. chromium-dev, I guess? Just be prepared for everyone to have an opinion :D
On 2013/07/10 16:59:37, Bernhard Bauer wrote: > LGTM! > > On 2013/07/10 16:52:39, Robert Liao wrote: > > > https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... > > File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): > > > > > https://codereview.chromium.org/18341016/diff/26002/chrome/browser/extensions... > > chrome/browser/extensions/api/preference/chrome_direct_setting.h:32: class > > PreferenceWhitelist { > > Where can I provide feedback on this rule? I've burned through a lot of time > > just waiting on anonymous namespaces to resolve in the debugger. > > chromium-dev, I guess? Just be prepared for everyone to have an opinion :D That's okay. That's the joy associated with coding standards :-)
asargent became OOO. miket: Can you provide owner approval for these files? Thanks! chrome/browser/extensions/api/preference/chrome_direct_setting.h chrome/browser/extensions/api/preference/chrome_direct_setting.cc chrome/browser/extensions/api/preference/preference_api.cc chrome/browser/extensions/api/preference/preference_helpers.cc chrome/browser/extensions/extension_function_histogram_value.h chrome/browser/extensions/extension_function_registry.cc chrome/common/extensions/api/_api_features.json chrome/common/extensions/api/extension_api.cc chrome/common/extensions/api/preferences_private.json chrome/common/extensions/api/types_private.json chrome/common/extensions_api_resources.grd chrome/renderer/extensions/dispatcher.cc chrome/renderer/resources/extensions/chrome_direct_setting.js
https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:19: DirectSettingFunctionBase() {} No inline constructors/destructors in non-virtual classes. http://www.chromium.org/developers/coding-style/cpp-dos-and-donts (I have to re-read this every time I encounter this in a review. If I've missed an exception, please push back.) https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... File chrome/common/extensions/api/types_private.json (right): https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. new file, current year https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:8: "description": "The <code>chrome.types.private</code> API contains private type declarations for Chrome. ", extra whitespace inside quote https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:15: "description": "An interface which allows component extensions direct access to a Chrome browser setting.", which => that (restrictive, not descriptive) https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:25: "description": "What setting to consider.", What => Which https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:30: "description": "Whether to return the setting that applies to the incognito session (default false)." Is it the setting, or the value? https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:66: "description": "What setting to change.", Which https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:76: "description": "Where to set the setting (default: regular). One of<br><var>regular</var>: setting for the regular profile (which is inherited by the incognito profile if not overridden elsewhere),<br><var>regular_only</var>: setting for the regular profile only (not inherited by the incognito profile),<br><var>incognito_persistent</var>: setting for the incognito profile that survives browser restarts (overrides regular preferences),<br><var>incognito_session_only</var>: setting for the incognito profile that can only be set during an incognito session and is deleted when the incognito session ends (overrides regular and incognito_persistent preferences)." Consider a <ul> rather than series of <br>. It might look nicer and will give more clues to accessibility tools. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:83: "description": "Called after the setting has been set.", To be insanely hyper-technical, this description could be misleading. Someone might read it as a guarantee that the callback will be called *immediately* after the setting's value changes. But it might actually happen after housekeeping, or firing other events. Who knows. For this reason, you might want to describe at a higher level: "Called at the completion of this operation" or something like that, which clearly doesn't make the same promises, and which is actually more true. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:92: "description": "Clears the setting. This way default settings can become effective again.", The style of this description isn't consistent with the others. How about something like this instead? "Clears the setting and restores any default value." https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:97: "description": "What setting to clear.", Which https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:103: "description": "Where to clear the setting (default: regular). One of<br><var>regular</var>: setting for the regular profile (which is inherited by the incognito profile if not overridden elsewhere),<br><var>regular_only</var>: setting for the regular profile only (not inherited by the incognito profile),<br><var>incognito_persistent</var>: setting for the incognito profile that survives browser restarts (overrides regular preferences),<br><var>incognito_session_only</var>: setting for the incognito profile that can only be set during an incognito session and is deleted when the incognito session ends (overrides regular and incognito_persistent preferences)." I think it'd be OK to say simply that this works the same way as set, rather than an error-prone copy/paste of the same text. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:110: "description": "Called after the setting has been cleared.", Same as above https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:120: "description": "Fired when the value of the setting changes.", I wonder whether "the value of" is overly verbose. Can a setting ever change without the value of the setting changing? https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:127: "description": "The value of the setting.", The *new* value? https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:131: "description": "Whether the value that has changed is specific to the incognito session.<br>This property will <em>only</em> be present if the user has enabled the extension in incognito mode.", It's more correct to say "This property will be present only if" rather than "This property will only be present if."
https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:19: DirectSettingFunctionBase() {} I think the intent behind this rule is for libraries consumed by other components. Given that no one directly constructs these objects (instantiation is handled via RegisterFunction), I think we are okay here. This was modeled after the Preference API Functions (e.g. GetPreferenceFunction) with a twist on adding DISALLOW_COPY_AND_ASSIGN, which requires an explicit construction declaration and implementation. If there is a strong desire to move the {}'s, I can do so. Unfortunately, it clutters the implementation file. Alternatively, I can remove DISALLOW_COPY_AND_ASSIGN. On 2013/07/10 18:34:47, miket wrote: > No inline constructors/destructors in non-virtual classes. > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts (I have to > re-read this every time I encounter this in a review. If I've missed an > exception, please push back.) https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... File chrome/common/extensions/api/types_private.json (right): https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/10 18:34:47, miket wrote: > new file, current year Done. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:8: "description": "The <code>chrome.types.private</code> API contains private type declarations for Chrome. ", On 2013/07/10 18:34:47, miket wrote: > extra whitespace inside quote Done. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:15: "description": "An interface which allows component extensions direct access to a Chrome browser setting.", On 2013/07/10 18:34:47, miket wrote: > which => that (restrictive, not descriptive) Done. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:25: "description": "What setting to consider.", All of these maintain consistency with the published types.json. We would need to update types.json as well. On 2013/07/10 18:34:47, miket wrote: > What => Which https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:30: "description": "Whether to return the setting that applies to the incognito session (default false)." Value. Fixed. A comment question above that was missed is can anyone envision a scenario where component extensions would need access to the incognito settings? If not, I'd prefer to remove these right here. On 2013/07/10 18:34:47, miket wrote: > Is it the setting, or the value? https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:66: "description": "What setting to change.", On 2013/07/10 18:34:47, miket wrote: > Which Done. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:76: "description": "Where to set the setting (default: regular). One of<br><var>regular</var>: setting for the regular profile (which is inherited by the incognito profile if not overridden elsewhere),<br><var>regular_only</var>: setting for the regular profile only (not inherited by the incognito profile),<br><var>incognito_persistent</var>: setting for the incognito profile that survives browser restarts (overrides regular preferences),<br><var>incognito_session_only</var>: setting for the incognito profile that can only be set during an incognito session and is deleted when the incognito session ends (overrides regular and incognito_persistent preferences)." The follows the style of types.json. If we want to update it, I'll send a separate CL for types.json. On 2013/07/10 18:34:47, miket wrote: > Consider a <ul> rather than series of <br>. It might look nicer and will give > more clues to accessibility tools. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:83: "description": "Called after the setting has been set.", This was copy+pasted from types.json. We would have to update both for consistency. On 2013/07/10 18:34:47, miket wrote: > To be insanely hyper-technical, this description could be misleading. Someone > might read it as a guarantee that the callback will be called *immediately* > after the setting's value changes. But it might actually happen after > housekeeping, or firing other events. Who knows. For this reason, you might want > to describe at a higher level: "Called at the completion of this operation" or > something like that, which clearly doesn't make the same promises, and which is > actually more true. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:92: "description": "Clears the setting. This way default settings can become effective again.", Agreed. types.json would need to be updated. On 2013/07/10 18:34:47, miket wrote: > The style of this description isn't consistent with the others. How about > something like this instead? > > "Clears the setting and restores any default value." https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:103: "description": "Where to clear the setting (default: regular). One of<br><var>regular</var>: setting for the regular profile (which is inherited by the incognito profile if not overridden elsewhere),<br><var>regular_only</var>: setting for the regular profile only (not inherited by the incognito profile),<br><var>incognito_persistent</var>: setting for the incognito profile that survives browser restarts (overrides regular preferences),<br><var>incognito_session_only</var>: setting for the incognito profile that can only be set during an incognito session and is deleted when the incognito session ends (overrides regular and incognito_persistent preferences)." For API reference docs, it's better to restate. Sometimes, methods are viewed in isolation. It's annoying to have to jump around. On 2013/07/10 18:34:47, miket wrote: > I think it'd be OK to say simply that this works the same way as set, rather > than an error-prone copy/paste of the same text. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:110: "description": "Called after the setting has been cleared.", See response above. On 2013/07/10 18:34:47, miket wrote: > Same as above https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:120: "description": "Fired when the value of the setting changes.", I would be better to omit this. types.json would also need updating. On 2013/07/10 18:34:47, miket wrote: > I wonder whether "the value of" is overly verbose. Can a setting ever change > without the value of the setting changing? https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:127: "description": "The value of the setting.", The new is redundant. The setting has already changed. (types.json copy as well) On 2013/07/10 18:34:47, miket wrote: > The *new* value? https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:131: "description": "Whether the value that has changed is specific to the incognito session.<br>This property will <em>only</em> be present if the user has enabled the extension in incognito mode.", Agreed. On all of these comments, if we can modify types.json with the new language, we can update these strings. If we cannot touch types.json (since it is already published), we should maintain consistency with the existing language. On 2013/07/10 18:34:47, miket wrote: > It's more correct to say "This property will be present only if" rather than > "This property will only be present if."
https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:19: DirectSettingFunctionBase() {} Furthermore, these classes shouldn't store state due to the transient nature of functions, so I think we should definitely be okay here. On 2013/07/10 20:20:09, Robert Liao wrote: > I think the intent behind this rule is for libraries consumed by other > components. Given that no one directly constructs these objects (instantiation > is handled via RegisterFunction), I think we are okay here. > > This was modeled after the Preference API Functions (e.g. GetPreferenceFunction) > with a twist on adding DISALLOW_COPY_AND_ASSIGN, which requires an explicit > construction declaration and implementation. > > If there is a strong desire to move the {}'s, I can do so. Unfortunately, it > clutters the implementation file. > > Alternatively, I can remove DISALLOW_COPY_AND_ASSIGN. > On 2013/07/10 18:34:47, miket wrote: > > No inline constructors/destructors in non-virtual classes. > > > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts (I have to > > re-read this every time I encounter this in a review. If I've missed an > > exception, please push back.) >
https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:19: DirectSettingFunctionBase() {} Furthermore, these classes shouldn't store state due to the transient nature of functions, so I think we should definitely be okay here. On 2013/07/10 20:20:09, Robert Liao wrote: > I think the intent behind this rule is for libraries consumed by other > components. Given that no one directly constructs these objects (instantiation > is handled via RegisterFunction), I think we are okay here. > > This was modeled after the Preference API Functions (e.g. GetPreferenceFunction) > with a twist on adding DISALLOW_COPY_AND_ASSIGN, which requires an explicit > construction declaration and implementation. > > If there is a strong desire to move the {}'s, I can do so. Unfortunately, it > clutters the implementation file. > > Alternatively, I can remove DISALLOW_COPY_AND_ASSIGN. > On 2013/07/10 18:34:47, miket wrote: > > No inline constructors/destructors in non-virtual classes. > > > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts (I have to > > re-read this every time I encounter this in a review. If I've missed an > > exception, please push back.) >
lgtm https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... File chrome/common/extensions/api/types_private.json (right): https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:25: "description": "What setting to consider.", On 2013/07/10 20:20:09, Robert Liao wrote: > All of these maintain consistency with the published types.json. We would need > to update types.json as well. There's absolutely nothing stopping you from improving the existing code, especially in the documentation case where there isn't any backward-compatibility issue. See http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule. But it's your CL. Do what you want. https://codereview.chromium.org/18341016/diff/61003/chrome/common/extensions/... chrome/common/extensions/api/types_private.json:127: "description": "The value of the setting.", On 2013/07/10 20:20:09, Robert Liao wrote: > The new is redundant. The setting has already changed. (types.json copy as well) > On 2013/07/10 18:34:47, miket wrote: > > The *new* value? A common pattern in some APIs like this is to return the existing value, figuring that if you changed it, you already know what you changed it to, but you're interested in the old value if you were doing some kind of audit log or undo functionality. It's up to you if you want it to be ambiguous. I suppose developers can test it if their natural reading is opposite yours.
https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:19: DirectSettingFunctionBase() {} On 2013/07/10 20:20:09, Robert Liao wrote: > I think the intent behind this rule is for libraries consumed by other > components. Given that no one directly constructs these objects (instantiation > is handled via RegisterFunction), I think we are okay here. No, the main purpose of the rule is to prevent unnecessary code; see the background section at http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... > This was modeled after the Preference API Functions (e.g. GetPreferenceFunction) > with a twist on adding DISALLOW_COPY_AND_ASSIGN, which requires an explicit > construction declaration and implementation. > > If there is a strong desire to move the {}'s, I can do so. Unfortunately, it > clutters the implementation file. > > Alternatively, I can remove DISALLOW_COPY_AND_ASSIGN. > On 2013/07/10 18:34:47, miket wrote: > > No inline constructors/destructors in non-virtual classes. > > > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts (I have to > > re-read this every time I encounter this in a review. If I've missed an > > exception, please push back.) > https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:19: DirectSettingFunctionBase() {} On 2013/07/10 20:33:33, Robert Liao wrote: > Furthermore, these classes shouldn't store state due to the transient nature of > functions, so I think we should definitely be okay here. Stateless or not is not the issue here, but whether the constructor/destructor constains a non-trivial amount of code. In this case, it does.
https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:19: DirectSettingFunctionBase() {} What non-trivial code is present here? The compiler would have generated a default constructor if I didn't have DISALLOW_COPY_AND_ASSIGN. On 2013/07/10 22:11:26, Bernhard Bauer wrote: > On 2013/07/10 20:33:33, Robert Liao wrote: > > Furthermore, these classes shouldn't store state due to the transient nature > of > > functions, so I think we should definitely be okay here. > > Stateless or not is not the issue here, but whether the constructor/destructor > constains a non-trivial amount of code. In this case, it does.
https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:19: DirectSettingFunctionBase() {} On 2013/07/10 22:18:45, Robert Liao wrote: > What non-trivial code is present here? The compiler would have generated a > default constructor if I didn't have DISALLOW_COPY_AND_ASSIGN. Exactly, and it still does generate the default constructor (which consists of calling the base constructor). The guide explicitly mentions "no base class" as one of the conditions for allowing inline constructors. Without the DISALLOW_COPY_AND_ASSIGN and the explicitly declared constructor, I probably wouldn't have caught it, but it would have been wrong nevertheless. > On 2013/07/10 22:11:26, Bernhard Bauer wrote: > > On 2013/07/10 20:33:33, Robert Liao wrote: > > > Furthermore, these classes shouldn't store state due to the transient nature > > of > > > functions, so I think we should definitely be okay here. > > > > Stateless or not is not the issue here, but whether the constructor/destructor > > constains a non-trivial amount of code. In this case, it does. >
https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... File chrome/browser/extensions/api/preference/chrome_direct_setting.h (right): https://codereview.chromium.org/18341016/diff/61003/chrome/browser/extensions... chrome/browser/extensions/api/preference/chrome_direct_setting.h:19: DirectSettingFunctionBase() {} I went ahead and removed the inline constructors and destructors. I feel it comes at the expense of readability in chrome_direct_setting.cc for a scenario that won't occur in the style guideline. On 2013/07/10 22:37:24, Bernhard Bauer wrote: > On 2013/07/10 22:18:45, Robert Liao wrote: > > What non-trivial code is present here? The compiler would have generated a > > default constructor if I didn't have DISALLOW_COPY_AND_ASSIGN. > > Exactly, and it still does generate the default constructor (which consists of > calling the base constructor). The guide explicitly mentions "no base class" as > one of the conditions for allowing inline constructors. Without the > DISALLOW_COPY_AND_ASSIGN and the explicitly declared constructor, I probably > wouldn't have caught it, but it would have been wrong nevertheless. > > > On 2013/07/10 22:11:26, Bernhard Bauer wrote: > > > On 2013/07/10 20:33:33, Robert Liao wrote: > > > > Furthermore, these classes shouldn't store state due to the transient > nature > > > of > > > > functions, so I think we should definitely be okay here. > > > > > > Stateless or not is not the issue here, but whether the > constructor/destructor > > > constains a non-trivial amount of code. In this case, it does. > > >
LGTM!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/18341016/29003
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/18341016/29003
Message was sent while issue was closed.
Change committed as 211300 |
