Fixed build enable_printing=2 for windows and removed dead code.

PdfToMetafile messages always required on Windows.

Restore Pepper printing support even without full printing.
If was guarded to work in full printing for Android, because Android
did not implemented that functionality. Proper guard should allow
even non-full printing, because code is usefull to print PDF.

Removed printing to DC code, related code already deleted from Chrome.

BUG=170859, 417967
Committed: https://crrev.com/3967c43228c59144237d1e92a30994ca1c640750
Cr-Commit-Position: refs/heads/master@{#297324}

[Vitaly Buka (NO REVIEWS), 2014-09-25 21:49:51]

vitalybuka@chromium.org changed reviewers:
+ tsepez@chromium.org, yzshen@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-09-25 21:53:40]

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

[Tom Sepez, 2014-09-25 22:14:01]

Messages LGTM.

Just a few typos in the description of this CL:
s/PdfToMetifile/PdfToMetafile/
s/peeper/pepper/

[yzshen1, 2014-09-25 22:39:43]

Hi,

It would be great if you redirect this CL to another Pepper OWNER.
I haven't worked on Pepper for a while and I am not very familiar with this
feature.

Thanks!

[Vitaly Buka (NO REVIEWS), 2014-09-25 22:51:10]

vitalybuka@chromium.org changed reviewers:
+ raymes@chromium.org
- yzshen@chromium.org

[raymes, 2014-09-26 03:06:32]

This change overall looks good (mostly removing code) but I don't really
understand the implications. Is there someone who is in a better place to
review? I'm happy to give OWNERS rubberstamp.

Also, is there a bug for this? What exactly is it fixing (i.e. why are the print
messages always needed on windows even when the #define isn't specified for
printing)?

Also, does this mean that all printing happens outside of the renderer process
now?

[raymes, 2014-09-26 03:06:54]

https://codereview.chromium.org/599633007/diff/180001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/599633007/diff/180001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1930: 
nit: blank line

[Vitaly Buka (NO REVIEWS), 2014-09-26 04:29:09]

On 2014/09/26 03:06:32, raymes wrote:
> This change overall looks good (mostly removing code) but I don't really
> understand the implications. Is there someone who is in a better place to
> review? I'm happy to give OWNERS rubberstamp.
> 
> Also, is there a bug for this? What exactly is it fixing (i.e. why are the
print
> messages always needed on windows even when the #define isn't specified for
> printing)?

Even without full printing windows need to convert PDF to metafiles using those
messages.
Added related comments.

> 
> Also, does this mean that all printing happens outside of the renderer process
> now?

Yes. Now it's in utility process. issue 170859.

[Vitaly Buka (NO REVIEWS), 2014-09-26 04:29:21]

https://codereview.chromium.org/599633007/diff/180001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/599633007/diff/180001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1930: 
On 2014/09/26 03:06:54, raymes wrote:
> nit: blank line

Done.

[Vitaly Buka (NO REVIEWS), 2014-09-26 04:31:55]

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

[Lei Zhang, 2014-09-26 04:46:09]

https://codereview.chromium.org/599633007/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/599633007/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1748: #if
defined(ENABLE_PRINTING) && !defined(OS_ANDROID)
pepper_plugin_instance_impl.cc isn't even compiled by default on Android.

[raymes, 2014-09-26 04:47:29]

https://codereview.chromium.org/599633007/diff/220001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_printing_messages.h (right):

https://codereview.chromium.org/599633007/diff/220001/chrome/common/chrome_ut...
chrome/common/chrome_utility_printing_messages.h:89: #if
defined(ENABLE_PRINTING) && defined(OS_WIN)
Should this be #if defined(ENABLE_PRINTING) && !defined(OS_ANDROID)
like in the other file?

https://codereview.chromium.org/599633007/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/599633007/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1750: // CEF
(crbug.com/417967) and Android.
nit: might as well duplicate the comment below exactly.

https://codereview.chromium.org/599633007/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1936: // CEF
(crbug.com/417967) and Android. Android does not call this code.
nit: Could you elaborate on why CEF needs to use this?

[Vitaly Buka (NO REVIEWS), 2014-09-26 05:29:03]

