New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 775827 link

Starred by 4 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Mojofy content/common/page_messages.h

Project Member Reported by slangley@chromium.org, Oct 18 2017

Issue description

- Convert all IPCs to mojo
- Delete content/common/page_messages.h
- Onion Soup files into //blink
 

Comment 1 by w...@chromium.org, Nov 30 2017

Cc: sa...@chromium.org nasko@chromium.org lfg@chromium.org
Adding some people who may be able to help with this. Please add others with context.

Today PageMsgs are sent via WebContents::SendPageMessage(), which finds the root frame and calls RenderFrameHostManager::SendPageMessage(), which sends the message via each RenderFrameHost and RenderFrameProxyHost corresponding to that frame (i.e., each process hosting a frame in the tree).

The handlers in RenderFrameImpl and RenderFrameProxy both pass the messages straight to their associated RenderView. And the messages are handled either by code in the RenderView or RenderWidget.

Three options I can think of:
1) A direct mojoification: RenderFrameImpl and RenderFrameProxy will both implement a "PageControl" interface, which delegates to RenderView. Requires adding support for vending mojo interfaces from RenderFrameProxy.

2) Skip the Frames and go straight to RenderView. RenderView implements a "PageControl" interface. WebContentsImpl finds all RenderViewHosts that are associated with the root frame, gets a PageControl interface and calls the relevant interface method. Requires adding support for vending mojo interfaces from RenderView but RenderView is deprecated so we shouldn't do that?

3) Move all the code that handles the messages to RenderWidget. RenderWidget implements a "PageControl" interface. WebContentsImpl finds all RenderWidgetHosts that are associated with the root frame, gets a PageControl interface and calls the relevant interface method. Requires adding support for vending mojo interfaces from RenderWidget. Potentially difficult refactor to move everything to RenderWidget: e.g., OnShown() calls webview()->SetVisibilityState().

Comment 2 by lfg@chromium.org, Dec 6 2017

Cc: kenrb@chromium.org
Sorry for the delay replying, I was OOO the last few days.

Option 1 seems like the best option here. Option 2 has the issue of deprecating the RenderView IPCs you already mentioned, and option 3 conflicts with the effort to split RenderView and RenderWidget (see issue 419087).

Comment 3 by w...@chromium.org, Dec 6 2017

No problem, thanks for the feedback. I was leaning towards Option 1 too, so I'll go ahead with that one for now. 

Comment 4 by kenrb@chromium.org, Dec 7 2017

Why are we deprecating RenderView? If I understand the above correctly, option 1 means adding interface implementations to RenderFrameImpl and RenderFrameProxy, but each will only ever receive connections when they represent the main frame, and each will proxy to the appropriate RenderView methods.

I don't think we generally want to migrate RenderView state to RenderFrameImpl/RenderFrameProxy, so why would this be preferred over having RenderView implement the interface? What happens if/when RenderView gets removed?

Comment 5 by w...@chromium.org, Dec 8 2017

Yeah, you understood option 1 correctly (with the minor detail that RFI and RFP will probably share a common PageControlImpl that delegates to the RVI). 

My only context on RenderView being deprecated is the class level comment that pretty strongly discourages adding anything to it https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.h?type=cs&sq=package:chromium&l=107

Comment 6 by kenrb@chromium.org, Dec 8 2017

Cc: creis@chromium.org
creis or nasko, any thoughts on below?

My understanding of that notice was to try to get people adding new code to RenderFrameImpl and RenderWidget where possible, since most new features belong in one of those. But I think a small subset of the things in RenderViewImpl don't belong in frames or widgets -- specifically, the page control things.

To me, it makes sense to add this interface implementation to RenderViewImpl. If we later end up removing or renaming RenderViewImpl then the page control code would still belong in an object with that scope.

Comment 7 by w...@chromium.org, Dec 13 2017

Friendly ping creis, nasko.

Comment 8 by lfg@chromium.org, Dec 15 2017

Components: Internals>Sandbox>SiteIsolation
I think Ken's suggestion makes sense. We'll still need a page-level object interacting with the various renderers, and RenderView{,Host} seems like the best candidate to become this object. One of the ideas we had in the past was to rename RenderView to RenderPage to make this more explicitly, but no one finished design work on this area, since there are still several more important issues.

The deprecation of RenderView{,Host} is more about not using the same IPC channel for things that are widget-related or frame-related. While the recommendation so far has been to use WebContentsImpl for page-related things, I think having RenderView fulfilling at least some of this role would make sense.

This is, however, somewhat different than how things are done today, so you may run into some issues you'll need to address while designing this solution.

Comment 9 by w...@chromium.org, Jan 9 2018

Cc: w...@chromium.org
Owner: ----
Status: Available (was: Assigned)
Unfortunately I don't think I'll have time to finish the design of this before starting on a new team, so I'll leave it for someone else to pick up, and I'll put a TODO in the code to link back here.
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/07c71f41d989e9da75e0f211dec182b545496e56

commit 07c71f41d989e9da75e0f211dec182b545496e56
Author: Chris Watkins <watk@chromium.org>
Date: Tue Jan 09 05:44:46 2018

Add a TODO with bug trackback for mojoifying page_messages.h

Bug: 775827
Change-Id: I4b446c0ea798a2a95a0351a0163aa3f0b3f09ff5
TBR: nasko@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/855717
Commit-Queue: Chris Watkins <watk@chromium.org>
Reviewed-by: Chris Watkins <watk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527911}
[modify] https://crrev.com/07c71f41d989e9da75e0f211dec182b545496e56/content/common/page_messages.h

Comment 11 by creis@chromium.org, Feb 26 2018

Cc: dcheng@chromium.org
Sorry for the delayed reply-- I think Ken is right that we may want to evolve RenderView into RenderPage, rather than entirely removing RenderView.  There are certain page-level messages and concepts that need to be preserved, and don't belong on frames or widgets.  Let's update the deprecation message accordingly.

Sign in to add a comment