Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2959383002:
[GRC] Hook up the process-level CoordinationUnit (Closed)
|
Created:
3 years, 5 months ago by lpy Modified:
3 years, 5 months ago CC:
chromium-reviews, creis+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, nasko+codewatch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[GRC] Hook up the process-level CoordinationUnit
This gets hooked up in the browser process and made available to the renderer
process on request. It's created on demand, and currently won't be instantiated
without the GlobalResourceCoordinator feature flag. An example of full GRC usage
can be seen in https://codereview.chromium.org/2710823003
BUG=691886
Review-Url: https://codereview.chromium.org/2959383002
Cr-Commit-Position: refs/heads/master@{#483749}
Committed: https://chromium.googlesource.com/chromium/src/+/817370ae855c288809072aecb07e6063a13eda79
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments #
Messages
Total messages: 37 (26 generated)
The CQ bit was checked by lpy@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...
lpy@chromium.org changed reviewers: + nasko@chromium.org, oysteine@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lpy@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 lpy@chromium.org
The CQ bit was checked by lpy@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This CL on its own looks very isolated and I don't get a good idea why and what will use the functionality. Why can't it be coupled with actual code that makes usage of this? https://codereview.chromium.org/2959383002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2959383002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_impl.cc:856: void CreateResourceCoordinatorProcessInterface( nit: I'd move this to be closer to CreateMemoryCoordinatorHandle, as they are similar type of functionality - coordination related.
On 2017/06/29 17:24:28, nasko (slow) wrote: > This CL on its own looks very isolated and I don't get a good idea why and what > will use the functionality. Why can't it be coupled with actual code that makes > usage of this? This is a follow-up patch of https://codereview.chromium.org/2913253002 to expose the mojo interface to renderer process, blink will use it by calling: mojom::CoordinationUnitPtr ptr; Platform::Current()::GetInterfaceProvider()->GetInterface(mojo::MakeRequest(&ptr)) to set up the mojo channel. There is a follow up patch ongoing to use this interface inside Blink.
On 2017/06/29 17:30:11, lpy wrote: > On 2017/06/29 17:24:28, nasko (slow) wrote: > > This CL on its own looks very isolated and I don't get a good idea why and > what > > will use the functionality. Why can't it be coupled with actual code that > makes > > usage of this? > > This is a follow-up patch of https://codereview.chromium.org/2913253002 to > expose the mojo interface to renderer process, blink will use it by calling: > > mojom::CoordinationUnitPtr ptr; > Platform::Current()::GetInterfaceProvider()->GetInterface(mojo::MakeRequest(&ptr)) > to set up the mojo channel. > > There is a follow up patch ongoing to use this interface inside Blink. Why can't the usage of it be coupled with this CL? Or at least made dependent? GRC is abstract enough that reviewing small patches like that in isolation does not help one form a good picture of what is happening.
> Why can't the usage of it be coupled with this CL? Or at least made dependent? > GRC is abstract enough that reviewing small patches like that in isolation > does not help one form a good picture of what is happening. I put up a change from the blink side in this patch https://codereview.chromium.org/2964043003/
https://codereview.chromium.org/2959383002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2959383002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_impl.cc:1674: base::Unretained(this))); Let's make this conditional on the GRC feature being enabled.
The CQ bit was checked by lpy@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 #2 (id:20001) has been deleted
The CQ bit was checked by lpy@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...
Thanks, I updated the patch and put interface registration in the feature check. https://codereview.chromium.org/2959383002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2959383002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_impl.cc:856: void CreateResourceCoordinatorProcessInterface( On 2017/06/29 17:24:28, nasko (slow) wrote: > nit: I'd move this to be closer to CreateMemoryCoordinatorHandle, as they are > similar type of functionality - coordination related. Done. https://codereview.chromium.org/2959383002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_impl.cc:1674: base::Unretained(this))); On 2017/06/29 21:52:46, nasko (slow) wrote: > Let's make this conditional on the GRC feature being enabled. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, though having more context in future CLs will be appreciated.
On 2017/06/30 16:18:26, nasko (out until Jul 17th) wrote: > LGTM, though having more context in future CLs will be appreciated. Thank you!
The CQ bit was checked by lpy@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498844732384890,
"parent_rev": "943e2ed8dd865bd67418babaaee0d72a20df91c8", "commit_rev":
"817370ae855c288809072aecb07e6063a13eda79"}
Message was sent while issue was closed.
Description was changed from ========== [GRC] Hook up the process-level CoordinationUnit This gets hooked up in the browser process and made available to the renderer process on request. It's created on demand, and currently won't be instantiated without the GlobalResourceCoordinator feature flag. An example of full GRC usage can be seen in https://codereview.chromium.org/2710823003 BUG=691886 ========== to ========== [GRC] Hook up the process-level CoordinationUnit This gets hooked up in the browser process and made available to the renderer process on request. It's created on demand, and currently won't be instantiated without the GlobalResourceCoordinator feature flag. An example of full GRC usage can be seen in https://codereview.chromium.org/2710823003 BUG=691886 Review-Url: https://codereview.chromium.org/2959383002 Cr-Commit-Position: refs/heads/master@{#483749} Committed: https://chromium.googlesource.com/chromium/src/+/817370ae855c288809072aecb07e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/817370ae855c288809072aecb07e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