https://codereview.chromium.org/599633007/diff/220001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_printing_messages.h (right):

https://codereview.chromium.org/599633007/diff/220001/chrome/common/chrome_ut...
chrome/common/chrome_utility_printing_messages.h:89: #if
defined(ENABLE_PRINTING) && defined(OS_WIN)
this messages windows only. it's for generation of Windows metafiles.
All other platforms consume PDFs.

On 2014/09/26 04:47:29, raymes wrote:
> Should this be #if defined(ENABLE_PRINTING) && !defined(OS_ANDROID)
> like in the other file?

https://codereview.chromium.org/599633007/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/599633007/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1748: #if
defined(ENABLE_PRINTING) && !defined(OS_ANDROID)
On 2014/09/26 04:46:09, Lei Zhang (OOO starting 09-27) wrote:
> pepper_plugin_instance_impl.cc isn't even compiled by default on Android.

Oh, I didn't check then.
ENABLE_PRINTING was switch to ENABLE_FULL_PRINTING for android
https://codereview.chromium.org/22577010/diff/79001/content/renderer/pepper/p...

and then android reference was added at
https://chromiumcodereview.appspot.com/16206002/diff/64001/content/renderer/p...



so now it's safe to revert back.

https://codereview.chromium.org/599633007/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1750: // CEF
(crbug.com/417967) and Android.
On 2014/09/26 04:47:29, raymes wrote:
> nit: might as well duplicate the comment below exactly.

Done.

https://codereview.chromium.org/599633007/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1936: // CEF
(crbug.com/417967) and Android. Android does not call this code.
On 2014/09/26 04:47:29, raymes wrote:
> nit: Could you elaborate on why CEF needs to use this? 

After Lei's notice about Android we don't need any excuse here.
It was originally correct, any platform with printing and
PepperPluginInstanceImpl needs this.

Also it's a good question why CEF needed this.
Primary reason is that they can't support print preview because support of
out-of-process pdf is too complicated.
However as I understand viewing and printing of PDFs would not be available
anyway.

So I guess changes in this file would not help them.

[Lei Zhang, 2014-09-26 05:38:17]

On 2014/09/26 05:29:03, Vitaly Buka wrote:
> Also it's a good question why CEF needed this.
> Primary reason is that they can't support print preview because support of
> out-of-process pdf is too complicated.
> However as I understand viewing and printing of PDFs would not be available
> anyway.
> 
> So I guess changes in this file would not help them.

Maybe ask Marshall and see if they have PPAPI support in general?

[Marshall, 2014-09-26 15:18:34]

On 2014/09/26 05:38:17, Lei Zhang (OOO starting 09-27) wrote:
> On 2014/09/26 05:29:03, Vitaly Buka wrote:
> > Also it's a good question why CEF needed this.
> > Primary reason is that they can't support print preview because support of
> > out-of-process pdf is too complicated.
> > However as I understand viewing and printing of PDFs would not be available
> > anyway.
> > 
> > So I guess changes in this file would not help them.
> 
> Maybe ask Marshall and see if they have PPAPI support in general?

CEF supports:
(1) in-process loading of the pepper pdf plugin for pdf viewing, and
(2) basic printing which, on Windows, loads the pepper pdf plugin in a utility
process.

CEF doesn't support full printing (print preview) due to dependencies on the
chrome extension system (chrome uses an extension to load the print preview UI
which then uses an out-of-process pdf plugin). We differentiate between no
printing, basic printing and full printing support in Chromium based on the
`enable_printing` gyp variable. CEF uses "`enable_printing': 2" which sets
ENABLE_PRINTING=1 and does not define ENABLE_FULL_PRINTING or
DISABLE_BASIC_PRINTING.

The PepperPluginInstanceImpl::PrintPage code path is exercised in CEF when
pressing the print button on the floating pdf viewer toolbar.

[Lei Zhang, 2014-09-26 16:11:53]

