PlzNavigate: Fix anchor-no-multiple-windows.html layout test.

PlzNavigate: Fix anchor-no-multiple-windows.html layout test.

This layouttest turns off the WebKitSupportsMultipleWindows property which means that popups
open in the same window. It then initiates a navigation to a data URL by clicking on the
corresponding href. It then compares the window counts and expects the window counts to
remain unchanged.

Without PlzNavigate the data URL navigation creates a provisional loader in blink which
is then cancelled when data is written to the main document, via Document::writeln.

With PlzNavigate the top level navigation for the data URL is sent to the browser. The
provisional loader is created when the navigation is committed via the
FrameMsg_CommitNavigation IPC. This is too late and hence the data written to the page
via the Document::writeln calls never shows up.

Fix is to add a check for whether there is a client side navigation in process for PlzNavigate
and if yes stop ongoing loaders.

BUG=673744, 673742
Review-Url: https://codereview.chromium.org/2628153002
Cr-Commit-Position: refs/heads/master@{#443458}
Committed: https://chromium.googlesource.com/chromium/src/+/683cb912ff3612c5ec943e4d45d1426452ebb596

[ananta, 2017-01-12 04:33:10]

ananta@chromium.org changed reviewers:
+ jam@chromium.org, japhet@chromium.org

[ananta, 2017-01-12 04:33:18]

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

[commit-bot: I haz the power, 2017-01-12 04:33:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2017-01-12 04:44:46]

Description was changed from

==========
hu# Enter a description of the change.
PlzNavigate: Fix anchor-no-multiple-windows.html layout test.

This layouttest turns off the WebKitSupportsMultipleWindows property which means
that popups
open in the same window. It then initiates a navigation to a data URL by
clicking on the
corresponding href. It then compares the window counts and expects the window
counts to
remain unchanged.

Without PlzNavigate the data URL navigation creates a provisional loader in
blink which
is then cancelled when data is written to the main document, via
Document::writeln.

With PlzNavigate the top level navigation for the data URL is sent to the
browser. The
provisional loader is created when the navigation is committed via the
FrameMsg_CommitNavigation IPC. This is too late and hence the data written to
the page
via the Document::writeln calls never shows up.

Fix is to add a check for whether there is a client side navigation in process
for PlzNavigate
and if yes stop ongoing loaders.

BUG=673744
==========

to

==========
PlzNavigate: Fix anchor-no-multiple-windows.html layout test.

This layouttest turns off the WebKitSupportsMultipleWindows property which means
that popups
open in the same window. It then initiates a navigation to a data URL by
clicking on the
corresponding href. It then compares the window counts and expects the window
counts to
remain unchanged.

Without PlzNavigate the data URL navigation creates a provisional loader in
blink which
is then cancelled when data is written to the main document, via
Document::writeln.

With PlzNavigate the top level navigation for the data URL is sent to the
browser. The
provisional loader is created when the navigation is committed via the
FrameMsg_CommitNavigation IPC. This is too late and hence the data written to
the page
via the Document::writeln calls never shows up.

Fix is to add a check for whether there is a client side navigation in process
for PlzNavigate
and if yes stop ongoing loaders.

BUG=673744
==========

[commit-bot: I haz the power, 2017-01-12 06:21:47]

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

[commit-bot: I haz the power, 2017-01-12 06:21:48]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-01-12 19:59:26]

