Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2566153007:
Implement PrintBrowser functionality. (Closed)
DescriptionImplement PrintBrowser functionality.
The --enable-print-browser flag triggers logic to display every page in
print preview mode.
BUG=667547
Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82nVmoWbucz1LYZtu8/edit?usp=sharing
CQ-DEPEND=CL:2556023002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #Patch Set 2 : Implement PrintBrowser functionality. #Patch Set 3 : Implement PrintBrowser functionality. #Patch Set 4 : Fix git weirdness #
Total comments: 6
Depends on Patchset: Messages
Total messages: 35 (25 generated)
Description was changed from ========== Implement PrintBrowser functionality. The --enable-print-browser flag triggers logic to display every page in print preview mode. BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... ========== to ========== Implement PrintBrowser functionality. The --enable-print-browser flag triggers logic to display every page in print preview mode. BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Implement PrintBrowser functionality. The --enable-print-browser flag triggers logic to display every page in print preview mode. BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement PrintBrowser functionality. The --enable-print-browser flag triggers logic to display every page in print preview mode. This issue is dependent on crrev.com/2556023002 BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
gozzard@google.com changed reviewers: + vitalybuka@chromium.org
PTAL
On 2016/12/14 23:09:32, gozzard wrote: > PTAL Probably you want CQ-DEPEND=CL:2556023002 instead of "This issue is dependent..." text
On 2016/12/15 00:05:00, Vitaly Buka wrote: > On 2016/12/14 23:09:32, gozzard wrote: > > PTAL > > Probably you want CQ-DEPEND=CL:2556023002 instead of "This issue is > dependent..." text Also better to keep design dock link inside of the bug instead of adding into every git commit
If this is for tests, could we have a trivial test, probably browser test, which demonstrates it?
On 2016/12/15 at 00:18:16, vitalybuka wrote: > If this is for tests, could we have a trivial test, probably browser test, which demonstrates it? The investigation and implementation of a testing infrastructure built around PrintBrowser mode is the next phase of this project. Currently I am unable to given an ETA for when such an example test could be available.
The CQ bit was checked by vitalybuka@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: 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...)
Description was changed from ========== Implement PrintBrowser functionality. The --enable-print-browser flag triggers logic to display every page in print preview mode. This issue is dependent on crrev.com/2556023002 BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement PrintBrowser functionality. The --enable-print-browser flag triggers logic to display every page in print preview mode. BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... CQ-DEPEND=CL:2556023002 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by gozzard@google.com 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 gozzard@google.com
The CQ bit was checked by gozzard@google.com 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: 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...)
The CQ bit was checked by gozzard@google.com 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: 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_...) linux_chromium_chromeos_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 gozzard@google.com 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 ========== Implement PrintBrowser functionality. The --enable-print-browser flag triggers logic to display every page in print preview mode. BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... CQ-DEPEND=CL:2556023002 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement PrintBrowser functionality. The --enable-print-browser flag triggers logic to display every page in print preview mode. BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... CQ-DEPEND=CL:2556023002 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
vitalybuka@chromium.org changed reviewers: + rbpotter@chromium.org, skau@chromium.org
+rbpotter +skau Folks, please review this as well. This probably affect your work. Maybe you need to discuss this with gozzard@. I will approve code changes if you are OK with them.
PrintPreview does quite a few things IRT retrieving printer properties. Is it possible to extract preview rendering as a separate component and integrate the mode that way? I'm worried that this integration could be quite brittle. https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... File chrome/browser/printing/print_preview_message_handler.cc (right): https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... chrome/browser/printing/print_preview_message_handler.cc:251: void PrintPreviewMessageHandler::DidFinishLoad( This seems to have a limited relationship to PrintPreviewMessageHandler. Does this belong in a different class? https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... chrome/browser/printing/print_preview_message_handler.cc:260: printing::StartPrint(initiator, false /* print_preview_disabled */, You should enforce that Print Preview is enabled for Print Browser to be enabled. Otherwise, this could send a print job every time you load a page. https://codereview.chromium.org/2566153007/diff/60001/chrome/renderer/printin... File chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc (left): https://codereview.chromium.org/2566153007/diff/60001/chrome/renderer/printin... chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc:54: if (!plugin_element.isNull()) { Why is there a situation where this is null now?
https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller.cc:406: CoreTabHelper::FromWebContents(initiator)->delegate()->SwapTabContents( I think you need to ensure the initiator web contents are destroyed at an appropriate point after calling this. https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... File chrome/browser/printing/print_preview_message_handler.cc (right): https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... chrome/browser/printing/print_preview_message_handler.cc:260: printing::StartPrint(initiator, false /* print_preview_disabled */, On 2016/12/16 18:32:50, skau wrote: > You should enforce that Print Preview is enabled for Print Browser to be > enabled. Otherwise, this could send a print job every time you load a page. In this case it may be implicitly enforced; I don't think PrintPreviewMessageHandler gets created if Print Preview isn't enabled; it's created in tab_helper.cc only if the ENABLE_PRINT_PREVIEW build flag is on. https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:25: $('navbar-container').style.display = 'none'; Not sure that this stops all the events from the navbar. If you open print preview, apply this styling, then hit "enter" you can still print the page, which seems unexpected.
On 2016/12/16 at 18:32:50, skau wrote: > PrintPreview does quite a few things IRT retrieving printer properties. Is it possible to extract preview rendering as a separate component and integrate the mode that way? I'm worried that this integration could be quite brittle. > > https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... > File chrome/browser/printing/print_preview_message_handler.cc (right): > > https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... > chrome/browser/printing/print_preview_message_handler.cc:251: void PrintPreviewMessageHandler::DidFinishLoad( > This seems to have a limited relationship to PrintPreviewMessageHandler. Does this belong in a different class? I thought this appropriate as it handles a message relating to a print preview component, and this class already implements OnMessageReceived from WebContentsObserver. If you have somewhere else you would prefer this goes I am happy to move it. > https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... > chrome/browser/printing/print_preview_message_handler.cc:260: printing::StartPrint(initiator, false /* print_preview_disabled */, > You should enforce that Print Preview is enabled for Print Browser to be enabled. Otherwise, this could send a print job every time you load a page. Do you mean the corresponding buildflags? crrev.com/2556023002 enforces that Print Browser functionality can not be built without print preview (though the exact method by which it enforces this is likely to change). > https://codereview.chromium.org/2566153007/diff/60001/chrome/renderer/printin... > File chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc (left): > > https://codereview.chromium.org/2566153007/diff/60001/chrome/renderer/printin... > chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc:54: if (!plugin_element.isNull()) { > Why is there a situation where this is null now? I previously thought this was because we were actually trying to extract the plugin from the background frame, which does not exist here because we have opened print preview in its own tab. Upon review to address your question I now think that this is being triggered because I am (accidentally) trying to automatically print the print preview window itself before the JS has run. I will investigate and fix.
On 2016/12/16 at 20:05:29, rbpotter wrote: > https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... > File chrome/browser/printing/print_preview_dialog_controller.cc (right): > > https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... > chrome/browser/printing/print_preview_dialog_controller.cc:406: CoreTabHelper::FromWebContents(initiator)->delegate()->SwapTabContents( > I think you need to ensure the initiator web contents are destroyed at an appropriate point after calling this. I was originally told this was out of scope as, for the intended use case, we are just going to be closing the browser entirely anyway. Having said that I am currently investigating how to reliably clean up the initiator. They have to remain long enough to be used as a source to render from, so I will probably have to delete them in reaction to OnNavEntryCommitted and OnWebContentsDestroyed. TL;DR: Working on it. > https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... > File chrome/browser/printing/print_preview_message_handler.cc (right): > > https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/printing... > chrome/browser/printing/print_preview_message_handler.cc:260: printing::StartPrint(initiator, false /* print_preview_disabled */, > On 2016/12/16 18:32:50, skau wrote: > > You should enforce that Print Preview is enabled for Print Browser to be > > enabled. Otherwise, this could send a print job every time you load a page. > > In this case it may be implicitly enforced; I don't think PrintPreviewMessageHandler gets created if Print Preview isn't enabled; it's created in tab_helper.cc only if the ENABLE_PRINT_PREVIEW build flag is on. This is correct, and the ENABLE_PRINT_BROWSER flag is explicitly dependent on the ENABLE_PRINT_PREVIEW build flag. > > https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/resource... > File chrome/browser/resources/print_preview/print_preview.js (right): > > https://codereview.chromium.org/2566153007/diff/60001/chrome/browser/resource... > chrome/browser/resources/print_preview/print_preview.js:25: $('navbar-container').style.display = 'none'; > Not sure that this stops all the events from the navbar. If you open print preview, apply this styling, then hit "enter" you can still print the page, which seems unexpected. Good catch. I will try to find a more robust way to do this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
