Don't abort the PDF stream if a navigation is intiated within the same page.

Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click with a history.replacestate call
in the pagehide/pageshow event for the main html page, we get a DidStartNavigation
notification on the guest webcontents created for the EmbeddedObserver. 
Currently we abort the stream if it is a top level navigation. 
It seems like we should not abort the stream if the navigation is within the same page.

The problem does not occur if the PDF is opened in a different tab or window

Should fix the black PDF issues with M55.

BUG=677580
TEST=Covered by browser test PDFExtensionTest.OpenPDFOnLinkClickWithReplaceState

Review-Url: https://codereview.chromium.org/2611643004
Cr-Commit-Position: refs/heads/master@{#441857}
Committed: https://chromium.googlesource.com/chromium/src/+/52df16bebd27db88af89d12460db4b4c857aeebb

[ananta, 2017-01-03 23:30:03]

ananta@chromium.org changed reviewers:
+ lazyboy@chromium.org

[ananta, 2017-01-03 23:30:14]

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

[commit-bot: I haz the power, 2017-01-03 23:30:47]

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

[commit-bot: I haz the power, 2017-01-04 00:55:17]

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

[commit-bot: I haz the power, 2017-01-04 00:55:18]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-01-04 02:12:08]

ananta@chromium.org changed reviewers:
+ creis@chromium.org

[ananta, 2017-01-04 02:16:20]

Description was changed from

==========
Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click for e.g. we get a navigation
started notification for the frame. We should not abort the stream here as
this is the same navigation which created the stream.

Should fix the black PDF issues with M55.

BUG=677580
==========

to

==========
Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click with a history.replacestate call
in the pagehide event for the main html page, we get a DidStartNavigation
notification on the guest webcontents created for the EmbeddedObserver. 
Currently we abort the stream if it is a top level navigation. 
It seems like we should not abort the stream if the navigation is within the
same page.

Should fix the black PDF issues with M55.

BUG=677580
==========

[ananta, 2017-01-04 02:16:50]

Description was changed from

==========
Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click with a history.replacestate call
in the pagehide event for the main html page, we get a DidStartNavigation
notification on the guest webcontents created for the EmbeddedObserver. 
Currently we abort the stream if it is a top level navigation. 
It seems like we should not abort the stream if the navigation is within the
same page.

Should fix the black PDF issues with M55.

BUG=677580
==========

to

==========
Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click with a history.replacestate call
in the pagehide event for the main html page, we get a DidStartNavigation
notification on the guest webcontents created for the EmbeddedObserver. 
Currently we abort the stream if it is a top level navigation. 
It seems like we should not abort the stream if the navigation is within the
same page.

The problem does not occur if the PDF is opened in a different tab or window

Should fix the black PDF issues with M55.

BUG=677580
==========

[ananta, 2017-01-04 03:18:46]

Description was changed from

==========
Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click with a history.replacestate call
in the pagehide event for the main html page, we get a DidStartNavigation
notification on the guest webcontents created for the EmbeddedObserver. 
Currently we abort the stream if it is a top level navigation. 
It seems like we should not abort the stream if the navigation is within the
same page.

The problem does not occur if the PDF is opened in a different tab or window

Should fix the black PDF issues with M55.

BUG=677580
==========

to

==========
Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click with a history.replacestate call
in the pagehide/pageshow event for the main html page, we get a
DidStartNavigation
notification on the guest webcontents created for the EmbeddedObserver. 
Currently we abort the stream if it is a top level navigation. 
It seems like we should not abort the stream if the navigation is within the
same page.

The problem does not occur if the PDF is opened in a different tab or window

Should fix the black PDF issues with M55.

BUG=677580
==========

[ananta, 2017-01-04 03:20:00]

On 2017/01/04 02:12:08, ananta wrote:

Thanks to lazyboy for bringing up the possibility of this bug occurring with the
pageshow event as well and for the test case. We verified that
the patch fixes it for both cases.

