|
|
Description[Extensions Bindings] Add a native type for ContentSettings
The ContentSettings allows extensions to modify the content settings
(e.g. javascript execution, microphone access, etc) for different sites.
It is implemented by exposing "ContentSetting" objects on the API.
Implement a native type for these, and hook it into the bindings system.
Add tests for the same.
BUG=653596
Review-Url: https://codereview.chromium.org/2823883004
Cr-Commit-Position: refs/heads/master@{#466744}
Committed: https://chromium.googlesource.com/chromium/src/+/c15ac888ad840b483c5abf18863fb48f090386e4
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 23
Patch Set 5 : jbroman, lazyboy #
Total comments: 11
Patch Set 6 : nits #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== ContentSetting BUG= ========== to ========== [Extensions Bindings] Add a native type for ContentSettings The ContentSettings allows extensions to modify the content settings (e.g. javscript execution, microphone access, etc) for different sites. It is implemented by exposing "ContentSetting" objects on the API. Implement a native type for these, and hook it into the bindings system. Add tests for the same. BUG=653596 ==========
rdevlin.cronin@chromium.org changed reviewers: + jbroman@chromium.org, lazyboy@chromium.org
Hey folks, mind taking a quick look?
Description was changed from ========== [Extensions Bindings] Add a native type for ContentSettings The ContentSettings allows extensions to modify the content settings (e.g. javscript execution, microphone access, etc) for different sites. It is implemented by exposing "ContentSetting" objects on the API. Implement a native type for these, and hook it into the bindings system. Add tests for the same. BUG=653596 ========== to ========== [Extensions Bindings] Add a native type for ContentSettings The ContentSettings allows extensions to modify the content settings (e.g. javascript execution, microphone access, etc) for different sites. It is implemented by exposing "ContentSetting" objects on the API. Implement a native type for these, and hook it into the bindings system. Add tests for the same. BUG=653596 ==========
https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/BUI... File extensions/renderer/BUILD.gn (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/BUI... extensions/renderer/BUILD.gn:59: "content_setting.cc", optional: maybe consider naming all custom type files with some prefix to easily call them out? e.g. it's nice that we have _custom_bindings.cc/js for all custom bindings, ... https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... File extensions/renderer/content_setting.cc (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:71: properties["primaryPattern"] = Can we add a comment in (top level) content_settings.json to point to this file? It's harder to find otherwise IMO. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:163: context, base::StringPrintf("contentSettings.%s is deprecated.", Isn't this a new behavior? Can we add this to CL description? https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:167: if (pref_name_ == "get") Describe this? https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... File extensions/renderer/content_setting.h (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.h:29: // The custom implementation of the ContentSetting type exposed to APIs. nit: contentSettings.ContentSetting https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.h:47: gin::ObjectTemplateBuilder GetObjectTemplateBuilder( private:
https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... File extensions/renderer/content_setting.cc (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:23: const char* kDeprecatedTypes[] = { nit: const char* const. The array itself can go in rodata as well. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:162: add_console_error_.Run( This should probably just be a warning, no? https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:167: if (pref_name_ == "get") Should this be |method_name == "get"|? https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... File extensions/renderer/content_setting.h (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.h:37: const binding::AddConsoleError& add_console_error, Incidentally, the code could presumably just do console::AddMessage( ScriptContextSet::GetContextByV8Context(context), content::CONSOLE_MESSAGE_LEVEL_WARNING, "..."); Is this dependency injection intended to be used for unit testing or similar?
https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/BUI... File extensions/renderer/BUILD.gn (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/BUI... extensions/renderer/BUILD.gn:59: "content_setting.cc", On 2017/04/20 19:29:49, lazyboy wrote: > optional: maybe consider naming all custom type files with some prefix to easily > call them out? e.g. it's nice that we have _custom_bindings.cc/js for all custom > bindings, ... Hmm, that's an interesting thought. If it's alright, I'll do it in a followup (to also move chrome setting and storage area). Suggestions on prefix/suffix? native_foo? https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... File extensions/renderer/content_setting.cc (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:23: const char* kDeprecatedTypes[] = { On 2017/04/20 19:38:37, jbroman wrote: > nit: const char* const. The array itself can go in rodata as well. Sure, done. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:71: properties["primaryPattern"] = On 2017/04/20 19:29:49, lazyboy wrote: > Can we add a comment in (top level) content_settings.json to point to this file? > It's harder to find otherwise IMO. Done. I'll add this to chrome_setting and storage_area in a followup. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:162: add_console_error_.Run( On 2017/04/20 19:38:37, jbroman wrote: > This should probably just be a warning, no? Probably, done. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:163: context, base::StringPrintf("contentSettings.%s is deprecated.", On 2017/04/20 19:29:49, lazyboy wrote: > Isn't this a new behavior? Can we add this to CL description? It's actually not - current content settings do this, too - e.g. https://chromium.googlesource.com/chromium/src/+/c94db29f17d79a58b665040a435e.... https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:167: if (pref_name_ == "get") On 2017/04/20 19:29:49, lazyboy wrote: > Describe this? Done. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:167: if (pref_name_ == "get") On 2017/04/20 19:38:37, jbroman wrote: > Should this be |method_name == "get"|? Yes. This also implied a gap in the tests. Expanded them to include testing a deprecated setting. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... File extensions/renderer/content_setting.h (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.h:29: // The custom implementation of the ContentSetting type exposed to APIs. On 2017/04/20 19:29:49, lazyboy wrote: > nit: contentSettings.ContentSetting Done. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.h:37: const binding::AddConsoleError& add_console_error, On 2017/04/20 19:38:37, jbroman wrote: > Incidentally, the code could presumably just do > > console::AddMessage( > ScriptContextSet::GetContextByV8Context(context), > content::CONSOLE_MESSAGE_LEVEL_WARNING, > "..."); > > Is this dependency injection intended to be used for unit testing or similar? Good point. Adjusted. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.h:47: gin::ObjectTemplateBuilder GetObjectTemplateBuilder( On 2017/04/20 19:29:49, lazyboy wrote: > private: We now have a stance of preferring public inheritance everywhere. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/XdsXTHy9lis (it will take some getting used to for me) https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... File extensions/renderer/content_setting.cc (right): https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:174: // Since we just defined this object, DefineOwnProperty() should never jbroman@, can you confirm this is right?
lgtm https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... File extensions/renderer/content_setting.cc (right): https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:38: v8::Local<v8::Context> context, nit: If you only want the isolate, just take the isolate? It seems misleading to take a context but then just rely on the isolate and its current context. https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:66: // The set() call takes an object { value: { type: <t> }, ... }, where <t> This comment hasn't been updated since you copied it from ChromeSetting. There's no value property here. https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:146: CHECK(arguments->GetRemaining(&argument_list)); You've added gin::Arguments::GetAll in another CL; is this a suitable place to use it? https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:174: // Since we just defined this object, DefineOwnProperty() should never On 2017/04/21 at 23:33:02, Devlin wrote: > jbroman@, can you confirm this is right? Yes, that's correct. I've walked through the code, and none of the issues (interceptors, access checks, etc.) should be possible here. https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:176: CHECK(result.IsJust() && result.FromJust()); nit: might as well CHECK(result.ToChecked()); ToChecked/FromJust internally check if they are Nothing<T>().
lgtm https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/BUI... File extensions/renderer/BUILD.gn (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/BUI... extensions/renderer/BUILD.gn:59: "content_setting.cc", On 2017/04/21 23:33:02, Devlin wrote: > On 2017/04/20 19:29:49, lazyboy wrote: > > optional: maybe consider naming all custom type files with some prefix to > easily > > call them out? e.g. it's nice that we have _custom_bindings.cc/js for all > custom > > bindings, ... > > Hmm, that's an interesting thought. If it's alright, I'll do it in a followup > (to also move chrome setting and storage area). Sure. Suggestions on prefix/suffix? > native_foo? native_ prefix sounds good to me. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... File extensions/renderer/content_setting.cc (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.cc:163: context, base::StringPrintf("contentSettings.%s is deprecated.", On 2017/04/21 23:33:02, Devlin wrote: > On 2017/04/20 19:29:49, lazyboy wrote: > > Isn't this a new behavior? Can we add this to CL description? > > It's actually not - current content settings do this, too - e.g. > https://chromium.googlesource.com/chromium/src/+/c94db29f17d79a58b665040a435e.... Acknowledged. https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... File extensions/renderer/content_setting.h (right): https://codereview.chromium.org/2823883004/diff/60001/extensions/renderer/con... extensions/renderer/content_setting.h:47: gin::ObjectTemplateBuilder GetObjectTemplateBuilder( On 2017/04/21 23:33:02, Devlin wrote: > On 2017/04/20 19:29:49, lazyboy wrote: > > private: > > We now have a stance of preferring public inheritance everywhere. > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/XdsXTHy9lis > (it will take some getting used to for me) Got it, thanks!
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... File extensions/renderer/content_setting.cc (right): https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:38: v8::Local<v8::Context> context, On 2017/04/24 15:44:42, jbroman wrote: > nit: If you only want the isolate, just take the isolate? It seems misleading to > take a context but then just rely on the isolate and its current context. This is just conforming to a callback signature right now. It looks like all the custom type creation methods can be converted to take an isolate, though - mind if I do it in a followup? (The reason it was this way to begin with was because I'm trying to rely on isolate, rather than context, for most of these areas, since it seems that's often the more-correct approach, but gin often only takes an isolate. But since we don't do anything else with the context here, I can see the confusion.) https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:66: // The set() call takes an object { value: { type: <t> }, ... }, where <t> On 2017/04/24 15:44:42, jbroman wrote: > This comment hasn't been updated since you copied it from ChromeSetting. There's > no value property here. Done. https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:146: CHECK(arguments->GetRemaining(&argument_list)); On 2017/04/24 15:44:42, jbroman wrote: > You've added gin::Arguments::GetAll in another CL; is this a suitable place to > use it? Yep; just hadn't rebased at the time. Done. https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:176: CHECK(result.IsJust() && result.FromJust()); On 2017/04/24 15:44:42, jbroman wrote: > nit: might as well CHECK(result.ToChecked()); > > ToChecked/FromJust internally check if they are Nothing<T>(). Done.
https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... File extensions/renderer/content_setting.cc (right): https://codereview.chromium.org/2823883004/diff/80001/extensions/renderer/con... extensions/renderer/content_setting.cc:38: v8::Local<v8::Context> context, On 2017/04/24 at 20:09:28, Devlin wrote: > On 2017/04/24 15:44:42, jbroman wrote: > > nit: If you only want the isolate, just take the isolate? It seems misleading to > > take a context but then just rely on the isolate and its current context. > > This is just conforming to a callback signature right now. Ah, I missed that. > It looks like all the custom type creation methods can be converted to take an isolate, though - mind if I do it in a followup? (The reason it was this way to begin with was because I'm trying to rely on isolate, rather than context, for most of these areas, since it seems that's often the more-correct approach, but gin often only takes an isolate. But since we don't do anything else with the context here, I can see the confusion.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2823883004/#ps100001 (title: "nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493065367083750, "parent_rev": "83b1bb154f0fcf2f3ecb5b28f1d76994d0d13b46", "commit_rev": "c15ac888ad840b483c5abf18863fb48f090386e4"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions Bindings] Add a native type for ContentSettings The ContentSettings allows extensions to modify the content settings (e.g. javascript execution, microphone access, etc) for different sites. It is implemented by exposing "ContentSetting" objects on the API. Implement a native type for these, and hook it into the bindings system. Add tests for the same. BUG=653596 ========== to ========== [Extensions Bindings] Add a native type for ContentSettings The ContentSettings allows extensions to modify the content settings (e.g. javascript execution, microphone access, etc) for different sites. It is implemented by exposing "ContentSetting" objects on the API. Implement a native type for these, and hook it into the bindings system. Add tests for the same. BUG=653596 Review-Url: https://codereview.chromium.org/2823883004 Cr-Commit-Position: refs/heads/master@{#466744} Committed: https://chromium.googlesource.com/chromium/src/+/c15ac888ad840b483c5abf18863f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c15ac888ad840b483c5abf18863f... |