Find a better place for setting Layout/Paint hook related flags.

Find a better place for setting Layout/Paint hook related flags.

These hooks are supposed to be called once after a commit, not after
the WebViewImpl's root graphics layer was reset to 0.
Setting these flags in didCommitLoad looks much more sane and
straightforward. Also this fixes a subtle bug: if you don't create
the WebView with a proper size right away, and instead create it with
0x0 size, navigate and then resize, you won't get the hooks called.

BUG=529340
Committed: https://crrev.com/deeb788beddc5a4c3027d2bd5eb3c5760cc6a574
Cr-Commit-Position: refs/heads/master@{#356298}

[Alexander Semashko, 2015-10-05 13:11:10]

ahest@yandex-team.ru changed reviewers:
+ bokan@chromium.org, dglazkov@chromium.org, kouhei@chromium.org

[Alexander Semashko, 2015-10-05 13:11:11]

https://codereview.chromium.org/1380363005/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1380363005/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:3889:
m_pageImportanceSignals.onCommitLoad();
As a side question, do these two lines need to be under
"if (isNewNavigation)"? Looks like we have to exec them on every commit
(including, for example, BackForwardCommit).

I'm adding the authors of these lines to look at this.

[dglazkov, 2015-10-05 16:10:22]

dglazkov@chromium.org changed reviewers:
+ esprehn@chromium.org

[dglazkov, 2015-10-05 16:10:44]

+esprehn, who's better fit to review this.

[bokan, 2015-10-05 18:03:30]

https://codereview.chromium.org/1380363005/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1380363005/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:3889:
m_pageImportanceSignals.onCommitLoad();
On 2015/10/05 13:11:11, ahest wrote:
> As a side question, do these two lines need to be under
> "if (isNewNavigation)"? Looks like we have to exec them on every commit
> (including, for example, BackForwardCommit).
> 
> I'm adding the authors of these lines to look at this.

I can only speak to the setNeedsReset on pageScaleConstraintsSet: It does need
to be limited to a new navigation since things like history navigations will
restore page scale to the saved value in the HistoryItem.

[Alexander Semashko, 2015-10-06 17:11:59]

ahest@yandex-team.ru changed reviewers:
+ nednguyen@google.com

[Alexander Semashko, 2015-10-06 17:12:00]

Hmm, there is something more. This issue actually manifests itself in
startup perftests (and probably some other things that depend on
Startup.FirstWebContents.NonEmptyPaint histogram, but I could not find
other uses in the repository).

Basically, Startup.FirstWebContents.NonEmptyPaint is recorded on the
second page load instead of the first one (I verified this by adding
some logs to FirstWebContentsProfiler). Furthermore, there is a similar
histogram Startup.FirstWebContents.MainFrameLoad, which is recorded on
the first load, as it should be.

So first_main_frame_load_time and first_non_empty_paint_time values in
startup perftests are not very meaningful, since they are computed on
different page loads.
And this CL, if committed, will make startup perftests report
considerably lower first_non_empty_paint_time. I don't know what is the
right procedure for such a change, so adding nednguyen, as he is an
owner of tools/perf who has touched startup.py recently.

[Alexander Semashko, 2015-10-09 18:14:07]

Any thoughts on this?

[nednguyen, 2015-10-09 18:39:48]

On 2015/10/09 18:14:07, ahest wrote:
> Any thoughts on this?

From the perf monitoring side, all you need is coordinate with the perf sheriffs
to let them know this will regress the metric and it's expected. I leave
reviewing this code to other experts.

[Alexander Semashko, 2015-10-09 22:28:12]

esprehn, PTAL

kouhei, what do you say about isNewNavigation and
m_pageImportanceSignals.onCommitLoad()?

[kouhei (in TOK), 2015-10-12 12:58:42]

> kouhei, what do you say about isNewNavigation and
> m_pageImportanceSignals.onCommitLoad()?
The intent here is to see how much web page views have triggered signals. This
should be limited to new page navigations and we don't want to count in-page
link navs.

[Alexander Semashko, 2015-10-15 14:07:19]

Found and attached a bug about this issue.

It seems that this patch should also revert
https://codereview.chromium.org/1307203006, which only hides away the fact that
didFirstVisuallyNonEmptyLayout does not occur on the first navigation. The
second navigation is not needed with the proposed fix.

Also, the change in WebViewImpl.cpp is still waiting for the review.

[Alexander Semashko, 2015-10-23 09:50:51]

On 2015/10/15 14:07:19, ahest wrote:
> Found and attached a bug about this issue.
> 
> It seems that this patch should also revert
> https://codereview.chromium.org/1307203006, which only hides away the fact
that
> didFirstVisuallyNonEmptyLayout does not occur on the first navigation. The
> second navigation is not needed with the proposed fix.
> 
> Also, the change in WebViewImpl.cpp is still waiting for the review.

PTAL!

[dglazkov, 2015-10-23 16:23:49]

On 2015/10/23 at 09:50:51, ahest wrote:
> On 2015/10/15 14:07:19, ahest wrote:
> > Found and attached a bug about this issue.
> > 
> > It seems that this patch should also revert
> > https://codereview.chromium.org/1307203006, which only hides away the fact
that
> > didFirstVisuallyNonEmptyLayout does not occur on the first navigation. The
> > second navigation is not needed with the proposed fix.
> > 
> > Also, the change in WebViewImpl.cpp is still waiting for the review.
> 
> PTAL!

I guess at a high level, I don't think we should couple loading and layout
concerns. If the current method doesn't work correctly, I would treat is a a
problem in frame-pumping stack of the code.

[Alexander Semashko, 2015-10-24 15:26:24]

On 2015/10/23 16:23:49, dglazkov wrote:
> On 2015/10/23 at 09:50:51, ahest wrote:
> > On 2015/10/15 14:07:19, ahest wrote:
> > > Found and attached a bug about this issue.
> > > 
> > > It seems that this patch should also revert
> > > https://codereview.chromium.org/1307203006, which only hides away the fact
> that
> > > didFirstVisuallyNonEmptyLayout does not occur on the first navigation. The
> > > second navigation is not needed with the proposed fix.
> > > 
> > > Also, the change in WebViewImpl.cpp is still waiting for the review.
> > 
> > PTAL!
> 
> I guess at a high level, I don't think we should couple loading and layout
> concerns. If the current method doesn't work correctly, I would treat is a a
> problem in frame-pumping stack of the code.

Please correct me if I'm wrong; to me there are following reasons why
this coupling looks much better than the current method.
a) We have to reset these flags when moving to a new document. The
didCommitLoad is probably the most basic and clear sign of that.
b) In the current method the behavior of notifications about the new
document is defined by some bits of the previous document's state.
This just does not seem right. If that old document was not painted,
or even was not laid out, why should it affect the new one?

