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

Issue 2944873002: Printing: Disable subframes when printing a selection. (Closed)

Created:
3 years, 6 months ago by arthursonzogni
Modified:
3 years, 6 months ago
Reviewers:
Lei Zhang, dcheng
CC:
chromium-reviews, clamy, nasko, jam, dcheng
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Printing: Disable subframes when printing a selection. Printing a selection that contains an iframe doesn't work. An empty rectangle is printed instead. This CL disable creation of subframes in PrepareFrameForPreviewDocument. Without this CL, the creation of the subframe lead to a new navigation to the iframe's src. This can't work with --enable-browser-side-navigation (aka PlzNavigate) because it expects the WebFrameClient to be implemented by the RenderFrameImpl. In that mode, the renderer is not expected to ask the browser for the subframe resource. For this reason, it gets killed in content::ResourceDispatcherHostImpl::BeginRequest Note: The main frame navigation works because the url is a data-url and doesn't have to go to the browser. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel BUG=732780 Review-Url: https://codereview.chromium.org/2944873002 Cr-Commit-Position: refs/heads/master@{#481291} Committed: https://chromium.googlesource.com/chromium/src/+/6a22ae37ea816b3dd750a17fe91d23e562cdb671

Patch Set 1 : Printing: Disable subframes when printing a selection. #

Patch Set 2 : Add Test #

Patch Set 3 : Fix names conflicts. #

Total comments: 18

Patch Set 4 : Rebase. #

Patch Set 5 : Addressed comments thestig@ #

Patch Set 6 : Fix name clashes #

Patch Set 7 : Nits. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+113 lines, -3 lines) Patch
A chrome/browser/printing/print_browsertest.cc View 1 2 3 4 5 6 1 chunk +85 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 2 chunks +2 lines, -0 lines 0 comments Download
A chrome/test/data/printing/selection_iframe.html View 1 1 chunk +21 lines, -0 lines 0 comments Download
M components/printing/renderer/print_web_view_helper.cc View 1 2 3 1 chunk +5 lines, -3 lines 0 comments Download

Messages

Total messages: 53 (39 generated)
arthursonzogni
Hi thestig@, Could you please review this CL? I didn't found any test base I ...
3 years, 6 months ago (2017-06-20 17:46:57 UTC) #22
Lei Zhang
Interestingly, this worked until r414879. So is the idea that since this is broken as ...
3 years, 6 months ago (2017-06-21 07:25:08 UTC) #25
Lei Zhang
On 2017/06/20 17:46:57, arthursonzogni wrote: > I didn't found any test base I could reuse ...
3 years, 6 months ago (2017-06-21 07:29:10 UTC) #26
Lei Zhang
https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printing/print_preview_browsertest.cc File chrome/browser/printing/print_preview_browsertest.cc (right): https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printing/print_preview_browsertest.cc#newcode15 chrome/browser/printing/print_preview_browsertest.cc:15: #include "net/dns/mock_host_resolver.h" Not used? https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printing/print_preview_browsertest.cc#newcode20 chrome/browser/printing/print_preview_browsertest.cc:20: namespace { nit: ...
3 years, 6 months ago (2017-06-21 07:36:32 UTC) #27
arthursonzogni
Thank you for the review. A released a new patch. On 2017/06/21 07:25:08, Lei Zhang ...
3 years, 6 months ago (2017-06-21 11:39:44 UTC) #29
arthursonzogni
https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printing/print_preview_browsertest.cc File chrome/browser/printing/print_preview_browsertest.cc (right): https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printing/print_preview_browsertest.cc#newcode15 chrome/browser/printing/print_preview_browsertest.cc:15: #include "net/dns/mock_host_resolver.h" On 2017/06/21 07:36:32, Lei Zhang wrote: > ...
3 years, 6 months ago (2017-06-21 14:01:20 UTC) #31
Lei Zhang
+dcheng FYI Sadly the PrintPreviewUIBrowserTests are broken. I don't know exactly why it's broken, but ...
3 years, 6 months ago (2017-06-21 15:30:34 UTC) #35
arthursonzogni
I don't think that's something serious. I think it is because I renamed PrintPreviewUiBrowserTests -> ...
3 years, 6 months ago (2017-06-21 15:39:57 UTC) #36
Lei Zhang
On 2017/06/21 15:39:57, arthursonzogni wrote: > I don't think that's something serious. > > I ...
3 years, 6 months ago (2017-06-21 16:14:50 UTC) #39
Lei Zhang
lgtm Let's wait a bit and see if dcheng@ or anyone else has additional comments. ...
3 years, 6 months ago (2017-06-21 16:17:48 UTC) #40
arthursonzogni
On 2017/06/21 16:17:48, Lei Zhang wrote: > Let's wait a bit and see if dcheng@ ...
3 years, 6 months ago (2017-06-21 16:26:22 UTC) #43
dcheng
changes to blink api usage lgtm
3 years, 6 months ago (2017-06-21 17:07:22 UTC) #45
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/2944873002/190001
3 years, 6 months ago (2017-06-21 20:25:58 UTC) #50
commit-bot: I haz the power
3 years, 6 months ago (2017-06-21 20:33:05 UTC) #53
Message was sent while issue was closed.
Committed patchset #7 (id:190001) as
https://chromium.googlesource.com/chromium/src/+/6a22ae37ea816b3dd750a17fe91d...

Powered by Google App Engine
This is Rietveld 408576698