Bug 52134 – [chromium] Add support to allow printing just a plugin in a frame

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Abd-El-Malek]

[chromium] Add support to allow printing just a plugin in a frame

[John Abd-El-Malek]

Created attachment 78358 [details]
Patch

[John Abd-El-Malek]

btw the corresponding Chromium bug is http://crbug.com/50285

[John Abd-El-Malek]

Created attachment 78606 [details]
Patch

[WebKit Review Bot]

Attachment 78606 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chrom..." exit_code: 1
WebKit/chromium/public/WebFrame.h:409:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Abd-El-Malek]

this is now ready for review, while I figure out the objc gibberish on the browse side :)

[WebKit Review Bot]

Attachment 78606 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7378150

[John Abd-El-Malek]

(In reply to comment #6)
> Attachment 78606 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/7378150

doh, forgot about the n-sided patches.  will check in a chrome change now that makes this compile and update the deps in this.

[John Abd-El-Malek]

Created attachment 78621 [details]
Patch

[WebKit Review Bot]

Attachment 78621 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chrom..." exit_code: 1
WebKit/chromium/public/WebFrame.h:412:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 78621 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7402137

[John Abd-El-Malek]

Created attachment 78628 [details]
Patch

[WebKit Review Bot]

Attachment 78628 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chrom..." exit_code: 1
WebKit/chromium/public/WebFrame.h:412:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKit/chromium/src/WebFrameImpl.h:154:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

(In reply to comment #12)
> Attachment 78628 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chrom..." exit_code: 1
> WebKit/chromium/public/WebFrame.h:412:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
> WebKit/chromium/src/WebFrameImpl.h:154:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 2 in 7 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

It looks like the style bot is trying to tell you something. (fwiw, it's correct).

[John Abd-El-Malek]

(In reply to comment #13)
> (In reply to comment #12)
> > Attachment 78628 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chrom..." exit_code: 1
> > WebKit/chromium/public/WebFrame.h:412:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
> > WebKit/chromium/src/WebFrameImpl.h:154:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
> > Total errors found: 2 in 7 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> It looks like the style bot is trying to tell you something. (fwiw, it's correct).

in this case, it looked to me like it would make the code more awkward.  i.e. the example given in the style guide only has one parameter and no default value.  but in this code:

virtual int printBegin(const WebSize& pageSize,
                       WebNode* = 0,
                       int printerDPI = 72,
                       bool* useBrowserOverlays = 0) = 0;

it looks a little strange to me having a function declaration with 3 named variables and one that's not named, especially if it has a default value.  it makes referencing the variable in a comment inconsistent with how the others are referenced, since |node| doesn't make sense anymore.

but, i don't care enough, if the script wants inconsistencies within one function declaration, so be it..

[John Abd-El-Malek]

Created attachment 78661 [details]
Patch

[David Levin]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Attachment 78628 [details] [details] [details] did not pass style-queue:
> > > 
> > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chrom..." exit_code: 1
> > > WebKit/chromium/public/WebFrame.h:412:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
> > > WebKit/chromium/src/WebFrameImpl.h:154:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
> > > Total errors found: 2 in 7 files
> > > 
> > > 
> > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > 
> > It looks like the style bot is trying to tell you something. (fwiw, it's correct).
> 
> in this case, it looked to me like it would make the code more awkward.  i.e. the example given in the style guide only has one parameter and no default value.  but in this code:
> 
> virtual int printBegin(const WebSize& pageSize,
>                        WebNode* = 0,
>                        int printerDPI = 72,
>                        bool* useBrowserOverlays = 0) = 0;
> 
> it looks a little strange to me having a function declaration with 3 named variables and one that's not named, especially if it has a default value. 

It sounds like you are used to a different style other than WebKit so it looks strange. Ok.

> it makes referencing the variable in a comment inconsistent with how the others are referenced, since |node| doesn't make sense anymore.

Feel free to say WebNode*.

> but, i don't care enough, if the script wants inconsistencies within one function declaration, so be it..

WebKit style is to omit parameter names that don't add information. The script is correctly reminding you of this. This isn't about some script wanting inconsistencies.

[Darin Fisher (:fishd, Google)]

Comment on attachment 78661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78661&action=review

> WebKit/chromium/public/WebFrame.h:411
> +    virtual int printBegin(const WebSize& pageSize,

if it makes your merging easier, it is also OK to put the WebNode parameter last,
and i might give it a self-documenting name.

it would also be a bit more conventional to pass the WebNode like this:

  const WebNode& constrainToNode = WebNode(),

Note: WebNode has an isNull method you can check.

> WebKit/chromium/src/WebFrameImpl.cpp:1285


nit: it is a bit more conventional to name webcore objects using the prefix "core", so "coreNode" instead of "webcoreNode"

nit: you can also just do this:

  Node* coreNode = node->unwrap<WebCore::Node>();

if you pass as const WebNode& node, then you would do this:

  const Node* coreNode = node.constUnwrap<Node>();

[John Abd-El-Malek]

Created attachment 78752 [details]
Patch

[John Abd-El-Malek]

(In reply to comment #17)
> (From update of attachment 78661 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78661&action=review
> 
> > WebKit/chromium/public/WebFrame.h:411
> > +    virtual int printBegin(const WebSize& pageSize,
> 
> if it makes your merging easier, it is also OK to put the WebNode parameter last,
> and i might give it a self-documenting name.
> 
> it would also be a bit more conventional to pass the WebNode like this:
> 
>   const WebNode& constrainToNode = WebNode(),
> 
> Note: WebNode has an isNull method you can check.
> 
> > WebKit/chromium/src/WebFrameImpl.cpp:1285
> 
> 
> nit: it is a bit more conventional to name webcore objects using the prefix "core", so "coreNode" instead of "webcoreNode"
> 
> nit: you can also just do this:
> 
>   Node* coreNode = node->unwrap<WebCore::Node>();
> 
> if you pass as const WebNode& node, then you would do this:
> 
>   const Node* coreNode = node.constUnwrap<Node>();


Done and uploaded

[John Abd-El-Malek]

Committed r75730: <http://trac.webkit.org/changeset/75730>