FYI, in "normal" situation currently the flags are properly reset in
this call chain:
... -> Document::detach -> LayoutView::setIsInWindow(false) ->
PaintLayerCompositor::setIsInWindow(false) ->
PaintLayerCompositor::detachRootLayer ->
ChromeClientImpl::attachRootGraphicsLayer(rootLayer=0) ->
WebViewImpl::setRootGraphicsLayer(layer=0).
In the "problem" case that affected the perf test this chain breaks at
PaintLayerCompositor::setIsInWindow, because staleInCompositingMode()
returns false.
There are more checks down the road that would also break the chain.

[dglazkov, 2015-10-26 16:39:27]

On 2015/10/24 at 15:26:24, ahest wrote:
> On 2015/10/23 16:23:49, dglazkov wrote:
> > On 2015/10/23 at 09:50:51, ahest wrote:
> > > On 2015/10/15 14:07:19, ahest wrote:
> > > > Found and attached a bug about this issue.
> > > > 
> > > > It seems that this patch should also revert
> > > > https://codereview.chromium.org/1307203006, which only hides away the
fact
> > that
> > > > didFirstVisuallyNonEmptyLayout does not occur on the first navigation.
The
> > > > second navigation is not needed with the proposed fix.
> > > > 
> > > > Also, the change in WebViewImpl.cpp is still waiting for the review.
> > > 
> > > PTAL!
> > 
> > I guess at a high level, I don't think we should couple loading and layout
> > concerns. If the current method doesn't work correctly, I would treat is a a
> > problem in frame-pumping stack of the code.
> 
> Please correct me if I'm wrong; to me there are following reasons why
> this coupling looks much better than the current method.
> a) We have to reset these flags when moving to a new document. The
> didCommitLoad is probably the most basic and clear sign of that.
> b) In the current method the behavior of notifications about the new
> document is defined by some bits of the previous document's state.
> This just does not seem right. If that old document was not painted,
> or even was not laid out, why should it affect the new one?
> 
> FYI, in "normal" situation currently the flags are properly reset in
> this call chain:
> ... -> Document::detach -> LayoutView::setIsInWindow(false) ->
> PaintLayerCompositor::setIsInWindow(false) ->
> PaintLayerCompositor::detachRootLayer ->
> ChromeClientImpl::attachRootGraphicsLayer(rootLayer=0) ->
> WebViewImpl::setRootGraphicsLayer(layer=0).
> In the "problem" case that affected the perf test this chain breaks at
> PaintLayerCompositor::setIsInWindow, because staleInCompositingMode()
> returns false.
> There are more checks down the road that would also break the chain.

Ok. I think you do need to rebase the patch to include the
m_shouldDispatchFirstLayoutAfterFinishedLoading.

[Alexander Semashko, 2015-10-26 17:13:57]

On 2015/10/26 16:39:27, dglazkov wrote:
> Ok. I think you do need to rebase the patch to include the
> m_shouldDispatchFirstLayoutAfterFinishedLoading.

Done.

[dglazkov, 2015-10-26 17:16:55]

lgtm

[esprehn, 2015-10-26 21:08:54]

lgtm, w/ a comment, can you explain what an in page nav is?

https://codereview.chromium.org/1380363005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1380363005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:3855: if (!isNavigationWithinPage)
{
What does isNavigationWithinPage mean? Does that mean hash changes or pushState?

[Alexander Semashko, 2015-10-26 21:57:50]

https://codereview.chromium.org/1380363005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1380363005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:3855: if (!isNavigationWithinPage)
{
On 2015/10/26 21:08:54, esprehn wrote:
> What does isNavigationWithinPage mean? Does that mean hash changes or
pushState?

Both are in-page navigations, for sure. I'd say that an in-page navigation is
one that does not involve loading a new document.

[Alexander Semashko, 2015-10-27 13:36:19]

Got approval from sullivan for landing.

[Alexander Semashko, 2015-10-27 13:36:32]

The CQ bit was checked by ahest@yandex-team.ru

[commit-bot: I haz the power, 2015-10-27 13:36:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380363005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380363005/20001

[commit-bot: I haz the power, 2015-10-27 15:10:30]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-27 15:12:18]

Patchset 2 (id:??) landed as
https://crrev.com/deeb788beddc5a4c3027d2bd5eb3c5760cc6a574
Cr-Commit-Position: refs/heads/master@{#356298}