Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2483593002:
RenderWidget: hoist synchronous IPC out of Init methods. (Closed)
|
Created:
4 years, 1 month ago by ncarter (slow) Modified:
4 years, 1 month ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRenderWidget: hoist synchronous IPC (needed to allocate routing IDs) out of Init methods.
RenderWidget now requires two Init phases that are always called, and always in this order:
- InitRoutingID (formerly SetRoutingID)
- Init (formerly DoInit).
Two distinct Init phases are needed to solve an ordering problem in RenderView.
Instead of Init being parameterized on a callback to allocate a
route ID, just do the IPC outside of Init, and pass the result in.
BUG=627852
Committed: https://crrev.com/ab31e24367180e818edd6065f8b43277d99e6944
Cr-Commit-Position: refs/heads/master@{#431386}
Patch Set 1 #Patch Set 2 : Rework #
Total comments: 4
Patch Set 3 : Rebase to pick up render_widget_fullscreen mojification #Patch Set 4 : Fix correctness issues. #Patch Set 5 : Fix unittest compile. #Patch Set 6 : Fix unittest compile. #
Total comments: 4
Patch Set 7 : Handle IPC failure #
Dependent Patchsets: Messages
Total messages: 40 (28 generated)
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) 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_...)
nick@chromium.org changed reviewers: + jcivelli@chromium.org
Hi Jay, Here's a sketch of the idea we were discussing. Please let me know if you're aware of a reason this wouldn't work. Thanks! https://codereview.chromium.org/2483593002/diff/20001/content/renderer/render... File content/renderer/render_widget.cc (left): https://codereview.chromium.org/2483593002/diff/20001/content/renderer/render... content/renderer/render_widget.cc:406: CreateWidgetCallback create_widget_callback) { Instead of passing a callback, we just have the caller execute the closure and pass the result as |widget_routing_id|. https://codereview.chromium.org/2483593002/diff/20001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2483593002/diff/20001/content/renderer/render... content/renderer/render_widget.cc:291: int32_t routing_id = MSG_ROUTING_NONE; The code in this function is a result of inlining the function it previously called: RenderWidget::Init(int32_t opener_id) https://codereview.chromium.org/2483593002/diff/20001/content/renderer/render... content/renderer/render_widget.cc:295: opener_id, popup_type, &routing_id); This line used to be in RenderWidget::CreateWidget, which was the body of the callback passed to DoInit by the old Init function. Instead of putting it in a callback, we just do the IPC here before calling into init. https://codereview.chromium.org/2483593002/diff/20001/content/renderer/render... content/renderer/render_widget.cc:393: void RenderWidget::Init(int32_t widget_routing_id, I eliminated the single-arg ::Init function, and then renamed the old DoInit() function to Init().
Nice clean-up. Overall looks good to me.
The CQ bit was checked by nick@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...
On 2016/11/07 19:28:30, Jay Civelli wrote: > Nice clean-up. Overall looks good to me. Great, I'll figure out how to land this then. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by nick@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...
Description was changed from ========== Sketch of hoisting sync IPC out of DoInit. BUG=None ========== to ========== RenderWidget: hoist synchronous IPC (needed to allocate routing ID) out of Init methods. RenderWidget now requires two Init phases: - InitRoutingID (formerly SetRoutingID) - Init (formerly DoInit). Instead of Init being parameterized on a callback to allocate a route ID, just do the IPC outside of Init, and pass the result in. BUG=627852 ==========
Description was changed from ========== RenderWidget: hoist synchronous IPC (needed to allocate routing ID) out of Init methods. RenderWidget now requires two Init phases: - InitRoutingID (formerly SetRoutingID) - Init (formerly DoInit). Instead of Init being parameterized on a callback to allocate a route ID, just do the IPC outside of Init, and pass the result in. BUG=627852 ========== to ========== RenderWidget: hoist synchronous IPC (needed to allocate routing ID) out of Init methods. RenderWidget now requires two Init phases that are always called, and always in this order: - InitRoutingID (formerly SetRoutingID) - Init (formerly DoInit). Two distinct Init phases are needed to solve an ordering problem in RenderView. Instead of Init being parameterized on a callback to allocate a route ID, just do the IPC outside of Init, and pass the result in. BUG=627852 ==========
Description was changed from ========== RenderWidget: hoist synchronous IPC (needed to allocate routing ID) out of Init methods. RenderWidget now requires two Init phases that are always called, and always in this order: - InitRoutingID (formerly SetRoutingID) - Init (formerly DoInit). Two distinct Init phases are needed to solve an ordering problem in RenderView. Instead of Init being parameterized on a callback to allocate a route ID, just do the IPC outside of Init, and pass the result in. BUG=627852 ========== to ========== RenderWidget: hoist synchronous IPC (needed to allocate routing IDs) out of Init methods. RenderWidget now requires two Init phases that are always called, and always in this order: - InitRoutingID (formerly SetRoutingID) - Init (formerly DoInit). Two distinct Init phases are needed to solve an ordering problem in RenderView. Instead of Init being parameterized on a callback to allocate a route ID, just do the IPC outside of Init, and pass the result in. BUG=627852 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nick@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.
nick@chromium.org changed reviewers: + lfg@chromium.org
+lfg, please review https://codereview.chromium.org/2483593002/diff/100001/content/renderer/rende... File content/renderer/render_widget.cc (left): https://codereview.chromium.org/2483593002/diff/100001/content/renderer/rende... content/renderer/render_widget.cc:292: if (widget->Init(opener_id)) { // adds reference on success. The one-arg Init() function has been inlined into RenderWidget::Create. https://codereview.chromium.org/2483593002/diff/100001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2483593002/diff/100001/content/renderer/rende... content/renderer/render_widget.cc:294: opener_id, popup_type, &routing_id); This comes from the old RenderWidget::CreateWidget() function in the old code.
Overall looks good, just one question. https://codereview.chromium.org/2483593002/diff/100001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2483593002/diff/100001/content/renderer/rende... content/renderer/render_widget.cc:296: if (routing_id == MSG_ROUTING_NONE) Is this for error handling when the IPC fails? The sync IPC should never return MSG_ROUTING_NONE if it's executed.
The CQ bit was checked by nick@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/2483593002/diff/100001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2483593002/diff/100001/content/renderer/rende... content/renderer/render_widget.cc:296: if (routing_id == MSG_ROUTING_NONE) On 2016/11/09 23:27:30, lfg wrote: > Is this for error handling when the IPC fails? The sync IPC should never return > MSG_ROUTING_NONE if it's executed. I read up on this, and asked on chromium-mojo. Mojo Sync IPCs can fail, in the same cases legacy IPCs did -- when the pipe is broken. In this case the method returns false and doesn't touch the value of |routing_id|. It looks like the preferred idiom is to check the return value of CreateNewWidget, so I've switched to that.
On 2016/11/10 18:44:45, ncarter wrote: > https://codereview.chromium.org/2483593002/diff/100001/content/renderer/rende... > File content/renderer/render_widget.cc (right): > > https://codereview.chromium.org/2483593002/diff/100001/content/renderer/rende... > content/renderer/render_widget.cc:296: if (routing_id == MSG_ROUTING_NONE) > On 2016/11/09 23:27:30, lfg wrote: > > Is this for error handling when the IPC fails? The sync IPC should never > return > > MSG_ROUTING_NONE if it's executed. > > I read up on this, and asked on chromium-mojo. Mojo Sync IPCs can fail, in the > same cases legacy IPCs did -- when the pipe is broken. In this case the method > returns false and doesn't touch the value of |routing_id|. > > It looks like the preferred idiom is to check the return value of > CreateNewWidget, so I've switched to that. lgtm
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 nick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== RenderWidget: hoist synchronous IPC (needed to allocate routing IDs) out of Init methods. RenderWidget now requires two Init phases that are always called, and always in this order: - InitRoutingID (formerly SetRoutingID) - Init (formerly DoInit). Two distinct Init phases are needed to solve an ordering problem in RenderView. Instead of Init being parameterized on a callback to allocate a route ID, just do the IPC outside of Init, and pass the result in. BUG=627852 ========== to ========== RenderWidget: hoist synchronous IPC (needed to allocate routing IDs) out of Init methods. RenderWidget now requires two Init phases that are always called, and always in this order: - InitRoutingID (formerly SetRoutingID) - Init (formerly DoInit). Two distinct Init phases are needed to solve an ordering problem in RenderView. Instead of Init being parameterized on a callback to allocate a route ID, just do the IPC outside of Init, and pass the result in. BUG=627852 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== RenderWidget: hoist synchronous IPC (needed to allocate routing IDs) out of Init methods. RenderWidget now requires two Init phases that are always called, and always in this order: - InitRoutingID (formerly SetRoutingID) - Init (formerly DoInit). Two distinct Init phases are needed to solve an ordering problem in RenderView. Instead of Init being parameterized on a callback to allocate a route ID, just do the IPC outside of Init, and pass the result in. BUG=627852 ========== to ========== RenderWidget: hoist synchronous IPC (needed to allocate routing IDs) out of Init methods. RenderWidget now requires two Init phases that are always called, and always in this order: - InitRoutingID (formerly SetRoutingID) - Init (formerly DoInit). Two distinct Init phases are needed to solve an ordering problem in RenderView. Instead of Init being parameterized on a callback to allocate a route ID, just do the IPC outside of Init, and pass the result in. BUG=627852 Committed: https://crrev.com/ab31e24367180e818edd6065f8b43277d99e6944 Cr-Commit-Position: refs/heads/master@{#431386} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ab31e24367180e818edd6065f8b43277d99e6944 Cr-Commit-Position: refs/heads/master@{#431386}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2496633004/ by dpranke@chromium.org. The reason for reverting is: Well, this is dcheck'ing all over the place, congrats :). Any number of builders are failing as a result, so I'm reverting this. See https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11%20%28... for the main waterfall builder's failures..
Message was sent while issue was closed.
On 2016/11/11 00:30:09, Dirk Pranke wrote: > A revert of this CL (patchset #7 id:120001) has been created in > https://codereview.chromium.org/2496633004/ by mailto:dpranke@chromium.org. > > The reason for reverting is: Well, this is dcheck'ing all over the place, > congrats :). > > Any number of builders are failing as a result, so I'm reverting this. > > See > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11%20%28... > for the main waterfall builder's failures.. Thanks for the revert. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
