Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552343006:
[Extensions Binding] Allow for registering custom hooks (Closed)
Description[Extensions Binding] Allow for registering custom hooks
Add an APIBindingHooks class to allow for registering custom hooks to
handle various API calls. This can be needed for a number of reasons,
such as returning renderer concepts, executing synchronously, or simply
not needing to be performed on the browser side.
This first patch only allows for the creation of C++ custom hooks, but
we'll need to allow for both C++ and JS versions in order to reuse any
of our current custom bindings code. That will come in the next patch.
BUG=653596
Committed: https://crrev.com/d399335bfa3e978a8bc5e74a2fcf31213099a45a
Cr-Commit-Position: refs/heads/master@{#438308}
Patch Set 1 #Patch Set 2 : polish #
Total comments: 20
Patch Set 3 : jbroman #
Total comments: 4
Patch Set 4 : . #
Total comments: 6
Patch Set 5 : lazyboy's #Patch Set 6 : . #
Messages
Total messages: 36 (21 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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
rdevlin.cronin@chromium.org changed reviewers: + jbroman@chromium.org, lazyboy@chromium.org
Hey folks, mind taking a look?
https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... File extensions/renderer/api_binding_hooks.cc (right): https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_binding_hooks.cc:20: auto global_iter = request_hooks_.find(method_name); What's global about this iterator? https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... File extensions/renderer/api_binding_hooks.h (right): https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_binding_hooks.h:28: class APIBindingHooks { Is this class expected to become more complicated when JS support is added? At the moment, it's just a map. https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_binding_hooks.h:31: base::Callback<void(const binding::APISignature*, gin::Arguments*)>; Is the APISignature* parameter useful here? I'd have expected each hook to already know the signature of the function it's implementing. Also, what sort of hooks are typical? This doesn't provide a lot of support to help with the tricky things like managing callback lifetime. Do custom hooks not need to do that, or will support for that be added later? https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_binding_hooks.h:46: std::string api_name_; This variable seems unused. https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... File extensions/renderer/api_bindings_system.cc (right): https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_bindings_system.cc:51: // Find the hooks for the API. If none are registered, we create one. nit: Is there a need to heap-allocate one when we know it will forever remain empty? https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_bindings_system.cc:82: auto iter = binding_hooks_.find(api_name); super-duper-nit: This can be written without doing two map lookups in the new case (it also winds up a little shorter): std::unique_ptr<APIBindingHooks>& hooks = binding_hooks_[api_name]; if (!hooks) hooks = base::MakeUnique<APIBindingHooks>(api_name); return hooks.get(); https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... File extensions/renderer/api_bindings_system.h (right): https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_bindings_system.h:76: APIBindingHooks* GetHooksForAPI(const std::string& api_name); The use of GetHooksForAPI to do the registration seems untested at present. https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_bindings_system.h:77: Is there a reason for the asymmetry, where API schemas are requested from the client lazily via GetAPISchemaMethod, but hooks are registered in advance? I'd have expected this class to either consistently require up-front registration, or consistently do that work lazily. https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_bindings_system.h:104: std::map<std::string, std::unique_ptr<APIBindingHooks>> binding_hooks_; We might want to watch out for this later -- it is very pointer-y (map already does separate allocations for its nodes, and we have two layers of them plus the separate allocation for APIBindingHooks). Not sure whether the number of entries is large enough for this to be a big deal for performance, though.
https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... File extensions/renderer/api_binding_hooks.cc (right): https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_binding_hooks.cc:20: auto global_iter = request_hooks_.find(method_name); On 2016/12/08 16:58:56, jbroman wrote: > What's global about this iterator? Named from when there was a per-context iter in the JS impl. Renamed. https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... File extensions/renderer/api_binding_hooks.h (right): https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_binding_hooks.h:28: class APIBindingHooks { On 2016/12/08 16:58:56, jbroman wrote: > Is this class expected to become more complicated when JS support is added? At > the moment, it's just a map. Yep, it will be more complicated. I actually started implementing it, but the CL started to get much larger, and I figured it best to split it up. The biggest difference is that with JS bindings, you have to have it associated with a context as well (because the JS function is associated with the context), and so there's a bit more work in the ability to get the proper request handler. https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_binding_hooks.h:31: base::Callback<void(const binding::APISignature*, gin::Arguments*)>; On 2016/12/08 16:58:56, jbroman wrote: > Is the APISignature* parameter useful here? I'd have expected each hook to > already know the signature of the function it's implementing. > > Also, what sort of hooks are typical? This doesn't provide a lot of support to > help with the tricky things like managing callback lifetime. Do custom hooks not > need to do that, or will support for that be added later? In JS, we have a few different hooks. Specifically: - updateArgumentsPreValidate (calls with a chance to massage the arguments before we compare to the expected signature) - updateArgumentsPostValidate (calls to massage after we match to the expected signature) - handleRequest (handles the implementation after verifying args) - customCallback (allows the hook to manipulate the response) In C++, I think we can achieve most of those with a single hook and exposing different methods. E.g., we can have a method to match args, and then if the handler wants to adjust args, then match, then handle, the impl is just MatchArgs(signature, args); UpdateArgs(args); Handle(); etc. Sorry for the long answer. So that's why APISignature is here. :) I'm not quite sure I follow about things like managing callback lifetime. What's the concern there? https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_binding_hooks.h:46: std::string api_name_; On 2016/12/08 16:58:56, jbroman wrote: > This variable seems unused. Ah, it is indeed now (leftover from when the JS impl was here). Removed. https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... File extensions/renderer/api_bindings_system.cc (right): https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_bindings_system.cc:51: // Find the hooks for the API. If none are registered, we create one. On 2016/12/08 16:58:56, jbroman wrote: > nit: Is there a need to heap-allocate one when we know it will forever remain > empty? Mostly just because I didn't want to have to nullcheck in the APIBinding, but that's not a very compelling reason. :) Removed the allocation. https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_bindings_system.cc:82: auto iter = binding_hooks_.find(api_name); On 2016/12/08 16:58:56, jbroman wrote: > super-duper-nit: This can be written without doing two map lookups in the new > case (it also winds up a little shorter): > > std::unique_ptr<APIBindingHooks>& hooks = binding_hooks_[api_name]; > if (!hooks) > hooks = base::MakeUnique<APIBindingHooks>(api_name); > return hooks.get(); I like it, done. https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... File extensions/renderer/api_bindings_system.h (right): https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_bindings_system.h:76: APIBindingHooks* GetHooksForAPI(const std::string& api_name); On 2016/12/08 16:58:56, jbroman wrote: > The use of GetHooksForAPI to do the registration seems untested at present. I wasn't sure there was much useful to test here (since it's mostly just in hooks/binding), but went ahead and added one. https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_bindings_system.h:77: On 2016/12/08 16:58:56, jbroman wrote: > Is there a reason for the asymmetry, where API schemas are requested from the > client lazily via GetAPISchemaMethod, but hooks are registered in advance? I'd > have expected this class to either consistently require up-front registration, > or consistently do that work lazily. Let's say we have a custom hook (call it CustomHook()). For testability (and isolation of knowledge), we don't want APIBindingsSystem to know about CustomHook(). That means that we can have this registered in advance of creation, like this, or on-demand. For the on-demand approach, we'd need to introduce another callback of some kind for GetHookForAPI(), but that seems like a needlessly messy function to implement. It's doable, but it feels clunkier from most aspects. So, next question, why don't we require all schemas up-front, which is performance. If we never request a schema, there should be no reason to fetch all the resources (particularly when we also need to parse each of those schemas into a dictionary value!). Looking ahead to when we have some sort of generated schema data structure, I think it *does* make sense to have schema knowledge up front, but we're not there yet. WDYT? https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_bindings_system.h:104: std::map<std::string, std::unique_ptr<APIBindingHooks>> binding_hooks_; On 2016/12/08 16:58:56, jbroman wrote: > We might want to watch out for this later -- it is very pointer-y (map already > does separate allocations for its nodes, and we have two layers of them plus the > separate allocation for APIBindingHooks). Not sure whether the number of entries > is large enough for this to be a big deal for performance, though. Added a note in the code.
ok lgtm https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... File extensions/renderer/api_binding_hooks.h (right): https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_binding_hooks.h:31: base::Callback<void(const binding::APISignature*, gin::Arguments*)>; On 2016/12/08 at 19:12:02, Devlin (in MON - PST plus 3) wrote: > On 2016/12/08 16:58:56, jbroman wrote: > > Is the APISignature* parameter useful here? I'd have expected each hook to > > already know the signature of the function it's implementing. > > > > Also, what sort of hooks are typical? This doesn't provide a lot of support to > > help with the tricky things like managing callback lifetime. Do custom hooks not > > need to do that, or will support for that be added later? > > In JS, we have a few different hooks. Specifically: > - updateArgumentsPreValidate (calls with a chance to massage the arguments before we compare to the expected signature) > - updateArgumentsPostValidate (calls to massage after we match to the expected signature) > - handleRequest (handles the implementation after verifying args) > - customCallback (allows the hook to manipulate the response) > > In C++, I think we can achieve most of those with a single hook and exposing different methods. E.g., we can have a method to match args, and then if the handler wants to adjust args, then match, then handle, the impl is just > MatchArgs(signature, args); > UpdateArgs(args); > Handle(); > etc. > > Sorry for the long answer. So that's why APISignature is here. :) > > I'm not quite sure I follow about things like managing callback lifetime. What's the concern there? Ah, I'd assumed the hook replaced the entire function implementation. At that point, I'd be worried about something custom that took a v8::Function out of the gin::Arguments, and then had to worry about managing its lifetime. If the callback will still go through the current flow (and these just "massage" the arguments and response), then maybe that's not such a worry. https://codereview.chromium.org/2552343006/diff/40001/extensions/renderer/api... File extensions/renderer/api_binding_hooks.h (right): https://codereview.chromium.org/2552343006/diff/40001/extensions/renderer/api... extensions/renderer/api_binding_hooks.h:30: using HandleRequestHook = Since I was confused, it might be helpful to describe exactly what the callback is expected to do to handle the request. I gather it's expected to take arguments from the gin::Arguments and then call gin::Arguments::Return? If so, does that imply that the C++ ones are synchronous? Or if they are asynchronous, how does that work? https://codereview.chromium.org/2552343006/diff/40001/extensions/renderer/api... File extensions/renderer/api_binding_unittest.cc (right): https://codereview.chromium.org/2552343006/diff/40001/extensions/renderer/api... extensions/renderer/api_binding_unittest.cc:199: base::MakeUnique<APIBindingHooks>(), &refs); nit: might as well use nullptr here, since the callee can now accept it
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
lazyboy@, wanna take a quick look? https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... File extensions/renderer/api_binding_hooks.h (right): https://codereview.chromium.org/2552343006/diff/20001/extensions/renderer/api... extensions/renderer/api_binding_hooks.h:31: base::Callback<void(const binding::APISignature*, gin::Arguments*)>; On 2016/12/09 15:53:08, jbroman wrote: > On 2016/12/08 at 19:12:02, Devlin (in MON - PST plus 3) wrote: > > On 2016/12/08 16:58:56, jbroman wrote: > > > Is the APISignature* parameter useful here? I'd have expected each hook to > > > already know the signature of the function it's implementing. > > > > > > Also, what sort of hooks are typical? This doesn't provide a lot of support > to > > > help with the tricky things like managing callback lifetime. Do custom hooks > not > > > need to do that, or will support for that be added later? > > > > In JS, we have a few different hooks. Specifically: > > - updateArgumentsPreValidate (calls with a chance to massage the arguments > before we compare to the expected signature) > > - updateArgumentsPostValidate (calls to massage after we match to the expected > signature) > > - handleRequest (handles the implementation after verifying args) > > - customCallback (allows the hook to manipulate the response) > > > > In C++, I think we can achieve most of those with a single hook and exposing > different methods. E.g., we can have a method to match args, and then if the > handler wants to adjust args, then match, then handle, the impl is just > > MatchArgs(signature, args); > > UpdateArgs(args); > > Handle(); > > etc. > > > > Sorry for the long answer. So that's why APISignature is here. :) > > > > I'm not quite sure I follow about things like managing callback lifetime. > What's the concern there? > > Ah, I'd assumed the hook replaced the entire function implementation. At that > point, I'd be worried about something custom that took a v8::Function out of the > gin::Arguments, and then had to worry about managing its lifetime. If the > callback will still go through the current flow (and these just "massage" the > arguments and response), then maybe that's not such a worry. Yeah, I'm hoping we can have the APIBinding/APIRequestHandler deal with most of the lifetime. This hooks API will probably change over time as we allow it to be used with more custom bindings, but hopefully the concept remains the same. https://codereview.chromium.org/2552343006/diff/40001/extensions/renderer/api... File extensions/renderer/api_binding_hooks.h (right): https://codereview.chromium.org/2552343006/diff/40001/extensions/renderer/api... extensions/renderer/api_binding_hooks.h:30: using HandleRequestHook = On 2016/12/09 15:53:08, jbroman wrote: > Since I was confused, it might be helpful to describe exactly what the callback > is expected to do to handle the request. I gather it's expected to take > arguments from the gin::Arguments and then call gin::Arguments::Return? If so, > does that imply that the C++ ones are synchronous? Or if they are asynchronous, > how does that work? Unfortunately, there isn't a full standard for this. Some APIs are synchronous, in which case they could use gin::Arguments::Return. Others allow for asynchronocity (pass in a callback), but actually execute synchronously. I've added a comment that tries to shed a bit of light here, but unfortunately, there's isn't necessarily a standard case. I've also added a TODO to make sure that we have a way for handlers to register a request so that the bindings system handles the callback lifetime maintenance, rather than the handler holding onto a reference directly. https://codereview.chromium.org/2552343006/diff/40001/extensions/renderer/api... File extensions/renderer/api_binding_unittest.cc (right): https://codereview.chromium.org/2552343006/diff/40001/extensions/renderer/api... extensions/renderer/api_binding_unittest.cc:199: base::MakeUnique<APIBindingHooks>(), &refs); On 2016/12/09 15:53:08, jbroman wrote: > nit: might as well use nullptr here, since the callee can now accept it Done.
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/2552343006/diff/60001/extensions/renderer/api... File extensions/renderer/api_bindings_system.cc (right): https://codereview.chromium.org/2552343006/diff/60001/extensions/renderer/api... extensions/renderer/api_bindings_system.cc:51: // Find the hooks for the API. If none are registered, we create one. "we create one" is probably obsolete in the final patch set? https://codereview.chromium.org/2552343006/diff/60001/extensions/renderer/api... File extensions/renderer/api_bindings_system_unittest.cc (right): https://codereview.chromium.org/2552343006/diff/60001/extensions/renderer/api... extensions/renderer/api_bindings_system_unittest.cc:317: APIBindingHooks* hooks = bindings_system()->GetHooksForAPI(kAlphaAPIName); I'm finding this pattern to add a hook harder to read: you first call GetHooksForAPI and then add hook(s) to the return value. Instead if APIBindingSystem exposed some RegisterHook(api_name, method_name, hook) and then that function called APIBindingsHooks::RegisterHandleRequest(), it would have been a bit better imo. Then there's also a possibility of code like: hooks = bindings_system()->GetHooksForAPI(...) binding_system()->CreateAPIInstance(...) hooks->RegisterHandleRequest("alpha.functionWithCallback", ...) You can't guarantee alpha.functionWithCallback will be dispatched properly (of course that's a user error). Line #78 won't complain in api_bindings_system.cc. Is that an issue?
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2552343006/diff/60001/extensions/renderer/api... File extensions/renderer/api_bindings_system.cc (right): https://codereview.chromium.org/2552343006/diff/60001/extensions/renderer/api... extensions/renderer/api_bindings_system.cc:51: // Find the hooks for the API. If none are registered, we create one. On 2016/12/13 01:52:32, lazyboy wrote: > "we create one" is probably obsolete in the final patch set? Good catch; done. https://codereview.chromium.org/2552343006/diff/60001/extensions/renderer/api... File extensions/renderer/api_bindings_system_unittest.cc (right): https://codereview.chromium.org/2552343006/diff/60001/extensions/renderer/api... extensions/renderer/api_bindings_system_unittest.cc:317: APIBindingHooks* hooks = bindings_system()->GetHooksForAPI(kAlphaAPIName); On 2016/12/13 01:52:32, lazyboy wrote: > I'm finding this pattern to add a hook harder to read: you first call > GetHooksForAPI and then add hook(s) to the return value. > > Instead if APIBindingSystem exposed some RegisterHook(api_name, method_name, > hook) and then that function called APIBindingsHooks::RegisterHandleRequest(), > it would have been a bit better imo. > > Then there's also a possibility of code like: > > hooks = bindings_system()->GetHooksForAPI(...) > binding_system()->CreateAPIInstance(...) > hooks->RegisterHandleRequest("alpha.functionWithCallback", ...) > > You can't guarantee alpha.functionWithCallback will be dispatched properly (of > course that's a user error). Line #78 won't complain in api_bindings_system.cc. > Is that an issue? > Hmm... depending on how complex the "add hooks" methods are, I'm worried about the implication of having APIBindingSystem need to expose all the bits and bobs. Additionally, it would mean that registering multiple hooks for the same API is less efficient, since you'd have to look it up each time. For the time being, I'd like to keep it like this so that we keep a more flexible interface, but I've added a TODO to re-evaluate in the future. Does that SGTY, or would you prefer to change it now and possibly change back later?
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/2552343006/diff/60001/extensions/renderer/api... File extensions/renderer/api_bindings_system_unittest.cc (right): https://codereview.chromium.org/2552343006/diff/60001/extensions/renderer/api... extensions/renderer/api_bindings_system_unittest.cc:317: APIBindingHooks* hooks = bindings_system()->GetHooksForAPI(kAlphaAPIName); On 2016/12/13 18:41:30, Devlin wrote: > On 2016/12/13 01:52:32, lazyboy wrote: > > I'm finding this pattern to add a hook harder to read: you first call > > GetHooksForAPI and then add hook(s) to the return value. > > > > Instead if APIBindingSystem exposed some RegisterHook(api_name, method_name, > > hook) and then that function called APIBindingsHooks::RegisterHandleRequest(), > > it would have been a bit better imo. > > > > Then there's also a possibility of code like: > > > > hooks = bindings_system()->GetHooksForAPI(...) > > binding_system()->CreateAPIInstance(...) > > hooks->RegisterHandleRequest("alpha.functionWithCallback", ...) > > > > You can't guarantee alpha.functionWithCallback will be dispatched properly (of > > course that's a user error). Line #78 won't complain in > api_bindings_system.cc. > > Is that an issue? > > > > Hmm... depending on how complex the "add hooks" methods are, I'm worried about > the implication of having APIBindingSystem need to expose all the bits and bobs. > Additionally, it would mean that registering multiple hooks for the same API is > less efficient, since you'd have to look it up each time. > > For the time being, I'd like to keep it like this so that we keep a more > flexible interface, but I've added a TODO to re-evaluate in the future. Does > that SGTY, or would you prefer to change it now and possibly change back later? Keeping this as is + TODO SG. Calling APIBindingHooks::RegisterHandleRequest *after* creating API instance still seems like a recipe for race condition to me though. Maybe you can add a DCHECK in RegisterHandleRequest that makes sure it's not called after any APIBindingHooks::GetHandleRequest() is called? Or do we imagine that this would also be valid in some lazy initialization scenario?
https://codereview.chromium.org/2552343006/diff/60001/extensions/renderer/api... File extensions/renderer/api_bindings_system_unittest.cc (right): https://codereview.chromium.org/2552343006/diff/60001/extensions/renderer/api... extensions/renderer/api_bindings_system_unittest.cc:317: APIBindingHooks* hooks = bindings_system()->GetHooksForAPI(kAlphaAPIName); On 2016/12/13 18:58:25, lazyboy wrote: > On 2016/12/13 18:41:30, Devlin wrote: > > On 2016/12/13 01:52:32, lazyboy wrote: > > > I'm finding this pattern to add a hook harder to read: you first call > > > GetHooksForAPI and then add hook(s) to the return value. > > > > > > Instead if APIBindingSystem exposed some RegisterHook(api_name, method_name, > > > hook) and then that function called > APIBindingsHooks::RegisterHandleRequest(), > > > it would have been a bit better imo. > > > > > > Then there's also a possibility of code like: > > > > > > hooks = bindings_system()->GetHooksForAPI(...) > > > binding_system()->CreateAPIInstance(...) > > > hooks->RegisterHandleRequest("alpha.functionWithCallback", ...) > > > > > > You can't guarantee alpha.functionWithCallback will be dispatched properly > (of > > > course that's a user error). Line #78 won't complain in > > api_bindings_system.cc. > > > Is that an issue? > > > > > > > Hmm... depending on how complex the "add hooks" methods are, I'm worried about > > the implication of having APIBindingSystem need to expose all the bits and > bobs. > > Additionally, it would mean that registering multiple hooks for the same API > is > > less efficient, since you'd have to look it up each time. > > > > For the time being, I'd like to keep it like this so that we keep a more > > flexible interface, but I've added a TODO to re-evaluate in the future. Does > > that SGTY, or would you prefer to change it now and possibly change back > later? > > Keeping this as is + TODO SG. > Calling APIBindingHooks::RegisterHandleRequest *after* creating API instance > still seems like a recipe for race condition to me though. Maybe you can add a > DCHECK in RegisterHandleRequest that makes sure it's not called after any > APIBindingHooks::GetHandleRequest() is called? Or do we imagine that this would > also be valid in some lazy initialization scenario? Yeah, that's reasonable. Added a DCHECK.
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...
lgtm.
The CQ bit was unchecked by rdevlin.cronin@chromium.org
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2552343006/#ps100001 (title: ".")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@chromium.org
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": 1481660588991510,
"parent_rev": "ab83831205042937f1eed9d6ebf10e6ab3817480", "commit_rev":
"286584cb9cf8ad1d057e3e9bbec5f3d909acd495"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions Binding] Allow for registering custom hooks Add an APIBindingHooks class to allow for registering custom hooks to handle various API calls. This can be needed for a number of reasons, such as returning renderer concepts, executing synchronously, or simply not needing to be performed on the browser side. This first patch only allows for the creation of C++ custom hooks, but we'll need to allow for both C++ and JS versions in order to reuse any of our current custom bindings code. That will come in the next patch. BUG=653596 ========== to ========== [Extensions Binding] Allow for registering custom hooks Add an APIBindingHooks class to allow for registering custom hooks to handle various API calls. This can be needed for a number of reasons, such as returning renderer concepts, executing synchronously, or simply not needing to be performed on the browser side. This first patch only allows for the creation of C++ custom hooks, but we'll need to allow for both C++ and JS versions in order to reuse any of our current custom bindings code. That will come in the next patch. BUG=653596 Review-Url: https://codereview.chromium.org/2552343006 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions Binding] Allow for registering custom hooks Add an APIBindingHooks class to allow for registering custom hooks to handle various API calls. This can be needed for a number of reasons, such as returning renderer concepts, executing synchronously, or simply not needing to be performed on the browser side. This first patch only allows for the creation of C++ custom hooks, but we'll need to allow for both C++ and JS versions in order to reuse any of our current custom bindings code. That will come in the next patch. BUG=653596 Review-Url: https://codereview.chromium.org/2552343006 ========== to ========== [Extensions Binding] Allow for registering custom hooks Add an APIBindingHooks class to allow for registering custom hooks to handle various API calls. This can be needed for a number of reasons, such as returning renderer concepts, executing synchronously, or simply not needing to be performed on the browser side. This first patch only allows for the creation of C++ custom hooks, but we'll need to allow for both C++ and JS versions in order to reuse any of our current custom bindings code. That will come in the next patch. BUG=653596 Committed: https://crrev.com/d399335bfa3e978a8bc5e74a2fcf31213099a45a Cr-Commit-Position: refs/heads/master@{#438308} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d399335bfa3e978a8bc5e74a2fcf31213099a45a Cr-Commit-Position: refs/heads/master@{#438308} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
