Enable resize and layout for frames with RemoteFrame ancestors

Enable resize and layout for frames with RemoteFrame ancestors

In order to support frame tree replication across processes, frames
must be able to render correctly even when they have ancestor frames
out of process. This will require a refactoring of WebViewImpl, but
to enable immediate progress on this goal we add the assumption to
WebViewImpl that the first LocalFrame found in a frame tree traversal
should be treated as a local root, and used instead of the actual
main frame in some cases.

BUG=346764
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178066

[kenrb, 2014-07-10 21:06:29]

Daniel: Can you please review with respect to the path forward for frame tree
replication?

Eric: Do you see any problem with what we are trying to do here?

For context, we realized recently that we need to have a WebWidget independent
of WebViewImpl in Source/web, because there should only be one WebViewImpl
associated with Page, but we need WebWidgets wherever we have disconnected
in-process pieces of the frame tree. (e.g. a site from origin A creates 2
iframes from origin B, they get rendered in the same process but do not share a
drawing surface).

We are trying to get frame tree replication working, however, before the
WebWidget refactor can happen. The WebViewImpl pieces here are a temporary
workaround. The rest should be permanent, however, and reflect behavior when
Page::mainFrame() is a remote frame.

[dcheng, 2014-07-10 21:14:55]

https://codereview.chromium.org/380303003/diff/20001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/380303003/diff/20001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:370: bool isMainFrame = this->isLocalRoot();
Should we rename the bool here too?

https://codereview.chromium.org/380303003/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/380303003/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:843: LocalFrame* mainFrame =
m_frame->localFrameRoot();
Why are these changes necessary for layout/resize?

https://codereview.chromium.org/380303003/diff/20001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.h (right):

https://codereview.chromium.org/380303003/diff/20001/Source/core/page/PageAni...
Source/core/page/PageAnimator.h:22: void
updateLayoutAndStyleForPainting(LocalFrame* rootFrame);
LocalFrame&, as I assume we're not allowed to pass in a null pointer here.

