Remove extra calls of didMeaningfulLayout() during the same navigation

Remove extra calls of didMeaningfulLayout() during the same navigation

didMeaningfulLayout() should be called once per WebMeaningfulLayout per
"visit". If the user initiates a navigation, refresh the current page, or
the page automatically navigates, or redirected, then it counts as another
"visit".

BUG=577968
Committed: https://crrev.com/2502755a32d31fb66c749e25cdee072dda868234
Cr-Commit-Position: refs/heads/master@{#371025}

[wychen, 2016-01-15 17:43:10]

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

[wychen, 2016-01-15 17:43:10]

PTAL

[dglazkov, 2016-01-15 19:41:48]

https://codereview.chromium.org/1596573002/diff/1/chrome/browser/dom_distille...
File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right):

https://codereview.chromium.org/1596573002/diff/1/chrome/browser/dom_distille...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:34: const
char* kAllPaths[] = {
We need to test this at a higher level, since the bug you're fixing is actually
in WebViewImpl.

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

https://codereview.chromium.org/1596573002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:483:
m_webFrame->viewImpl()->didCommitLoad(commitType, false);
The bug is here. WebViewImpl::didCommitLoad only expects to hear about main
frame load commit. Let's just not call it for other frames:

    if (!m_webFrame->parent())
        m_webFrame->viewImpl()->didCommitLoad(commitType == StandardCommit,
false);

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

https://codereview.chromium.org/1596573002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:4306:
m_shouldDispatchFirstVisuallyNonEmptyLayout = true;
Interesting! This is a partial revert of
https://codereview.chromium.org/1380363005/, which indeed caused this problem.
We shouldn't need it though if we ignore didCommitLoad from child frames, right?

[esprehn, 2016-01-15 22:15:31]

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

[esprehn, 2016-01-15 22:15:31]

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

https://codereview.chromium.org/1596573002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:483:
m_webFrame->viewImpl()->didCommitLoad(commitType, false);
On 2016/01/15 at 19:41:48, dglazkov wrote:
> The bug is here. WebViewImpl::didCommitLoad only expects to hear about main
frame load commit. Let's just not call it for other frames:
> 
>     if (!m_webFrame->parent())
>         m_webFrame->viewImpl()->didCommitLoad(commitType == StandardCommit,
false);

It should be trivial to write a test for this with Sim.

[wychen, 2016-01-15 22:30:21]

Patchset #2 (id:20001) has been deleted

[wychen, 2016-01-15 22:31:02]

https://codereview.chromium.org/1596573002/diff/1/chrome/browser/dom_distille...
File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right):

