Use expected main frame size from WebViewImpl when creating a new FrameView.

Use expected main frame size from WebViewImpl when creating a new FrameView.

The main frame's size is no longer simply the viewport size. Instead,
WebViewImpl sizes it after layout to be the either the content width with
a height matching the viewport aspect ratio, or the viewport at minimum page
scale. This fixes a bug where the innerWidth/innerHeight values would be
wrong during an onload event if the onload would happen before the content was
properly sized.

BUG=431097, 428722
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185145

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

[bokan, 2014-11-08 00:09:15]

bokan@chromium.org changed reviewers:
+ aelias@chromium.org

[bokan, 2014-11-08 00:09:16]

Hey Alex,

This change fixes the perf regression we were seeing on the thread_times metric.
Document loading seems incredibly complex so I'm out of my depth here, any
insights you may have would be appreciated.

It looks like when a new renderer spins up it does a Resize followed by a Layout
before the document is loaded. Then when the document is loading, it creates a
new FrameView with the same size and when it finishes loading it fires the
'load' event. After that it does the first layout on the document. Because the
'load' event comes before the layout, the FrameView still had the size passed to
it at creation which was the viewport's size. This change passes the frame's
size rather than the viewport so we get the defaultLayoutWidth that gets set on
that initial layout that happens on the renderer init.

It looks like we still have some sort of race happening. You can see this by
repeatedly loading jsbin.com/yakagevo/1/quiet (new renderer, not refreshing) on
stable channel, it will occasionally show different results as innerWidth will
be 0 sometimes in onload. With virtual-viewport it looks like this happens more
often, I'm still looking into that.

Let me know what you think, thanks.

[aelias_OOO_until_Jul13, 2014-11-08 00:35:02]

The main concern that comes to mind is that it seems odd that webView->size() is
still used if there's no existing FrameView.  Maybe we should instead add a new
getter to WebView that returns intended mainFrameSize to use it here?

[bokan, 2014-11-10 18:03:01]

How's this?

[aelias_OOO_until_Jul13, 2014-11-10 19:13:35]

Looks good, just needs test.

[bokan, 2014-11-11 14:13:14]

On 2014/11/10 19:13:35, aelias wrote:
> Looks good, just needs test.

Done, ptal.

[aelias_OOO_until_Jul13, 2014-11-11 19:30:03]

lgtm

[bokan, 2014-11-11 19:30:58]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-11-11 19:31:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/705423002/40001

[commit-bot: I haz the power, 2014-11-11 20:50:34]

Committed patchset #3 (id:40001) as 185145

[Stephen White, 2014-11-12 04:42:15]

I think this CL may be causing failures on chromium_chromeos_rel, and blocking
the Blink roll (it's something in the range
https://chromium.googlesource.com/chromium/blink/+log/dcf905c..d43def0 , and
this seems the most likely suspect).

See
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

CrossProcessFrameTreeBrowserTest.CreateCrossProcessSubframeProxies
SitePerProcessBrowserTest.NavigateRemoteFrame
SitePerProcessBrowserTest.ProxyCreationSkipsSubtree
SitePerProcessBrowserTest.CrossSiteIframe

[Stephen White, 2014-11-12 12:49:20]

On 2014/11/12 04:42:15, Stephen White wrote:
> I think this CL may be causing failures on chromium_chromeos_rel, and blocking
> the Blink roll (it's something in the range
> https://chromium.googlesource.com/chromium/blink/+log/dcf905c..d43def0 , and
> this seems the most likely suspect).
> 
> See
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> 
> CrossProcessFrameTreeBrowserTest.CreateCrossProcessSubframeProxies
> SitePerProcessBrowserTest.NavigateRemoteFrame
> SitePerProcessBrowserTest.ProxyCreationSkipsSubtree
> SitePerProcessBrowserTest.CrossSiteIframe

FYI, I reverted this CL in r185176 aka dda4b8fa006cc7a7846cbc321f72b8ddc1aa1867,
and the tests above went green.

Please check content_browsertests on Android (dbg), ChromeOS (dbg and rel)
before re-landing. Thanks!

[bokan, 2014-11-13 19:15:30]

On 2014/11/12 12:49:20, Stephen White wrote:
> On 2014/11/12 04:42:15, Stephen White wrote:
> > I think this CL may be causing failures on chromium_chromeos_rel, and
blocking
> > the Blink roll (it's something in the range
> > https://chromium.googlesource.com/chromium/blink/+log/dcf905c..d43def0 , and
> > this seems the most likely suspect).
> > 
> > See
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> > 
> > CrossProcessFrameTreeBrowserTest.CreateCrossProcessSubframeProxies
> > SitePerProcessBrowserTest.NavigateRemoteFrame
> > SitePerProcessBrowserTest.ProxyCreationSkipsSubtree
> > SitePerProcessBrowserTest.CrossSiteIframe
> 
> FYI, I reverted this CL in r185176 aka
dda4b8fa006cc7a7846cbc321f72b8ddc1aa1867,
> and the tests above went green.
> 
> Please check content_browsertests on Android (dbg), ChromeOS (dbg and rel)
> before re-landing. Thanks!

Thanks, I've checked and fixed both problems. I'll reland now and monitor for
redness.

[bokan, 2014-11-13 19:15:35]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-11-13 19:16:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/705423002/80001

[commit-bot: I haz the power, 2014-11-13 19:29:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-13 19:29:03]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[bokan, 2014-11-13 20:03:26]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-11-13 20:04:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/705423002/100001

[commit-bot: I haz the power, 2014-11-13 21:15:47]

Committed patchset #6 (id:100001) as 185321