https://codereview.chromium.org/380303003/diff/20001/Source/web/PageWidgetDel...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/380303003/diff/20001/Source/web/PageWidgetDel...
Source/web/PageWidgetDelegate.cpp:77: if (!rootFrame) {
How can we end up with a null pointer here? I only see one call to this in
WebViewImpl.cpp, and it passes in a local root (which by definition, can never
be null). Is there somewhere else calling this and passing in 0?

https://codereview.chromium.org/380303003/diff/20001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/380303003/diff/20001/Source/web/WebViewImpl.h...
Source/web/WebViewImpl.h:319: WebLocalFrameImpl* localFrameRoot();
I think we should name this something that discourages people from using it
while we're working on upstreaming stuff. Maybe randomLocalFrameRoot() to
reflect that it really just grabs a random one? Also, this should probably
return a reference if we can convince the compiler that we'll never fall out of
the for loop. It should be logically impossible to have zero local frames in a
frame tree.

[kenrb, 2014-07-11 16:06:18]

https://codereview.chromium.org/380303003/diff/20001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/380303003/diff/20001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:370: bool isMainFrame = this->isLocalRoot();
On 2014/07/10 21:14:54, dcheng (OOO) wrote:
> Should we rename the bool here too?

Done.

https://codereview.chromium.org/380303003/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/380303003/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:843: LocalFrame* mainFrame =
m_frame->localFrameRoot();
On 2014/07/10 21:14:54, dcheng (OOO) wrote:
> Why are these changes necessary for layout/resize?

They are not, but I think this one line change fixes hit testing for the remote
mainFrame case. Since it is related, I put it in here.

https://codereview.chromium.org/380303003/diff/20001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.h (right):

https://codereview.chromium.org/380303003/diff/20001/Source/core/page/PageAni...
Source/core/page/PageAnimator.h:22: void
updateLayoutAndStyleForPainting(LocalFrame* rootFrame);
On 2014/07/10 21:14:54, dcheng (OOO) wrote:
> LocalFrame&, as I assume we're not allowed to pass in a null pointer here.

Actually I am allowing passing null as a signal to revert to the original
behavior: layout from page->mainFrame(). This is for WebPagePopupImpl layout.

https://codereview.chromium.org/380303003/diff/20001/Source/web/PageWidgetDel...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/380303003/diff/20001/Source/web/PageWidgetDel...
Source/web/PageWidgetDelegate.cpp:77: if (!rootFrame) {
On 2014/07/10 21:14:54, dcheng (OOO) wrote:
> How can we end up with a null pointer here? I only see one call to this in
> WebViewImpl.cpp, and it passes in a local root (which by definition, can never
> be null). Is there somewhere else calling this and passing in 0?

See above.

https://codereview.chromium.org/380303003/diff/20001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/380303003/diff/20001/Source/web/WebViewImpl.h...
Source/web/WebViewImpl.h:319: WebLocalFrameImpl* localFrameRoot();
On 2014/07/10 21:14:55, dcheng (OOO) wrote:
> I think we should name this something that discourages people from using it
> while we're working on upstreaming stuff. Maybe randomLocalFrameRoot() to
> reflect that it really just grabs a random one? Also, this should probably
> return a reference if we can convince the compiler that we'll never fall out
of
> the for loop. It should be logically impossible to have zero local frames in a
> frame tree.

Is the last statement true even while the frame tree is being constructed? I
wasn't sure if there might be race conditions.

I have appended 'temporary' to the name to discourage use for other purposes.

[eseidel, 2014-07-11 16:33:04]

I'm glad to see this taking shape.

rslgtm, I don't need to see this again.

However I left you some comments about the legibility of these changes.  I'm
sure this design is in flux, I just hope one of the outcomes of you all
re-writing frame ownership being that its all much easier for the casual blink
hacker to understand. :)

Thanks.

https://codereview.chromium.org/380303003/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1164: if (m_frame->owner() &&
!hasCompositedContent() && m_frame->page() &&
m_frame->localFrameRoot()->view()->hasCompositedContent())
Folks more active in the compositor or repaint system than I could better
comment on this codepath.

https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
Source/web/PageWidgetDelegate.cpp:74: if (!page)
Why is it valid to call this method with a null page?

