Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
104433003:
Attach a WebLayer to a frame element for out-of-process iframes. (Closed)
|
Created:
7 years ago by kenrb Modified:
7 years ago CC:
blink-reviews, jamesr, bemjb+rendering_chromium.org, zoltan1, eae+blinkwatch, leviw+renderwatch, abarth-chromium, blink-layers+watch_chromium.org, dglazkov+blink, jchaffraix+rendering, site-isolation-reviews_chromium.org, dcheng Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAttach a WebLayer to a frame element for out-of-process iframes.
This patch allows a WebLayer to be attached to an
HTMLFrameOwnerElement and causes it to be composited as
the content of its frame. The WebLayer will contain
compositor frame information received from another process
that has rendered the content of an out-of-process iframe.
BUG=325803
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163890
Patch Set 1 #Patch Set 2 : Moved WebLayer pointer to HTMLFrameOwnerElement #Patch Set 3 : Removed unnecessary #include #
Total comments: 1
Patch Set 4 : Support for OOP RenderFrames as well as RenderIFrames #Patch Set 5 : Moved WebLayer back to Frame again. #
Total comments: 16
Patch Set 6 : addressing review comments #
Total comments: 1
Patch Set 7 : Added missing null check #
Messages
Total messages: 36 (0 generated)
James, can you ptal?
This feels a little awkward on the Frame itself. Wouldn't it make more sense on the HTMLFrameElement for the wrapper since it's part of how that element renders?
On 2013/12/04 22:39:56, jamesr wrote: > This feels a little awkward on the Frame itself. Wouldn't it make more sense on > the HTMLFrameElement for the wrapper since it's part of how that element > renders? I had done it that way originally, putting it on HTMLFrameOwnerElement, but that felt a bit awkward to me since DOM nodes don't usually have that much information about how they are being rendered. I don't have a particularly strong opinion, but would it feel better to attach it to WebFrame? Those were the three places I could think of to put it, and Frame was just a guess.
Storing on the WebFrame won't help you a ton since you need access to it from core. I don't have a great idea what the best place to put this is.
On 2013/12/05 00:01:44, jamesr wrote: > Storing on the WebFrame won't help you a ton since you need access to it from > core. I don't have a great idea what the best place to put this is. In the oopif prototype I attached it to the FrameView, which felt pretty sane because there is a direct pointer to that from the renderer and it is very similar to how the layer is accessed for plugins. Since we won't have FrameViews for out of process iframes this won't work, though. FrameView is owned by Frame so my inclination was to just move the pointer up a level. I can put up a new patch in the morning with the pointer on HTMLFrameOwnerElement, but do you know of anybody who would have an opinion on this question?
On 2013/12/04 22:39:56, jamesr wrote: > This feels a little awkward on the Frame itself. Wouldn't it make more sense on > the HTMLFrameElement for the wrapper since it's part of how that element > renders? Done
James does this look okay to you? Is there anybody else you think should review?
Quick question, since we would need this to work for frameset + frame as well as iframe. https://codereview.chromium.org/104433003/diff/40001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/104433003/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderLayerCompositor.cpp:1676: if (frameRenderer->isRenderIFrame() && frameRenderer->node() && toHTMLFrameOwnerElement(frameRenderer->node())->platformLayer()) Is isRenderIFrame() specific to iframes? We'd like this to work for frame tags as well.
On 2013/12/06 18:00:04, creis wrote: > Quick question, since we would need this to work for frameset + frame as well as > iframe. > > https://codereview.chromium.org/104433003/diff/40001/Source/core/rendering/Re... > File Source/core/rendering/RenderLayerCompositor.cpp (right): > > https://codereview.chromium.org/104433003/diff/40001/Source/core/rendering/Re... > Source/core/rendering/RenderLayerCompositor.cpp:1676: if > (frameRenderer->isRenderIFrame() && frameRenderer->node() && > toHTMLFrameOwnerElement(frameRenderer->node())->platformLayer()) > Is isRenderIFrame() specific to iframes? We'd like this to work for frame tags > as well. Done.
On 2013/12/06 19:53:45, kenrb wrote: https://codereview.chromium.org/104433003/diff/40001/Source/core/rendering/Re... > > Source/core/rendering/RenderLayerCompositor.cpp:1676: if > > (frameRenderer->isRenderIFrame() && frameRenderer->node() && > > toHTMLFrameOwnerElement(frameRenderer->node())->platformLayer()) > > Is isRenderIFrame() specific to iframes? We'd like this to work for frame > tags > > as well. > > Done. Thanks! I'll let James finish the review; no need to wait for me.
public/ and Source/web/ lgtm but I don't know enough about the intended organization inside /core/ to really give a good review there. I don't know who does.
Adam, can you take a look here?
This CL strikes me as very strange. Why would the DOM need to know about WebLayer? eseidel is probably your best reviewer because he's been thinking about how we need to change Blink to support out-of-process iframes.
On 2013/12/07 22:11:31, abarth wrote: > This CL strikes me as very strange. Why would the DOM need to know about > WebLayer? eseidel is probably your best reviewer because he's been thinking > about how we need to change Blink to support out-of-process iframes. There is some discussion on that above. Aside from FrameView, which we don't want to keep for out-of-process iframes, the only objects in core with appropriate scope and lifetimes that I can find are Frame and HTMLFrameOwnerElement. The first iteration of this CL used Frame. Eric: any thoughts on the best way to go?
On 2013/12/09 00:11:22, kenrb wrote: > On 2013/12/07 22:11:31, abarth wrote: > > This CL strikes me as very strange. Why would the DOM need to know about > > WebLayer? eseidel is probably your best reviewer because he's been thinking > > about how we need to change Blink to support out-of-process iframes. > > There is some discussion on that above. Aside from FrameView, which we don't > want to keep for out-of-process iframes, the only objects in core with > appropriate scope and lifetimes that I can find are Frame and > HTMLFrameOwnerElement. The first iteration of this CL used Frame. > > Eric: any thoughts on the best way to go? I think the eventual plan was to have FrameProxy/RemoteFrame expose the WebLayer, so maybe it makes sense to it on Frame until that happens.
On 2013/12/09 00:47:19, dcheng wrote: > On 2013/12/09 00:11:22, kenrb wrote: > > On 2013/12/07 22:11:31, abarth wrote: > > > This CL strikes me as very strange. Why would the DOM need to know about > > > WebLayer? eseidel is probably your best reviewer because he's been thinking > > > about how we need to change Blink to support out-of-process iframes. > > > > There is some discussion on that above. Aside from FrameView, which we don't > > want to keep for out-of-process iframes, the only objects in core with > > appropriate scope and lifetimes that I can find are Frame and > > HTMLFrameOwnerElement. The first iteration of this CL used Frame. > > > > Eric: any thoughts on the best way to go? > > I think the eventual plan was to have FrameProxy/RemoteFrame expose the > WebLayer, so maybe it makes sense to it on Frame until that happens. put it on Frame*
On Sunday, December 8, 2013, wrote: > On 2013/12/09 00:47:19, dcheng wrote: > >> On 2013/12/09 00:11:22, kenrb wrote: >> > On 2013/12/07 22:11:31, abarth wrote: >> > > This CL strikes me as very strange. Why would the DOM need to know >> about >> > > WebLayer? eseidel is probably your best reviewer because he's been >> > thinking > >> > > about how we need to change Blink to support out-of-process iframes. >> > >> > There is some discussion on that above. Aside from FrameView, which we >> don't >> > want to keep for out-of-process iframes, the only objects in core with >> > appropriate scope and lifetimes that I can find are Frame and >> > HTMLFrameOwnerElement. The first iteration of this CL used Frame. >> > >> > Eric: any thoughts on the best way to go? >> > > I think the eventual plan was to have FrameProxy/RemoteFrame expose the >> WebLayer, so maybe it makes sense to it on Frame until that happens. >> > > put it on Frame* That sounds like a better plan. Otherwise, you're at the mercy of the garbage collector and have to deal with issues like elements that aren't in a document. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
New patch up. eseidel: PTAL?
We really need to build an abstraction around Frame first. This is a RemoteFrame concept.
Sorry I was horribly slow to respond. I'm working on OOPI (finally) this week. Happy to discuss more in the morning!
On 2013/12/10 07:27:49, eseidel wrote: > We really need to build an abstraction around Frame first. This is a > RemoteFrame concept. This is true. Part of the problem is that we don't have the pieces in Blink. We want to land this before all of the Blink infrastructure is in place, though. Being able to render iframes out of process makes it easier to proceed in a lot of the areas we are working on. Do you want me to put this on FrameView as a temporary solution, on the assumption that the creation of RemoteFrame will be before or at the same time as the removal of FrameViews from them?
No, it correctly belongs on Frame. However, this part of OOPI has no value to the rest of Blink when not enabled. Do we have a compile-time define for oopi? Normally we would do this sort of experimentation in a branch? Is this going to be enough to get oopi to a shipping state?
Sorry my first sentence was overly terse. I don't think this has anything to do with FrameView. FrameView is a concept for Local Frame objects to do their actual rendering. This is a concept for reaching WebLayers from another frame, correct? I presume that the Frame objects which have one of these will be otherwise empty (or largely?) Maybe I'm misunderstanding.
Lets just create a RemoteFrame concept today. I'm nearly done with the FocusController patch I'm working on and am happy to help move FrameTree logic over to Frame and add a RemoteFrame class. Alternatively I would accept adding a RemoteFrame pointer onto Frame and having it be a helper object on Frame for now until the rest can be untwisted.
Typing too quickly. In my last comment I meant "move the FrameTree logic over to Page" not Frame. Last we talked in person I think we agreed to move FrameTree to the other side of the embedding boundary.
On 2013/12/10 16:56:50, eseidel wrote: > Sorry my first sentence was overly terse. I don't think this has anything to do > with FrameView. FrameView is a concept for Local Frame objects to do their > actual rendering. This is a concept for reaching WebLayers from another frame, > correct? > The WebLayer is created in the local process and populated with a reference to a texture or a compositor frame that was created by another process. > I presume that the Frame objects which have one of these will be otherwise empty > (or largely?) Maybe I'm misunderstanding. Yes that is correct. Having a non-NULL platformLayer() in this case does correspond to being a RemoteFrame, which means its content was rendered elsewhere. If you are going to add RemoteFrame now then I can wait and rebase this CL. We don't have a compiler define, we have just been using a runtime flag to enable site isolation (--site-per-process).
I don't want to keep blocking you. Give these less generic names so others won't be tempted to use this experimental code, and I'm OK with you landing this approach. Obviously we need to create a real RemoteFrame, etc, but that's blocked on work from Daniel or I. https://codereview.chromium.org/104433003/diff/80001/Source/core/frame/Frame.h File Source/core/frame/Frame.h (right): https://codereview.chromium.org/104433003/diff/80001/Source/core/frame/Frame.... Source/core/frame/Frame.h:159: void setPlatformLayer(blink::WebLayer* platformLayer) { m_platformLayer = platformLayer; } I would give this a less generic name. setRemotePlatformLayer? I would also ideally compile-time guard this and only have it on for OOPI enabled builds. We don't generally do large experiments like this on trunk (e.g. oilpan). OOPI's design doesn't seem solidified and we're still experimenting here. https://codereview.chromium.org/104433003/diff/80001/Source/core/frame/Frame.... Source/core/frame/Frame.h:247: // This layer is for frames whose contents are being rendered in a separate process. You could just convey that in the member name. https://codereview.chromium.org/104433003/diff/80001/Source/core/rendering/Co... File Source/core/rendering/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/104433003/diff/80001/Source/core/rendering/Co... Source/core/rendering/CompositedLayerMapping.cpp:507: } else if ((renderer->isRenderIFrame() || renderer->isFrame()) && toHTMLFrameOwnerElement(renderer->node())->contentFrame() && toHTMLFrameOwnerElement(renderer->node())->contentFrame()->platformLayer()) { Don't we have a frame base class for these both? Maybe you just want to just grab the node and see if the node is a frameOwnerElement directly? https://codereview.chromium.org/104433003/diff/80001/Source/core/rendering/Co... Source/core/rendering/CompositedLayerMapping.cpp:508: m_graphicsLayer->setContentsToPlatformLayer(toHTMLFrameOwnerElement(renderer->node())->contentFrame()->platformLayer()); You just if-checked this long lookup, maybe you wanted to save it in a local? https://codereview.chromium.org/104433003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/104433003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayerCompositor.cpp:1676: if ((frameRenderer->isRenderIFrame() || frameRenderer->isFrame()) && frameRenderer->node() && toHTMLFrameOwnerElement(frameRenderer->node())->contentFrame() && toHTMLFrameOwnerElement(frameRenderer->node())->contentFrame()->platformLayer()) Same problem as before. Maybe we should have a helper: PlatformLayer* layer = remoteWebLayer(renderer) https://codereview.chromium.org/104433003/diff/80001/Source/web/WebFrameImpl.cpp File Source/web/WebFrameImpl.cpp (right): https://codereview.chromium.org/104433003/diff/80001/Source/web/WebFrameImpl.... Source/web/WebFrameImpl.cpp:561: if (frame()) { early return is generally preferred: if (!frame()) return; https://codereview.chromium.org/104433003/diff/80001/Source/web/WebFrameImpl.... Source/web/WebFrameImpl.cpp:567: frame()->ownerElement()->setNeedsStyleRecalc(WebCore::SubtreeStyleChange, WebCore::StyleChangeFromRenderer); I don't think you want to be scheduling your own style recalc here. What are you trying to do? https://codereview.chromium.org/104433003/diff/80001/public/web/WebFrame.h File public/web/WebFrame.h (right): https://codereview.chromium.org/104433003/diff/80001/public/web/WebFrame.h#ne... public/web/WebFrame.h:162: virtual void setWebLayer(blink::WebLayer*) = 0; I would give this a less generic name. void setOutOfPRocessWebLayer? setRemoteWebLayer?
https://codereview.chromium.org/104433003/diff/80001/Source/core/frame/Frame.h File Source/core/frame/Frame.h (right): https://codereview.chromium.org/104433003/diff/80001/Source/core/frame/Frame.... Source/core/frame/Frame.h:159: void setPlatformLayer(blink::WebLayer* platformLayer) { m_platformLayer = platformLayer; } On 2013/12/11 18:50:01, eseidel wrote: > I would give this a less generic name. setRemotePlatformLayer? > > I would also ideally compile-time guard this and only have it on for OOPI > enabled builds. We don't generally do large experiments like this on trunk > (e.g. oilpan). OOPI's design doesn't seem solidified and we're still > experimenting here. Is the oilpan comparison apt? I think our design is fairly solidified, but as with all software development, the act of reducing the design to code will involve some process of discovery and experimentation. A compile-time guard puts a substantial burden on development: you now need a custom builder for any kind of test automation, which in practice may mean that we simply won't have tests running on trybots or builders until we are ready to drop the #define. It also adds significant friction to switching a local build into the OOPIF mode, which may inhibit our ability to successfully convince domain experts to jump in and offer a hand in development. In my opinion, protecting the codepath with a runtime flag would be a better choice, provided of course that this is acceptable from a regression-risk perspective.
I see oilpan as a big bet. I see OOPI as a big bet. I think both are the future, but neither are as certain as "omg fix this regression in X so we can ship 31 now". There is a lot of development going on in Blink and big-bets ideally are taken on branches when possible. One of the big wins we got with Blink was deleting all the code we weren't using in the shipping product. Actively developed features small enough to hide behind a runtime flag were left that way, and the rest was deleted. I'm not suggesting we move OOPI to a branch. But I am noting that adding effectively dead code (currently and for quite a while to come) is generally a bad thing. At least to core objects which many others hack on. I'm OK with this going forward to unblock you. dcheng and I are working on the rest of the Blink rearchitecting so that this code can move somewhere where 99% of blink hackers won't have to know it exists. Again, my apologies for being so slow with reviews this week.
On 2013/12/11 21:43:35, eseidel wrote: > > I'm not suggesting we move OOPI to a branch. But I am noting that adding > effectively dead code (currently and for quite a while to come) is generally a > bad thing. At least to core objects which many others hack on. > The vast majority of the changes for site isolation in Blink is refactoring: changing ownership, locations, and lifetimes. It doesn't seem like this can reasonably go behind a guard, and the remainder of it is code that should soon be very localized, once RemoteFrame is in place. > I'm OK with this going forward to unblock you. dcheng and I are working on the > rest of the Blink rearchitecting so that this code can move somewhere where 99% > of blink hackers won't have to know it exists. > Ok, thanks.
https://codereview.chromium.org/104433003/diff/80001/Source/core/frame/Frame.h File Source/core/frame/Frame.h (right): https://codereview.chromium.org/104433003/diff/80001/Source/core/frame/Frame.... Source/core/frame/Frame.h:247: // This layer is for frames whose contents are being rendered in a separate process. On 2013/12/11 18:50:01, eseidel wrote: > You could just convey that in the member name. Done. https://codereview.chromium.org/104433003/diff/80001/Source/core/rendering/Co... File Source/core/rendering/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/104433003/diff/80001/Source/core/rendering/Co... Source/core/rendering/CompositedLayerMapping.cpp:507: } else if ((renderer->isRenderIFrame() || renderer->isFrame()) && toHTMLFrameOwnerElement(renderer->node())->contentFrame() && toHTMLFrameOwnerElement(renderer->node())->contentFrame()->platformLayer()) { On 2013/12/11 18:50:01, eseidel wrote: > Don't we have a frame base class for these both? Maybe you just want to just > grab the node and see if the node is a frameOwnerElement directly? The base class (RenderFrameBase) was removed a few months ago because it turned out to have only code that wasn't relevant to Blink. But I've implemented your suggestion. https://codereview.chromium.org/104433003/diff/80001/Source/core/rendering/Co... Source/core/rendering/CompositedLayerMapping.cpp:508: m_graphicsLayer->setContentsToPlatformLayer(toHTMLFrameOwnerElement(renderer->node())->contentFrame()->platformLayer()); On 2013/12/11 18:50:01, eseidel wrote: > You just if-checked this long lookup, maybe you wanted to save it in a local? Done. https://codereview.chromium.org/104433003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/104433003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayerCompositor.cpp:1676: if ((frameRenderer->isRenderIFrame() || frameRenderer->isFrame()) && frameRenderer->node() && toHTMLFrameOwnerElement(frameRenderer->node())->contentFrame() && toHTMLFrameOwnerElement(frameRenderer->node())->contentFrame()->platformLayer()) On 2013/12/11 18:50:01, eseidel wrote: > Same problem as before. Maybe we should have a helper: > > PlatformLayer* layer = remoteWebLayer(renderer) I don't see a convenient place to put that helper. The lookups are in different places. https://codereview.chromium.org/104433003/diff/80001/Source/web/WebFrameImpl.cpp File Source/web/WebFrameImpl.cpp (right): https://codereview.chromium.org/104433003/diff/80001/Source/web/WebFrameImpl.... Source/web/WebFrameImpl.cpp:561: if (frame()) { On 2013/12/11 18:50:01, eseidel wrote: > early return is generally preferred: > if (!frame()) return; Done. https://codereview.chromium.org/104433003/diff/80001/Source/web/WebFrameImpl.... Source/web/WebFrameImpl.cpp:567: frame()->ownerElement()->setNeedsStyleRecalc(WebCore::SubtreeStyleChange, WebCore::StyleChangeFromRenderer); On 2013/12/11 18:50:01, eseidel wrote: > I don't think you want to be scheduling your own style recalc here. What are > you trying to do? The goal is to trigger a layout so that the new layer gets picked up and composited. Is there a better way to do that? https://codereview.chromium.org/104433003/diff/80001/public/web/WebFrame.h File public/web/WebFrame.h (right): https://codereview.chromium.org/104433003/diff/80001/public/web/WebFrame.h#ne... public/web/WebFrame.h:162: virtual void setWebLayer(blink::WebLayer*) = 0; On 2013/12/11 18:50:01, eseidel wrote: > I would give this a less generic name. void setOutOfPRocessWebLayer? > setRemoteWebLayer? Done, I used the latter.
I still suspect you may be doing too much work with your recalc style. Style invalidation is most often just an implmentation detail of the rendering tree, and invalidating manually can be a source of bugs/overinvalidations. Regardless. lgtm. we can iterate from here. https://codereview.chromium.org/104433003/diff/100001/Source/web/WebFrameImpl... File Source/web/WebFrameImpl.cpp (right): https://codereview.chromium.org/104433003/diff/100001/Source/web/WebFrameImpl... Source/web/WebFrameImpl.cpp:569: frame()->ownerElement()->setNeedsStyleRecalc(WebCore::SubtreeStyleChange, WebCore::StyleChangeFromRenderer); Presumably you want the layer tree to be re-created? Are you sure you want a full sub-tree recalc?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kenrb@chromium.org/104433003/100001
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kenrb@chromium.org/104433003/120001
Message was sent while issue was closed.
Change committed as 163890 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
