Support print preset options for pdf document

Support print preset options for pdf document

Blink changes to add support for print preset options
for pdf documents. When pdf document has print preset options
set, then print preview mode should use those options.

This patch adds changes required to get print preset options
from plugin. Also, this patch removes getPrintCopiesForPlugin() 
that was added initially and is no longer required.

BUG=169120
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185276

[Nikhil, 2014-09-23 18:49:35]

n.bansal@samsung.com changed reviewers:
+ vitalybuka@chromium.org

[Nikhil, 2014-09-23 18:51:38]

vitalybuka@ - PTAL at blink side changes to support print presets feature.
Chrome side changes are being reviewed at -
https://codereview.chromium.org/375253002/

Thanks

[Nikhil, 2014-09-29 04:50:29]

n.bansal@samsung.com changed reviewers:
+ raymes@chromium.org

[Nikhil, 2014-09-29 04:52:42]

vitalybuka@, raymes@ - Could you please take a look at this? I'll request blink
OWNERS to review once you've taken a look at this. These changes are required to
support print preset feature. Chrome side changes are available at -
https://codereview.chromium.org/375253002/

Thanks!

[raymes, 2014-09-29 05:24:14]

https://codereview.chromium.org/591053002/diff/20001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/591053002/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.h:142: // Returns true if print preset options
are updated from document.
nit: // Returns true on success and sets the out parameter to the print preset
options for the document.

https://codereview.chromium.org/591053002/diff/20001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/591053002/diff/20001/public/web/WebFrame.h#ne...
public/web/WebFrame.h:507: // Returns true if print preset options are updated
from pdf document.
Have a similar comment here. Also, don't mention PDF specifically since you
could presumably have a different plugin that returns this information.

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPlugin.h
File public/web/WebPlugin.h (right):

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPlugin.h#n...
public/web/WebPlugin.h:124: virtual bool isPrintScalingDisabled() { return
false; }
Can we delete this function too...? Is it providing the same information as
what's in the struct?

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPlugin.h#n...
public/web/WebPlugin.h:125: // Returns true if print preset options are updated
from document.
same here.

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPrintPrese...
File public/web/WebPrintPresetOptions.h (right):

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPrintPrese...
public/web/WebPrintPresetOptions.h:11: typedef std::vector<WebPageRange>
WebPageRanges;
Do you need to #include <vector>?

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPrintPrese...
public/web/WebPrintPresetOptions.h:27: : duplexMode(WebUnknownDuplexMode) { }
You will want to initialize all of the members.

[Nikhil, 2014-10-09 08:39:25]

Patchset #3 (id:40001) has been deleted

[Nikhil, 2014-10-09 08:43:56]

raymes@ - Updated the patch based on your comments. PTAL. Thanks!

https://codereview.chromium.org/591053002/diff/20001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/591053002/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.h:142: // Returns true if print preset options
are updated from document.
On 2014/09/29 05:24:14, raymes wrote:
> nit: // Returns true on success and sets the out parameter to the print preset
> options for the document.

Done.

https://codereview.chromium.org/591053002/diff/20001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/591053002/diff/20001/public/web/WebFrame.h#ne...
public/web/WebFrame.h:507: // Returns true if print preset options are updated
from pdf document.
On 2014/09/29 05:24:14, raymes wrote:
> Have a similar comment here. Also, don't mention PDF specifically since you
> could presumably have a different plugin that returns this information.

Done.

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPlugin.h
File public/web/WebPlugin.h (right):

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPlugin.h#n...
public/web/WebPlugin.h:124: virtual bool isPrintScalingDisabled() { return
false; }
On 2014/09/29 05:24:14, raymes wrote:
> Can we delete this function too...? Is it providing the same information as
> what's in the struct?

I think it's not possible to remove this method. It is used for
PPP_Printing_Dev's function IsScalingDisabled().

Please see :
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The information is same for scaling and we have already updated PPP_Pdf
interface with a commet about it. Here :
https://codereview.chromium.org/375253002/diff/260001/ppapi/c/private/ppp_pdf.h

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPlugin.h#n...
public/web/WebPlugin.h:125: // Returns true if print preset options are updated
from document.
On 2014/09/29 05:24:14, raymes wrote:
> same here.

Done.

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPrintPrese...
File public/web/WebPrintPresetOptions.h (right):

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPrintPrese...
public/web/WebPrintPresetOptions.h:11: typedef std::vector<WebPageRange>
WebPageRanges;
On 2014/09/29 05:24:14, raymes wrote:
> Do you need to #include <vector>?

It compiled fine earlier but I've included it now.

https://codereview.chromium.org/591053002/diff/20001/public/web/WebPrintPrese...
public/web/WebPrintPresetOptions.h:27: : duplexMode(WebUnknownDuplexMode) { }
On 2014/09/29 05:24:14, raymes wrote:
> You will want to initialize all of the members.

Thanks! Initialized everything except pageRanges based on :
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/prin...

Please let me know WDYT.

[dcheng, 2014-10-09 08:53:41]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2014-10-09 08:53:42]

https://codereview.chromium.org/591053002/diff/60001/Source/web/WebRemoteFram...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/591053002/diff/60001/Source/web/WebRemoteFram...
Source/web/WebRemoteFrameImpl.cpp:664: bool
WebRemoteFrameImpl::getPrintPresetOptionsForPlugin(const WebNode&,
WebPrintPresetOptions*)
Since we're changing this signature anyway, how hard would it be to make this
WebLocalFrame specific? There's never a reason to call this on remote frames,
right?

