Bug 49257

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

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.

Created attachment 113387 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 113387 [details] Patch This adds the remaining missing printing functionality from LayoutTestController.

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).

Comment on attachment 113387 [details] Patch I'll address fishd's comments before proceeding. Thanks for helping me learn.

Created attachment 113528 [details] Patch

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.

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.

Created attachment 113706 [details] Patch

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 ...

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

Created attachment 114370 [details] Patch

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

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().

(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'.

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.

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.

Created attachment 114524 [details] Patch

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.

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.

Created attachment 114528 [details] Patch Good call on casting. Fixed

Comment on attachment 114528 [details] Patch Thanks Tony for digging up that webkit-dev thread.

Comment on attachment 114528 [details] Patch Clearing flags on attachment: 114528 Committed r100040: <http://trac.webkit.org/changeset/100040>

All reviewed patches have been landed. Closing bug.