Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(24)

Issue 2611643004: Don't abort the PDF stream if a navigation is intiated within the same page. (Closed)

Created:
3 years, 11 months ago by ananta
Modified:
3 years, 11 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Update test #

Total comments: 6

Patch Set 3 : Address review comments #

Patch Set 4 : Fix redness. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+80 lines, -1 line) Patch
M chrome/browser/pdf/pdf_extension_test.cc View 1 2 3 2 chunks +65 lines, -0 lines 0 comments Download
A chrome/test/data/pdf/pdf_href_replace_state.html View 1 2 1 chunk +14 lines, -0 lines 0 comments Download
M extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 47 (31 generated)
ananta
3 years, 11 months ago (2017-01-03 23:30:03 UTC) #2
ananta
3 years, 11 months ago (2017-01-04 02:12:08 UTC) #8
ananta
On 2017/01/04 02:12:08, ananta wrote: Thanks to lazyboy for bringing up the possibility of this ...
3 years, 11 months ago (2017-01-04 03:20:00 UTC) #12
lazyboy
On 2017/01/04 03:20:00, ananta wrote: > On 2017/01/04 02:12:08, ananta wrote: > > Thanks to ...
3 years, 11 months ago (2017-01-04 04:06:52 UTC) #13
Charlie Reis
Seems reasonable to me-- for any same-page navigation, we won't be leaving the page, and ...
3 years, 11 months ago (2017-01-04 18:38:43 UTC) #14
ananta
On 2017/01/04 18:38:43, Charlie Reis wrote: > Seems reasonable to me-- for any same-page navigation, ...
3 years, 11 months ago (2017-01-05 03:40:28 UTC) #16
lazyboy
lgtm w/ nits https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_extension_test.cc File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_extension_test.cc#newcode927 chrome/browser/pdf/pdf_extension_test.cc:927: std::string pdf_link_click = "document.getElementById('link').click();"; nit: kPdfLinkClick ...
3 years, 11 months ago (2017-01-05 04:03:14 UTC) #21
ananta
https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_extension_test.cc File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2611643004/diff/20001/chrome/browser/pdf/pdf_extension_test.cc#newcode927 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: ...
3 years, 11 months ago (2017-01-05 04:28:57 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2611643004/60001
3 years, 11 months ago (2017-01-05 23:49:59 UTC) #33
commit-bot: I haz the power
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_presubmit/builds/335990)
3 years, 11 months ago (2017-01-05 23:57:42 UTC) #35
ananta
+thestig for chrome\browser\pdf owners
3 years, 11 months ago (2017-01-05 23:59:20 UTC) #37
ananta
+dsinclair for pdf owners
3 years, 11 months ago (2017-01-06 00:29:17 UTC) #39
ananta
+jochen for the pdf test changes
3 years, 11 months ago (2017-01-06 02:41:10 UTC) #41
dsinclair
pdf lgtm
3 years, 11 months ago (2017-01-06 02:46:57 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2611643004/60001
3 years, 11 months ago (2017-01-06 02:55:09 UTC) #44
commit-bot: I haz the power
3 years, 11 months ago (2017-01-06 03:00:06 UTC) #47
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/52df16bebd27db88af89d12460db...

Powered by Google App Engine
This is Rietveld 408576698