Bug 49257 – [Chromium] DRT does not have LayoutTestController.pageSizeAndMarginsInPixels

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Mihai Parparita]

The following layout test is failing on all Chromium platforms:

printing/page-format-data-display-none.html

Probable cause:

The result shows CONSOLE MESSAGE: line 25: Uncaught TypeError: Object [object Object] has no method 'pageSizeAndMarginsInPixels'

This is because the Chromium DRT LayoutTestController doesn't expose that functionality. 

See also http://crbug.com/62515 about adding that same functionality to test_shell.

[Stephen Chenney]

Adding support for pageSizeAndMarginInPixels and isPageBoxVisible simply requires pushing the query through from LayoutTestController to the existing methods in WebFrameImpl.

[Stephen Chenney]

I have a patch ready for this that I will put up as soon as 46152 is reviewed committed. It also fixes the other missing printing layout test methods.

[Stephen Chenney]

Created attachment 113387 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Stephen Chenney]

Comment on attachment 113387 [details]
Patch

This adds the remaining missing printing functionality from LayoutTestController.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113387 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrame.h:483
> +    virtual WebString pageProperty(const WebSize& pageSize,

nit: this should be called "getPageProperty"... the attribute-style name pageProperty is
generally only used for things that resemble attribute getters.  here, we have a method
with a complex set of input arguments that returns a result.  it seems like a "get" prefix
is therefore warranted.

another question:  is it possible that you will want to get multiple page properties?
if so, then it seems like there is some overhead with having to repeatedly setup the
print context.  why not just say that you can only query page properties between
printBegin and printEnd?

also, have you considered making printBegin possibly return a WebPrintContext?  even
the pageCount attribute could be a property of WebPrintContext (i.e., printBegin
would return a |const WebPrintContext*| instead of an integer page count).

[Stephen Chenney]

Comment on attachment 113387 [details]
Patch

I'll address fishd's comments before proceeding. Thanks for helping me learn.

[Stephen Chenney]

Created attachment 113528 [details]
Patch

[Stephen Chenney]

In response to Darin suggestions, I have renamed the get functions to reflect their getting nature. It includes a rename on one existing method to maintain consistency.

I also moved the printBegin/printEnd set-up out of WebFrameImpl and into the test code. This would allow multiple calls to obtain multiple properties without re-starting print layout. However, it is not used that way right now because this functionality is only accessed through JS calls. I really don't want to change all the printing tests to make it more rational, at least not right now.

I don't want to change anything about printBegin/printEnd because that code is used in several places and it would be a major change. In particular, the declarations for PrintContext are in WebCore, and are not included in the Layout Test code that is a big customer for WebKit::WebFrame::printBegin. And it would affect other WebKit ports, I suspect.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113528 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1479
> +                                                 int& marginTop,

not that you need to change this, but this should really take a WebRect output parameter to describe the margin instead of 4 distinct ints.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1653
> +        if (!arguments[argOffset].isNumber()

hmm, so much repetition... this kind of looks like it could be expressed using a for loop:

{
   int* results[] = {
       pageNumber,
       width,
       height,
       marginTop,
       marginRight,
       marginBottom,
       marginLeft
   };
   const int resultsLen = sizeof(results) / sizeof(results[0]);
   for (int i = 0; i < resultsLen; ++i)
       *results[i] = 0;
   if (arguments.size() - argOffset == resultsLen) {
       for (int i = 0; i < resultsLen; ++i) {
           if (!arguments[argOffset + i].isNumber())
               return false;
           *results[i] = arguments[argOffset + i].toInt32();
       }
       return true;
   }
   return arguments.size() == argOffset;
}

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1751
> +    result->set((frame->getPageProperty(cppVariantToWebString(arguments[0]), pageNumber)).utf8());

nit: no need for the extra ()'s around the frame->getPageProperty call.

[Stephen Chenney]

Created attachment 113706 [details]
Patch

[Stephen Chenney]

Comment on attachment 113706 [details]
Patch

More work to make the code prettier. Rather than the loop option for param passing, I went with encapsulating the parsing of an int from the argument list. Somewhat cleaner, I think, but not perfect. Still, I don't want to mess with it too much.

Regarding the WebSize for the margin data, I am generally against using a class to hold values that have a meaning significant;y different from the member names/class. In this case, the margins are not a rectangle - they are 4 distinct integers actually representing an offset from a rectangle.

And I got rid of those unnecessary parans around the function call.

Still learning ...

[Stephen Chenney]

hamaji or marrita, time to look at it I think. Let's contribute to reducing failing test count.

[Stephen Chenney]

Created attachment 114370 [details]
Patch

[Stephen Chenney]

Updated patch just to resolve merge conflicts with Changelogs. Someone please review soon so that it commits cleanly.

[Tony Chang]

Comment on attachment 114370 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrame.h:475
> +    virtual void getPageSizeAndMarginsInPixels(int pageIndex,

Is it safe to rename this?  I see it called from chrome:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/renderer/print_web_view_helper.cc&exact_package=chromium&ct=rc&cd=21&q=pageSizeAndMarginsInPixels&sq=

Also, webkit style is to not use the 'get' prefix.

> Source/WebKit/chromium/public/WebFrame.h:485
> +    virtual WebString getPageProperty(const WebString& propertyName,
> +                                      int pageIndex) = 0;

No 'get'.  For example, LayoutTestController{Mac,Win,Qt,etc}.cpp all use pageProperty().

[Tony Chang]

(In reply to comment #16)
> > Source/WebKit/chromium/public/WebFrame.h:485
> > +    virtual WebString getPageProperty(const WebString& propertyName,
> > +                                      int pageIndex) = 0;
> 
> No 'get'.  For example, LayoutTestController{Mac,Win,Qt,etc}.cpp all use pageProperty().

Hah, I see this is the opposite of what Darin said.  That said, I think it's still better to not have 'get' here since the other ports don't use 'get'.

[Darin Fisher (:fishd, Google)]

Comment on attachment 114370 [details]
Patch

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

>> Source/WebKit/chromium/public/WebFrame.h:475
>> +    virtual void getPageSizeAndMarginsInPixels(int pageIndex,
> 
> Is it safe to rename this?  I see it called from chrome:
> http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/renderer/print_web_view_helper.cc&exact_package=chromium&ct=rc&cd=21&q=pageSizeAndMarginsInPixels&sq=
> 
> Also, webkit style is to not use the 'get' prefix.

I seem to recall a discussion about the use of the 'get' prefix.  I thought we only drop the 'get' prefix for things that are intended as attribute getters.  An attribute getter takes no parameters.  Maybe I'm mistaken?

Instead of 'get' we could also use 'query' :-)  My point is that it seems like it is perhaps helpful to signal that we are doing some work here.  I can see that WebKit is very inconsistent on the usage of the 'get' prefix.  Just look at Document!

My main immediate concern is about making the WebKit API use a consistent style for things like this.  It is easier to fixup WebCore style underneath the API.

[Tony Chang]

I'm wrong, get is for functions with out arguments:
https://lists.webkit.org/pipermail/webkit-dev/2011-October/018154.html

Seems like we want to fix pageSizeAndMarginsInPixels in all the ports.  That said, this change can't rename pageSizeAndMarginsInPixels without breaking Chromium.

getPageProperty doesn't have out params, so it should just be pageProperty.

[Stephen Chenney]

Created attachment 114524 [details]
Patch

[Stephen Chenney]

Patch fixes the naming of methods. I can't believe I screwed up the dependency of chromium code on the renamed method. Anyway, I think now everything is named correctly.

[Tony Chang]

Comment on attachment 114524 [details]
Patch

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

LGTM, but I defer to fishd for final review.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1604
> +    if ((int)arguments.size() > argIndex) {

Nit: Use a C++ style cast: static_cast<int>

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1622
> +    int argCount = (int)arguments.size() - argOffset;

ditto

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1633
> +    if ((int)arguments.size() > argOffset + 1)

ditto

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1644
> +    int argCount = (int)arguments.size() - argOffset;

ditto

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1704
> +    result->set(resultString.str().c_str());

Nit: You can drop the .c_str() since CppVariant::set can take a std::string.

[Stephen Chenney]

Created attachment 114528 [details]
Patch

Good call on casting. Fixed

[Darin Fisher (:fishd, Google)]

Comment on attachment 114528 [details]
Patch

Thanks Tony for digging up that webkit-dev thread.

[WebKit Review Bot]

Comment on attachment 114528 [details]
Patch

Clearing flags on attachment: 114528

Committed r100040: <http://trac.webkit.org/changeset/100040>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.