On 2014/09/26 15:18:34, Marshall wrote:
> On 2014/09/26 05:38:17, Lei Zhang (OOO starting 09-27) wrote:
> > On 2014/09/26 05:29:03, Vitaly Buka wrote:
> > > Also it's a good question why CEF needed this.
> > > Primary reason is that they can't support print preview because support of
> > > out-of-process pdf is too complicated.
> > > However as I understand viewing and printing of PDFs would not be
available
> > > anyway.
> > > 
> > > So I guess changes in this file would not help them.
> > 
> > Maybe ask Marshall and see if they have PPAPI support in general?
> 
> CEF supports:
> (1) in-process loading of the pepper pdf plugin for pdf viewing, and

Which will eventually go away in favor of out-of-process PPAPI, FYI.

> CEF doesn't support full printing (print preview) due to dependencies on the
> chrome extension system (chrome uses an extension to load the print preview UI
> which then uses an out-of-process pdf plugin). We differentiate between no
> printing, basic printing and full printing support in Chromium based on the
> `enable_printing` gyp variable. CEF uses "`enable_printing': 2" which sets
> ENABLE_PRINTING=1 and does not define ENABLE_FULL_PRINTING or
> DISABLE_BASIC_PRINTING.

But print preview doesn't use extensions. It's still using in-process PPAPI. One
would have to pass --out-of-process-pdf to get OOP PDF.

[Marshall, 2014-09-26 16:37:56]

On 2014/09/26 16:11:53, Lei Zhang (OOO starting 09-27) wrote:
> On 2014/09/26 15:18:34, Marshall wrote:
> > On 2014/09/26 05:38:17, Lei Zhang (OOO starting 09-27) wrote:
> > > On 2014/09/26 05:29:03, Vitaly Buka wrote:
> > > > Also it's a good question why CEF needed this.
> > > > Primary reason is that they can't support print preview because support
of
> > > > out-of-process pdf is too complicated.
> > > > However as I understand viewing and printing of PDFs would not be
> available
> > > > anyway.
> > > > 
> > > > So I guess changes in this file would not help them.
> > > 
> > > Maybe ask Marshall and see if they have PPAPI support in general?
> > 
> > CEF supports:
> > (1) in-process loading of the pepper pdf plugin for pdf viewing, and
> 
> Which will eventually go away in favor of out-of-process PPAPI, FYI.
> 
> > CEF doesn't support full printing (print preview) due to dependencies on the
> > chrome extension system (chrome uses an extension to load the print preview
UI
> > which then uses an out-of-process pdf plugin). We differentiate between no
> > printing, basic printing and full printing support in Chromium based on the
> > `enable_printing` gyp variable. CEF uses "`enable_printing': 2" which sets
> > ENABLE_PRINTING=1 and does not define ENABLE_FULL_PRINTING or
> > DISABLE_BASIC_PRINTING.
> 
> But print preview doesn't use extensions. It's still using in-process PPAPI.
One
> would have to pass --out-of-process-pdf to get OOP PDF.

Ah, my mistake. I was misremembering the conversation from
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hLBbJe_NEGw/iZFG8....

The in-process pdf print preview appears to have a dependency on WebUI
(chrome/browser/ui/webui/print_preview) which, like the extension system, would
be difficult to utilize outside of the Google Chrome browser.

[Lei Zhang, 2014-09-26 16:40:34]

On 2014/09/26 15:18:34, Marshall wrote:
> (2) basic printing which, on Windows, loads the pepper pdf plugin in a utility
> process.

Given this, I think we can remove the PDF code here, since no one is using this
path anymore.

[Lei Zhang, 2014-09-26 16:41:25]

lgtm, but I'm not an owner.

[Vitaly Buka (NO REVIEWS), 2014-09-26 16:41:26]

