Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1447103002:
MD Settings: FakeSettingsPrivate for tests (Closed)
|
Created:
5 years, 1 month ago by michaelpg Modified:
5 years ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: FakeSettingsPrivate for tests
Move MockSettingsApi to its own file and rename it. Provide
initializeForTesting method on <settings-prefs> instead of
doing hacky stuff on window.
BUG=425627
Committed: https://crrev.com/4001b0cc67123ac7a8f7afe3f50b5ebe4c012dc8
Cr-Commit-Position: refs/heads/master@{#361032}
Patch Set 1 #
Total comments: 2
Patch Set 2 : mock -> fake #
Total comments: 12
Patch Set 3 : ; #Patch Set 4 : Change how fake is set #
Total comments: 4
Patch Set 5 : assertNotReached #
Total comments: 8
Patch Set 6 : proto -> interface #Dependent Patchsets: Messages
Total messages: 35 (11 generated)
Description was changed from ========== MD Settings: MockSettingsPrivate for tests Mock chrome.settingsPrivate API to test stuff. We already do this but it's hacky. BUG=425627 ========== to ========== MD Settings: MockSettingsPrivate for tests Mock chrome.settingsPrivate API to test stuff. We already do this but it's hacky and prevents closure from checking settingsPrivate API calls in prefs.js. BUG=425627 ==========
Description was changed from ========== MD Settings: MockSettingsPrivate for tests Mock chrome.settingsPrivate API to test stuff. We already do this but it's hacky and prevents closure from checking settingsPrivate API calls in prefs.js. BUG=425627 ========== to ========== MD Settings: MockSettingsPrivate for tests Move MockSettingsApi to its own file and rename it. Override chrome.settingsPrivate instead of doing hacky stuff on window (which prevents closure from checking settingsPrivate API calls in prefs.js). BUG=425627 ==========
michaelpg@chromium.org changed reviewers: + dbeam@chromium.org, stevenjb@chromium.org
Is this better than what we have, and a decent pattern to follow for any other APIs we might want to manually mock?
Generally speaking I like this better than what we currently have. https://codereview.chromium.org/1447103002/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/mock_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/mock_settings_private.js:31: for (var testCase of prefsTestCases) Where does prefsTestCases come from?
Description was changed from ========== MD Settings: MockSettingsPrivate for tests Move MockSettingsApi to its own file and rename it. Override chrome.settingsPrivate instead of doing hacky stuff on window (which prevents closure from checking settingsPrivate API calls in prefs.js). BUG=425627 ========== to ========== MD Settings: FakeSettingsPrivate for tests Move MockSettingsApi to its own file and rename it. Override chrome.settingsPrivate instead of doing hacky stuff on window (which prevents closure from checking settingsPrivate API calls in prefs.js). BUG=425627 ==========
Patchset #2 (id:20001) has been deleted
Renamed Mock to Fake. https://codereview.chromium.org/1447103002/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/mock_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/mock_settings_private.js:31: for (var testCase of prefsTestCases) On 2015/11/16 18:51:13, stevenjb wrote: > Where does prefsTestCases come from? oops, it comes from prefs_test_cases.js which is specific to prefs_tests.js. I reworked this constructor to take an optional list of initial prefs, and changed how the test cases were structured so their prefs can be passed into the constructor.
lgtm https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/fake_settings_private.js:13: return JSON.parse(JSON.stringify(obj)); Any reason we can't use Object.assign here?
https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/fake_settings_private.js:13: return JSON.parse(JSON.stringify(obj)); On 2015/11/17 18:24:19, stevenjb wrote: > Any reason we can't use Object.assign here? it's not recursive
https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/fake_settings_private.js:13: return JSON.parse(JSON.stringify(obj)); On 2015/11/17 22:02:11, michaelpg wrote: > On 2015/11/17 18:24:19, stevenjb wrote: > > Any reason we can't use Object.assign here? > > it's not recursive Ah, right.
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1447103002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1447103002/40001
https://codereview.chromium.org/1447103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:276: chrome.settingsPrivate.onPrefsChanged.addListener( why are you [able to] undo this? https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/fake_settings_private.js:155: settings.FakeSettingsPrivate.Pref nit: ; at end
The CQ bit was unchecked by dbeam@chromium.org
https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/fake_settings_private.js:94: chrome.settingsPrivate = this; why is this better?
https://codereview.chromium.org/1447103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:276: chrome.settingsPrivate.onPrefsChanged.addListener( On 2015/11/17 23:01:15, Dan Beam wrote: > why are you [able to] undo this? I don't like this because: * checking for a global sucks * grepping chrome.settingsPrivate.foo to check usage becomes ineffective * i don't think Closure can type-check the previous version because there's no "type" for the API (I confirmed it wasn't checking usage of this.settingsApi_, but IDK if it would be possible to tell Closure that this.settingsApi_ is an "alias" for chrome.settingsPrivate) As far as ability, this is done by the fake setting chrome.settingsPrivate to itself. https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/fake_settings_private.js:94: chrome.settingsPrivate = this; On 2015/11/17 23:02:07, Dan Beam wrote: > why is this better? The code already uses chrome.settingsPrivate so this means less changing of code and (to me) is a more elegant way to inject the fake. https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/fake_settings_private.js:155: settings.FakeSettingsPrivate.Pref On 2015/11/17 23:01:15, Dan Beam wrote: > nit: ; at end Done.
https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/fake_settings_private.js:94: chrome.settingsPrivate = this; On 2015/11/17 23:51:51, michaelpg wrote: > On 2015/11/17 23:02:07, Dan Beam wrote: > > why is this better? > > The code already uses chrome.settingsPrivate so this means less changing of code > and (to me) is a more elegant way to inject the fake. generally prefer dependency injection to changing globals. when you change globals it's very likely to change assumptions that other code relies on. this is the reason that modifying global prototypes in JavaScript is discouraged (i.e. messing with String.prototype, etc.). i usually don't pipe up because if there's no existing tests anything's better than nothing. but in this case you're undoing something that was already dependency injected. you can't inject the global into the ctor like typical dependency injection because you want to use the element declaratively but that doesn't prevent you from just doing setSettingsPrivate() that changes this.settingsPrivate_ (or this.settingsApi_). unless you actually need to use the settingsPrivate in the ctor, we can use set*() stuff. if you do need to use the settingsPrivate in the ctor, change the ctor to do less, then call setSettingsPrivate(), then initialize() (<-- and move the code that accesses settingsPrivate to initialize()).
https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/fake_settings_private.js:94: chrome.settingsPrivate = this; On 2015/11/18 02:01:01, Dan Beam wrote: > On 2015/11/17 23:51:51, michaelpg wrote: > > On 2015/11/17 23:02:07, Dan Beam wrote: > > > why is this better? > > > > The code already uses chrome.settingsPrivate so this means less changing of > code > > and (to me) is a more elegant way to inject the fake. > > generally prefer dependency injection to changing globals. when you change > globals it's very likely to change assumptions that other code relies on. this > is the reason that modifying global prototypes in JavaScript is discouraged > (i.e. messing with String.prototype, etc.). > > i usually don't pipe up because if there's no existing tests anything's better > than nothing. but in this case you're undoing something that was already > dependency injected. you can't inject the global into the ctor like typical > dependency injection because you want to use the element declaratively but that > doesn't prevent you from just doing setSettingsPrivate() that changes > this.settingsPrivate_ (or this.settingsApi_). unless you actually need to use > the settingsPrivate in the ctor, we can use set*() stuff. if you do need to use > the settingsPrivate in the ctor, change the ctor to do less, then call > setSettingsPrivate(), then initialize() (<-- and move the code that accesses > settingsPrivate to initialize()). OK, that's doable. Would you help me figure out how to make Closure treat this.settingsApi_ as if it matched the externs of chrome.settingsPrivate?
https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/fake_settings_private.js:94: chrome.settingsPrivate = this; On 2015/11/18 02:03:44, michaelpg wrote: > On 2015/11/18 02:01:01, Dan Beam wrote: > > On 2015/11/17 23:51:51, michaelpg wrote: > > > On 2015/11/17 23:02:07, Dan Beam wrote: > > > > why is this better? > > > > > > The code already uses chrome.settingsPrivate so this means less changing of > > code > > > and (to me) is a more elegant way to inject the fake. > > > > generally prefer dependency injection to changing globals. when you change > > globals it's very likely to change assumptions that other code relies on. > this > > is the reason that modifying global prototypes in JavaScript is discouraged > > (i.e. messing with String.prototype, etc.). > > > > i usually don't pipe up because if there's no existing tests anything's better > > than nothing. but in this case you're undoing something that was already > > dependency injected. you can't inject the global into the ctor like typical > > dependency injection because you want to use the element declaratively but > that > > doesn't prevent you from just doing setSettingsPrivate() that changes > > this.settingsPrivate_ (or this.settingsApi_). unless you actually need to use > > the settingsPrivate in the ctor, we can use set*() stuff. if you do need to > use > > the settingsPrivate in the ctor, change the ctor to do less, then call > > setSettingsPrivate(), then initialize() (<-- and move the code that accesses > > settingsPrivate to initialize()). > > OK, that's doable. Would you help me figure out how to make Closure treat > this.settingsApi_ as if it matched the externs of chrome.settingsPrivate? /** @interface */ function SettingsPrivate() {} SettingsPrivate.prototype = { /** Gets a thing. */ getThing: assertNotReached, }; /** @implements {SettingsPrivate} */ function RealSettingsPrivate() {} RealSettingsPrivate.prototype = { getThing: function() { chrome.settingsPrivate.getThing(); }, }; /** @implements {SettingsPrivate} */ function FakeSettingsPrivate() {} FakeSettingsPrivate.prototype = { getThing: createFunctionMock(), };
Patchset #4 (id:80001) has been deleted
Description was changed from ========== MD Settings: FakeSettingsPrivate for tests Move MockSettingsApi to its own file and rename it. Override chrome.settingsPrivate instead of doing hacky stuff on window (which prevents closure from checking settingsPrivate API calls in prefs.js). BUG=425627 ========== to ========== MD Settings: FakeSettingsPrivate for tests Move MockSettingsApi to its own file and rename it. Provide initializeForTesting method on <settings-prefs> instead of doing hacky stuff on window. BUG=425627 ==========
For some reason, this CL (before and after this patch) adds 500ms to the test, making the whole browser test go from 3.4s to 3.9s on my Z620. On 2015/11/18 02:26:34, Dan Beam wrote: > https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... > File chrome/test/data/webui/settings/fake_settings_private.js (right): > > https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... > chrome/test/data/webui/settings/fake_settings_private.js:94: > chrome.settingsPrivate = this; > On 2015/11/18 02:03:44, michaelpg wrote: > > On 2015/11/18 02:01:01, Dan Beam wrote: > > > On 2015/11/17 23:51:51, michaelpg wrote: > > > > On 2015/11/17 23:02:07, Dan Beam wrote: > > > > > why is this better? > > > > > > > > The code already uses chrome.settingsPrivate so this means less changing > of > > > code > > > > and (to me) is a more elegant way to inject the fake. > > > > > > generally prefer dependency injection to changing globals. when you change > > > globals it's very likely to change assumptions that other code relies on. > > this > > > is the reason that modifying global prototypes in JavaScript is discouraged > > > (i.e. messing with String.prototype, etc.). > > > > > > i usually don't pipe up because if there's no existing tests anything's > better > > > than nothing. but in this case you're undoing something that was already > > > dependency injected. you can't inject the global into the ctor like typical > > > dependency injection because you want to use the element declaratively but > > that > > > doesn't prevent you from just doing setSettingsPrivate() that changes > > > this.settingsPrivate_ (or this.settingsApi_). Except we don't want to delay the call to chrome.settingsPrivate.getAllPrefs, so like you say: > > > unless you actually need to > use > > > the settingsPrivate in the ctor, we can use set*() stuff. if you do need to > > use > > > the settingsPrivate in the ctor, change the ctor to do less, then call > > > setSettingsPrivate(), then initialize() (<-- and move the code that accesses > > > settingsPrivate to initialize()). That's possible, but where do we call initialize()? In <cr-settings>, I guess, which ends up sucking because we have to wait for <cr-settings>'s ready, which means the entire page loads even more slowly and there's a serious delay between the UI showing up and the controls being populated by the prefs. Instead, hopefully this is at least a little better than the original: set a deferInitialization flag on CrSettingsPrefsInternal; if that flag isn't set, no dependency injection happens, <settings-prefs> calls initialize() immediately when the element is created. If the flag is set, <settings-prefs> waits until initializeForTesting(fakeApi) is called. Since the fake is set on the singleton, it shouldn't affect other code much. And chrome.settingsPrivate will still work (although it'll still produce conflicting results with the rest of the UI.) > > > > OK, that's doable. Would you help me figure out how to make Closure treat > > this.settingsApi_ as if it matched the externs of chrome.settingsPrivate? > > /** @interface */ > function SettingsPrivate() {} > > SettingsPrivate.prototype = { > /** Gets a thing. */ > getThing: assertNotReached, > }; > > /** @implements {SettingsPrivate} */ > function RealSettingsPrivate() {} > > RealSettingsPrivate.prototype = { > getThing: function() { > chrome.settingsPrivate.getThing(); > }, > }; > > /** @implements {SettingsPrivate} */ > function FakeSettingsPrivate() {} > > FakeSettingsPrivate.prototype = { > getThing: createFunctionMock(), > }; I took your idea and simplified it: no need to create an actual RealSettingsPrivate that just forwards to chrome.settingsPrivate when chrome.settingsPrivate is what we're basing the interface on anyway. LMK if that's bad. https://codereview.chromium.org/1447103002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1447103002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:160: if (!CrSettingsPrefs.deferInitialization) This is how a test tells <settings-prefs> not to initialize using the default api (the real chrome.settingsPrivate). https://codereview.chromium.org/1447103002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:288: settingsApi_: /** @type {SettingsPrivate} */(chrome.settingsPrivate), I got lazy and decided there was no reason to create a SettingsPrivateImpl that just wraps chrome.settingsPrivate.
https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_private_proto.js (right): https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_private_proto.js:23: setPref: assertNotReached, this is weird, why is closure OK with a named function that doesn't match the @params (assertNotReached in this patch), and a function expression matching the @params (last patch), but warns on a function expression with the wrong number of params (function(){})?
https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_private_proto.js (right): https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_private_proto.js:23: setPref: assertNotReached, On 2015/11/20 00:51:06, michaelpg wrote: > this is weird, why is closure OK with a named function that doesn't match the > @params (assertNotReached in this patch), and a function expression matching the > @params (last patch), but warns on a function expression with the wrong number > of params (function(){})? because we have magic to identify "assertNotReached", maybe? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...
On 2015/11/20 00:45:25, michaelpg wrote: > For some reason, this CL (before and after this patch) adds 500ms to the test, > making the whole browser test go from 3.4s to 3.9s on my Z620. 'Tis not so good. > > On 2015/11/18 02:26:34, Dan Beam wrote: > > > https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... > > File chrome/test/data/webui/settings/fake_settings_private.js (right): > > > > > https://codereview.chromium.org/1447103002/diff/40001/chrome/test/data/webui/... > > chrome/test/data/webui/settings/fake_settings_private.js:94: > > chrome.settingsPrivate = this; > > On 2015/11/18 02:03:44, michaelpg wrote: > > > On 2015/11/18 02:01:01, Dan Beam wrote: > > > > On 2015/11/17 23:51:51, michaelpg wrote: > > > > > On 2015/11/17 23:02:07, Dan Beam wrote: > > > > > > why is this better? > > > > > > > > > > The code already uses chrome.settingsPrivate so this means less changing > > of > > > > code > > > > > and (to me) is a more elegant way to inject the fake. > > > > > > > > generally prefer dependency injection to changing globals. when you > change > > > > globals it's very likely to change assumptions that other code relies on. > > > this > > > > is the reason that modifying global prototypes in JavaScript is > discouraged > > > > (i.e. messing with String.prototype, etc.). > > > > > > > > i usually don't pipe up because if there's no existing tests anything's > > better > > > > than nothing. but in this case you're undoing something that was already > > > > dependency injected. you can't inject the global into the ctor like > typical > > > > dependency injection because you want to use the element declaratively but > > > that > > > > doesn't prevent you from just doing setSettingsPrivate() that changes > > > > this.settingsPrivate_ (or this.settingsApi_). > > Except we don't want to delay the call to chrome.settingsPrivate.getAllPrefs, so > like you say: > > > > > unless you actually need to > > use > > > > the settingsPrivate in the ctor, we can use set*() stuff. if you do need > to > > > use > > > > the settingsPrivate in the ctor, change the ctor to do less, then call > > > > setSettingsPrivate(), then initialize() (<-- and move the code that > accesses > > > > settingsPrivate to initialize()). > > That's possible, but where do we call initialize()? In <cr-settings>, I guess, > which > ends up sucking because we have to wait for <cr-settings>'s ready, which means > the > entire page loads even more slowly and there's a serious delay between the UI > showing > up and the controls being populated by the prefs. Which is why I was apprehensive to couple logic + DOM when first investigating Polymer (and mentioned to you long ago). > > Instead, hopefully this is at least a little better than the original: set a > deferInitialization flag on CrSettingsPrefsInternal; if that flag isn't set, > no dependency injection happens, <settings-prefs> calls initialize() immediately > when > the element is created. If the flag is set, <settings-prefs> waits until > initializeForTesting(fakeApi) is called. > > Since the fake is set on the singleton, it shouldn't affect other code much. And > chrome.settingsPrivate will still work (although it'll still produce conflicting > results > with the rest of the UI.) Testing singletons or global state is hard. This is better than straight-up changing the global, but may still create unexpected results when run via multiple async tests. In an ideal world, we'd just instantiate an instance of the singleton's type in an isolated way and inject it into a class in the ctor. In a less than ideal world, you change how the class gets access to the service after creation (i.e. instantiate a single instance of the class you want to test and change instance.getService = myFunction). There are plenty of even less ideal ways as well. > > > > > > > OK, that's doable. Would you help me figure out how to make Closure treat > > > this.settingsApi_ as if it matched the externs of chrome.settingsPrivate? > > > > /** @interface */ > > function SettingsPrivate() {} > > > > SettingsPrivate.prototype = { > > /** Gets a thing. */ > > getThing: assertNotReached, > > }; > > > > /** @implements {SettingsPrivate} */ > > function RealSettingsPrivate() {} > > > > RealSettingsPrivate.prototype = { > > getThing: function() { > > chrome.settingsPrivate.getThing(); > > }, > > }; > > > > /** @implements {SettingsPrivate} */ > > function FakeSettingsPrivate() {} > > > > FakeSettingsPrivate.prototype = { > > getThing: createFunctionMock(), > > }; > > I took your idea and simplified it: no need to create an actual > RealSettingsPrivate that just forwards to chrome.settingsPrivate when > chrome.settingsPrivate is what we're basing the interface on anyway. LMK if > that's bad. I don't see any reason this is bad currently. Let's see if patterns emerge. Obviously it's better not to duplicate the API of settingsPrivate, if possible. https://codereview.chromium.org/1447103002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1447103002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:160: if (!CrSettingsPrefs.deferInitialization) On 2015/11/20 00:45:25, michaelpg wrote: > This is how a test tells <settings-prefs> not to initialize using the default > api (the real chrome.settingsPrivate). Acknowledged. https://codereview.chromium.org/1447103002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:288: settingsApi_: /** @type {SettingsPrivate} */(chrome.settingsPrivate), On 2015/11/20 00:45:25, michaelpg wrote: > I got lazy and decided there was no reason to create a SettingsPrivateImpl that > just wraps chrome.settingsPrivate. if this works, I suppose it still accomplishes the goal of being able to inject a double https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_private_proto.js (right): https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_private_proto.js:7: assertNotReached('Proto file should not be executed.'); what is a proto file?
i think this CL is a good step in the right direction (though larger than is easy to read). we can change the injection mechanism afterward if necessary. lgtm https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_private_proto.js (right): https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_private_proto.js:13: nit: remove \n
I'm still a little fuzzy on what the SettingsPrivate prototype does for us, but I will make some time to chat with Michael or Dan about that. lgtm
https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_private_proto.js (right): https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_private_proto.js:7: assertNotReached('Proto file should not be executed.'); On 2015/11/20 02:16:06, Dan Beam wrote: > what is a proto file? renamed to interface. https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_private_proto.js:13: On 2015/11/20 02:18:20, Dan Beam wrote: > nit: remove \n Done. https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_private_proto.js:23: setPref: assertNotReached, On 2015/11/20 01:58:17, Dan Beam wrote: > On 2015/11/20 00:51:06, michaelpg wrote: > > this is weird, why is closure OK with a named function that doesn't match the > > @params (assertNotReached in this patch), and a function expression matching > the > > @params (last patch), but warns on a function expression with the wrong number > > of params (function(){})? > > because we have magic to identify "assertNotReached", maybe? > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... Acknowledged.
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1447103002/#ps140001 (title: "proto -> interface")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1447103002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1447103002/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4001b0cc67123ac7a8f7afe3f50b5ebe4c012dc8 Cr-Commit-Position: refs/heads/master@{#361032}
Message was sent while issue was closed.
https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_private_proto.js (right): https://codereview.chromium.org/1447103002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_private_proto.js:7: assertNotReached('Proto file should not be executed.'); On 2015/11/21 23:03:50, michaelpg wrote: > On 2015/11/20 02:16:06, Dan Beam wrote: > > what is a proto file? > > renamed to interface. you should probably keep the file name and class name the same |
