Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791533002:
Convert Web Store Inline Install IPCs to mojo (Closed)
|
Created:
3 years, 8 months ago by catmullings Modified:
3 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert Web Store Inline Install IPCs to mojo
Migrating the following inline install related IPCs to mojo:
- ExtensionMsg_InlineInstallStageChanged
- ExtensionMsg_InlineInstallDownloadProgress
- ExtensionMsg_InlineWebstoreInstallResponse
- ExtensionMsg_InlineWebstoreInstal
BUG=697613
Review-Url: https://codereview.chromium.org/2791533002
Cr-Commit-Position: refs/heads/master@{#475090}
Committed: https://chromium.googlesource.com/chromium/src/+/ba539108932530a3cd7a6a3bb59c0ab253698772
Patch Set 1 #
Total comments: 20
Patch Set 2 : WebContentsFrameBindingSet in TabHelper, weak binding in WebstoreBindings #
Total comments: 21
Patch Set 3 : Rebase master; addressed patch 2 cl comments #Patch Set 4 : Trybot error fix #Patch Set 5 : Added mojom to json #Patch Set 6 : Fix trybot error: Change InlineInstall to an associated interface #Patch Set 7 : Rebase master #
Total comments: 2
Patch Set 8 : Refactor DoInlineInstall to take InterfacePtr arg; Remove static fn in WebstoreBindings #
Total comments: 9
Patch Set 9 : Removed WebstoreBindingsHelper; Added bindings set to WebstoreBindings #
Total comments: 19
Patch Set 10 : Reverted InlineInstallResponse back to a mojo msg #
Total comments: 3
Patch Set 11 : Move ProgressListener interface ptr to TabHelper #
Total comments: 7
Patch Set 12 : Added map of InlineInstallProgressListenerPtrs in TabHelper #
Total comments: 13
Patch Set 13 #
Total comments: 6
Patch Set 14 : Addressed Ken's comments #
Total comments: 17
Patch Set 15 : Rebase master; Addressed Ken's and mostly palmer's comments #
Total comments: 17
Patch Set 16 : Addressed Devlin and palmer comments #
Total comments: 4
Patch Set 17 : Added OWNERS file to mojom/ directory; Curried install_id to callback on WebstoreBindings #
Total comments: 4
Patch Set 18 : Addressed Devlin's comments #Messages
Total messages: 133 (90 generated)
catmullings@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... File chrome/browser/extensions/tab_helper.cc (left): https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:439: void TabHelper::OnInlineWebstoreInstall(content::RenderFrameHost* host, Note that the original definition of OnInlineWebstoreInstall had a parameter for the RenderFrameHost which is handled and tracked behind-the-scenes by the IPC mechanisms. Because handling/tracking the RenderFrameHost needs to be more explicit in mojo, note how this has been done on line 404 and if this SGTY. https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:208: base::Bind(&TabHelper::BindInlineInstallRequest, web_contents)); This line is causing a linker error: error: undefined reference to 'extensions::TabHelper::BindInlineInstallRequest(content::WebContents*, mojo::InterfaceRequest<extensions::mojom::InlineInstall>)' Devlin, would you have any idea what might be the cause? Maybe an improper setup of the inline_install.mojom dependancy in the BUILD.gn files? https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/ch... File chrome/common/extensions/chrome_extension_messages.h (right): https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/ch... chrome/common/extensions/chrome_extension_messages.h:34: IPC_ENUM_TRAITS_MAX_VALUE(extensions::webstore_install::Result, Can these IPC_ENUMs be removed? They've been redefined in chrome/common/extensions/mojo/inline_install_traits.h in this CL. Despite the redefinition, deleting them here poses a linker error. e.g: /mojo/public/cpp/bindings/lib/native_enum_serialization.h:34: error: undefined reference to 'IPC::ParamTraits<extensions::api::webstore::InstallStage>::Write ... so right now, these IPC_ENUMs been preserved.
Cool! https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:155: std::move(request)); This won't be quite right, I think. A TabHelper is designed to be a one-instance-per-web-contents object, but this will create a new TabHelper for each frame that makes a request. Looking around, I found content::WebContentsBindingSet and content::WebContentsFrameBindingSet - I think that's closer to what we want. https://cs.chromium.org/chromium/src/content/public/browser/web_contents_bind... https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:208: base::Bind(&TabHelper::BindInlineInstallRequest, web_contents)); On 2017/03/31 00:08:21, catmullings wrote: > This line is causing a linker error: > > error: undefined reference to > 'extensions::TabHelper::BindInlineInstallRequest(content::WebContents*, > mojo::InterfaceRequest<extensions::mojom::InlineInstall>)' > > Devlin, would you have any idea what might be the cause? Maybe an improper setup > of the inline_install.mojom dependancy in the BUILD.gn files? This error is because BindInlineInstallRequest is declared as a method of TabHelper (in the .h file, and specified here by TabHelper::BindInlineInstallRequest), but it's defined as only "void BindInlineInstallRequest" rather than "void TabHelper::BindInlineInstallRequest". The result is that there's a declared method in TabHelper (so the compiler knows it should exist) and a defined method in the global namespace (that ends up never being used), but during linking, the compiler can't find the definition of the TabHelper version. https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:419: inline_install_status_->InlineWebstoreInstallResponse( note: see also comment in mojom file. https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:470: void TabHelper::OnDidGetWebApplicationInfo(const WebApplicationInfo& info) { Is there a reason to move this method? If not, we should avoid it in this CL because it makes the diff harder to process (it's not obvious if anything in the method changed, or if it's just copy-paste). If it's just for stylistic reasons (e.g. matching the ordering of declaration to the order of definition), let's go ahead and do that in a separate CL. https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/ch... File chrome/common/extensions/chrome_extension_messages.h (right): https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/ch... chrome/common/extensions/chrome_extension_messages.h:34: IPC_ENUM_TRAITS_MAX_VALUE(extensions::webstore_install::Result, On 2017/03/31 00:08:21, catmullings wrote: > Can these IPC_ENUMs be removed? They've been redefined in > chrome/common/extensions/mojo/inline_install_traits.h in this CL. Despite the > redefinition, deleting them here poses a linker error. e.g: > > /mojo/public/cpp/bindings/lib/native_enum_serialization.h:34: error: undefined > reference to 'IPC::ParamTraits<extensions::api::webstore::InstallStage>::Write > > ... so right now, these IPC_ENUMs been preserved. Hmm... the [native] tag on the mojo equivalents means that mojo should reuse existing IPC_ENUM_TRAITS declarations. I see that they're added to inline_install_traits.h, but inline_install_traits.h isn't being included in any build targets. Perhaps it's not being built, since I think the [traits_headers] is typically used to define struct traits? That's just wild guessing, though. Do you know of other ipc enums that use this pattern? https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/mo... File chrome/common/extensions/mojo/inline_install_status.mojom (right): https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/mo... chrome/common/extensions/mojo/inline_install_status.mojom:13: interface InlineInstallStatus { Any reason to not combine this with the other mojom? https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/mo... chrome/common/extensions/mojo/inline_install_status.mojom:16: InlineWebstoreInstallResponse(int32 install_id, bool success, string error, One of the advantages of mojo is that it can provide a callback for methods that used to need to do a call -> response pattern. That way, you can pass the callback directly in and just run it when the task completes. So for this, we could do something like: DoInlineInstall(...) => (int32 install_id, bool success, string error, WebStoreInstallResult result) and omit the InlineWebstoreInstallResponse message. https://codereview.chromium.org/2791533002/diff/1/chrome/renderer/extensions/... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/1/chrome/renderer/extensions/... chrome/renderer/extensions/webstore_bindings.cc:71: mojo::MakeStrongBinding(base::MakeUnique<WebstoreBindings>(context), Here, too, this pattern will result in creating a new instance of WebstoreBindings for each request.
https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:155: std::move(request)); On 2017/03/31 14:18:39, Devlin wrote: > This won't be quite right, I think. A TabHelper is designed to be a > one-instance-per-web-contents object, but this will create a new TabHelper for > each frame that makes a request. > > Looking around, I found content::WebContentsBindingSet and > content::WebContentsFrameBindingSet - I think that's closer to what we want. > > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_bind... Done. https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:208: base::Bind(&TabHelper::BindInlineInstallRequest, web_contents)); On 2017/03/31 14:18:39, Devlin wrote: > On 2017/03/31 00:08:21, catmullings wrote: > > This line is causing a linker error: > > > > error: undefined reference to > > 'extensions::TabHelper::BindInlineInstallRequest(content::WebContents*, > > mojo::InterfaceRequest<extensions::mojom::InlineInstall>)' > > > > Devlin, would you have any idea what might be the cause? Maybe an improper > setup > > of the inline_install.mojom dependancy in the BUILD.gn files? > > This error is because BindInlineInstallRequest is declared as a method of > TabHelper (in the .h file, and specified here by > TabHelper::BindInlineInstallRequest), but it's defined as only "void > BindInlineInstallRequest" rather than "void > TabHelper::BindInlineInstallRequest". The result is that there's a declared > method in TabHelper (so the compiler knows it should exist) and a defined method > in the global namespace (that ends up never being used), but during linking, the > compiler can't find the definition of the TabHelper version. That makes sense. Thanks! I assume that WebContentsFrameBindingSet handles adding the bindings to the registry, but it'll be worth confirming this with mojo folks once they are added to the cl review. https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:419: inline_install_status_->InlineWebstoreInstallResponse( On 2017/03/31 14:18:39, Devlin wrote: > note: see also comment in mojom file. Done. https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:470: void TabHelper::OnDidGetWebApplicationInfo(const WebApplicationInfo& info) { On 2017/03/31 14:18:39, Devlin wrote: > Is there a reason to move this method? If not, we should avoid it in this CL > because it makes the diff harder to process (it's not obvious if anything in the > method changed, or if it's just copy-paste). If it's just for stylistic reasons > (e.g. matching the ordering of declaration to the order of definition), let's go > ahead and do that in a separate CL. Done. https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/ch... File chrome/common/extensions/chrome_extension_messages.h (right): https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/ch... chrome/common/extensions/chrome_extension_messages.h:34: IPC_ENUM_TRAITS_MAX_VALUE(extensions::webstore_install::Result, On 2017/03/31 14:18:39, Devlin wrote: > On 2017/03/31 00:08:21, catmullings wrote: > > Can these IPC_ENUMs be removed? They've been redefined in > > chrome/common/extensions/mojo/inline_install_traits.h in this CL. Despite the > > redefinition, deleting them here poses a linker error. e.g: > > > > /mojo/public/cpp/bindings/lib/native_enum_serialization.h:34: error: undefined > > reference to 'IPC::ParamTraits<extensions::api::webstore::InstallStage>::Write > > > > ... so right now, these IPC_ENUMs been preserved. > > Hmm... the [native] tag on the mojo equivalents means that mojo should reuse > existing IPC_ENUM_TRAITS declarations. I see that they're added to > inline_install_traits.h, but inline_install_traits.h isn't being included in any > build targets. Perhaps it's not being built, since I think the [traits_headers] > is typically used to define struct traits? That's just wild guessing, though. > Do you know of other ipc enums that use this pattern? I think you are right that inline_install_traits.h should be added to a build target, which I have now done (see chrome/common/BUILD.gn). However, the same linker error persists. Once all the IPCs in this file are converted, I'll revisit removing these IPC enums. https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/mo... File chrome/common/extensions/mojo/inline_install_status.mojom (right): https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/mo... chrome/common/extensions/mojo/inline_install_status.mojom:13: interface InlineInstallStatus { On 2017/03/31 14:18:39, Devlin wrote: > Any reason to not combine this with the other mojom? Done. https://codereview.chromium.org/2791533002/diff/1/chrome/common/extensions/mo... chrome/common/extensions/mojo/inline_install_status.mojom:16: InlineWebstoreInstallResponse(int32 install_id, bool success, string error, On 2017/03/31 14:18:39, Devlin wrote: > One of the advantages of mojo is that it can provide a callback for methods that > used to need to do a call -> response pattern. That way, you can pass the > callback directly in and just run it when the task completes. > > So for this, we could do something like: > DoInlineInstall(...) => (int32 install_id, bool success, string error, > WebStoreInstallResult result) > and omit the InlineWebstoreInstallResponse message. Done. https://codereview.chromium.org/2791533002/diff/1/chrome/renderer/extensions/... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/1/chrome/renderer/extensions/... chrome/renderer/extensions/webstore_bindings.cc:71: mojo::MakeStrongBinding(base::MakeUnique<WebstoreBindings>(context), On 2017/03/31 14:18:39, Devlin wrote: > Here, too, this pattern will result in creating a new instance of > WebstoreBindings for each request. Done.
Don't forget to run trybots on your patches - it can help flush out some errors that we might not expect, and it can give a lot more confidence in the patch when it's shown to not break anything. :) https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.cc:441: if (bindings_.GetCurrentTargetFrame() == host) { We should handle the case of the message not being from the main frame (probably with a notreached, since there should be renderer-side checks for this). Also, when the full body of a method needs to be behind some check like this, it's often cleaner to do: if (!some_condition) return; // lots of code here rather than if (some_condition) { // lots of code here } https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.cc:450: host->GetParent()) { if host is the main frame, it can't have a parent. https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.cc:491: this, web_contents()->GetBrowserContext(), return_route_id, Does this compile? InlineInstallObserver doesn't take a return_route_id param anymore. https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/tab_helper.h (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.h:124: explicit TabHelper(content::WebContents* web_contents); Is this change still necessary? https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.h:129: protected: Why protected? https://codereview.chromium.org/2791533002/diff/20001/chrome/common/extension... File chrome/common/extensions/mojo/inline_install.mojom (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/common/extension... chrome/common/extensions/mojo/inline_install.mojom:26: DoInlineInstall(int32 install_id, int32 route_id, Do we need route id? https://codereview.chromium.org/2791533002/diff/20001/chrome/common/extension... File chrome/common/extensions/mojo/inline_install_traits.h (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/common/extension... chrome/common/extensions/mojo/inline_install_traits.h:9: IPC_ENUM_TRAITS_MAX_VALUE( It still seems like we should only need this *or* the entries in extension_messages.h, but we can let a mojo guru enlighten us. :) https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... chrome/renderer/extensions/webstore_bindings.cc:60: WebstoreBindings::SetContext(context); This would only work if the WebstoreBindings we wanted happened to be the most-recently constructed. Otherwise, we could end up executing in the wrong context. https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... File chrome/renderer/extensions/webstore_bindings_helper.cc (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... chrome/renderer/extensions/webstore_bindings_helper.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) in new code https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... File chrome/renderer/extensions/webstore_bindings_helper.h (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... chrome/renderer/extensions/webstore_bindings_helper.h:14: class WebstoreBindingsHelper : public ObjectBackedNativeHandler, I think for this, this class can just be an inner class of WebstoreBindings rather than in its own file. Then, each instance of WebstoreBindings can own a WebstoreBindingsHelper.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by catmullings@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 catmullings@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...)
The CQ bit was checked by catmullings@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...)
https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.cc:441: if (bindings_.GetCurrentTargetFrame() == host) { On 2017/04/12 01:16:07, Devlin wrote: > We should handle the case of the message not being from the main frame (probably > with a notreached, since there should be renderer-side checks for this). > > Also, when the full body of a method needs to be behind some check like this, > it's often cleaner to do: > if (!some_condition) > return; > // lots of code here > > rather than > if (some_condition) { > // lots of code here > } Done. https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.cc:450: host->GetParent()) { On 2017/04/12 01:16:07, Devlin wrote: > if host is the main frame, it can't have a parent. Done. https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.cc:491: this, web_contents()->GetBrowserContext(), return_route_id, On 2017/04/12 01:16:07, Devlin wrote: > Does this compile? InlineInstallObserver doesn't take a return_route_id param > anymore. It does compile. According to the constructor definition, it takes a routing_id https://cs.chromium.org/chromium/src/chrome/browser/extensions/tab_helper.cc?... I've removed the return_route_id from the InlineInstallObserver class, since it's no longer needed. https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/tab_helper.h (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.h:124: explicit TabHelper(content::WebContents* web_contents); On 2017/04/12 01:16:07, Devlin wrote: > Is this change still necessary? Done. https://codereview.chromium.org/2791533002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.h:129: protected: On 2017/04/12 01:16:07, Devlin wrote: > Why protected? Done. https://codereview.chromium.org/2791533002/diff/20001/chrome/common/extension... File chrome/common/extensions/mojo/inline_install.mojom (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/common/extension... chrome/common/extensions/mojo/inline_install.mojom:26: DoInlineInstall(int32 install_id, int32 route_id, On 2017/04/12 01:16:07, Devlin wrote: > Do we need route id? Done. https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... chrome/renderer/extensions/webstore_bindings.cc:60: WebstoreBindings::SetContext(context); On 2017/04/12 01:16:07, Devlin wrote: > This would only work if the WebstoreBindings we wanted happened to be the > most-recently constructed. Otherwise, we could end up executing in the wrong > context. Ok. Is it possible to check for the most-recently constructed WebstoreBindings? What is a way around this? https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... File chrome/renderer/extensions/webstore_bindings_helper.cc (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... chrome/renderer/extensions/webstore_bindings_helper.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/04/12 01:16:07, Devlin wrote: > no (c) in new code Done. https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... File chrome/renderer/extensions/webstore_bindings_helper.h (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... chrome/renderer/extensions/webstore_bindings_helper.h:14: class WebstoreBindingsHelper : public ObjectBackedNativeHandler, On 2017/04/12 01:16:07, Devlin wrote: > I think for this, this class can just be an inner class of WebstoreBindings > rather than in its own file. Then, each instance of WebstoreBindings can own a > WebstoreBindingsHelper. Done.
The CQ bit was checked by catmullings@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...)
The CQ bit was checked by catmullings@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...)
The CQ bit was checked by catmullings@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...)
The CQ bit was checked by catmullings@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...)
https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... chrome/renderer/extensions/webstore_bindings.cc:60: WebstoreBindings::SetContext(context); On 2017/04/14 20:59:40, catmullings wrote: > On 2017/04/12 01:16:07, Devlin wrote: > > This would only work if the WebstoreBindings we wanted happened to be the > > most-recently constructed. Otherwise, we could end up executing in the wrong > > context. > > Ok. Is it possible to check for the most-recently constructed WebstoreBindings? > What is a way around this? We don't necessarily want to check for the most-recently constructed WebstoreBindings, as in the case of a new context being constructed in the same render process. See comment in newest patchset for suggestions. https://codereview.chromium.org/2791533002/diff/140001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/140001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:131: base::Bind(&WebstoreBindingsHelper::Create, context)); This isn't necessary what we want. In particular, we don't necessarily want to strictly bind all requests for a frame to a single access point. Instead, we want each context to be able to receive its own messages. Previously, the way we accomplished this was by having WebstoreBindings inherit ChromeV8ExtensionHandler - a poorly named class that let these receive IPC messages directly. Now that we're moving away from these IPCs, we need a different approach. The issue with this current way is that a) it relies too much on which contexts may or may not be created/valid and the order in which they are, and b) it binds all requests to a single context. My suggestion to fix this would be to have the DoInlineInstall() message take an interface to an InlineInstallStatus: DoInlineInstall( int32 install_id, string webstore_item_id, int32 listeners_mask, InlineInstallStatus install_status) => (int32 install_id, bool success, string error, WebstoreInstallResult result); (Ken pointed me to https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi... for references on passing interfaces) Then, the TabHelper on the browser side uses that passed interface to call these update methods. On the renderer side, we'd have the WebstoreBindingsHelper inherit from InlineInstallStatus just as it does now, but be owned by the WebstoreBindings and passed as an argument to the DoInlineInstall() method. This allows us to have a separate InlineInstallStatus for each context in the frame, similar to what we do now, and couples the lifetime of the InlineInstallStatus to the WebstoreBindings (which is good from a deterministic point of view). Does that make sense? (Separately - maybe we should rename InlineInstallStatus to InlineInstallProgressListener or something?)
The CQ bit was checked by catmullings@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...)
https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/1/chrome/browser/extensions/t... chrome/browser/extensions/tab_helper.cc:155: std::move(request)); On 2017/04/06 20:50:15, catmullings wrote: > On 2017/03/31 14:18:39, Devlin wrote: > > This won't be quite right, I think. A TabHelper is designed to be a > > one-instance-per-web-contents object, but this will create a new TabHelper for > > each frame that makes a request. > > > > Looking around, I found content::WebContentsBindingSet and > > content::WebContentsFrameBindingSet - I think that's closer to what we want. > > > > > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_bind... > > Done. Just an FYI, the WebContentsFrameBindingSet is meant for (channel) associated interface bindings. Since ordering is not a concern among these IPCs, this binding set is a bit of an overkill. That being said, the WebContentsFrameBindingSet handles many things behind the scenes that are useful in this case over a WebContents, so migrating to a vanilla binding set might be involved.
https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/20001/chrome/renderer/extensi... chrome/renderer/extensions/webstore_bindings.cc:60: WebstoreBindings::SetContext(context); On 2017/04/24 20:08:32, Devlin wrote: > On 2017/04/14 20:59:40, catmullings wrote: > > On 2017/04/12 01:16:07, Devlin wrote: > > > This would only work if the WebstoreBindings we wanted happened to be the > > > most-recently constructed. Otherwise, we could end up executing in the > wrong > > > context. > > > > Ok. Is it possible to check for the most-recently constructed > WebstoreBindings? > > What is a way around this? > > We don't necessarily want to check for the most-recently constructed > WebstoreBindings, as in the case of a new context being constructed in the same > render process. See comment in newest patchset for suggestions. Done. https://codereview.chromium.org/2791533002/diff/140001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/140001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:131: base::Bind(&WebstoreBindingsHelper::Create, context)); On 2017/04/24 20:08:32, Devlin wrote: > This isn't necessary what we want. In particular, we don't necessarily want to > strictly bind all requests for a frame to a single access point. Instead, we > want each context to be able to receive its own messages. > > Previously, the way we accomplished this was by having WebstoreBindings inherit > ChromeV8ExtensionHandler - a poorly named class that let these receive IPC > messages directly. Now that we're moving away from these IPCs, we need a > different approach. > > The issue with this current way is that a) it relies too much on which contexts > may or may not be created/valid and the order in which they are, and b) it binds > all requests to a single context. > > My suggestion to fix this would be to have the DoInlineInstall() message take an > interface to an InlineInstallStatus: > > DoInlineInstall( > int32 install_id, string webstore_item_id, > int32 listeners_mask, InlineInstallStatus install_status) => > (int32 install_id, bool success, string error, > WebstoreInstallResult result); > > (Ken pointed me to > https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi... > for references on passing interfaces) > > Then, the TabHelper on the browser side uses that passed interface to call these > update methods. > > On the renderer side, we'd have the WebstoreBindingsHelper inherit from > InlineInstallStatus just as it does now, but be owned by the WebstoreBindings > and passed as an argument to the DoInlineInstall() method. > > This allows us to have a separate InlineInstallStatus for each context in the > frame, similar to what we do now, and couples the lifetime of the > InlineInstallStatus to the WebstoreBindings (which is good from a deterministic > point of view). > > Does that make sense? > > (Separately - maybe we should rename InlineInstallStatus to > InlineInstallProgressListener or something?) Done.
https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:57: : public ObjectBackedNativeHandler, I don't think this should need to be an ObjectBackedNativeHandler. https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:74: WebstoreBindings::WebstoreBindingsHelper::Create() { This create method is... odd. We call it on an instance of the class. Instead, we should probably just construct the class directly (as this method currently does), or we should make the constructor private and make the Create method static and take a ScriptContext. https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:83: void WebstoreBindings::WebstoreBindingsHelper::InlineInstallStageChanged( nit: Can we instead leave these methods in WebstoreBindings, and just call the method from here? (This would involve taking a pointer [or weak ptr, depending on lifetime concerns] to the parent WebstoreBindings, but I think that should be okay). e.g. void WebstoreBindings::WebstoreBindingsHelper::InlineInstallStateChanged( api::webstore::InstallStage stage) { parent_->InlineInstallStateChanged(stage); } This has two main advantages: - It centralizes the logic, and makes this helper class more clearly a "helper" (rather than the source of implementation). - It makes reviewing easier because the diff can be smaller. :D https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:185: std::move(install_progress_listener), Does this need to be a strong (i.e., owning) pointer? Or can ownership be kept within the WebstoreBindings parent, allowing reuse between calls?
The CQ bit was checked by catmullings@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...)
On 2017/04/27 23:16:15, Devlin wrote: > https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... > File chrome/renderer/extensions/webstore_bindings.cc (right): > > https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... > chrome/renderer/extensions/webstore_bindings.cc:57: : public > ObjectBackedNativeHandler, > I don't think this should need to be an ObjectBackedNativeHandler. > > https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... > chrome/renderer/extensions/webstore_bindings.cc:74: > WebstoreBindings::WebstoreBindingsHelper::Create() { > This create method is... odd. We call it on an instance of the class. Instead, > we should probably just construct the class directly (as this method currently > does), or we should make the constructor private and make the Create method > static and take a ScriptContext. > > https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... > chrome/renderer/extensions/webstore_bindings.cc:83: void > WebstoreBindings::WebstoreBindingsHelper::InlineInstallStageChanged( > nit: Can we instead leave these methods in WebstoreBindings, and just call the > method from here? (This would involve taking a pointer [or weak ptr, depending > on lifetime concerns] to the parent WebstoreBindings, but I think that should be > okay). > > e.g. > > void WebstoreBindings::WebstoreBindingsHelper::InlineInstallStateChanged( > api::webstore::InstallStage stage) { > parent_->InlineInstallStateChanged(stage); > } > > This has two main advantages: > - It centralizes the logic, and makes this helper class more clearly a "helper" > (rather than the source of implementation). > - It makes reviewing easier because the diff can be smaller. :D > > https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... > chrome/renderer/extensions/webstore_bindings.cc:185: > std::move(install_progress_listener), > Does this need to be a strong (i.e., owning) pointer? Or can ownership be kept > within the WebstoreBindings parent, allowing reuse between calls? Documenting the main changes from patch 8 to 9: - Moving the mojo msgs off the WebstoreBindingsHelper and back to WebstoreBindings, removing WebstoreBindingsHelper altogether. (Sidenote: WebstoreBindingsHelper was initially created to avoid instantiating multiple instances of WebstoreBindings (see reviewer comment: https://codereview.chromium.org/2791533002/diff/1/chrome/renderer/extensions/...) and so instead multiple instances of the WebstoreBindingsHelper would be created.) - Keep the mojo binding on WebstoreBindings instead of WebstoreBindingsHelper. It would be a binding set of interface ptrs to InlineInstallProgressListener. - And finally do: WebstoreBindings::DoInlineInstall( .. binding_set_.CreateInterfacePtrAndBind(this) ... ), which is useful in the case that WebstoreBindings::Install() (and therefore, DoInlineInstall()) is called multiple times.
https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:57: : public ObjectBackedNativeHandler, On 2017/04/27 23:16:14, Devlin wrote: > I don't think this should need to be an ObjectBackedNativeHandler. Just for documentation: Making WebstoreBindingsHelper an ObjectBackedNativeHandler was causing tests to fail. This is because nothing was calling NativeHandler::Invalidate() after a WebstoreBindingsHelper was destroyed. The reason why WebstoreBindings, also an ObjectBackedNativeHandler, did not have this error is because the ModuleSystem that it's hooked up to invalidates it (https://cs.chromium.org/chromium/src/extensions/renderer/module_system.cc?l=202). https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:57: : public ObjectBackedNativeHandler, On 2017/04/27 23:16:14, Devlin wrote: > I don't think this should need to be an ObjectBackedNativeHandler. Done. https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:74: WebstoreBindings::WebstoreBindingsHelper::Create() { On 2017/04/27 23:16:14, Devlin wrote: > This create method is... odd. We call it on an instance of the class. Instead, > we should probably just construct the class directly (as this method currently > does), or we should make the constructor private and make the Create method > static and take a ScriptContext. Done. https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:83: void WebstoreBindings::WebstoreBindingsHelper::InlineInstallStageChanged( On 2017/04/27 23:16:14, Devlin wrote: > nit: Can we instead leave these methods in WebstoreBindings, and just call the > method from here? (This would involve taking a pointer [or weak ptr, depending > on lifetime concerns] to the parent WebstoreBindings, but I think that should be > okay). > > e.g. > > void WebstoreBindings::WebstoreBindingsHelper::InlineInstallStateChanged( > api::webstore::InstallStage stage) { > parent_->InlineInstallStateChanged(stage); > } > > This has two main advantages: > - It centralizes the logic, and makes this helper class more clearly a "helper" > (rather than the source of implementation). > - It makes reviewing easier because the diff can be smaller. :D Done. https://codereview.chromium.org/2791533002/diff/160001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:185: std::move(install_progress_listener), On 2017/04/27 23:16:14, Devlin wrote: > Does this need to be a strong (i.e., owning) pointer? Or can ownership be kept > within the WebstoreBindings parent, allowing reuse between calls? If std::move() isn't used, then there's a compiler error: call to deleted constructor of 'extensions::mojom::InlineInstallProgressListenerPtr' https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.h (right): https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:64: mojo::BindingSet<mojom::InlineInstallProgressListener> A (weak) binding set does not own the object that implements the corresponding interface -- only a strong binding set would. Using a (weak) binding set would prevent double deletion, in the case that the interface implementation were to own the binding and the binding also owns the implementation (through passing the object via |this|). However, by default, a (weak) binding set doesn't treat its own destruction as a signal to unregister all the current bindings. I've had to handle this manually via set_connection_error (see https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... and #newcode66). Finally, a (weak) binding set may not do the following: "When the pipe a binding is bound to is disconnected, the binding is automatically destroyed and removed from the set, and the interface implementation is deleted." Would this be a problem in context of this CL? If it is, lmk and I'll try to determine a workaround.
It looks like some tests are still failing, but I think this is looking a lot better. Nice work! https://codereview.chromium.org/2791533002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:85: TabHelper* tab_helper, nit: no need to pass a TabHelper here now. https://codereview.chromium.org/2791533002/diff/180001/chrome/common/extensio... File chrome/common/extensions/mojo/inline_install.mojom (right): https://codereview.chromium.org/2791533002/diff/180001/chrome/common/extensio... chrome/common/extensions/mojo/inline_install.mojom:23: interface InlineInstall { nit: maybe InlineInstaller? WDYT? (It seems weird to me to have an interface called InlineInstall, since that makes it sound like it represents the installation itself) https://codereview.chromium.org/2791533002/diff/180001/chrome/common/extensio... chrome/common/extensions/mojo/inline_install.mojom:26: DoInlineInstall(int32 install_id, string webstore_item_id, nit: all parameters should have the same indentation (so move install_id and webstore_item_id to a new line) https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:72: void WebstoreBindings::InlineInstallResponse(int install_id, These methods are basically copy-paste, right? (looks like; just confirming) https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.h (right): https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:22: class WebstoreBindingsHelper; not needed https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:37: void InlineInstallResponse(int install_id, I think this can be private, right? https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:62: extensions::mojom::InlineInstallAssociatedPtr inline_install_; no need for extensions:: namespace https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:64: mojo::BindingSet<mojom::InlineInstallProgressListener> On 2017/04/28 03:07:30, catmullings wrote: > A (weak) binding set does not own the object that implements the corresponding > interface -- only a strong binding set would. Using a (weak) binding set would > prevent double deletion, in the case that the interface implementation were to > own the binding and the binding also owns the implementation (through passing > the object via |this|). > > However, by default, a (weak) binding set doesn't treat its own destruction as a > signal to unregister all the current bindings. I've had to handle this manually > via set_connection_error (see > https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... > and #newcode66). > > Finally, a (weak) binding set may not do the following: "When the pipe a binding > is bound to is disconnected, the binding is automatically destroyed and removed > from the set, and the interface implementation is deleted." Would this be a > problem in context of this CL? If it is, lmk and I'll try to determine a > workaround. I think that's desired - otherwise, we'd end up deleting this object artificially soon.
The CQ bit was checked by catmullings@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: 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_...)
Patchset #10 (id:200001) has been deleted
The CQ bit was checked by catmullings@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/2791533002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:85: TabHelper* tab_helper, On 2017/04/28 18:33:02, Devlin wrote: > nit: no need to pass a TabHelper here now. Done. https://codereview.chromium.org/2791533002/diff/180001/chrome/common/extensio... File chrome/common/extensions/mojo/inline_install.mojom (right): https://codereview.chromium.org/2791533002/diff/180001/chrome/common/extensio... chrome/common/extensions/mojo/inline_install.mojom:23: interface InlineInstall { On 2017/04/28 18:33:02, Devlin wrote: > nit: maybe InlineInstaller? WDYT? (It seems weird to me to have an interface > called InlineInstall, since that makes it sound like it represents the > installation itself) Done. https://codereview.chromium.org/2791533002/diff/180001/chrome/common/extensio... chrome/common/extensions/mojo/inline_install.mojom:26: DoInlineInstall(int32 install_id, string webstore_item_id, On 2017/04/28 18:33:02, Devlin wrote: > nit: all parameters should have the same indentation (so move install_id and > webstore_item_id to a new line) Done. https://codereview.chromium.org/2791533002/diff/180001/chrome/common/extensio... chrome/common/extensions/mojo/inline_install.mojom:30: WebstoreInstallResult result); Documentation: We removed the callback response on the mojo message because it was causing WebstoreInlineInstallerTest.NavigateBeforeInstallConfirmation and WebstoreInlineInstallerTest.CloseTabBeforeInstallConfirmation to fail. In those tests, inline installation does not complete, so a callback response is never made, and mojo throws an error. This error check is conducted before the TabHelper is destroyed. Because mojo response callbacks do not automatically close the pipe connection, the following workarounds were considered: 1.) If there are cases when we know the callback won't be used, then close the pipe connection 2.) Pass a param to the callback in cases of no-op 3.) Put the callback on a separate interface (i.e. make it a separate mojo msg) Option 1 entailed closing the pipe on the webstore installer, which conceptually wasn't correct since pipe closing should be isolated to the tab helper. The easiest and simplest solution was option 3. https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:72: void WebstoreBindings::InlineInstallResponse(int install_id, On 2017/04/28 18:33:02, Devlin wrote: > These methods are basically copy-paste, right? (looks like; just confirming) Yes, all copy paste. https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:72: void WebstoreBindings::InlineInstallResponse(int install_id, On 2017/04/28 18:33:02, Devlin wrote: > These methods are basically copy-paste, right? (looks like; just confirming) Done. https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.h (right): https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:22: class WebstoreBindingsHelper; On 2017/04/28 18:33:02, Devlin wrote: > not needed Done. https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:37: void InlineInstallResponse(int install_id, On 2017/04/28 18:33:02, Devlin wrote: > I think this can be private, right? With InlineInstallResponse as an mojo msg instead of a callback, it should remain public. https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:62: extensions::mojom::InlineInstallAssociatedPtr inline_install_; On 2017/04/28 18:33:02, Devlin wrote: > no need for extensions:: namespace Done. https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:64: mojo::BindingSet<mojom::InlineInstallProgressListener> On 2017/04/28 18:33:02, Devlin wrote: > On 2017/04/28 03:07:30, catmullings wrote: > > A (weak) binding set does not own the object that implements the corresponding > > interface -- only a strong binding set would. Using a (weak) binding set would > > prevent double deletion, in the case that the interface implementation were to > > own the binding and the binding also owns the implementation (through passing > > the object via |this|). > > > > However, by default, a (weak) binding set doesn't treat its own destruction as > a > > signal to unregister all the current bindings. I've had to handle this > manually > > via set_connection_error (see > > > https://codereview.chromium.org/2791533002/diff/180001/chrome/renderer/extens... > > and #newcode66). > > > > Finally, a (weak) binding set may not do the following: "When the pipe a > binding > > is bound to is disconnected, the binding is automatically destroyed and > removed > > from the set, and the interface implementation is deleted." Would this be a > > problem in context of this CL? If it is, lmk and I'll try to determine a > > workaround. > > I think that's desired - otherwise, we'd end up deleting this object > artificially soon. Confirming that indeed that a regular binding set does destroy and remove a binding if the binding's corresponding pipe becomes disconnected. https://codereview.chromium.org/2791533002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:608: success ? std::string() : error, result); Documentation: To avoid calling std::move(install_progress_listener) multiple times in DoInlineInstall(), passing it from OnReenable to OnComplete, we created an accessor to the interface ptr on InlineInstallObserver. https://codereview.chromium.org/2791533002/diff/220001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/220001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:60: &inline_installer_); Note: The render_frame is NULL in some cases. Reason why ServerWorkerTest.* tests were failing.
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...)
The CQ bit was checked by catmullings@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...
Patchset #11 (id:240001) has been deleted
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...)
Fixed some trybot failings on WebstoreStartupInstaller* and WesbtoreInlineInstaller* tests. https://codereview.chromium.org/2791533002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:608: success ? std::string() : error, result); On 2017/05/02 19:02:15, catmullings wrote: > Documentation: To avoid calling std::move(install_progress_listener) multiple > times in DoInlineInstall(), passing it from OnReenable to OnComplete, we created > an accessor to the interface ptr on InlineInstallObserver. Update: Sometimes install_observers_[extension_id] can be NULL, as in the case of WebstoreStartupInstallUnpackFailureTest.* tests. To resolve this, have TabHelper own the InstallProgressListener interface ptr. In this way, the interface ptr is accessible by the inner class, InlineInstallObserver, and the outer class TabHelper.
The CQ bit was checked by catmullings@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...)
Nice! Just a few last comments on TabHelper, but this is looking really close. https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:464: if (observe_install_stage || observe_download_progress) { The reason install_observers_[extension_id] is sometimes null is because there isn't always an associated listener, and right now we only instantiate it for that case. We could instead always instantiate an InlineInstallObserver, have it own the InstallProgressListener interface, and only send updates if a listener of that type is registered. https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.h (right): https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:273: extensions::mojom::InlineInstallProgressListenerPtr remember, no need for extensions:: prefix inside the extensions namespace. https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:274: inline_install_progress_listener_; The problem with having the TabHelper own this directly is that there could, potentially, be multiple inline installations** for a single tab helper, in which case this could be wrong. There are two reasonable options here: 1. We already have the mapping of ExtensionId -> InlineInstallObserver above, which handles this gracefully. I think it'd be better to have the InlineInstallProgressListenerPtr owned by the InlineInstallObserver. See also note in the .cc. 2. We could have a second map of <ExtensionId, InlineInstallProgressListenerPtr>. **In practice, this is unlikely since we restrict to the main frame. But if/when we address that, it should be in a separate CL, since the reorganization of the class is non-trivial.
The CQ bit was checked by catmullings@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...)
https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:464: if (observe_install_stage || observe_download_progress) { On 2017/05/03 14:18:29, Devlin wrote: > The reason install_observers_[extension_id] is sometimes null is because there > isn't always an associated listener, and right now we only instantiate it for > that case. We could instead always instantiate an InlineInstallObserver, have > it own the InstallProgressListener interface, and only send updates if a > listener of that type is registered. Done. https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.h (right): https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:273: extensions::mojom::InlineInstallProgressListenerPtr On 2017/05/03 14:18:29, Devlin wrote: > remember, no need for extensions:: prefix inside the extensions namespace. Done. https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:274: inline_install_progress_listener_; On 2017/05/03 14:18:29, Devlin wrote: > The problem with having the TabHelper own this directly is that there could, > potentially, be multiple inline installations** for a single tab helper, in > which case this could be wrong. > > There are two reasonable options here: > 1. We already have the mapping of ExtensionId -> InlineInstallObserver above, > which handles this gracefully. I think it'd be better to have the > InlineInstallProgressListenerPtr owned by the InlineInstallObserver. See also > note in the .cc. > 2. We could have a second map of <ExtensionId, > InlineInstallProgressListenerPtr>. > > **In practice, this is unlikely since we restrict to the main frame. But > if/when we address that, it should be in a separate CL, since the reorganization > of the class is non-trivial. Documentation: Opted for approach #2. https://codereview.chromium.org/2791533002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:274: inline_install_progress_listener_; On 2017/05/03 14:18:29, Devlin wrote: > The problem with having the TabHelper own this directly is that there could, > potentially, be multiple inline installations** for a single tab helper, in > which case this could be wrong. > > There are two reasonable options here: > 1. We already have the mapping of ExtensionId -> InlineInstallObserver above, > which handles this gracefully. I think it'd be better to have the > InlineInstallProgressListenerPtr owned by the InlineInstallObserver. See also > note in the .cc. > 2. We could have a second map of <ExtensionId, > InlineInstallProgressListenerPtr>. > > **In practice, this is unlikely since we restrict to the main frame. But > if/when we address that, it should be in a separate CL, since the reorganization > of the class is non-trivial. Done.
Cool! A few more comments, but I don't think much more will have to change. Do you want to add a mojo reviewer to take a look, too? https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:112: DCHECK(tab_helper_->inline_install_progress_listeners_[extension_id]); This will cause a double lookup into the map. Prefer something like: auto iter = tab_helper_->inline_install_progress_listeners_.find(extension_id); DCHECK(iter != tab_helper_->inline_install_progress_listeners_.end()); iter->second->InlineInstallDownloadProgress(...); https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:126: DCHECK(tab_helper_->inline_install_progress_listeners_[extension_id]); ditto https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:430: if (pending_inline_installations_.count(webstore_item_id) != 0) { With the map of pointers, we can actually just use that instead of pending_inline_installations_. https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:437: inline_install_progress_listeners_[webstore_item_id] = std::move(install_progress_listener); don't forget git cl format :) https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:603: DCHECK(inline_install_progress_listeners_[extension_id]); ditto re the double lookup https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.h (right): https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:165: extensions::mojom::InlineInstallProgressListenerPtr Remember, no need for extensions:: prefix
The CQ bit was checked by catmullings@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...)
catmullings@chromium.org changed reviewers: + rockot@chromium.org
For mojo expertise, Ken PTAL. https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:112: DCHECK(tab_helper_->inline_install_progress_listeners_[extension_id]); On 2017/05/05 20:43:46, Devlin wrote: > This will cause a double lookup into the map. Prefer something like: > auto iter = tab_helper_->inline_install_progress_listeners_.find(extension_id); > DCHECK(iter != tab_helper_->inline_install_progress_listeners_.end()); > iter->second->InlineInstallDownloadProgress(...); Done. https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:126: DCHECK(tab_helper_->inline_install_progress_listeners_[extension_id]); On 2017/05/05 20:43:47, Devlin wrote: > ditto Done. https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:430: if (pending_inline_installations_.count(webstore_item_id) != 0) { On 2017/05/05 20:43:46, Devlin wrote: > With the map of pointers, we can actually just use that instead of > pending_inline_installations_. Removed. Documentation: pending_inline_installations_ is a set of the ids of extensions that were being installed, so it is equivalent to the keys of the new map inline_install_progress_listeners_. https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:430: if (pending_inline_installations_.count(webstore_item_id) != 0) { On 2017/05/05 20:43:46, Devlin wrote: > With the map of pointers, we can actually just use that instead of > pending_inline_installations_. Done. https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:437: inline_install_progress_listeners_[webstore_item_id] = std::move(install_progress_listener); On 2017/05/05 20:43:46, Devlin wrote: > don't forget git cl format :) Done. https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:603: DCHECK(inline_install_progress_listeners_[extension_id]); On 2017/05/05 20:43:46, Devlin wrote: > ditto re the double lookup Done. https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.h (right): https://codereview.chromium.org/2791533002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:165: extensions::mojom::InlineInstallProgressListenerPtr On 2017/05/05 20:43:47, Devlin wrote: > Remember, no need for extensions:: prefix Done.
Mostly looks great, just one blocking question in the mojom https://codereview.chromium.org/2791533002/diff/300001/chrome/common/extensio... File chrome/common/extensions/mojo/inline_install.mojom (right): https://codereview.chromium.org/2791533002/diff/300001/chrome/common/extensio... chrome/common/extensions/mojo/inline_install.mojom:1: // Copyright 2017 The Chromium Authors. All rights reserved. minor nit: Moving forward, we would prefer that if a subdirectory is dedicated to mojom+typemap+traits definitions, it be named "mojom" instead of "mojo". We'll be adapting other directories to follow that pattern as well. https://codereview.chromium.org/2791533002/diff/300001/chrome/common/extensio... chrome/common/extensions/mojo/inline_install.mojom:16: InlineInstallResponse(int32 install_id, bool success, string error, I may be overlooking something. At a glance it seems like install_id is only used to link a response to the original request. Is that accurate? If so, this message could just be a reply to DoInlineInstall, e.g.: interface InlineInstaller { DoInlineInstall(string webstore_item_id, int32 listeners_mask, InlineInstallProgressListener install_progress_listener) => (bool success, string error, WebStoreInstallResult result); };
https://codereview.chromium.org/2791533002/diff/300001/content/public/app/moj... File content/public/app/mojo/content_renderer_manifest.json (right): https://codereview.chromium.org/2791533002/diff/300001/content/public/app/moj... content/public/app/mojo/content_renderer_manifest.json:46: "extensions::mojom::InlineInstallStatus", nit: Please remove this
catmullings@chromium.org changed reviewers: + meacer@chromium.org
+meacer for IPC Security on chrome_content_browser_manifest_overlay.json and chrome_extension_messages.h Also Ken, please take another look at chrome_extension_messages.h. You can see here (https://codereview.chromium.org/2791533002/diff2/1:320001/chrome/common/exten...) from a previous patch upload about the IPC_ENUMs cannot be deleted. They have have been redefined in the inline_install_traits.h here (https://codereview.chromium.org/2791533002/diff/20001/chrome/common/extension...). I would think that only one definition of the IPC_ENUMs is required. Is this correct? https://codereview.chromium.org/2791533002/diff/300001/chrome/common/extensio... File chrome/common/extensions/mojo/inline_install.mojom (right): https://codereview.chromium.org/2791533002/diff/300001/chrome/common/extensio... chrome/common/extensions/mojo/inline_install.mojom:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/08 16:12:45, Ken Rockot wrote: > minor nit: Moving forward, we would prefer that if a subdirectory is dedicated > to mojom+typemap+traits definitions, it be named "mojom" instead of "mojo". > We'll be adapting other directories to follow that pattern as well. Done. https://codereview.chromium.org/2791533002/diff/300001/chrome/common/extensio... chrome/common/extensions/mojo/inline_install.mojom:16: InlineInstallResponse(int32 install_id, bool success, string error, On 2017/05/08 16:12:45, Ken Rockot wrote: > I may be overlooking something. At a glance it seems like install_id is only > used to link a response to the original request. Is that accurate? If so, this > message could just be a reply to DoInlineInstall, e.g.: > > > interface InlineInstaller { > DoInlineInstall(string webstore_item_id, int32 listeners_mask, > InlineInstallProgressListener install_progress_listener) > => (bool success, string error, WebStoreInstallResult result); > }; Yes, install_id is only used to link the response to the original request. However, there is an intentional reason why InlineInstallResponse is a separate mojo message. In the case when an inline installation does not complete, the callback response would never be called, causing a mojo error. One workaround I considered was manually closing the pipe connection in that case. However, the pipe would need to be closed on WebstoreInstaller (when the installation incompletion occurs), which conceptually wasn't the best solution because pipe closing should be isolated to the tab helper. https://codereview.chromium.org/2791533002/diff/300001/content/public/app/moj... File content/public/app/mojo/content_renderer_manifest.json (right): https://codereview.chromium.org/2791533002/diff/300001/content/public/app/moj... content/public/app/mojo/content_renderer_manifest.json:46: "extensions::mojom::InlineInstallStatus", On 2017/05/08 16:13:12, Ken Rockot wrote: > nit: Please remove this Done.
On 2017/05/08 at 20:41:35, catmullings wrote: > +meacer for IPC Security on chrome_content_browser_manifest_overlay.json and chrome_extension_messages.h > > Also Ken, please take another look at chrome_extension_messages.h. You can see here (https://codereview.chromium.org/2791533002/diff2/1:320001/chrome/common/exten...) from a previous patch upload about the IPC_ENUMs cannot be deleted. They have have been redefined in the inline_install_traits.h here (https://codereview.chromium.org/2791533002/diff/20001/chrome/common/extension...). I would think that only one definition of the IPC_ENUMs is required. Is this correct? You definitely don't need multiple incarnations of the same IPC_ENUM macros. I would recomend moving the ones from chrome_extension_messages.h into their own header, and adding that header to the typemap's traits_headers list. (It looks like your latest patch set deleted this typemap file?) > > https://codereview.chromium.org/2791533002/diff/300001/chrome/common/extensio... > File chrome/common/extensions/mojo/inline_install.mojom (right): > > https://codereview.chromium.org/2791533002/diff/300001/chrome/common/extensio... > chrome/common/extensions/mojo/inline_install.mojom:1: // Copyright 2017 The Chromium Authors. All rights reserved. > On 2017/05/08 16:12:45, Ken Rockot wrote: > > minor nit: Moving forward, we would prefer that if a subdirectory is dedicated > > to mojom+typemap+traits definitions, it be named "mojom" instead of "mojo". > > We'll be adapting other directories to follow that pattern as well. > > Done. > > https://codereview.chromium.org/2791533002/diff/300001/chrome/common/extensio... > chrome/common/extensions/mojo/inline_install.mojom:16: InlineInstallResponse(int32 install_id, bool success, string error, > On 2017/05/08 16:12:45, Ken Rockot wrote: > > I may be overlooking something. At a glance it seems like install_id is only > > used to link a response to the original request. Is that accurate? If so, this > > message could just be a reply to DoInlineInstall, e.g.: > > > > > > interface InlineInstaller { > > DoInlineInstall(string webstore_item_id, int32 listeners_mask, > > InlineInstallProgressListener install_progress_listener) > > => (bool success, string error, WebStoreInstallResult result); > > }; > > Yes, install_id is only used to link the response to the original request. However, there is an intentional reason why InlineInstallResponse is a separate mojo message. In the case when an inline installation does not complete, the callback response would never be called, causing a mojo error. > > One workaround I considered was manually closing the pipe connection in that case. However, the pipe would need to be closed on WebstoreInstaller (when the installation incompletion occurs), which conceptually wasn't the best solution because pipe closing should be isolated to the tab helper. We should come up with a way to use a message response here, because the use of request IDs to link messages across interfaces is completely antithetical to mojom design. > > https://codereview.chromium.org/2791533002/diff/300001/content/public/app/moj... > File content/public/app/mojo/content_renderer_manifest.json (right): > > https://codereview.chromium.org/2791533002/diff/300001/content/public/app/moj... > content/public/app/mojo/content_renderer_manifest.json:46: "extensions::mojom::InlineInstallStatus", > On 2017/05/08 16:13:12, Ken Rockot wrote: > > nit: Please remove this > > Done.
catmullings: I'm OOO so unlikely to get to this very soon, so you might want to add other IPC reviewers.
catmullings@chromium.org changed reviewers: + palmer@chromium.org - meacer@chromium.org
Replacing meacer@, who is OOO, with palmer@ for IPC security review. palmer@, PTAL at chrome_content_browser_manifest_overlay.json and chrome_extension_messages.h
https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:114: DCHECK(iter != tab_helper_->inline_install_progress_listeners_.end()); DCHECK suggests to me that this would only happen in case of programmer error, but I wonder if it is possible for |find| to return |end| legitimately (due to the slings and arrows of outrageous fortune at runtime)? Should this be if (iter != tab_helper_->inline_install_progress_listeners_.end()) { iter->second->InlineInstallDownloadProgress(percent_downloaded); } ? But, if it really would only happen due to programmer error, then I'd agree that a DCHECK is right. https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:129: DCHECK(iter != tab_helper_->inline_install_progress_listeners_.end()); Same question here. https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:604: DCHECK(iter != inline_install_progress_listeners_.end()); Same question here. https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.h (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:269: // Map of mojo interace ptrs to InlineInstallProgressListeners. Nit: typo: "interface" Medium-size nit: the comment doesn't seem to match the code (is an |ExtensionId| a Mojo interface pointer?). In any case, it seems redundant so you can probably remove it. https://codereview.chromium.org/2791533002/diff/320001/chrome/common/extensio... File chrome/common/extensions/chrome_extension_messages.h (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/common/extensio... chrome/common/extensions/chrome_extension_messages.h:32: // TODO(catmullings): Remove these ipc enums once all ipc messages here are Might be good to attach a bug number to this TODO. https://codereview.chromium.org/2791533002/diff/320001/chrome/common/extensio... File chrome/common/extensions/typemaps.gni (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/common/extensio... chrome/common/extensions/typemaps.gni:7: "//chrome/common/extensions/mojom/inline_install.typemap", Is this file new? It doesn't seem to be in my source tree, but also isn't in this CL. https://codereview.chromium.org/2791533002/diff/320001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:48: // (successful or not) via InlineInstallResponse. Nit: |InlineInstallResponse|. https://codereview.chromium.org/2791533002/diff/320001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.h (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:36: // mojom::InlineInstallProgressListener Infinitesimal :) nit: Use a :, same as on line 33.
Patchset #15 (id:340001) has been deleted
The CQ bit was checked by catmullings@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...)
https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:114: DCHECK(iter != tab_helper_->inline_install_progress_listeners_.end()); On 2017/05/08 23:39:03, palmer wrote: > DCHECK suggests to me that this would only happen in case of programmer error, > but I wonder if it is possible for |find| to return |end| legitimately (due to > the slings and arrows of outrageous fortune at runtime)? Should this be > > if (iter != tab_helper_->inline_install_progress_listeners_.end()) { > iter->second->InlineInstallDownloadProgress(percent_downloaded); > } > > ? But, if it really would only happen due to programmer error, then I'd agree > that a DCHECK is right. My hunch is that this case would never happen, hence the use of DCHECK. Devlin, can you confirm? https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.h (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:269: // Map of mojo interace ptrs to InlineInstallProgressListeners. On 2017/05/08 23:39:03, palmer wrote: > Nit: typo: "interface" > > Medium-size nit: the comment doesn't seem to match the code (is an |ExtensionId| > a Mojo interface pointer?). In any case, it seems redundant so you can probably > remove it. Nice catch. An |ExtensionId| is not a Mojo interface pointer. Deleting comment in any case. https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:269: // Map of mojo interace ptrs to InlineInstallProgressListeners. On 2017/05/08 23:39:03, palmer wrote: > Nit: typo: "interface" > > Medium-size nit: the comment doesn't seem to match the code (is an |ExtensionId| > a Mojo interface pointer?). In any case, it seems redundant so you can probably > remove it. Done. https://codereview.chromium.org/2791533002/diff/320001/chrome/common/extensio... File chrome/common/extensions/chrome_extension_messages.h (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/common/extensio... chrome/common/extensions/chrome_extension_messages.h:32: // TODO(catmullings): Remove these ipc enums once all ipc messages here are On 2017/05/08 23:39:03, palmer wrote: > Might be good to attach a bug number to this TODO. Done. https://codereview.chromium.org/2791533002/diff/320001/chrome/common/extensio... File chrome/common/extensions/typemaps.gni (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/common/extensio... chrome/common/extensions/typemaps.gni:7: "//chrome/common/extensions/mojom/inline_install.typemap", On 2017/05/08 23:39:03, palmer wrote: > Is this file new? It doesn't seem to be in my source tree, but also isn't in > this CL. It should now be included in patch 15. https://codereview.chromium.org/2791533002/diff/320001/chrome/common/extensio... chrome/common/extensions/typemaps.gni:7: "//chrome/common/extensions/mojom/inline_install.typemap", On 2017/05/08 23:39:03, palmer wrote: > Is this file new? It doesn't seem to be in my source tree, but also isn't in > this CL. Done. https://codereview.chromium.org/2791533002/diff/320001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:48: // (successful or not) via InlineInstallResponse. On 2017/05/08 23:39:03, palmer wrote: > Nit: |InlineInstallResponse|. Done. https://codereview.chromium.org/2791533002/diff/320001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.h (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:36: // mojom::InlineInstallProgressListener On 2017/05/08 23:39:03, palmer wrote: > Infinitesimal :) nit: Use a :, same as on line 33. Done.
Re Ken's comments: > You definitely don't need multiple incarnations of the same IPC_ENUM macros. I > would recomend moving the ones from chrome_extension_messages.h into their own > header, and adding that header to the typemap's traits_headers list. (It looks > like your latest patch set deleted this typemap file?) I'll remove the IPC_ENUM macros from chrome_extension_messages.h once all the IPCs (2 left) are converted. Here's the bug to track this: crbug.com/725275 > > Yes, install_id is only used to link the response to the original request. > However, there is an intentional reason why InlineInstallResponse is a separate > mojo message. In the case when an inline installation does not complete, the > callback response would never be called, causing a mojo error. > > > > One workaround I considered was manually closing the pipe connection in that > case. However, the pipe would need to be closed on WebstoreInstaller (when the > installation incompletion occurs), which conceptually wasn't the best solution > because pipe closing should be isolated to the tab helper. > > We should come up with a way to use a message response here, because the use of > request IDs to link messages across interfaces is completely antithetical to > mojom design. TL;DR: This patch addresses the above request, namely reverting InlineInstallResponse to a callback instead of a separate message. It turns out that the pipe connection did not need to be closed. In an older patch that kept InlineInstallResponse as a callback, it was being passed to from TabHelper to WebstoreInstaller. When the WebstoreInstaller was being destroyed due to an incomplete installation, mojo would throw an error saying that the callback was never called. Now, in this patch, the InlineInstallResponse callback is owned by the TabHelper and is not passed to WebstoreInstaller. I think this works because when the WebstoreInstaller does not complete installation, it notifies the TabHelper, which records (callback is invoked) the installation status/result as a failure.
https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/2791533002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.cc:114: DCHECK(iter != tab_helper_->inline_install_progress_listeners_.end()); On 2017/05/23 02:38:37, catmullings wrote: > On 2017/05/08 23:39:03, palmer wrote: > > DCHECK suggests to me that this would only happen in case of programmer error, > > but I wonder if it is possible for |find| to return |end| legitimately (due to > > the slings and arrows of outrageous fortune at runtime)? Should this be > > > > if (iter != tab_helper_->inline_install_progress_listeners_.end()) { > > iter->second->InlineInstallDownloadProgress(percent_downloaded); > > } > > > > ? But, if it really would only happen due to programmer error, then I'd agree > > that a DCHECK is right. > > My hunch is that this case would never happen, hence the use of DCHECK. Devlin, > can you confirm? The way this is written, this should never happen. The TabHelper owns the inline install progress listeners and the inline install observers, and should erase them at the same time. If one exists, so should the other. So DCHECK here should be correct.
LGTM; some nits that are up to you. https://codereview.chromium.org/2791533002/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.h (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:268: // Map of function callback that are invoked when the inline installation, for Nit: "...callbacks that are...", and then no ",". https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:93: break; Nit: Do you need a default: NOTREACHED() or anything like that?
Nice! Getting close! https://codereview.chromium.org/2791533002/diff/360001/chrome/common/extensio... File chrome/common/extensions/mojom/inline_install.mojom (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/common/extensio... chrome/common/extensions/mojom/inline_install.mojom:29: (int32 install_id, bool success, string error, I think Ken's hope was to do-away with install_id completely at the mojo/browser layers, which we should be able to do by binding it on the renderer side (since the callback will only be associated with a single request). https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:84: const char* stage_string = NULL; nit: nullptr https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:86: static_cast<api::webstore::InstallStage>(stage); No need for this cast - it's already the proper type. https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:156: base::Unretained(this))); comment why this Unretained is safe. https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.h (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:36: void InlineInstallResponse(int install_id, nit: This can be private
https://codereview.chromium.org/2791533002/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/tab_helper.h (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/tab_helper.h:268: // Map of function callback that are invoked when the inline installation, for On 2017/05/23 22:41:47, palmer wrote: > Nit: "...callbacks that are...", and then no ",". Done. https://codereview.chromium.org/2791533002/diff/360001/chrome/common/extensio... File chrome/common/extensions/mojom/inline_install.mojom (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/common/extensio... chrome/common/extensions/mojom/inline_install.mojom:29: (int32 install_id, bool success, string error, On 2017/05/24 21:33:59, Devlin (catching up) wrote: > I think Ken's hope was to do-away with install_id completely at the mojo/browser > layers, which we should be able to do by binding it on the renderer side (since > the callback will only be associated with a single request). Done. https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:84: const char* stage_string = NULL; On 2017/05/24 21:33:59, Devlin (catching up) wrote: > nit: nullptr Done. https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:86: static_cast<api::webstore::InstallStage>(stage); On 2017/05/24 21:33:59, Devlin (catching up) wrote: > No need for this cast - it's already the proper type. Done. https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:93: break; On 2017/05/23 22:41:47, palmer wrote: > Nit: Do you need a default: NOTREACHED() or anything like that? Short answer is no, but I will follow up with an explanation. https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:156: base::Unretained(this))); On 2017/05/24 21:33:59, Devlin (catching up) wrote: > comment why this Unretained is safe. Ken, is it safe to use Unretained here? We're thinking yes because if the WebstoreBindings instance is destroyed before the callback is called, mojo will close the pipe connection (WebstoreBindings owns a binding set of InlineInstallProgressListener requests). https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.h (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.h:36: void InlineInstallResponse(int install_id, On 2017/05/24 21:33:59, Devlin (catching up) wrote: > nit: This can be private Done.
The CQ bit was checked by catmullings@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/2791533002/diff/360001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:93: break; On 2017/05/25 00:14:44, catmullings wrote: > On 2017/05/23 22:41:47, palmer wrote: > > Nit: Do you need a default: NOTREACHED() or anything like that? > > Short answer is no, but I will follow up with an explanation. Follow up: If a new enum value is introduced, it will just be a compile error. If, instead of a valid enum, an improper value is passed (as in the case of static_cast<MyEnum>(5)), mojo should validate it and kill the process.
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...)
https://codereview.chromium.org/2791533002/diff/380001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (left): https://codereview.chromium.org/2791533002/diff/380001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:98: args.GetReturnValue().Set(static_cast<int32_t>(install_id)); Devlin, since install_id is no longer used here, is it okay to delete line 98?
https://codereview.chromium.org/2791533002/diff/380001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (left): https://codereview.chromium.org/2791533002/diff/380001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:98: args.GetReturnValue().Set(static_cast<int32_t>(install_id)); On 2017/05/25 00:44:23, catmullings wrote: > Devlin, since install_id is no longer used here, is it okay to delete line 98? This was a little too much removed. :) We should be able to isolate knowledge of the install id to the renderer, but it's still important on the renderer side to differentiate between inline install attempts. So instead of removing it completely, we should still be returning it here and currying the value in the callback (as opposed to bouncing it through the browser process). Does that make sense?
https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:156: base::Unretained(this))); On 2017/05/25 at 00:14:44, catmullings wrote: > On 2017/05/24 21:33:59, Devlin (catching up) wrote: > > comment why this Unretained is safe. > > Ken, is it safe to use Unretained here? We're thinking yes because if the WebstoreBindings instance is destroyed before the callback is called, mojo will close the pipe connection (WebstoreBindings owns a binding set of InlineInstallProgressListener requests). Yes it's safe, but the safety has nothing to do with theInlineInstallProgressListener binding lifetime. This bound callback is for the response message to DoInlineInstall. A response will not be dispatched if the InterfacePtr which sent the request has been destroyed. Since |this| owns the InterfacePtr, everything is fine. I would not encourage adding comments about Unretained in these cases -- if that were the rule then every InterfacePtr call with a response would have a similar comment, and that doesn't seem terribly useful.
Patchset #17 (id:400001) has been deleted
The CQ bit was checked by catmullings@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...)
Patchset #18 (id:440001) has been deleted
Patchset #17 (id:420001) has been deleted
The CQ bit was checked by catmullings@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/2791533002/diff/360001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/360001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:93: break; On 2017/05/23 22:41:47, palmer wrote: > Nit: Do you need a default: NOTREACHED() or anything like that? Done. https://codereview.chromium.org/2791533002/diff/380001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (left): https://codereview.chromium.org/2791533002/diff/380001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:98: args.GetReturnValue().Set(static_cast<int32_t>(install_id)); On 2017/05/25 15:22:22, Devlin (catching up) wrote: > On 2017/05/25 00:44:23, catmullings wrote: > > Devlin, since install_id is no longer used here, is it okay to delete line 98? > > This was a little too much removed. :) We should be able to isolate knowledge > of the install id to the renderer, but it's still important on the renderer side > to differentiate between inline install attempts. So instead of removing it > completely, we should still be returning it here and currying the value in the > callback (as opposed to bouncing it through the browser process). Does that > make sense? Yup. The mojom definition of the InlineInstallResponse callback doesn't take the install_id, which is instead curried in on the WebstoreBindings side. https://codereview.chromium.org/2791533002/diff/380001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:98: args.GetReturnValue().Set(static_cast<int32_t>(install_id)); On 2017/05/25 15:22:22, Devlin (catching up) wrote: > On 2017/05/25 00:44:23, catmullings wrote: > > Devlin, since install_id is no longer used here, is it okay to delete line 98? > > This was a little too much removed. :) We should be able to isolate knowledge > of the install id to the renderer, but it's still important on the renderer side > to differentiate between inline install attempts. So instead of removing it > completely, we should still be returning it here and currying the value in the > callback (as opposed to bouncing it through the browser process). Does that > make sense? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! This is looking great. A few last comments and waiting for Ken's lg in case anything changes, but otherwise this looks good. https://codereview.chromium.org/2791533002/diff/460001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (left): https://codereview.chromium.org/2791533002/diff/460001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:189: bool WebstoreBindings::OnMessageReceived(const IPC::Message& message) { We can remove this method entirely, since we no longer receive any messages. https://codereview.chromium.org/2791533002/diff/460001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/460001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:77: api::webstore::kInstallResultCodes[static_cast<int>(result)])}; is this git cl format'd?
lgtm
https://codereview.chromium.org/2791533002/diff/460001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (left): https://codereview.chromium.org/2791533002/diff/460001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:189: bool WebstoreBindings::OnMessageReceived(const IPC::Message& message) { On 2017/05/26 15:00:54, Devlin (catching up) wrote: > We can remove this method entirely, since we no longer receive any messages. Done. https://codereview.chromium.org/2791533002/diff/460001/chrome/renderer/extens... File chrome/renderer/extensions/webstore_bindings.cc (right): https://codereview.chromium.org/2791533002/diff/460001/chrome/renderer/extens... chrome/renderer/extensions/webstore_bindings.cc:77: api::webstore::kInstallResultCodes[static_cast<int>(result)])}; On 2017/05/26 15:00:54, Devlin (catching up) wrote: > is this git cl format'd? Yes
Nice! LGTM!
The CQ bit was checked by catmullings@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, rockot@chromium.org Link to the patchset: https://codereview.chromium.org/2791533002/#ps480001 (title: "Addressed Devlin's comments")
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": 480001, "attempt_start_ts": 1495821925290200,
"parent_rev": "c95383ac0847a949d9b54964d0c2cd7c2b253a31", "commit_rev":
"ba539108932530a3cd7a6a3bb59c0ab253698772"}
Message was sent while issue was closed.
Description was changed from ========== Convert Web Store Inline Install IPCs to mojo Migrating the following inline install related IPCs to mojo: - ExtensionMsg_InlineInstallStageChanged - ExtensionMsg_InlineInstallDownloadProgress - ExtensionMsg_InlineWebstoreInstallResponse - ExtensionMsg_InlineWebstoreInstal BUG=697613 ========== to ========== Convert Web Store Inline Install IPCs to mojo Migrating the following inline install related IPCs to mojo: - ExtensionMsg_InlineInstallStageChanged - ExtensionMsg_InlineInstallDownloadProgress - ExtensionMsg_InlineWebstoreInstallResponse - ExtensionMsg_InlineWebstoreInstal BUG=697613 Review-Url: https://codereview.chromium.org/2791533002 Cr-Commit-Position: refs/heads/master@{#475090} Committed: https://chromium.googlesource.com/chromium/src/+/ba539108932530a3cd7a6a3bb59c... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:480001) as https://chromium.googlesource.com/chromium/src/+/ba539108932530a3cd7a6a3bb59c... |