On 2014/09/26 16:37:56, Marshall wrote:
> On 2014/09/26 16:11:53, Lei Zhang (OOO starting 09-27) wrote:
> > On 2014/09/26 15:18:34, Marshall wrote:
> > > On 2014/09/26 05:38:17, Lei Zhang (OOO starting 09-27) wrote:
> > > > On 2014/09/26 05:29:03, Vitaly Buka wrote:
> > > > > Also it's a good question why CEF needed this.
> > > > > Primary reason is that they can't support print preview because
support
> of
> > > > > out-of-process pdf is too complicated.
> > > > > However as I understand viewing and printing of PDFs would not be
> > available
> > > > > anyway.
> > > > > 
> > > > > So I guess changes in this file would not help them.
> > > > 
> > > > Maybe ask Marshall and see if they have PPAPI support in general?
> > > 
> > > CEF supports:
> > > (1) in-process loading of the pepper pdf plugin for pdf viewing, and
> > 
> > Which will eventually go away in favor of out-of-process PPAPI, FYI.
> > 
> > > CEF doesn't support full printing (print preview) due to dependencies on
the
> > > chrome extension system (chrome uses an extension to load the print
preview
> UI
> > > which then uses an out-of-process pdf plugin). We differentiate between no
> > > printing, basic printing and full printing support in Chromium based on
the
> > > `enable_printing` gyp variable. CEF uses "`enable_printing': 2" which sets
> > > ENABLE_PRINTING=1 and does not define ENABLE_FULL_PRINTING or
> > > DISABLE_BASIC_PRINTING.
> > 
> > But print preview doesn't use extensions. It's still using in-process PPAPI.
> One
> > would have to pass --out-of-process-pdf to get OOP PDF.
> 
> Ah, my mistake. I was misremembering the conversation from
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hLBbJe_NEGw/iZFG8....
> 
> The in-process pdf print preview appears to have a dependency on WebUI
> (chrome/browser/ui/webui/print_preview) which, like the extension system,
would
> be difficult to utilize outside of the Google Chrome browser.

In other words, I guess your goal is to removed dependency on src/hrome and use
src/content?
This would be possible if we move basic printing to content.
Which would

[Marshall, 2014-09-26 16:46:20]

On 2014/09/26 16:41:26, Vitaly Buka wrote:
> In other words, I guess your goal is to removed dependency on src/hrome and
use
> src/content?
> This would be possible if we move basic printing to content.
> Which would

Yes, exactly. We're currently tracking that in https://crbug.com/311308
specifically for printing-related functionality. It would be ideal if print
preview (both in-process and out-of-process pdf plugin paths) could be used
without /chrome dependencies.

[Marshall, 2014-09-26 17:01:52]

This change works with CEF, so lgtm.

[raymes, 2014-09-29 00:09:37]

https://codereview.chromium.org/599633007/diff/260001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_printing_messages.h (right):

https://codereview.chromium.org/599633007/diff/260001/chrome/common/chrome_ut...
chrome/common/chrome_utility_printing_messages.h:89: #if
defined(ENABLE_PRINTING) && defined(OS_WIN)
Do we really need the check for OS_WIN here too? It seems like these ifdefs
should mirror the ones in the other file, but maybe I'm wrong.

[Vitaly Buka (NO REVIEWS), 2014-09-29 17:08:07]

https://codereview.chromium.org/599633007/diff/260001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_printing_messages.h (right):

https://codereview.chromium.org/599633007/diff/260001/chrome/common/chrome_ut...
chrome/common/chrome_utility_printing_messages.h:89: #if
defined(ENABLE_PRINTING) && defined(OS_WIN)
There are 3 files that refers to these messages

pdf_to_emf_converter.cc: windows only, gyp rule
service_utility_process_host.cc: windows only, gyp rule
printing_handler.cc: has #if defined(OS_WIN)


On 2014/09/29 00:09:37, raymes wrote:
> Do we really need the check for OS_WIN here too? It seems like these ifdefs
> should mirror the ones in the other file, but maybe I'm wrong.

[raymes, 2014-09-30 00:35:03]

lgtm

[Vitaly Buka (NO REVIEWS), 2014-09-30 00:35:53]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-09-30 00:37:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/599633007/260001

[commit-bot: I haz the power, 2014-09-30 01:09:17]

Committed patchset #14 (id:260001) as 0ebcbf71501f73d1cc4076c623f16608b65fa3f1

[commit-bot: I haz the power, 2014-09-30 01:11:32]

Patchset 14 (id:??) landed as
https://crrev.com/3967c43228c59144237d1e92a30994ca1c640750
Cr-Commit-Position: refs/heads/master@{#297324}