[lazyboy, 2017-01-04 04:06:52]

On 2017/01/04 03:20:00, ananta wrote:
> On 2017/01/04 02:12:08, ananta wrote:
> 
> Thanks to lazyboy for bringing up the possibility of this bug occurring with
the
> pageshow event as well and for the test case. We verified that
> the patch fixes it for both cases.

From my digging around, this seems fine, although a bit tricky for me to be
entirely
comfortable reviewing this. I suggested ananta@ to add Charlie in case there are
other edge cases to worry about.

For reference, the pagehide example is:
From main page:
window.addEventListener('pagehide', function(e) {
history.replaceState({time:+new Date}, ''); });
then open a <a href="any pdf">..</a> link from main page.

And the pageshow example is:
From a main page button's event handler, do:
var w = window.open('same_site_pdf_link')
w.addEventListener('pageshow', function(e) { history.replaceState({time: +new
Date}, ''); });

In both cases, history.replaceState ends up producing a WCO::DidStartNavigation
with
NavigationHandle::IsSamePage() = true

[Charlie Reis, 2017-01-04 18:38:43]

Seems reasonable to me-- for any same-page navigation, we won't be leaving the
page, and thus the stream is still needed.  LGTM, but please add a browser test
for it if you can (since there's a simple HTML file to repro it).

[ananta, 2017-01-05 03:40:17]

Description was changed from

==========
Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click with a history.replacestate call
in the pagehide/pageshow event for the main html page, we get a
DidStartNavigation
notification on the guest webcontents created for the EmbeddedObserver. 
Currently we abort the stream if it is a top level navigation. 
It seems like we should not abort the stream if the navigation is within the
same page.

The problem does not occur if the PDF is opened in a different tab or window

Should fix the black PDF issues with M55.

BUG=677580
==========

to

==========
Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click with a history.replacestate call
in the pagehide/pageshow event for the main html page, we get a
DidStartNavigation
notification on the guest webcontents created for the EmbeddedObserver. 
Currently we abort the stream if it is a top level navigation. 
It seems like we should not abort the stream if the navigation is within the
same page.

The problem does not occur if the PDF is opened in a different tab or window

Should fix the black PDF issues with M55.

BUG=677580
TEST=Covered by browser test PDFExtensionTest.OpenPDFOnLinkClickWithReplaceState
==========

[ananta, 2017-01-05 03:40:28]

On 2017/01/04 18:38:43, Charlie Reis wrote:
> Seems reasonable to me-- for any same-page navigation, we won't be leaving the
> page, and thus the stream is still needed.  LGTM, but please add a browser
test
> for it if you can (since there's a simple HTML file to repro it).

Added a test

[ananta, 2017-01-05 03:40:59]

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

[commit-bot: I haz the power, 2017-01-05 03:41:22]

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

[commit-bot: I haz the power, 2017-01-05 03:43:18]

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

[commit-bot: I haz the power, 2017-01-05 03:43:19]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[lazyboy, 2017-01-05 04:03:14]

lgtm w/ nits

https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:927: std::string pdf_link_click =
"document.getElementById('link').click();";
nit: kPdfLinkClick

https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:932:
ASSERT_TRUE(current_url.spec().find("pdf/test-link.pdf") !=
current_url.path() == "/pdf/test-link.pdf"

https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:972:
ASSERT_EQ(std::string("http://www.example.com/"), url.spec());
nit: Maybe ASSERT_EQ(GURL("http://www.example.com"), url) is better?

[ananta, 2017-01-05 04:28:57]

https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:927: std::string pdf_link_click =
"document.getElementById('link').click();";
On 2017/01/05 04:03:14, lazyboy wrote:
> nit: kPdfLinkClick

Done.

https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:932:
ASSERT_TRUE(current_url.spec().find("pdf/test-link.pdf") !=
On 2017/01/05 04:03:14, lazyboy wrote:
> current_url.path() == "/pdf/test-link.pdf"

Done.

https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:972:
ASSERT_EQ(std::string("http://www.example.com/"), url.spec());
On 2017/01/05 04:03:14, lazyboy wrote:
> nit: Maybe ASSERT_EQ(GURL("http://www.example.com"), url) is better?

Done.

[ananta, 2017-01-05 04:29:07]

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

[commit-bot: I haz the power, 2017-01-05 04:29:31]

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

[commit-bot: I haz the power, 2017-01-05 04:52:01]

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

[commit-bot: I haz the power, 2017-01-05 04:52:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2017-01-05 22:19:03]

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

[commit-bot: I haz the power, 2017-01-05 22:19:50]

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

[commit-bot: I haz the power, 2017-01-05 23:46:23]

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

[commit-bot: I haz the power, 2017-01-05 23:46:24]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-01-05 23:49:15]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-01-05 23:49:15]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2611643004/#ps60001
(title: "Fix redness.")

[commit-bot: I haz the power, 2017-01-05 23:49:59]

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

[commit-bot: I haz the power, 2017-01-05 23:57:41]

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

[commit-bot: I haz the power, 2017-01-05 23:57:42]

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

[ananta, 2017-01-05 23:59:19]

ananta@chromium.org changed reviewers:
+ thestig@chromium.org

[ananta, 2017-01-05 23:59:20]

+thestig for chrome\browser\pdf owners

[ananta, 2017-01-06 00:28:57]

ananta@chromium.org changed reviewers:
+ dsinclair@chromium.org

[ananta, 2017-01-06 00:29:17]

+dsinclair for pdf owners

[ananta, 2017-01-06 02:40:53]

ananta@chromium.org changed reviewers:
+ jochen@chromium.org

[ananta, 2017-01-06 02:41:10]

+jochen for the pdf test changes

[dsinclair, 2017-01-06 02:46:57]

pdf lgtm

[ananta, 2017-01-06 02:54:57]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2017-01-06 02:55:09]

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

[commit-bot: I haz the power, 2017-01-06 02:59:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483671297002970,
"parent_rev": "ed3ef90ecc3c01246a3d838fbeefdab72c94c228", "commit_rev":
"52df16bebd27db88af89d12460db4b4c857aeebb"}

[commit-bot: I haz the power, 2017-01-06 03:00:05]

Description was changed from

==========
Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click with a history.replacestate call
in the pagehide/pageshow event for the main html page, we get a
DidStartNavigation
notification on the guest webcontents created for the EmbeddedObserver. 
Currently we abort the stream if it is a top level navigation. 
It seems like we should not abort the stream if the navigation is within the
same page.

The problem does not occur if the PDF is opened in a different tab or window

Should fix the black PDF issues with M55.

BUG=677580
TEST=Covered by browser test PDFExtensionTest.OpenPDFOnLinkClickWithReplaceState
==========

to

==========
Don't abort the PDF stream if a navigation is intiated within the same page.

If a PDF file is loaded via a link click with a history.replacestate call
in the pagehide/pageshow event for the main html page, we get a
DidStartNavigation
notification on the guest webcontents created for the EmbeddedObserver. 
Currently we abort the stream if it is a top level navigation. 
It seems like we should not abort the stream if the navigation is within the
same page.

The problem does not occur if the PDF is opened in a different tab or window

Should fix the black PDF issues with M55.

BUG=677580
TEST=Covered by browser test PDFExtensionTest.OpenPDFOnLinkClickWithReplaceState

Review-Url: https://codereview.chromium.org/2611643004
Cr-Commit-Position: refs/heads/master@{#441857}
Committed:
https://chromium.googlesource.com/chromium/src/+/52df16bebd27db88af89d12460db...
==========

[commit-bot: I haz the power, 2017-01-06 03:00:06]

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