https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
Source/web/PageWidgetDelegate.cpp:77: if (!rootFrame) {
Why not make this a default =0 argument then?  Or is the goal that eventually
teh Page argument will be gone from this function and thus rootFrame makes sense
to be required?

https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
Source/web/PageWidgetDelegate.cpp:78: if (!page->mainFrame() ||
!page->mainFrame()->isLocalFrame())
How can a page not have a mainFrame?  Or a page's mainFrame be remote?

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebPagePopupI...
File Source/web/WebPagePopupImpl.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebPagePopupI...
Source/web/WebPagePopupImpl.cpp:296: PageWidgetDelegate::layout(m_page.get(),
0);
Initially Confused by the 0.  I now understand it, but it's still rather
unreadable. Waht's the long term vision for this call?

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1631: // FIXME: FastTextAutosizer does not yet
support out-of-process frames.
Might want to keep +pdr in the loop.

[pdr., 2014-07-11 17:14:52]

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1631: // FIXME: FastTextAutosizer does not yet
support out-of-process frames.
On 2014/07/11 16:33:03, eseidel wrote:
> Might want to keep +pdr in the loop.

Nice catch eseidel!

@kenrb, please file a bug for this.

[skobes, 2014-07-11 19:05:28]

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1632: if (!mainFrameImpl()->frame()->isLocalFrame())
{
This check doesn't make sense in the context of the comment.  Why do we want to
construct the DeferUpdatePageInfo when the main frame is NOT local?

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1634: FastTextAutosizer::DeferUpdatePageInfo
deferUpdatePageInfo(page());
Putting this inside the brace scope of the 'if' makes it a nop.  Was that the
intent?

[kenrb, 2014-07-11 20:03:34]

leviw@: Are you able to review the FrameView changes here?

https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
Source/web/PageWidgetDelegate.cpp:74: if (!page)
On 2014/07/11 16:33:03, eseidel wrote:
> Why is it valid to call this method with a null page?

It was added explicitly in r169672. I am not certain, but it looks like it is
needed for some unit tests.

https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
Source/web/PageWidgetDelegate.cpp:77: if (!rootFrame) {
On 2014/07/11 16:33:03, eseidel wrote:
> Why not make this a default =0 argument then?  Or is the goal that eventually
> teh Page argument will be gone from this function and thus rootFrame makes
sense
> to be required?

Done.

https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
Source/web/PageWidgetDelegate.cpp:78: if (!page->mainFrame() ||
!page->mainFrame()->isLocalFrame())
On 2014/07/11 16:33:03, eseidel wrote:
> How can a page not have a mainFrame?  Or a page's mainFrame be remote?

I don't know about a null mainFrame (see above revision number). However,
mainFrames can be remote when we are rendering an out-of-process iframe. The
entire frame tree has to be replicated, and all frames from the original process
(including the mainFrame) become remoteFrames in the child frame's process.

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebPagePopupI...
File Source/web/WebPagePopupImpl.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebPagePopupI...
Source/web/WebPagePopupImpl.cpp:296: PageWidgetDelegate::layout(m_page.get(),
0);
On 2014/07/11 16:33:03, eseidel wrote:
> Initially Confused by the 0.  I now understand it, but it's still rather
> unreadable. Waht's the long term vision for this call?

I removed this change and made 0 the default argument value. I don't know if
this will need to change in future.

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1631: // FIXME: FastTextAutosizer does not yet
support out-of-process frames.
On 2014/07/11 17:14:52, pdr wrote:
> On 2014/07/11 16:33:03, eseidel wrote:
> > Might want to keep +pdr in the loop.
> 
> Nice catch eseidel!
> 
> @kenrb, please file a bug for this.

Filed (https://code.google.com/p/chromium/issues/detail?id=393285), but it might
be worth mentioning that that bug might be open for a while because IIUC text
autosizing is a mobile feature, and out-of-process iframes on mobile are
considered a lower priority.

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1634: FastTextAutosizer::DeferUpdatePageInfo
deferUpdatePageInfo(page());
On 2014/07/11 19:05:28, skobes wrote:
> Putting this inside the brace scope of the 'if' makes it a nop.  Was that the
> intent?

Good catch, this is an error. The ! should not be in the condition.

[skobes, 2014-07-11 20:30:36]

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1634: FastTextAutosizer::DeferUpdatePageInfo
deferUpdatePageInfo(page());
On 2014/07/11 20:03:33, kenrb wrote:
> On 2014/07/11 19:05:28, skobes wrote:
> > Putting this inside the brace scope of the 'if' makes it a nop.  Was that
the
> > intent?
> 
> Good catch, this is an error. The ! should not be in the condition.

The ! is one issue.  The other issue is that DeferUpdatePageInfo only "defers"
the update until its dtor is called.  So if there is no code after it in the
same scope, it will update immediately which defeats the purpose.

If you wanted it to apply only for local frames I think you would need to pull
everything from here to the } on line 1659 into a helper function and do
something like

  if (mainFrameImpl()->frame()->isLocalFrame()) {
    FastTextAutosizer::DeferUpdatePageInfo deferUpdatePageInfo(page());
    performResize();
  } else {
    performResize();
  }

But, a higher level concern: DeferUpdatePageInfo is just a performance
optimization.  Removing it doesn't mean we don't autosize, it just means the
autosizer thrashes more.  The problem that DeferUpdatePageInfo solves is where

1. WebViewImpl says "hey your frame width is now 600 instead of 800..."
2. FTA says "oh noes! autosize all the things"
3. WebViewImpl says "... and also your LAYOUT width is 600"
4. FTA says "lol just kidding no autosizing pls"

If the intent is to *suppress* autosizing in OOP iframes, then you want to add a
check inside FTA::updatePageInfo, where it uses
m_document->page()->deprecatedLocalMainFrame().

[kenrb, 2014-07-11 21:14:19]

On 2014/07/11 20:30:36, skobes wrote:
>
https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.c...
> Source/web/WebViewImpl.cpp:1634: FastTextAutosizer::DeferUpdatePageInfo
> deferUpdatePageInfo(page());
> On 2014/07/11 20:03:33, kenrb wrote:
> > On 2014/07/11 19:05:28, skobes wrote:
> > > Putting this inside the brace scope of the 'if' makes it a nop.  Was that
> the
> > > intent?
> > 
> > Good catch, this is an error. The ! should not be in the condition.
> 
> The ! is one issue.  The other issue is that DeferUpdatePageInfo only "defers"
> the update until its dtor is called.  So if there is no code after it in the
> same scope, it will update immediately which defeats the purpose.
> 
> If you wanted it to apply only for local frames I think you would need to pull
> everything from here to the } on line 1659 into a helper function and do
> something like
> 
>   if (mainFrameImpl()->frame()->isLocalFrame()) {
>     FastTextAutosizer::DeferUpdatePageInfo deferUpdatePageInfo(page());
>     performResize();
>   } else {
>     performResize();
>   }
> 
> But, a higher level concern: DeferUpdatePageInfo is just a performance
> optimization.  Removing it doesn't mean we don't autosize, it just means the
> autosizer thrashes more.  The problem that DeferUpdatePageInfo solves is where
> 
> 1. WebViewImpl says "hey your frame width is now 600 instead of 800..."
> 2. FTA says "oh noes! autosize all the things"
> 3. WebViewImpl says "... and also your LAYOUT width is 600"
> 4. FTA says "lol just kidding no autosizing pls"
> 
> If the intent is to *suppress* autosizing in OOP iframes, then you want to add
a
> check inside FTA::updatePageInfo, where it uses
> m_document->page()->deprecatedLocalMainFrame().

I am uploading a patch fixing the scope problem, sorry I didn't understand that
before.

For reference, the intent right now is to make cross-process frame tree
replication work for out-of-process iframes, behind the --site-per-process flag.

There is a bit of a chicken and egg problem because the architectural changes we
need break all sorts of Blink features (we are trying to make a list, and text
autosizing is on it), necessitating FIXMEs like this one all over the code base.
But we can't start fixing any of these features until the cross-process
architecture is to a reasonable state.

Instantiating DeferUpdatePageInfo was causing a crash because of its call to
deprecatedLocalMainFrame(). We are not immediately fussed about the actual
behavior of text autosizing with --site-per-process, but there is a bug filed
now to address that. One thing of note is that out-of-process iframes for mobile
is a long way away.

[dcheng, 2014-07-11 21:22:52]

https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
Source/web/PageWidgetDelegate.cpp:74: if (!page)
On 2014/07/11 20:03:33, kenrb wrote:
> On 2014/07/11 16:33:03, eseidel wrote:
> > Why is it valid to call this method with a null page?
> 
> It was added explicitly in r169672. I am not certain, but it looks like it is
> needed for some unit tests.

Do you mind filing a bug and assigning it to me? This is kind of bizarre;
ideally we should fix those tests rather than adding test-specific handling
here.

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/380303003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1634: FastTextAutosizer::DeferUpdatePageInfo
deferUpdatePageInfo(page());
On 2014/07/11 20:30:36, skobes wrote:
> On 2014/07/11 20:03:33, kenrb wrote:
> > On 2014/07/11 19:05:28, skobes wrote:
> > > Putting this inside the brace scope of the 'if' makes it a nop.  Was that
> the
> > > intent?
> > 
> > Good catch, this is an error. The ! should not be in the condition.
> 
> The ! is one issue.  The other issue is that DeferUpdatePageInfo only "defers"
> the update until its dtor is called.  So if there is no code after it in the
> same scope, it will update immediately which defeats the purpose.
> 
> If you wanted it to apply only for local frames I think you would need to pull
> everything from here to the } on line 1659 into a helper function and do
> something like
> 
>   if (mainFrameImpl()->frame()->isLocalFrame()) {
>     FastTextAutosizer::DeferUpdatePageInfo deferUpdatePageInfo(page());
>     performResize();
>   } else {
>     performResize();
>   }
> 
> But, a higher level concern: DeferUpdatePageInfo is just a performance
> optimization.  Removing it doesn't mean we don't autosize, it just means the
> autosizer thrashes more.  The problem that DeferUpdatePageInfo solves is where
> 
> 1. WebViewImpl says "hey your frame width is now 600 instead of 800..."
> 2. FTA says "oh noes! autosize all the things"
> 3. WebViewImpl says "... and also your LAYOUT width is 600"
> 4. FTA says "lol just kidding no autosizing pls"
> 
> If the intent is to *suppress* autosizing in OOP iframes, then you want to add
a
> check inside FTA::updatePageInfo, where it uses
> m_document->page()->deprecatedLocalMainFrame().

I thought fast text-autosizing only ran on mobile? I made several changes to
FastTextAutosizer which will should crash in the presence of RemoteFrames, but
those changes have not blocked us on desktop
(https://codereview.chromium.org/336553003). This particular location does,
because constructing the defer info always treats the main frame as local, but
it's not with OOPI. We don't actually really care if autosizing works at all in
the presence of RemoteFrames; we just care that it doesn't crash while we
bootstrap the architecture.

[kenrb, 2014-07-11 21:31:19]

On 2014/07/11 21:22:52, dcheng (OOO) wrote:
>
https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
> File Source/web/PageWidgetDelegate.cpp (right):
> 
>
https://codereview.chromium.org/380303003/diff/40001/Source/web/PageWidgetDel...
> Source/web/PageWidgetDelegate.cpp:74: if (!page)
> On 2014/07/11 20:03:33, kenrb wrote:
> > On 2014/07/11 16:33:03, eseidel wrote:
> > > Why is it valid to call this method with a null page?
> > 
> > It was added explicitly in r169672. I am not certain, but it looks like it
is
> > needed for some unit tests.
> 
> Do you mind filing a bug and assigning it to me? This is kind of bizarre;
> ideally we should fix those tests rather than adding test-specific handling
> here.
> 

https://code.google.com/p/chromium/issues/detail?id=393339

[skobes, 2014-07-11 22:47:44]

On 2014/07/11 21:22:52, dcheng (OOO) wrote:
> I thought fast text-autosizing only ran on mobile?

Generally yes, though it can run on desktop if you enable device emulation in
dev tools.

> I made several changes to
> FastTextAutosizer which will should crash in the presence of RemoteFrames, but
> those changes have not blocked us on desktop
> (https://codereview.chromium.org/336553003). This particular location does,
> because constructing the defer info always treats the main frame as local, but
> it's not with OOPI. We don't actually really care if autosizing works at all
in
> the presence of RemoteFrames; we just care that it doesn't crash while we
> bootstrap the architecture.

Makes sense if OOPI is desktop only.  Just wanted to make sure you were aware
this patch wouldn't actually suppress any autosizing.

[Julien - ping for review, 2014-07-11 23:03:05]

lgtm for the FrameView.cpp changes.

[kenrb, 2014-07-14 14:37:54]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-07-14 14:38:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kenrb@chromium.org/380303003/100001

[commit-bot: I haz the power, 2014-07-14 15:13:20]

Change committed as 178066