https://codereview.chromium.org/2628153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2628153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2667:
m_frame->loader().navigationHandledByClient())) {
Do you have any sense how many other callers of provisionalDocumentLoader()
should be checking navigationHandledByClient()? I'm suspicious that all callers
that just null-check provisionalDocumentLoader() should, and that instead of
exposing navigationHandledByClient(), we should have a
FrameLoader::hasProvisionalNavigation() that checks both.

[ananta, 2017-01-12 20:36:55]

https://codereview.chromium.org/2628153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2628153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2667:
m_frame->loader().navigationHandledByClient())) {
On 2017/01/12 19:59:26, Nate Chapin wrote:
> Do you have any sense how many other callers of provisionalDocumentLoader()
> should be checking navigationHandledByClient()? I'm suspicious that all
callers
> that just null-check provisionalDocumentLoader() should, and that instead of
> exposing navigationHandledByClient(), we should have a
> FrameLoader::hasProvisionalNavigation() that checks both.

Added the function and called it from here and from
WebLocalFrameImpl::isLoading().

Don't think the other callsites here
https://cs.chromium.org/search/?q=provisionalDocumentLoader()&sq=package:chro...
need to be changed

[ananta, 2017-01-12 20:37:35]

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

[commit-bot: I haz the power, 2017-01-12 20:38:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-01-12 20:43:44]

On 2017/01/12 20:36:55, ananta wrote:
>
https://codereview.chromium.org/2628153002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2628153002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:2667:
> m_frame->loader().navigationHandledByClient())) {
> On 2017/01/12 19:59:26, Nate Chapin wrote:
> > Do you have any sense how many other callers of provisionalDocumentLoader()
> > should be checking navigationHandledByClient()? I'm suspicious that all
> callers
> > that just null-check provisionalDocumentLoader() should, and that instead of
> > exposing navigationHandledByClient(), we should have a
> > FrameLoader::hasProvisionalNavigation() that checks both.
> 
> Added the function and called it from here and from
> WebLocalFrameImpl::isLoading().
> 
> Don't think the other callsites here
>
https://cs.chromium.org/search/?q=provisionalDocumentLoader()&sq=package:chro...
> need to be changed

NavigationScheduler::schedule is debatable.

shouldSendCompleteNotification() in FrameLoader.cpp checks both currently, so
that can be simplified by calling hasPendingNavigation(). Would you mind
updating that one, too?

LGTM otherwise.

[ananta, 2017-01-12 22:21:37]

On 2017/01/12 20:43:44, Nate Chapin wrote:
> On 2017/01/12 20:36:55, ananta wrote:
> >
>
https://codereview.chromium.org/2628153002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/Document.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2628153002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/Document.cpp:2667:
> > m_frame->loader().navigationHandledByClient())) {
> > On 2017/01/12 19:59:26, Nate Chapin wrote:
> > > Do you have any sense how many other callers of
provisionalDocumentLoader()
> > > should be checking navigationHandledByClient()? I'm suspicious that all
> > callers
> > > that just null-check provisionalDocumentLoader() should, and that instead
of
> > > exposing navigationHandledByClient(), we should have a
> > > FrameLoader::hasProvisionalNavigation() that checks both.
> > 
> > Added the function and called it from here and from
> > WebLocalFrameImpl::isLoading().
> > 
> > Don't think the other callsites here
> >
>
https://cs.chromium.org/search/?q=provisionalDocumentLoader()&sq=package:chro...
> > need to be changed
> 
> NavigationScheduler::schedule is debatable.
> 
> shouldSendCompleteNotification() in FrameLoader.cpp checks both currently, so
> that can be simplified by calling hasPendingNavigation(). Would you mind
> updating that one, too?
> 
> LGTM otherwise.

Thanks done

[ananta, 2017-01-12 22:21:57]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-01-12 22:21:57]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2628153002/#ps60001
(title: "Update the shouldSendCompleteNotification() function to use the
hasProvisionalNavigation() function")

[commit-bot: I haz the power, 2017-01-12 22:22:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2017-01-13 00:35:14]

Description was changed from

==========
PlzNavigate: Fix anchor-no-multiple-windows.html layout test.

This layouttest turns off the WebKitSupportsMultipleWindows property which means
that popups
open in the same window. It then initiates a navigation to a data URL by
clicking on the
corresponding href. It then compares the window counts and expects the window
counts to
remain unchanged.

Without PlzNavigate the data URL navigation creates a provisional loader in
blink which
is then cancelled when data is written to the main document, via
Document::writeln.

With PlzNavigate the top level navigation for the data URL is sent to the
browser. The
provisional loader is created when the navigation is committed via the
FrameMsg_CommitNavigation IPC. This is too late and hence the data written to
the page
via the Document::writeln calls never shows up.

Fix is to add a check for whether there is a client side navigation in process
for PlzNavigate
and if yes stop ongoing loaders.

BUG=673744
==========

to

==========
PlzNavigate: Fix anchor-no-multiple-windows.html layout test.

This layouttest turns off the WebKitSupportsMultipleWindows property which means
that popups
open in the same window. It then initiates a navigation to a data URL by
clicking on the
corresponding href. It then compares the window counts and expects the window
counts to
remain unchanged.

Without PlzNavigate the data URL navigation creates a provisional loader in
blink which
is then cancelled when data is written to the main document, via
Document::writeln.

With PlzNavigate the top level navigation for the data URL is sent to the
browser. The
provisional loader is created when the navigation is committed via the
FrameMsg_CommitNavigation IPC. This is too late and hence the data written to
the page
via the Document::writeln calls never shows up.

Fix is to add a check for whether there is a client side navigation in process
for PlzNavigate
and if yes stop ongoing loaders.

BUG=673744, 673742
==========

[commit-bot: I haz the power, 2017-01-13 01:59:27]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484259717517870,
"parent_rev": "bf359cbc46ede102f3ebfc5f1d1c019dfef49cf0", "commit_rev":
"683cb912ff3612c5ec943e4d45d1426452ebb596"}

[commit-bot: I haz the power, 2017-01-13 02:00:01]

Description was changed from

==========
PlzNavigate: Fix anchor-no-multiple-windows.html layout test.

This layouttest turns off the WebKitSupportsMultipleWindows property which means
that popups
open in the same window. It then initiates a navigation to a data URL by
clicking on the
corresponding href. It then compares the window counts and expects the window
counts to
remain unchanged.

Without PlzNavigate the data URL navigation creates a provisional loader in
blink which
is then cancelled when data is written to the main document, via
Document::writeln.

With PlzNavigate the top level navigation for the data URL is sent to the
browser. The
provisional loader is created when the navigation is committed via the
FrameMsg_CommitNavigation IPC. This is too late and hence the data written to
the page
via the Document::writeln calls never shows up.

Fix is to add a check for whether there is a client side navigation in process
for PlzNavigate
and if yes stop ongoing loaders.

BUG=673744, 673742
==========

to

==========
PlzNavigate: Fix anchor-no-multiple-windows.html layout test.

This layouttest turns off the WebKitSupportsMultipleWindows property which means
that popups
open in the same window. It then initiates a navigation to a data URL by
clicking on the
corresponding href. It then compares the window counts and expects the window
counts to
remain unchanged.

Without PlzNavigate the data URL navigation creates a provisional loader in
blink which
is then cancelled when data is written to the main document, via
Document::writeln.

With PlzNavigate the top level navigation for the data URL is sent to the
browser. The
provisional loader is created when the navigation is committed via the
FrameMsg_CommitNavigation IPC. This is too late and hence the data written to
the page
via the Document::writeln calls never shows up.

Fix is to add a check for whether there is a client side navigation in process
for PlzNavigate
and if yes stop ongoing loaders.

BUG=673744, 673742

Review-Url: https://codereview.chromium.org/2628153002
Cr-Commit-Position: refs/heads/master@{#443458}
Committed:
https://chromium.googlesource.com/chromium/src/+/683cb912ff3612c5ec943e4d45d1...
==========

[commit-bot: I haz the power, 2017-01-13 02:00:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/683cb912ff3612c5ec943e4d45d1...