Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1015623005:
chrome.settingsPrivate: Basic stub implementation (Closed)
|
Created:
5 years, 9 months ago by Oren Blasberg Modified:
5 years, 9 months ago Reviewers:
Ken Rockot(use gerrit already), stevenjb, Dan Beam, michaelpg, Jeremy Klein, Mark P, James Hawkins, not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, michaelpg, James Hawkins, Kyle Horimoto, Jeremy Klein Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionchrome.settingsPrivate: Add a basic stub implementation for the API.
Design doc: https://docs.google.com/a/chromium.org/document/d/1PCQltNDdyZyuPHUdqYIjeObRrVOhf8WTr6BLfuTQ4ew/edit?pli=1#
BUG=468093
NOPRESUBMIT=true
Committed: https://crrev.com/87ba87f5a758cfe8252a591984db7f7ad8a29be4
Cr-Commit-Position: refs/heads/master@{#322069}
Patch Set 1 #Patch Set 2 : Add stub implementations of the methods. #
Total comments: 10
Patch Set 3 : Respond to initial comments. #
Total comments: 1
Patch Set 4 : Update the histograms file. #Patch Set 5 : Add a PrefData type to the json. #Patch Set 6 : #Patch Set 7 : Update to reflect design #Patch Set 8 : Update API to reflect design changes. Still error on calling method. #
Total comments: 18
Patch Set 9 : (intermediate) Attempting to synchronously implement setbooleanpref #Patch Set 10 : Fixed all issues + addressed comments. Just need to add getPref and we're done.... #Patch Set 11 : Adding getPref function. #Patch Set 12 : Some small cleanups #
Total comments: 15
Patch Set 13 : Updating the externs file #Patch Set 14 : Extracting and logging the parameters #
Total comments: 10
Patch Set 15 : Addressing nits #
Total comments: 4
Patch Set 16 : Sync + merge conflicts in histogram #
Total comments: 2
Patch Set 17 : Add new permission. #Patch Set 18 : Update the permission message #Messages
Total messages: 72 (27 generated)
orenb@chromium.org changed reviewers: + stevenjb@chromium.org
jhawkins@chromium.org changed reviewers: + jhawkins@chromium.org
https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/settings_private_api.h (right): https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.h:17: namespace settings_private { This is empty; please remove.
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.cc:24: SendResponse(true); What is this for? ExtensionFunction says not to call this: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/settings_private_api.h (right): https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.h:5: #ifndef EXTENSIONS_BROWSER_API_SETTINGS_PRIVATE_SETTINGS_PRIVATE_API_H_ nit: should be BROWSER_EXTENSIONS not EXTENISONS_BROWSER
orenb@chromium.org changed reviewers: + mpearson@chromium.org
+Mark for owners on histogram file (thanks in advance! lmk if you have any questions).. https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/03/17 23:32:29, michaelpg wrote: > 2015 Done. https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.cc:24: SendResponse(true); On 2015/03/17 23:32:29, michaelpg wrote: > What is this for? ExtensionFunction says not to call this: > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... IIRC, it's so that the js gets a 'success' response (in other words, so that a value actually gets returned or a response is provided, rather than just some error being thrown). I think when the documentation there says "don't call this directly" it means "don't call this from outside places." Here I'm extending that class so it's ok to call it. https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/settings_private_api.h (right): https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/03/17 23:32:29, michaelpg wrote: > 2015 Done. https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.h:5: #ifndef EXTENSIONS_BROWSER_API_SETTINGS_PRIVATE_SETTINGS_PRIVATE_API_H_ On 2015/03/17 23:32:29, michaelpg wrote: > nit: should be BROWSER_EXTENSIONS not EXTENISONS_BROWSER Done. https://codereview.chromium.org/1015623005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.h:17: namespace settings_private { On 2015/03/17 23:28:42, James Hawkins wrote: > This is empty; please remove. Done.
histograms.xml is missing
On 2015/03/17 23:50:13, Mark P wrote: > histograms.xml is missing whoops. Fixed.
https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... File chrome/common/extensions/api/settings_private.json (right): https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... chrome/common/extensions/api/settings_private.json:57: "description": "Gets all the prefs in one dictionary.", I think that rather than return an Object, the callback should return something like Array<PrefEntry>, where PrefEntry is an object with a key property, and optional string/bool/numberValue properties.
histograms.xml lgtm
On 2015/03/18 00:01:04, stevenjb wrote: > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > File chrome/common/extensions/api/settings_private.json (right): > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > chrome/common/extensions/api/settings_private.json:57: "description": "Gets all > the prefs in one dictionary.", > I think that rather than return an Object, the callback should return something > like Array<PrefEntry>, where PrefEntry is an object with a key property, and > optional string/bool/numberValue properties. Sounds good. A couple follow up questions though: 1. Can PrefEntry be defined as a type (via typedef) someplace though? If so, where? Or, will the callback just have to take an Array of Objects as far as the settings_private.json is concerned? (and we'd just add in the documentation someplace in that it's actually a PrefEntry) 2. Would PrefEntry have an entry for each of those 3 types of values, and then exactly one of those 3 would have an actual defined value? (So in the JS callback it'd have to check which of the 3 is populated?) I guess we would want the type checking (which, depending on the answer to question 1, may or may not be attainable)... so to keep it, maybe we'd add a 'type' key so the client knows which of the 3 other keys to query for the value. Thoughts on this?
On 2015/03/18 00:09:57, Oren Blasberg wrote: > On 2015/03/18 00:01:04, stevenjb wrote: > > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > > File chrome/common/extensions/api/settings_private.json (right): > > > > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > > chrome/common/extensions/api/settings_private.json:57: "description": "Gets > all > > the prefs in one dictionary.", > > I think that rather than return an Object, the callback should return > something > > like Array<PrefEntry>, where PrefEntry is an object with a key property, and > > optional string/bool/numberValue properties. > > Sounds good. A couple follow up questions though: > > 1. Can PrefEntry be defined as a type (via typedef) someplace though? If so, > where? Or, will the callback just have to take an Array of Objects as far as the > settings_private.json is concerned? (and we'd just add in the documentation > someplace in that it's actually a PrefEntry) > > 2. Would PrefEntry have an entry for each of those 3 types of values, and then > exactly one of those 3 would have an actual defined value? (So in the JS > callback it'd have to check which of the 3 is populated?) I guess we would want > the type checking (which, depending on the answer to question 1, may or may not > be attainable)... so to keep it, maybe we'd add a 'type' key so the client knows > which of the 3 other keys to query for the value. Thoughts on this? I think it would be nice to have a typedef for this. My thought is that I would be parsing these into Preference objects (https://docs.google.com/a/chromium.org/document/d/1PnGKrP6_dXS3L1h36buxHPW3zS...), but maybe we don't need that if we have a nice typedef already?
On 2015/03/18 00:09:57, Oren Blasberg wrote: > On 2015/03/18 00:01:04, stevenjb wrote: > > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > > File chrome/common/extensions/api/settings_private.json (right): > > > > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > > chrome/common/extensions/api/settings_private.json:57: "description": "Gets > all > > the prefs in one dictionary.", > > I think that rather than return an Object, the callback should return > something > > like Array<PrefEntry>, where PrefEntry is an object with a key property, and > > optional string/bool/numberValue properties. > > Sounds good. A couple follow up questions though: > > 1. Can PrefEntry be defined as a type (via typedef) someplace though? If so, > where? Or, will the callback just have to take an Array of Objects as far as the > settings_private.json is concerned? (and we'd just add in the documentation > someplace in that it's actually a PrefEntry) > > 2. Would PrefEntry have an entry for each of those 3 types of values, and then > exactly one of those 3 would have an actual defined value? (So in the JS > callback it'd have to check which of the 3 is populated?) I guess we would want > the type checking (which, depending on the answer to question 1, may or may not > be attainable)... so to keep it, maybe we'd add a 'type' key so the client knows > which of the 3 other keys to query for the value. Thoughts on this? 1. We can and should define the type in the .json, but we also need to define it in something like settings_private_externs.js unless/until we auto-generate that from the .json. See chrome_extentions.js for current examples, but we do not want to add to that already too large file. I started playing with that with networkingPrivate but did not get very far. I am open to suggestions here. 2. It depends on how careful we want to be with types. Again, I am open to suggestions here.
On 2015/03/18 00:17:59, stevenjb wrote: > On 2015/03/18 00:09:57, Oren Blasberg wrote: > > On 2015/03/18 00:01:04, stevenjb wrote: > > > > > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > > > File chrome/common/extensions/api/settings_private.json (right): > > > > > > > > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > > > chrome/common/extensions/api/settings_private.json:57: "description": "Gets > > all > > > the prefs in one dictionary.", > > > I think that rather than return an Object, the callback should return > > something > > > like Array<PrefEntry>, where PrefEntry is an object with a key property, and > > > optional string/bool/numberValue properties. > > > > Sounds good. A couple follow up questions though: > > > > 1. Can PrefEntry be defined as a type (via typedef) someplace though? If so, > > where? Or, will the callback just have to take an Array of Objects as far as > the > > settings_private.json is concerned? (and we'd just add in the documentation > > someplace in that it's actually a PrefEntry) > > > > 2. Would PrefEntry have an entry for each of those 3 types of values, and then > > exactly one of those 3 would have an actual defined value? (So in the JS > > callback it'd have to check which of the 3 is populated?) I guess we would > want > > the type checking (which, depending on the answer to question 1, may or may > not > > be attainable)... so to keep it, maybe we'd add a 'type' key so the client > knows > > which of the 3 other keys to query for the value. Thoughts on this? > > 1. We can and should define the type in the .json, but we also need to define it > in something like settings_private_externs.js unless/until we auto-generate that > from the .json. See chrome_extentions.js for current examples, but we do not > want to add to that already too large file. I started playing with that with > networkingPrivate but did not get very far. I am open to suggestions here. > > 2. It depends on how careful we want to be with types. Again, I am open to > suggestions here. OK I've added a new PrefData type in settings_private.json and updated getAllPrefs so its callback takes an array of these. Hurrah. Now, my question: where does it make sense to put the new settings_private_externs.js file which will have the typedef for this PrefData?
On 2015/03/18 23:33:00, Oren Blasberg wrote: > On 2015/03/18 00:17:59, stevenjb wrote: > > On 2015/03/18 00:09:57, Oren Blasberg wrote: > > > On 2015/03/18 00:01:04, stevenjb wrote: > > > > > > > > > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > > > > File chrome/common/extensions/api/settings_private.json (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > > > > chrome/common/extensions/api/settings_private.json:57: "description": > "Gets > > > all > > > > the prefs in one dictionary.", > > > > I think that rather than return an Object, the callback should return > > > something > > > > like Array<PrefEntry>, where PrefEntry is an object with a key property, > and > > > > optional string/bool/numberValue properties. > > > > > > Sounds good. A couple follow up questions though: > > > > > > 1. Can PrefEntry be defined as a type (via typedef) someplace though? If so, > > > where? Or, will the callback just have to take an Array of Objects as far as > > the > > > settings_private.json is concerned? (and we'd just add in the documentation > > > someplace in that it's actually a PrefEntry) > > > > > > 2. Would PrefEntry have an entry for each of those 3 types of values, and > then > > > exactly one of those 3 would have an actual defined value? (So in the JS > > > callback it'd have to check which of the 3 is populated?) I guess we would > > want > > > the type checking (which, depending on the answer to question 1, may or may > > not > > > be attainable)... so to keep it, maybe we'd add a 'type' key so the client > > knows > > > which of the 3 other keys to query for the value. Thoughts on this? > > > > 1. We can and should define the type in the .json, but we also need to define > it > > in something like settings_private_externs.js unless/until we auto-generate > that > > from the .json. See chrome_extentions.js for current examples, but we do not > > want to add to that already too large file. I started playing with that with > > networkingPrivate but did not get very far. I am open to suggestions here. > > > > 2. It depends on how careful we want to be with types. Again, I am open to > > suggestions here. > > OK I've added a new PrefData type in settings_private.json and updated > getAllPrefs so its callback takes an array of these. Hurrah. > > Now, my question: where does it make sense to put the new > settings_private_externs.js file which will have the typedef for this PrefData? Should the new externs file go in src/third_party/closure_compiler/externs/ ? If so, does that just automatically get bundled with the build or do I need to modify some other generated resources or gyp file to make that work?
On 2015/03/18 23:37:32, Oren Blasberg wrote: > On 2015/03/18 23:33:00, Oren Blasberg wrote: > > On 2015/03/18 00:17:59, stevenjb wrote: > > > On 2015/03/18 00:09:57, Oren Blasberg wrote: > > > > On 2015/03/18 00:01:04, stevenjb wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > > > > > File chrome/common/extensions/api/settings_private.json (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1015623005/diff/40001/chrome/common/extension... > > > > > chrome/common/extensions/api/settings_private.json:57: "description": > > "Gets > > > > all > > > > > the prefs in one dictionary.", > > > > > I think that rather than return an Object, the callback should return > > > > something > > > > > like Array<PrefEntry>, where PrefEntry is an object with a key property, > > and > > > > > optional string/bool/numberValue properties. > > > > > > > > Sounds good. A couple follow up questions though: > > > > > > > > 1. Can PrefEntry be defined as a type (via typedef) someplace though? If > so, > > > > where? Or, will the callback just have to take an Array of Objects as far > as > > > the > > > > settings_private.json is concerned? (and we'd just add in the > documentation > > > > someplace in that it's actually a PrefEntry) > > > > > > > > 2. Would PrefEntry have an entry for each of those 3 types of values, and > > then > > > > exactly one of those 3 would have an actual defined value? (So in the JS > > > > callback it'd have to check which of the 3 is populated?) I guess we would > > > want > > > > the type checking (which, depending on the answer to question 1, may or > may > > > not > > > > be attainable)... so to keep it, maybe we'd add a 'type' key so the client > > > knows > > > > which of the 3 other keys to query for the value. Thoughts on this? > > > > > > 1. We can and should define the type in the .json, but we also need to > define > > it > > > in something like settings_private_externs.js unless/until we auto-generate > > that > > > from the .json. See chrome_extentions.js for current examples, but we do not > > > want to add to that already too large file. I started playing with that with > > > networkingPrivate but did not get very far. I am open to suggestions here. > > > > > > 2. It depends on how careful we want to be with types. Again, I am open to > > > suggestions here. > > > > OK I've added a new PrefData type in settings_private.json and updated > > getAllPrefs so its callback takes an array of these. Hurrah. > > > > Now, my question: where does it make sense to put the new > > settings_private_externs.js file which will have the typedef for this > PrefData? > > Should the new externs file go in src/third_party/closure_compiler/externs/ ? > If so, does that just automatically get bundled with the build or do I need to > modify some other generated resources or gyp file to make that work? You'll need to add settings_private.js to c/b/resources/options/compiled_resources.gyp (and make sure the closure compiler succeeds).
I think I may have identified why you were seeing that error yesterday, see my comment in settings_private_api.cc. Otherwise looking good. https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:20: // TODO(orenb): Implement. We should VLOG something since we haven't implemented it yet (here and below). https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:21: return true; Also, this needs to call Success() asynchronously, e.g.: base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(&SettingsPrivateSetBooleanPrefFunction::Success, this, true)); https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:25: base::FundamentalValue* value = new base::FundamentalValue(true); This needs to be a PrefObject type, which will appear in the core_api::settings_private namespace. You'll need to include "extensions/common/api/settings_private.h", which is a generated header that will show up in out/Debug/gen/extensions/common/api/. https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.json (right): https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... chrome/common/extensions/api/settings_private.json:20: "description": "The key for the setting." nit: s/setting/pref throughout the descriptions. (It's a subtle difference, but an individual stored value is a "pref", whereas "settings" refers to the API, UI, etc, affecting the prefs). https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... chrome/common/extensions/api/settings_private.json:24: "optional": false, This should be an enum. https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... chrome/common/extensions/api/settings_private.json:30: "description": "The value of the setting." nit: The current value of the setting https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... chrome/common/extensions/api/settings_private.json:35: "enum": ["user", "device"], , "none" https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... File third_party/closure_compiler/externs/settings_private.js (right): https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:10: * type: string, enum https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:12: * source: string enum
orenb@chromium.org changed reviewers: + dbeam@chromium.org
Hey Steven, I addressed all the comments. However, I am having trouble implementing getPref() (singular). I want to pass to the callback a PrefObject, but in the CC file, I can't require the settings_private.h since it apparently isn't being generated. out/Debug/gen/extensions/common/api/ doesn't have a settings_private.h in it. Is there some other change I need to make so that this gets generated? https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:20: // TODO(orenb): Implement. On 2015/03/20 15:45:29, stevenjb wrote: > We should VLOG something since we haven't implemented it yet (here and below). Done. https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:21: return true; On 2015/03/20 15:45:29, stevenjb wrote: > Also, this needs to call Success() asynchronously, e.g.: > > base::MessageLoop::current()->PostTask(FROM_HERE, > base::Bind(&SettingsPrivateSetBooleanPrefFunction::Success, this, true)); Acknowledged. I got this working by switching to having the 3rd arg optional callback. The true will be passed into that now (for now). https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:25: base::FundamentalValue* value = new base::FundamentalValue(true); On 2015/03/20 15:45:29, stevenjb wrote: > This needs to be a PrefObject type, which will appear in the > core_api::settings_private namespace. You'll need to include > "extensions/common/api/settings_private.h", which is a generated header that > will show up in out/Debug/gen/extensions/common/api/. As discussed offline, this is a set method not a get method https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.json (right): https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... chrome/common/extensions/api/settings_private.json:20: "description": "The key for the setting." On 2015/03/20 15:45:29, stevenjb wrote: > nit: s/setting/pref throughout the descriptions. (It's a subtle difference, but > an individual stored value is a "pref", whereas "settings" refers to the API, > UI, etc, affecting the prefs). Done. https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... chrome/common/extensions/api/settings_private.json:24: "optional": false, On 2015/03/20 15:45:29, stevenjb wrote: > This should be an enum. Done. https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... chrome/common/extensions/api/settings_private.json:30: "description": "The value of the setting." On 2015/03/20 15:45:29, stevenjb wrote: > nit: The current value of the setting > Done. https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... chrome/common/extensions/api/settings_private.json:35: "enum": ["user", "device"], On 2015/03/20 15:45:29, stevenjb wrote: > , "none" Adding None causes a compiler error because None is already implicitly present. So, no need to add it in the idl. https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... File third_party/closure_compiler/externs/settings_private.js (right): https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:10: * type: string, On 2015/03/20 15:45:29, stevenjb wrote: > enum Done. https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:12: * source: string On 2015/03/20 15:45:29, stevenjb wrote: > enum Done.
On 2015/03/20 18:58:53, Oren Blasberg wrote: > Hey Steven, I addressed all the comments. However, I am having trouble > implementing getPref() (singular). I want to pass to the callback a > PrefObject, but in the CC file, I can't require the settings_private.h since it > apparently isn't being generated. > > out/Debug/gen/extensions/common/api/ doesn't have a settings_private.h in it. > > Is there some other change I need to make so that this gets generated? > > https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... > File chrome/browser/extensions/api/settings_private/settings_private_api.cc > (right): > > https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/api/settings_private/settings_private_api.cc:20: // > TODO(orenb): Implement. > On 2015/03/20 15:45:29, stevenjb wrote: > > We should VLOG something since we haven't implemented it yet (here and below). > > Done. > > https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/api/settings_private/settings_private_api.cc:21: > return true; > On 2015/03/20 15:45:29, stevenjb wrote: > > Also, this needs to call Success() asynchronously, e.g.: > > > > base::MessageLoop::current()->PostTask(FROM_HERE, > > base::Bind(&SettingsPrivateSetBooleanPrefFunction::Success, this, true)); > > Acknowledged. I got this working by switching to having the 3rd arg optional > callback. The true will be passed into that now (for now). > > https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/api/settings_private/settings_private_api.cc:25: > base::FundamentalValue* value = new base::FundamentalValue(true); > On 2015/03/20 15:45:29, stevenjb wrote: > > This needs to be a PrefObject type, which will appear in the > > core_api::settings_private namespace. You'll need to include > > "extensions/common/api/settings_private.h", which is a generated header that > > will show up in out/Debug/gen/extensions/common/api/. > > As discussed offline, this is a set method not a get method > > https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... > File chrome/common/extensions/api/settings_private.json (right): > > https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... > chrome/common/extensions/api/settings_private.json:20: "description": "The key > for the setting." > On 2015/03/20 15:45:29, stevenjb wrote: > > nit: s/setting/pref throughout the descriptions. (It's a subtle difference, > but > > an individual stored value is a "pref", whereas "settings" refers to the API, > > UI, etc, affecting the prefs). > > Done. > > https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... > chrome/common/extensions/api/settings_private.json:24: "optional": false, > On 2015/03/20 15:45:29, stevenjb wrote: > > This should be an enum. > > Done. > > https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... > chrome/common/extensions/api/settings_private.json:30: "description": "The value > of the setting." > On 2015/03/20 15:45:29, stevenjb wrote: > > nit: The current value of the setting > > > > Done. > > https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... > chrome/common/extensions/api/settings_private.json:35: "enum": ["user", > "device"], > On 2015/03/20 15:45:29, stevenjb wrote: > > , "none" > > Adding None causes a compiler error because None is already implicitly present. > So, no need to add it in the idl. > > https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... > File third_party/closure_compiler/externs/settings_private.js (right): > > https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... > third_party/closure_compiler/externs/settings_private.js:10: * type: string, > On 2015/03/20 15:45:29, stevenjb wrote: > > enum > > Done. > > https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... > third_party/closure_compiler/externs/settings_private.js:12: * source: string > On 2015/03/20 15:45:29, stevenjb wrote: > > enum > > Done. Ahh... it WAS being generated; it was just in a different directory. Please hold off ; I will finish up my getPref changes now that this is working. Will reply again when it's ready.
On 2015/03/20 19:03:15, Oren Blasberg wrote: > On 2015/03/20 18:58:53, Oren Blasberg wrote: > > Hey Steven, I addressed all the comments. However, I am having trouble > > implementing getPref() (singular). I want to pass to the callback a > > PrefObject, but in the CC file, I can't require the settings_private.h since > it > > apparently isn't being generated. > > > > out/Debug/gen/extensions/common/api/ doesn't have a settings_private.h in it. > > > > Is there some other change I need to make so that this gets generated? > > > > > https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... > > File chrome/browser/extensions/api/settings_private/settings_private_api.cc > > (right): > > > > > https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... > > chrome/browser/extensions/api/settings_private/settings_private_api.cc:20: // > > TODO(orenb): Implement. > > On 2015/03/20 15:45:29, stevenjb wrote: > > > We should VLOG something since we haven't implemented it yet (here and > below). > > > > Done. > > > > > https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... > > chrome/browser/extensions/api/settings_private/settings_private_api.cc:21: > > return true; > > On 2015/03/20 15:45:29, stevenjb wrote: > > > Also, this needs to call Success() asynchronously, e.g.: > > > > > > base::MessageLoop::current()->PostTask(FROM_HERE, > > > base::Bind(&SettingsPrivateSetBooleanPrefFunction::Success, this, true)); > > > > Acknowledged. I got this working by switching to having the 3rd arg optional > > callback. The true will be passed into that now (for now). > > > > > https://codereview.chromium.org/1015623005/diff/140001/chrome/browser/extensi... > > chrome/browser/extensions/api/settings_private/settings_private_api.cc:25: > > base::FundamentalValue* value = new base::FundamentalValue(true); > > On 2015/03/20 15:45:29, stevenjb wrote: > > > This needs to be a PrefObject type, which will appear in the > > > core_api::settings_private namespace. You'll need to include > > > "extensions/common/api/settings_private.h", which is a generated header that > > > will show up in out/Debug/gen/extensions/common/api/. > > > > As discussed offline, this is a set method not a get method > > > > > https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... > > File chrome/common/extensions/api/settings_private.json (right): > > > > > https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... > > chrome/common/extensions/api/settings_private.json:20: "description": "The key > > for the setting." > > On 2015/03/20 15:45:29, stevenjb wrote: > > > nit: s/setting/pref throughout the descriptions. (It's a subtle difference, > > but > > > an individual stored value is a "pref", whereas "settings" refers to the > API, > > > UI, etc, affecting the prefs). > > > > Done. > > > > > https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... > > chrome/common/extensions/api/settings_private.json:24: "optional": false, > > On 2015/03/20 15:45:29, stevenjb wrote: > > > This should be an enum. > > > > Done. > > > > > https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... > > chrome/common/extensions/api/settings_private.json:30: "description": "The > value > > of the setting." > > On 2015/03/20 15:45:29, stevenjb wrote: > > > nit: The current value of the setting > > > > > > > Done. > > > > > https://codereview.chromium.org/1015623005/diff/140001/chrome/common/extensio... > > chrome/common/extensions/api/settings_private.json:35: "enum": ["user", > > "device"], > > On 2015/03/20 15:45:29, stevenjb wrote: > > > , "none" > > > > Adding None causes a compiler error because None is already implicitly > present. > > So, no need to add it in the idl. > > > > > https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... > > File third_party/closure_compiler/externs/settings_private.js (right): > > > > > https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... > > third_party/closure_compiler/externs/settings_private.js:10: * type: string, > > On 2015/03/20 15:45:29, stevenjb wrote: > > > enum > > > > Done. > > > > > https://codereview.chromium.org/1015623005/diff/140001/third_party/closure_co... > > third_party/closure_compiler/externs/settings_private.js:12: * source: > string > > On 2015/03/20 15:45:29, stevenjb wrote: > > > enum > > > > Done. > > > Ahh... it WAS being generated; it was just in a different directory. Please hold > off ; I will finish up my getPref changes now that this is working. Will reply > again when it's ready. Oh, sorry, I didn't notice that settings_private is being implemented in src/chrome/brwoser instead of src/extensions/browser. That's why the generated file is elsewhere. (I think that it is fine in src/c/b/, since we are only going to use this API for chrome pref settings; if we want to make some of the new Settings UI available outside of chrome, e.g. for kiosk apps, we'll want to use dedicated APIs for that).
This should finally be fully implemented and working. PTAL
stevenjb@chromium.org changed reviewers: + kalman@chromium.org
stevenjb@chromium.org changed required reviewers: + kalman@chromium.org
+kalman@ - Should we start a thread to discuss this API? Or is this CL + the linked design doc sufficient? If so, what email alias (since it's private API)? https://codereview.chromium.org/1015623005/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1015623005/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:22: VLOG(1) << "chrome.settingsPrivate.setBooleanPref is not yet implemented."; We should go ahead and get the params and validate them for each of these, and log the value. https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... File third_party/closure_compiler/externs/settings_private.js (right): https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:10: chrome.settingsPrivate.PrefType = { With networkingPrivate, it seems that the extensions system redefines these properties with casing that matches the string. kalman@, is that expected? If so we will need to match that here, even if it is inconsistent with our JS conventions. https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:35: var PrefObject; Shouldn't these be chrome.settingsPrivate.PrefObject, etc?
kalman@chromium.org changed reviewers: + rockot@chromium.org - kalman@chromium.org
kalman@chromium.org changed required reviewers: - kalman@chromium.org
Email an FYI to apps-dev@chromium.org, you'll still need signoff from security. You have mine. I'm about to go on vacation so Ken (rockot@) can take over from here.
stevenjb@chromium.org changed reviewers: + asargent@chromium.org, kalman@chromium.org - rockot@chromium.org
stevenjb@chromium.org changed required reviewers: + asargent@chromium.org
On 2015/03/20 21:53:49, stevenjb wrote: +asargant@ (since kalman@ is OOO) - Should we start a thread to discuss this API? Or is this CL + the linked design doc sufficient? If so, what email alias (since it's private API)? Note: We've discussed this verbally with kalman@, and we're not exposing it publicly, so if there aren't any giant red flags we'd like to move forward provisionally at least. Thanks!
stevenjb@chromium.org changed reviewers: + asargent@chromium.org, kalman@chromium.org - rockot@chromium.org
stevenjb@chromium.org changed required reviewers: + asargent@chromium.org
On 2015/03/20 21:53:49, stevenjb wrote: +asargant@ (since kalman@ is OOO) - Should we start a thread to discuss this API? Or is this CL + the linked design doc sufficient? If so, what email alias (since it's private API)? Note: We've discussed this verbally with kalman@, and we're not exposing it publicly, so if there aren't any giant red flags we'd like to move forward provisionally at least. Thanks!
jlklein@chromium.org changed reviewers: + jlklein@chromium.org
https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... File third_party/closure_compiler/externs/settings_private.js (right): https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:5: /** @fileoverview Externs generated from namespace: settingsPrivate */ @externs https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:21: chrome.settingsPrivate.PrefSource = { These enums shouldn't really be in externs because extern code shouldn't ever actually run. It's just fed to the compiler for type info and it's assumed that the real implementations will be available in the browser at runtime. Check out: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:35: var PrefObject; On 2015/03/20 21:53:48, stevenjb wrote: > Shouldn't these be chrome.settingsPrivate.PrefObject, etc? +1. Check out some of the typedefs in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...
https://codereview.chromium.org/1015623005/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1015623005/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:22: VLOG(1) << "chrome.settingsPrivate.setBooleanPref is not yet implemented."; On 2015/03/20 21:53:48, stevenjb wrote: > We should go ahead and get the params and validate them for each of these, and > log the value. For the sake of making quicker progress can we table this for a follow up cl in which we actually implement these apis? https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... File third_party/closure_compiler/externs/settings_private.js (right): https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:5: /** @fileoverview Externs generated from namespace: settingsPrivate */ On 2015/03/20 22:06:42, Jeremy Klein wrote: > @externs Done. https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:5: /** @fileoverview Externs generated from namespace: settingsPrivate */ On 2015/03/20 22:06:42, Jeremy Klein wrote: > @externs Done. https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:10: chrome.settingsPrivate.PrefType = { On 2015/03/20 21:53:48, stevenjb wrote: > With networkingPrivate, it seems that the extensions system redefines these > properties with casing that matches the string. kalman@, is that expected? If so > we will need to match that here, even if it is inconsistent with our JS > conventions. Done. https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:21: chrome.settingsPrivate.PrefSource = { On 2015/03/20 22:06:42, Jeremy Klein wrote: > These enums shouldn't really be in externs because extern code shouldn't ever > actually run. It's just fed to the compiler for type info and it's assumed that > the real implementations will be available in the browser at runtime. Check out: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... Done. https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:21: chrome.settingsPrivate.PrefSource = { On 2015/03/20 22:06:42, Jeremy Klein wrote: > These enums shouldn't really be in externs because extern code shouldn't ever > actually run. It's just fed to the compiler for type info and it's assumed that > the real implementations will be available in the browser at runtime. Check out: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... As discussed offline we will keep these since C++ glue code will result in these enums indeed being generated and exposed to JS after all. So these help the compiler to know about these enums. https://codereview.chromium.org/1015623005/diff/220001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:35: var PrefObject; On 2015/03/20 22:06:42, Jeremy Klein wrote: > On 2015/03/20 21:53:48, stevenjb wrote: > > Shouldn't these be chrome.settingsPrivate.PrefObject, etc? > > +1. Check out some of the typedefs in > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... Done.
https://codereview.chromium.org/1015623005/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1015623005/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:22: VLOG(1) << "chrome.settingsPrivate.setBooleanPref is not yet implemented."; On 2015/03/20 23:30:30, Oren Blasberg wrote: > On 2015/03/20 21:53:48, stevenjb wrote: > > We should go ahead and get the params and validate them for each of these, and > > log the value. > > For the sake of making quicker progress can we table this for a follow up cl in > which we actually implement these apis? I don't agree that will make progress any faster. We'll need to write the code to extract the args regardless. I would rather do it initially so that the API initially does something moderately useful, even if it is just logging. Besides, I'm pretty sure you're the one that's going to be doing the follow up CL anyway :)
https://codereview.chromium.org/1015623005/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1015623005/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:22: VLOG(1) << "chrome.settingsPrivate.setBooleanPref is not yet implemented."; On 2015/03/20 23:40:57, stevenjb wrote: > On 2015/03/20 23:30:30, Oren Blasberg wrote: > > On 2015/03/20 21:53:48, stevenjb wrote: > > > We should go ahead and get the params and validate them for each of these, > and > > > log the value. > > > > For the sake of making quicker progress can we table this for a follow up cl > in > > which we actually implement these apis? > > I don't agree that will make progress any faster. We'll need to write the code > to extract the args regardless. I would rather do it initially so that the API > initially does something moderately useful, even if it is just logging. > > Besides, I'm pretty sure you're the one that's going to be doing the follow up > CL anyway :) Done.
On 2015/03/21 00:59:43, Oren Blasberg wrote: > https://codereview.chromium.org/1015623005/diff/220001/chrome/browser/extensi... > File chrome/browser/extensions/api/settings_private/settings_private_api.cc > (right): > > https://codereview.chromium.org/1015623005/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/api/settings_private/settings_private_api.cc:22: > VLOG(1) << "chrome.settingsPrivate.setBooleanPref is not yet implemented."; > On 2015/03/20 23:40:57, stevenjb wrote: > > On 2015/03/20 23:30:30, Oren Blasberg wrote: > > > On 2015/03/20 21:53:48, stevenjb wrote: > > > > We should go ahead and get the params and validate them for each of these, > > and > > > > log the value. > > > > > > For the sake of making quicker progress can we table this for a follow up cl > > in > > > which we actually implement these apis? > > > > I don't agree that will make progress any faster. We'll need to write the code > > to extract the args regardless. I would rather do it initially so that the API > > initially does something moderately useful, even if it is just logging. > > > > Besides, I'm pretty sure you're the one that's going to be doing the follow up > > CL anyway :) > > Done. Do things look good now?
lgtm w/nits https://codereview.chromium.org/1015623005/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1015623005/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:25: << ", " << (!!parameters->value ? "true" : "false") << ")"; nit: parameters->value should just serialize, i.e. ... << parameters->value << ... https://codereview.chromium.org/1015623005/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:28: VLOG(1) << "chrome.settingsPrivate.setBooleanPref is not yet implemented."; nit: I don't think we need these extra VLOGs now. https://codereview.chromium.org/1015623005/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:29: base::FundamentalValue* value = new base::FundamentalValue(true); nit: No need for a temporary here, could just inline the new() here and below.
rockot@chromium.org changed reviewers: + rockot@chromium.org
lgtm with some nits https://codereview.chromium.org/1015623005/diff/260001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/1015623005/diff/260001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:36: // nit: I don't think this vertical whitespace is necessary https://codereview.chromium.org/1015623005/diff/260001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:38: static void setBooleanPref(DOMString name, boolean value, optional OnPrefSetCallback callback); nit: Please fix line length here and elsewhere. The 80-char width rule applies to IDL.
https://codereview.chromium.org/1015623005/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1015623005/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:25: << ", " << (!!parameters->value ? "true" : "false") << ")"; On 2015/03/23 17:51:29, stevenjb wrote: > nit: parameters->value should just serialize, i.e. ... << parameters->value << > ... I tried that first, and that makes it print 0 or 1 instead of false or true. https://codereview.chromium.org/1015623005/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:28: VLOG(1) << "chrome.settingsPrivate.setBooleanPref is not yet implemented."; On 2015/03/23 17:51:29, stevenjb wrote: > nit: I don't think we need these extra VLOGs now. Done. https://codereview.chromium.org/1015623005/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.cc:29: base::FundamentalValue* value = new base::FundamentalValue(true); On 2015/03/23 17:51:29, stevenjb wrote: > nit: No need for a temporary here, could just inline the new() here and below. Done. https://codereview.chromium.org/1015623005/diff/260001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/1015623005/diff/260001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:36: // On 2015/03/23 19:53:39, Ken Rockot wrote: > nit: I don't think this vertical whitespace is necessary Done. https://codereview.chromium.org/1015623005/diff/260001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:38: static void setBooleanPref(DOMString name, boolean value, optional OnPrefSetCallback callback); On 2015/03/23 19:53:39, Ken Rockot wrote: > nit: Please fix line length here and elsewhere. The 80-char width rule applies > to IDL. Done.
lgtm w/questions https://codereview.chromium.org/1015623005/diff/280001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/1015623005/diff/280001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:14: dictionary PrefObject { will we ever be able to specialize this? e.g. BooleanPref, NumberPref so that we can change `value` from any to -> specialized type? https://codereview.chromium.org/1015623005/diff/280001/third_party/closure_co... File third_party/closure_compiler/externs/settings_private.js (right): https://codereview.chromium.org/1015623005/diff/280001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:11: * @const what does @const mean in this context?
https://codereview.chromium.org/1015623005/diff/280001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/1015623005/diff/280001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:14: dictionary PrefObject { On 2015/03/23 21:54:14, Dan Beam wrote: > will we ever be able to specialize this? e.g. BooleanPref, NumberPref so that > we can change `value` from any to -> specialized type? I don't know too much about the IDL, but I think the ideal would be for us to get generic types here in the JS: PrefObject<string>, PrefObject<boolean>, etc. Does anyone know if this is at all possible from IDL?
https://codereview.chromium.org/1015623005/diff/280001/third_party/closure_co... File third_party/closure_compiler/externs/settings_private.js (right): https://codereview.chromium.org/1015623005/diff/280001/third_party/closure_co... third_party/closure_compiler/externs/settings_private.js:11: * @const On 2015/03/23 21:54:14, Dan Beam wrote: > what does @const mean in this context? I think it's just there to prevent people from accidentally reusing the name by reassigning chrome.settingsPrivate to something else. This is commonly used all over the externs.. take a look at chrome_extensions.js for instance: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...
The CQ bit was checked by orenb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, rockot@chromium.org, stevenjb@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1015623005/#ps300001 (title: "Sync + merge conflicts in histogram")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015623005/300001
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
orenb@chromium.org changed reviewers: - asargent@chromium.org
orenb@chromium.org changed required reviewers: - asargent@chromium.org
The CQ bit was checked by orenb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015623005/300001
lgtm but some comments for the future https://codereview.chromium.org/1015623005/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/settings_private_api.h (right): https://codereview.chromium.org/1015623005/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/settings_private_api.h:34: class SettingsPrivateSetNumericPrefFunction : public UIThreadExtensionFunction { I maintain that when we implement this API we should differentiate between integer and double prefs. Just because JavaScript only has the "number" type doesn't mean the distinction doesn't matter. Case in point: we have setURLPref but in JS a URL is just a string. This gives us two benefits: the API can create the correct Value subtype for calling PrefService::Set, and the UI can do sanity checks or even inferences ("if the type is integer, limit the slider to integers"). https://codereview.chromium.org/1015623005/diff/300001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/1015623005/diff/300001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:68: static void onPrefsChanged(GetAllPrefsCallback callback); nit: should this be "GetPrefsCallback" or "GetMultiplePrefsCallback"?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by orenb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, stevenjb@chromium.org, dbeam@chromium.org, michaelpg@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1015623005/#ps320001 (title: "Add new permission.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015623005/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by orenb@chromium.org
On 2015/03/20 22:00:42, stevenjb wrote: > On 2015/03/20 21:53:49, stevenjb wrote: > +asargant@ (since kalman@ is OOO) - Should we start a thread to discuss this > API? Or is this CL + the linked design doc sufficient? If so, what email alias > (since it's private API)? Note: We've discussed this verbally with kalman@, and > we're not exposing it publicly, so if there aren't any giant red flags we'd like > to move forward provisionally at least. Thanks! Sorry not to respond to this question before (maybe my filters missed it due to my username being misspelled?). I think if kalman and rockot are both ok with this you should be good to go.
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, stevenjb@chromium.org, dbeam@chromium.org, michaelpg@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1015623005/#ps340001 (title: "Update the permission message")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015623005/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/87ba87f5a758cfe8252a591984db7f7ad8a29be4 Cr-Commit-Position: refs/heads/master@{#322069} |