https://codereview.chromium.org/1596573002/diff/1/chrome/browser/dom_distille...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:34: const
char* kAllPaths[] = {
On 2016/01/15 19:41:47, dglazkov wrote:
> We need to test this at a higher level, since the bug you're fixing is
actually
> in WebViewImpl.

I'll add one in WebViewTest. I think it would still make sense to keep the test
here just to be safe, because I'll be adding reader mode features that relies on
the behavior.

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

https://codereview.chromium.org/1596573002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:483:
m_webFrame->viewImpl()->didCommitLoad(commitType, false);
On 2016/01/15 19:41:48, dglazkov wrote:
> The bug is here. WebViewImpl::didCommitLoad only expects to hear about main
> frame load commit. Let's just not call it for other frames:
> 
>     if (!m_webFrame->parent())
>         m_webFrame->viewImpl()->didCommitLoad(commitType == StandardCommit,
> false);

Done.

https://codereview.chromium.org/1596573002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:483:
m_webFrame->viewImpl()->didCommitLoad(commitType, false);
On 2016/01/15 22:15:31, esprehn wrote:
> On 2016/01/15 at 19:41:48, dglazkov wrote:
> > The bug is here. WebViewImpl::didCommitLoad only expects to hear about main
> frame load commit. Let's just not call it for other frames:
> > 
> >     if (!m_webFrame->parent())
> >         m_webFrame->viewImpl()->didCommitLoad(commitType == StandardCommit,
> false);
> 
> It should be trivial to write a test for this with Sim.

Could you elaborate a bit, or show me an example? I know it might take you
longer to explain than to write it yourself, but I'd like to learn this.

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

https://codereview.chromium.org/1596573002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:4306:
m_shouldDispatchFirstVisuallyNonEmptyLayout = true;
On 2016/01/15 19:41:48, dglazkov wrote:
> Interesting! This is a partial revert of
> https://codereview.chromium.org/1380363005/, which indeed caused this problem.
> We shouldn't need it though if we ignore didCommitLoad from child frames,
right?

This is still needed, for cases like refreshing, or redirection.

[dglazkov, 2016-01-15 22:36:43]

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

https://codereview.chromium.org/1596573002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:4306:
m_shouldDispatchFirstVisuallyNonEmptyLayout = true;
On 2016/01/15 at 22:31:02, wychen wrote:
> On 2016/01/15 19:41:48, dglazkov wrote:
> > Interesting! This is a partial revert of
> > https://codereview.chromium.org/1380363005/, which indeed caused this
problem.
> > We shouldn't need it though if we ignore didCommitLoad from child frames,
right?
> 
> This is still needed, for cases like refreshing, or redirection.

Both refresh and redirect will result in a new didCommitLoad call: they both
signal destroying and re-creating a new main frame doc, which will trigger the
load and the didCommitLoad. Are there any others we could be forgetting?

[wychen, 2016-01-21 01:47:52]

I've added Sim tests. It's indeed pretty trivial.

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

https://codereview.chromium.org/1596573002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:4306:
m_shouldDispatchFirstVisuallyNonEmptyLayout = true;
On 2016/01/15 22:36:43, dglazkov wrote:
> Both refresh and redirect will result in a new didCommitLoad call: they both
> signal destroying and re-creating a new main frame doc, which will trigger the
> load and the didCommitLoad. Are there any others we could be forgetting?

Ah. I filtered out HistoryInertCommit in this patch set (#1), so I needed these.
They are removed now.

[wychen, 2016-01-21 02:15:25]

The CQ bit was checked by wychen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 02:16:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596573002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596573002/60001

[commit-bot: I haz the power, 2016-01-21 03:05:07]

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

[commit-bot: I haz the power, 2016-01-21 03:05:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[wychen, 2016-01-21 04:14:23]

The CQ bit was checked by wychen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 04:15:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596573002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596573002/80001

[commit-bot: I haz the power, 2016-01-21 05:26:47]

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

[commit-bot: I haz the power, 2016-01-21 05:26:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dglazkov, 2016-01-21 05:35:13]

lgtm

[wychen, 2016-01-21 20:00:36]

wychen@chromium.org changed reviewers:
+ mdjones@chromium.org

[dglazkov, 2016-01-22 18:54:23]

The CQ bit was checked by dglazkov@chromium.org

[dglazkov, 2016-01-22 18:54:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dglazkov@chromium.org
Link to the patchset: https://codereview.chromium.org/1596573002/#ps100001
(title: "fix tests")

[commit-bot: I haz the power, 2016-01-22 18:55:39]

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

[commit-bot: I haz the power, 2016-01-22 19:06:17]

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

[commit-bot: I haz the power, 2016-01-22 19:06:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mdjones, 2016-01-22 19:07:52]

lgtm

[wychen, 2016-01-22 19:51:01]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2016-01-22 19:53:02]

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

[commit-bot: I haz the power, 2016-01-22 20:28:29]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-01-22 20:29:50]

Description was changed from

==========
Remove extra calls of didMeaningfulLayout() during the same navigation

didMeaningfulLayout() should be called once per WebMeaningfulLayout per
"visit". If the user initiates a navigation, refresh the current page, or
the page automatically navigates, or redirected, then it counts as another
"visit".

BUG=577968
==========

to

==========
Remove extra calls of didMeaningfulLayout() during the same navigation

didMeaningfulLayout() should be called once per WebMeaningfulLayout per
"visit". If the user initiates a navigation, refresh the current page, or
the page automatically navigates, or redirected, then it counts as another
"visit".

BUG=577968

Committed: https://crrev.com/2502755a32d31fb66c749e25cdee072dda868234
Cr-Commit-Position: refs/heads/master@{#371025}
==========

[commit-bot: I haz the power, 2016-01-22 20:29:51]

Patchset 5 (id:??) landed as
https://crrev.com/2502755a32d31fb66c749e25cdee072dda868234
Cr-Commit-Position: refs/heads/master@{#371025}