Fixed print_web_view_helper for CEF.

Fixed print_web_view_helper for CEF.

Restored disabling preview.
Restored printing throttling.

Restoring printing without print preview removed in https://codereview.chromium.org/488853002

Restored code is not used in Chrome or Chromium browsers.

BUG=374321
TBR=gene@chromium.org

Committed: https://crrev.com/f470ae06577e8f07c0664c1a8997b5d219401882
Cr-Commit-Position: refs/heads/master@{#294063}

[Vitaly Buka (NO REVIEWS), 2014-09-08 09:32:24]

vitalybuka@chromium.org changed reviewers:
+ marshall@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-09-08 09:43:59]

Hi Marshall,
I had no time to finish that, but I'll do that Monday.

This is your patch with some my comments.

https://codereview.chromium.org/550033002/diff/20001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/550033002/diff/20001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:778: bool preview_enabled,
preview_enabled is always false because you build without ENABLE_FULL_PRINTING.

Do you think scripted_print_throttling_disabled is necessary?

https://codereview.chromium.org/550033002/diff/20001/printing/printing_contex...
File printing/printing_context_win.h (right):

https://codereview.chromium.org/550033002/diff/20001/printing/printing_contex...
printing/printing_context_win.h:44: static bool AllocateContext(const
std::wstring& printer_name,
I'll restore this under !DISABLE_BASIC_PRINTING which would be defined on
Windows

[Vitaly Buka (NO REVIEWS), 2014-09-09 06:58:38]

PTAL

[Vitaly Buka (NO REVIEWS), 2014-09-09 07:48:51]

Patchset #4 (id:60001) has been deleted

[Vitaly Buka (NO REVIEWS), 2014-09-09 07:48:56]

Patchset #3 (id:40001) has been deleted

[Marshall, 2014-09-09 16:24:51]

In combination with the commented changes related to ENABLE_FULL_PRINTING and
https://codereview.chromium.org/557663002/ I verified that the following works
from CEF on Windows:

1. Print via JS window.print().
2. Print via printing::PrintViewManager::PrintNow().
3. Print via in-process PDF plugin menu bar.

https://codereview.chromium.org/550033002/diff/20001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/550033002/diff/20001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:778: bool preview_enabled,
On 2014/09/08 09:43:59, Vitaly Buka wrote:
> preview_enabled is always false because you build without
ENABLE_FULL_PRINTING.

Building without ENABLE_FULL_PRINTING is problematic currently.

1. Compilation will fail in chrome/common/chrome_utility_printing_messages.h if
ENABLE_FULL_PRINTING is not enabled. CEF shares this file with Chrome for the
PDF utility process implementation.

2. Printing from a PDF file will fail because code in
content/renderer/pepper/pepper_plugin_instance_impl.cc requires
ENABLE_FULL_PRINTING. To reproduce: Load a PDF file as a in-process plugin and
click the Print button from the PDF toolbar.

Is it OK to remove the checks for ENABLE_FULL_PRINTING in these files?

> Do you think scripted_print_throttling_disabled is necessary?

It seems that scripted print throttling is intended to protect against code like
this:

setInterval(function(){ window.print(); }, 1000);

Without any throttling the print dialog will be displayed again before the
previous print operation has completed. For example, if you select print to file
the next print dialog will displayed while the "Save As" file dialog is still
displayed.

[Vitaly Buka (NO REVIEWS), 2014-09-09 18:04:30]

On 2014/09/09 16:24:51, Marshall wrote:
> In combination with the commented changes related to ENABLE_FULL_PRINTING and
> https://codereview.chromium.org/557663002/ I verified that the following works
> from CEF on Windows:
> 
> 1. Print via JS window.print().
> 2. Print via printing::PrintViewManager::PrintNow().
> 3. Print via in-process PDF plugin menu bar.
> 
>
https://codereview.chromium.org/550033002/diff/20001/chrome/renderer/printing...
> File chrome/renderer/printing/print_web_view_helper.cc (right):
> 
>
https://codereview.chromium.org/550033002/diff/20001/chrome/renderer/printing...
> chrome/renderer/printing/print_web_view_helper.cc:778: bool preview_enabled,
> On 2014/09/08 09:43:59, Vitaly Buka wrote:
> > preview_enabled is always false because you build without
> ENABLE_FULL_PRINTING.
> 
> Building without ENABLE_FULL_PRINTING is problematic currently.

Actually I just recovered building without ENABLE_FULL_PRINTING
Last piece is https://codereview.chromium.org/549423002/

> 
> 1. Compilation will fail in chrome/common/chrome_utility_printing_messages.h
if
> ENABLE_FULL_PRINTING is not enabled. CEF shares this file with Chrome for the
> PDF utility process implementation.
> 
> 2. Printing from a PDF file will fail because code in
> content/renderer/pepper/pepper_plugin_instance_impl.cc requires
> ENABLE_FULL_PRINTING. To reproduce: Load a PDF file as a in-process plugin and
> click the Print button from the PDF toolbar.
> 
> Is it OK to remove the checks for ENABLE_FULL_PRINTING in these files?
> 
> > Do you think scripted_print_throttling_disabled is necessary?
> 
> It seems that scripted print throttling is intended to protect against code
like
> this:
> 
> setInterval(function(){ window.print(); }, 1000);
> 
> Without any throttling the print dialog will be displayed again before the
> previous print operation has completed. For example, if you select print to
file
> the next print dialog will displayed while the "Save As" file dialog is still
> displayed.

Actually it's invalid JS code. Browser needed this to protect users from sites
with code like this. Print preview is not modal, so we don't need.
If you have no goal to run any sites, including malicious, than you just don't
need it.
I'll temporary return this, but not sure if we are going to keep this after
moving printing out of chrome/

[Marshall, 2014-09-09 18:15:28]

On 2014/09/09 18:04:30, Vitaly Buka wrote:
> On 2014/09/09 16:24:51, Marshall wrote:
> > Building without ENABLE_FULL_PRINTING is problematic currently.
> 
> Actually I just recovered building without ENABLE_FULL_PRINTING
> Last piece is https://codereview.chromium.org/549423002/

Did the specific files that I listed below get fixed? Code Search still shows
that they include ENABLE_FULL_PRINTING checks.

> 
> > 
> > 1. Compilation will fail in chrome/common/chrome_utility_printing_messages.h
> if
> > ENABLE_FULL_PRINTING is not enabled. CEF shares this file with Chrome for
the
> > PDF utility process implementation.
> > 
> > 2. Printing from a PDF file will fail because code in
> > content/renderer/pepper/pepper_plugin_instance_impl.cc requires
> > ENABLE_FULL_PRINTING. To reproduce: Load a PDF file as a in-process plugin
and
> > click the Print button from the PDF toolbar.
> > 
> > Is it OK to remove the checks for ENABLE_FULL_PRINTING in these files?
> > 
> > > Do you think scripted_print_throttling_disabled is necessary?
> > 
> > It seems that scripted print throttling is intended to protect against code
> like
> > this:
> > 
> > setInterval(function(){ window.print(); }, 1000);
> > 
> > Without any throttling the print dialog will be displayed again before the
> > previous print operation has completed. For example, if you select print to
> file
> > the next print dialog will displayed while the "Save As" file dialog is
still
> > displayed.
> 
> Actually it's invalid JS code. Browser needed this to protect users from sites
> with code like this. Print preview is not modal, so we don't need.
> If you have no goal to run any sites, including malicious, than you just don't
> need it.
> I'll temporary return this, but not sure if we are going to keep this after
> moving printing out of chrome/

CEF can be used to load any type of content (trusted or not) depending on the
user's application. It would be unfortunate to loose this protection.

[Vitaly Buka (NO REVIEWS), 2014-09-09 18:26:51]

#1 is fixed https://codereview.chromium.org/552703002/
#2 you are correct. I'll fix this.


On Tue, Sep 9, 2014 at 11:15 AM, <marshall@chromium.org> wrote:

> On 2014/09/09 18:04:30, Vitaly Buka wrote:
>
>> On 2014/09/09 16:24:51, Marshall wrote:
>> > Building without ENABLE_FULL_PRINTING is problematic currently.
>>
>
>  Actually I just recovered building without ENABLE_FULL_PRINTING
>> Last piece is https://codereview.chromium.org/549423002/
>>
>
> Did the specific files that I listed below get fixed? Code Search still
> shows
> that they include ENABLE_FULL_PRINTING checks.
>
>
>
>  >
>> > 1. Compilation will fail in chrome/common/chrome_utility_
>> printing_messages.h
>> if
>> > ENABLE_FULL_PRINTING is not enabled. CEF shares this file with Chrome
>> for
>>
> the
>
>> > PDF utility process implementation.
>> >
>> > 2. Printing from a PDF file will fail because code in
>> > content/renderer/pepper/pepper_plugin_instance_impl.cc requires
>> > ENABLE_FULL_PRINTING. To reproduce: Load a PDF file as a in-process
>> plugin
>>
> and
>
>> > click the Print button from the PDF toolbar.
>> >
>> > Is it OK to remove the checks for ENABLE_FULL_PRINTING in these files?
>> >
>> > > Do you think scripted_print_throttling_disabled is necessary?
>> >
>> > It seems that scripted print throttling is intended to protect against
>> code
>> like
>> > this:
>> >
>> > setInterval(function(){ window.print(); }, 1000);
>> >
>> > Without any throttling the print dialog will be displayed again before
>> the
>> > previous print operation has completed. For example, if you select
>> print to
>> file
>> > the next print dialog will displayed while the "Save As" file dialog is
>>
> still
>
>> > displayed.
>>
>
>  Actually it's invalid JS code. Browser needed this to protect users from
>> sites
>> with code like this. Print preview is not modal, so we don't need.
>> If you have no goal to run any sites, including malicious, than you just
>> don't
>> need it.
>> I'll temporary return this, but not sure if we are going to keep this
>> after
>> moving printing out of chrome/
>>
>
> CEF can be used to load any type of content (trusted or not) depending on
> the
> user's application. It would be unfortunate to loose this protection.
>
> https://codereview.chromium.org/550033002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Vitaly Buka (NO REVIEWS), 2014-09-09 22:50:58]

Patchset #4 (id:90001) has been deleted

[Vitaly Buka (NO REVIEWS), 2014-09-09 23:52:50]

Done.
You also need to undef DISABLE_BASIC_PRINTING.

[Vitaly Buka (NO REVIEWS), 2014-09-09 23:53:18]

vitalybuka@chromium.org changed reviewers:
+ gene@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-09-09 23:53:27]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-09-10 00:06:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/550033002/150001

[commit-bot: I haz the power, 2014-09-10 01:24:20]

Committed patchset #6 (id:150001) as dcda40ae859590e3920458ea4b84130bcee08d2c

[commit-bot: I haz the power, 2014-09-10 03:57:19]

Patchset 6 (id:??) landed as
https://crrev.com/f470ae06577e8f07c0664c1a8997b5d219401882
Cr-Commit-Position: refs/heads/master@{#294063}