Find a better place for setting Layout/Paint hook related flags. (issue 1380363005 by ahest@yandex-team.ru)

[ah...@yandex-team.ru]

Reviewers: dglazkov, bokan, kouhei (catching-up),


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

https://codereview.chromium.org/1380363005/diff/1/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode3889
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.

Description:
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=

Please review this at https://codereview.chromium.org/1380363005/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+8, -5 lines):
M third_party/WebKit/Source/web/WebViewImpl.cpp


Index: third_party/WebKit/Source/web/WebViewImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebViewImpl.cpp
b/third_party/WebKit/Source/web/WebViewImpl.cpp
index
b708684218ae54a865aeae4eeb2b26bc81067bdf..aaae12c8c6983414012c430a304fe752f3665f82
100644
--- a/third_party/WebKit/Source/web/WebViewImpl.cpp
+++ b/third_party/WebKit/Source/web/WebViewImpl.cpp
@@ -3880,9 +3880,14 @@ void WebViewImpl::setSelectionColors(unsigned
activeBackgroundColor,

void WebViewImpl::didCommitLoad(bool isNewNavigation, bool
isNavigationWithinPage)
{
- if (isNewNavigation && !isNavigationWithinPage) {
- pageScaleConstraintsSet().setNeedsReset(true);
- m_pageImportanceSignals.onCommitLoad();
+ if (!isNavigationWithinPage) {
+ m_shouldDispatchFirstVisuallyNonEmptyLayout = true;
+ m_shouldDispatchFirstLayoutAfterFinishedParsing = true;
+
+ if (isNewNavigation) {
+ pageScaleConstraintsSet().setNeedsReset(true);
+ m_pageImportanceSignals.onCommitLoad();
+ }
}

// Give the visual viewport's scroll layer its initial size.
@@ -4163,8 +4168,6 @@ void WebViewImpl::setRootGraphicsLayer(GraphicsLayer*
layer)
// attempt to paint too early in the next page load.
m_layerTreeView->setDeferCommits(true);
m_layerTreeView->clearRootLayer();
- m_shouldDispatchFirstVisuallyNonEmptyLayout = true;
- m_shouldDispatchFirstLayoutAfterFinishedParsing = true;

page()->frameHost().visualViewport().clearLayersForTreeView(m_layerTreeView);
}
}

[dgla...@chromium.org]

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

https://codereview.chromium.org/1380363005/

[bo...@chromium.org]

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

https://codereview.chromium.org/1380363005/diff/1/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode3889
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.

https://codereview.chromium.org/1380363005/

[ah...@yandex-team.ru]

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.


https://codereview.chromium.org/1380363005/

[ah...@yandex-team.ru]

Any thoughts on this?

https://codereview.chromium.org/1380363005/

[nedn...@google.com]

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.

https://codereview.chromium.org/1380363005/

[ah...@yandex-team.ru]

esprehn, PTAL

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

https://codereview.chromium.org/1380363005/

[kou...@chromium.org]

> 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.

https://codereview.chromium.org/1380363005/

[ah...@yandex-team.ru]

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.

https://codereview.chromium.org/1380363005/

[ah...@yandex-team.ru]

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!

https://codereview.chromium.org/1380363005/

[dgla...@chromium.org]

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.

https://codereview.chromium.org/1380363005/

[ah...@yandex-team.ru]

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.


https://codereview.chromium.org/1380363005/

[dgla...@chromium.org]

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

https://codereview.chromium.org/1380363005/

[dgla...@chromium.org]

lgtm



https://codereview.chromium.org/1380363005/

[esp...@chromium.org]

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


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

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

https://codereview.chromium.org/1380363005/

[ah...@yandex-team.ru]

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

https://codereview.chromium.org/1380363005/diff/20001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode3855
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.

https://codereview.chromium.org/1380363005/

[ah...@yandex-team.ru]

Got approval from sullivan for landing.

https://codereview.chromium.org/1380363005/

[commit-bot@chromium.org via codereview.chromium.org]

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


https://codereview.chromium.org/1380363005/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #2 (id:20001)

https://codereview.chromium.org/1380363005/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/1380363005/