Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2098743002:
Always enable register local implementaions into remote interfaces of rfh. (Closed)
|
Created:
4 years, 6 months ago by leonhsl(Using Gerrit) Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlways enable register local implementaions into remote interfaces of rfh.
As for test codes using MockRenderProcessHost(No remote interfaces by default),
render frame host won't create remote_interfaces_ instance,
this will forbid test codes registering local implementations into it.
For such case, this CL just creates an shell::InterfaceProvider without
any binding, such InterfaceProvider is only for managing local implementations.
BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Patch Set 1 #Patch Set 2 #Patch Set 3 : Rebase #
Total comments: 2
Patch Set 4 : Revise code comments #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== Always enable register local implementaions into remote interfaces of rfh. As for test codes using MockRenderProcessHost(No service registry by default), render frame host won't create remote_interfaces_ instance, this will forbid test codes registering local implementations into it. For such case, this CL just creates an shell::InterfaceProvider with a nullptr mojom::InterfaceProviderPtr, this InterfaceProvider is only for managing local implementations of mojo interface. BUG= ========== to ========== Always enable register local implementaions into remote interfaces of rfh. As for test codes using MockRenderProcessHost(No service registry by default), render frame host won't create remote_interfaces_ instance, this will forbid test codes registering local implementations into it. For such case, this CL just creates an shell::InterfaceProvider with a nullptr mojom::InterfaceProviderPtr, this InterfaceProvider is only for managing local implementations of mojo interface. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2098743002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
leon.han@intel.com changed reviewers: + ben@chromium.org
I encountered the problem in https://codereview.chromium.org/2007473004/diff/80001/components/autofill/con..., where I found the remote_interfaces_ of main render frame host is always nullptr. This CL could solve the problem. Would you PTAL to see whether this makes sense? Thanks~
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Always enable register local implementaions into remote interfaces of rfh. As for test codes using MockRenderProcessHost(No service registry by default), render frame host won't create remote_interfaces_ instance, this will forbid test codes registering local implementations into it. For such case, this CL just creates an shell::InterfaceProvider with a nullptr mojom::InterfaceProviderPtr, this InterfaceProvider is only for managing local implementations of mojo interface. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Always enable register local implementaions into remote interfaces of rfh. As for test codes using MockRenderProcessHost(No remote interfaces by default), render frame host won't create remote_interfaces_ instance, this will forbid test codes registering local implementations into it. For such case, this CL just creates an shell::InterfaceProvider without any binding, such InterfaceProvider is only for managing local implementations. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Soft ping ben@, Thanks. The point is to enable Shell::InterfaceProvider instance be constructed without Binding to a real remote mojom::InterfaceProvider, it just manages local interface implementations for testing, which is the same function of original ServiceRegistry::AddOverrideServiceForTesting. Otherwise if some one test suite wants to register a local callback to RFH, it has to set a dummy remote interfaces into MockRenderProcessHost, only this way can lead RFH to create Shell::InterfaceProvider.
https://codereview.chromium.org/2098743002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2098743002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2401: remote_interfaces_.reset(new shell::InterfaceProvider); Is this blocking something you want to do? I don't see code in this CL that relies on this behavior. (context: I'm working towards making it be the case that the check on the following line is unnecessary)
https://codereview.chromium.org/2098743002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2098743002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2401: remote_interfaces_.reset(new shell::InterfaceProvider); On 2016/06/29 17:55:54, Ben Goodger (Google) wrote: > Is this blocking something you want to do? I don't see code in this CL that > relies on this behavior. > > (context: I'm working towards making it be the case that the check on the > following line is unnecessary) In my another CL https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... line#268, the remote_interfaces of main RFH is found to be nullptr. And, also in that CL https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... because line#261 is trying to access RFH's InterfaceProvider, some test codes maybe trigger this line execute ended with a nullptr access. Although we can add some guard codes to avoid the nullptr access there. I think this would be a general issue so I created this CL separately and want to land it firstly.
leon.han@intel.com changed reviewers: + rockot@chromium.org
Hi, Ken, I already separated this CL from my original CL https://codereview.chromium.org/2007473004/
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