[raymes, 2014-10-09 20:44:02]

lgtm from my side

I still think it would be nice to expose the information in only 1 way from
blink. The idea is to only expose getPrintPresetOptionsFromDocument and remove
any redundant functions that expose the same information. Under the covers,
getPrintPresetOptionsFromDocument can fill in the right pieces of information in
the struct based on what functions the plugin exposes. This might be a nice
cleanup but I'm not sure. It might be worth trying in a separate CL.

[dcheng, 2014-10-09 20:45:03]

https://codereview.chromium.org/591053002/diff/60001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/591053002/diff/60001/public/web/WebFrame.h#ne...
public/web/WebFrame.h:499: virtual bool getPrintPresetOptionsForPlugin(const
WebNode&, WebPrintPresetOptions*) = 0;
Actually it looks like this isn't even used yet. Please move this to
WebLocalFrame.h =)

[Vitaly Buka (NO REVIEWS), 2014-10-10 00:51:01]

lgtm

https://codereview.chromium.org/591053002/diff/60001/public/web/WebPrintPrese...
File public/web/WebPrintPresetOptions.h (right):

https://codereview.chromium.org/591053002/diff/60001/public/web/WebPrintPrese...
public/web/WebPrintPresetOptions.h:21: 
Should we put WebPageRange, WebPageRanges and WebDuplexMode inside
WebPrintPresetOptions, removing "Web" prefix?

[Nikhil, 2014-10-20 10:00:30]

Patchset #4 (id:190001) has been deleted

[Nikhil, 2014-10-20 10:04:23]

dcheng@, raymes@, vitalybuka@ - Sorry for the delay! I've updated the patch
based on your comments and have also replied to your comments.

Please take a look :)

Thanks!

https://codereview.chromium.org/591053002/diff/60001/Source/web/WebRemoteFram...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/591053002/diff/60001/Source/web/WebRemoteFram...
Source/web/WebRemoteFrameImpl.cpp:664: bool
WebRemoteFrameImpl::getPrintPresetOptionsForPlugin(const WebNode&,
WebPrintPresetOptions*)
On 2014/10/09 08:53:42, dcheng wrote:
> Since we're changing this signature anyway, how hard would it be to make this
> WebLocalFrame specific? There's never a reason to call this on remote frames,
> right?

Based on your suggestion, I've moved the new method to WebLocalFrame.h and
removed it from here. PTAL :)

https://codereview.chromium.org/591053002/diff/60001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/591053002/diff/60001/public/web/WebFrame.h#ne...
public/web/WebFrame.h:499: virtual bool getPrintPresetOptionsForPlugin(const
WebNode&, WebPrintPresetOptions*) = 0;
On 2014/10/09 20:45:03, dcheng wrote:
> Actually it looks like this isn't even used yet. Please move this to
> WebLocalFrame.h =)

Done! Yes, this method isn't yet used. It will be used with this CL :
https://codereview.chromium.org/375253002/

https://codereview.chromium.org/591053002/diff/60001/public/web/WebPrintPrese...
File public/web/WebPrintPresetOptions.h (right):

https://codereview.chromium.org/591053002/diff/60001/public/web/WebPrintPrese...
public/web/WebPrintPresetOptions.h:21: 
On 2014/10/10 00:51:01, Vitaly Buka wrote:
> Should we put WebPageRange, WebPageRanges and WebDuplexMode inside
> WebPrintPresetOptions, removing "Web" prefix?

We can do that. I don't have strong preference here. But, for PPAPI, similar
structs are outside of the main struct. Please see :

PP_PdfPrintPresetOptions_Dev :
https://codereview.chromium.org/375253002/diff/260001/ppapi/c/private/ppp_pdf.h

PP_PrintDuplexMode_Dev and PP_PrintRange_Dev :
https://codereview.chromium.org/375253002/diff/260001/ppapi/c/dev/pp_print_se...

Please let me know WDYT.

[Nikhil, 2014-11-05 09:05:08]

dcheng@ - <ping!> Could you please take a look at this?

[dcheng, 2014-11-06 22:45:29]

Sorry, I was at BlinkOn the last few days. I'll be able to take a look at this
tomorrow.

[dcheng, 2014-11-07 18:44:42]

lgtm, sorry for the delay

[Nikhil, 2014-11-10 16:35:15]

n.bansal@samsung.com changed reviewers:
+ mkwst@chromium.org

[Nikhil, 2014-11-10 16:35:16]

mkwst@ - OWNERS review for Source/web/* please :)

Chromium changes for this are already done and are available at
https://codereview.chromium.org/375253002/

Thanks!

[Nikhil, 2014-11-12 16:04:51]

mkwst@ - <ping!> Could you please take a look at this for OWNERS review for
Source/web/*?

Thanks!

[Mike West, 2014-11-12 17:56:36]

LGTM.

[Nikhil, 2014-11-13 07:34:25]

Thank you all for reviewing this! CQing this CL now :)

[Nikhil, 2014-11-13 07:34:36]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-11-13 07:35:18]

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

[commit-bot: I haz the power, 2014-11-13 08:39:43]

Committed patchset #6 (id:250001) as 185276