|
|
Created:
3 years, 6 months ago by arthursonzogni Modified:
3 years, 6 months ago CC:
chromium-reviews, clamy, nasko, jam, dcheng Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrinting: 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. #
Messages
Total messages: 53 (39 generated)
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== 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 main frame or subframes resources. For this reason, it gets killed in content::ResourceDispatcherHostImpl::BeginRequest Note: The main frame navigation works because its url is a data-url. Here, data-urls doesn't go to the browser and can be resolved on the renderer. BUG=732780 ========== to ========== 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 main frame or subframes resources. For this reason, it gets killed in content::ResourceDispatcherHostImpl::BeginRequest Note: The main frame navigation works because its url is a data-url. Here, data-urls doesn't go to the browser and can be resolved on the renderer. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel BUG=732780 ==========
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== 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 main frame or subframes resources. For this reason, it gets killed in content::ResourceDispatcherHostImpl::BeginRequest Note: The main frame navigation works because its url is a data-url. Here, data-urls doesn't go to the browser and can be resolved on the renderer. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel BUG=732780 ========== to ========== 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 main frame or subframes resources. 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 ==========
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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 main frame or subframes resources. 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 ========== to ========== 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 ==========
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Patchset #2 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
arthursonzogni@chromium.org changed reviewers: + thestig@chromium.org
Hi thestig@, Could you please review this CL? I didn't found any test base I could reuse easily for this test. If you know one or want me to move this test anywhere else, let me know.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Interestingly, this worked until r414879. So is the idea that since this is broken as is anyway, just disable it further? https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc (right): https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:40: class PrintPreviewUiBrowserTest : public InProcessBrowserTest { Can you name this PrintPreviewUIBrowserTest since the class it's associated with is named PrintPreviewUI?
On 2017/06/20 17:46:57, arthursonzogni wrote: > I didn't found any test base I could reuse easily for this test. If you know one > or want me to move this test anywhere else, let me know. There's components/printing/test/print_web_view_helper_browsertest.cc but that's not going to work if you are using PrintPreviewUI::TestingDelegate. So I guess new test it is.
https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... File chrome/browser/printing/print_preview_browsertest.cc (right): https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... 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/printin... chrome/browser/printing/print_preview_browsertest.cc:20: namespace { nit: new line after, and before the end of the anonymous namespace. Just like with namespace printing. https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:24: ~PrintPreviewObserver() { PrintPreviewUI::SetDelegateForTesting(NULL); } nit: s/NULL/nullptr/ https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:39: } nit: blank line after. https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:64: false, /* print_preview_disabled */ I've been told the latest and greatest way is to write: /* print_preview_disabled=*/false https://codereview.chromium.org/2944873002/diff/100001/chrome/test/data/print... File chrome/test/data/printing/selection_iframe.html (right): https://codereview.chromium.org/2944873002/diff/100001/chrome/test/data/print... chrome/test/data/printing/selection_iframe.html:17: window.getSelection().addRange(range); Neat. Did not know JS + DOM operations supported this.
Patchset #4 (id:120001) has been deleted
Thank you for the review. A released a new patch. On 2017/06/21 07:25:08, Lei Zhang wrote: > Interestingly, this worked until r414879. So is the idea that since this is > broken as is anyway, just disable it further? I didn't know it has worked before. Do we know why it doesn't work anymore? Note: r414879 => https://codereview.chromium.org/2285883002 So Yes, the idea is that since it is broken, we can at least disable it such that it doesn't crash with PlzNavigate. The alternative would be to make it working ;-) but that looks hard to do for the moment.
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... File chrome/browser/printing/print_preview_browsertest.cc (right): https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... 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: > Not used? Yes, thanks! Done. https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:20: namespace { On 2017/06/21 07:36:32, Lei Zhang wrote: > nit: new line after, and before the end of the anonymous namespace. Just like > with namespace printing. Done. https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:24: ~PrintPreviewObserver() { PrintPreviewUI::SetDelegateForTesting(NULL); } On 2017/06/21 07:36:32, Lei Zhang wrote: > nit: s/NULL/nullptr/ Done. https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:39: } On 2017/06/21 07:36:32, Lei Zhang wrote: > nit: blank line after. Done. https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:64: false, /* print_preview_disabled */ On 2017/06/21 07:36:32, Lei Zhang wrote: > I've been told the latest and greatest way is to write: > > /* print_preview_disabled=*/false Done. https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc (right): https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:40: class PrintPreviewUiBrowserTest : public InProcessBrowserTest { On 2017/06/21 07:25:08, Lei Zhang wrote: > Can you name this PrintPreviewUIBrowserTest since the class it's associated with > is named PrintPreviewUI? Done. https://codereview.chromium.org/2944873002/diff/100001/chrome/test/data/print... File chrome/test/data/printing/selection_iframe.html (right): https://codereview.chromium.org/2944873002/diff/100001/chrome/test/data/print... chrome/test/data/printing/selection_iframe.html:17: window.getSelection().addRange(range); On 2017/06/21 07:36:32, Lei Zhang wrote: > Neat. Did not know JS + DOM operations supported this. Yes I was surprised too. I did not know either before.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
+dcheng FYI Sadly the PrintPreviewUIBrowserTests are broken. I don't know exactly why it's broken, but I suspect printing the selection is done in OOPIF mode, and multi-frame OOPIF printing is known to not work in the manner seen on this bug.
I don't think that's something serious. I think it is because I renamed PrintPreviewUiBrowserTests -> PrintPreviewUIBrowserTests. PrintPreviewUIBrowserTests name is already taken by some generated tests. See print_preview_ui_browsertest-gen.cc I think I will in the next patch keep PrintPreviewBrowserTests as it was and rename my test PrintingBrowsertests or something like that.
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/21 15:39:57, arthursonzogni wrote: > I don't think that's something serious. > > I think it is because I renamed PrintPreviewUiBrowserTests -> > PrintPreviewUIBrowserTests. > PrintPreviewUIBrowserTests name is already taken by some generated tests. > See print_preview_ui_browsertest-gen.cc > > I think I will in the next patch keep PrintPreviewBrowserTests as it was and > rename my test PrintingBrowsertests or something like that. OK, I will rename everything as a follow up to sanitize it all.
lgtm Let's wait a bit and see if dcheng@ or anyone else has additional comments. I can put this in CQ for you later in the day. https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... File chrome/browser/printing/print_preview_browsertest.cc (right): https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:64: false, /* print_preview_disabled */ On 2017/06/21 14:01:20, arthursonzogni wrote: > On 2017/06/21 07:36:32, Lei Zhang wrote: > > I've been told the latest and greatest way is to write: > > > > /* print_preview_disabled=*/false > > Done. I didn't write it with the right spacing. I meant to write: /*print_preview_disabled=*/false with no spaces. Run: git grep '=\*/' to see other examples. https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:80: PrintAndWaitUntilPreviewIsReady(true /* print_only_selection */); Same nit as above.
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/21 16:17:48, Lei Zhang wrote: > Let's wait a bit and see if dcheng@ or anyone else has additional comments. I > can put this in CQ for you later in the day. Okay, thanks! https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... File chrome/browser/printing/print_preview_browsertest.cc (right): https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:64: false, /* print_preview_disabled */ On 2017/06/21 16:17:48, Lei Zhang wrote: > On 2017/06/21 14:01:20, arthursonzogni wrote: > > On 2017/06/21 07:36:32, Lei Zhang wrote: > > > I've been told the latest and greatest way is to write: > > > > > > /* print_preview_disabled=*/false > > > > Done. > > I didn't write it with the right spacing. I meant to write: > > /*print_preview_disabled=*/false > > with no spaces. Run: > > git grep '=\*/' > > to see other examples. Done. https://codereview.chromium.org/2944873002/diff/100001/chrome/browser/printin... chrome/browser/printing/print_preview_browsertest.cc:80: PrintAndWaitUntilPreviewIsReady(true /* print_only_selection */); On 2017/06/21 16:17:48, Lei Zhang wrote: > Same nit as above. Done.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
changes to blink api usage lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2944873002/#ps190001 (title: "Nits.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 190001, "attempt_start_ts": 1498076736579670, "parent_rev": "75660bb3a3fa76d365bef335b4d428efd2da37de", "commit_rev": "6a22ae37ea816b3dd750a17fe91d23e562cdb671"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/6a22ae37ea816b3dd750a17fe91d... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:190001) as https://chromium.googlesource.com/chromium/src/+/6a22ae37ea816b3dd750a17fe91d... |