Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2412783002:
Only inject mojo builtins to contexts with the mojoPrivate API feature. (Closed)
|
Created:
4 years, 2 months ago by Sam McNally Modified:
4 years, 2 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly inject mojo builtins to contexts with the mojoPrivate API feature.
The mojo JS bindings are only used by the mimeHandlerPrivate and
mojoPrivate APIs.
BUG=636655
Committed: https://crrev.com/9152fe906781b3e9fe03b98820cb9d3c8cbb7802
Cr-Commit-Position: refs/heads/master@{#425196}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Depends on Patchset: Messages
Total messages: 24 (16 generated)
The CQ bit was checked by sammc@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.
sammc@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/2412783002/diff/1/extensions/renderer/module_... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/2412783002/diff/1/extensions/renderer/module_... extensions/renderer/module_system.cc:174: context_->GetAvailability("mojoPrivate").is_available()) { This doesn't seem to be quite the right relationship - mime types handler uses the mojo builtins, but not mojo private. I think instead of coupling the two features, I'd rather just have a list of features that use mojo builtins and check for the presence of any of them (and add a comment requesting it remain small).
The CQ bit was checked by sammc@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.
https://codereview.chromium.org/2412783002/diff/1/extensions/renderer/module_... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/2412783002/diff/1/extensions/renderer/module_... extensions/renderer/module_system.cc:174: context_->GetAvailability("mojoPrivate").is_available()) { On 2016/10/12 14:58:09, Devlin wrote: > This doesn't seem to be quite the right relationship - mime types handler uses > the mojo builtins, but not mojo private. I think instead of coupling the two > features, I'd rather just have a list of features that use mojo builtins and > check for the presence of any of them (and add a comment requesting it remain > small). Done. mojoPrivate does need to make the builtins available; it doesn't use them directly but instead exposes them to its user.
Thanks! lgtm with nits https://codereview.chromium.org/2412783002/diff/20001/extensions/renderer/mod... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/2412783002/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:116: // bindings at least one is available. nit: "if at least one is available"? (missing "if") https://codereview.chromium.org/2412783002/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:119: static const char* const kMojoUsingApis[] = { nitty nit: since variable names can't express the difference between "mojo-using apis" (what this means) and "mojo using apis" (apis that are used by mojo), let's rename this to something like "kApisUsingMojo" or "kApisRequiringMojo" (or something along those lines).
The CQ bit was checked by sammc@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.
https://codereview.chromium.org/2412783002/diff/20001/extensions/renderer/mod... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/2412783002/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:116: // bindings at least one is available. On 2016/10/13 14:44:14, Devlin wrote: > nit: "if at least one is available"? (missing "if") Done. https://codereview.chromium.org/2412783002/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:119: static const char* const kMojoUsingApis[] = { On 2016/10/13 14:44:14, Devlin wrote: > nitty nit: since variable names can't express the difference between "mojo-using > apis" (what this means) and "mojo using apis" (apis that are used by mojo), > let's rename this to something like "kApisUsingMojo" or "kApisRequiringMojo" (or > something along those lines). Done.
The CQ bit was checked by sammc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2412783002/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Only inject mojo builtins to contexts with the mojoPrivate API feature. The mojo JS bindings are only used by the mimeHandlerPrivate and mojoPrivate APIs. BUG=636655 ========== to ========== Only inject mojo builtins to contexts with the mojoPrivate API feature. The mojo JS bindings are only used by the mimeHandlerPrivate and mojoPrivate APIs. BUG=636655 Committed: https://crrev.com/9152fe906781b3e9fe03b98820cb9d3c8cbb7802 Cr-Commit-Position: refs/heads/master@{#425196} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9152fe906781b3e9fe03b98820cb9d3c8cbb7802 Cr-Commit-Position: refs/heads/master@{#425196} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
