Bug 46152

Created attachment 97851 [details] Potential patch for missing setPrinting in chromium This patch is tentative because I am uncertain of the expected results for chromium printing. In particular, the background color of the large div in the printing/setPrinting.html layout test is ignored in the output, and the font scaling seems different. Somone who knows what to expect needs to weigh in on the results of applying this patch.

Comment on attachment 97851 [details] Potential patch for missing setPrinting in chromium View in context: https://bugs.webkit.org/attachment.cgi?id=97851&action=review Hi Stephen, Thank you for doing this! Once you implement testing API, you can enable a test which is marked as to fail. To see which test can be flipped, please see original bug. In this case, you need to remove line "BUGWK20011 : printing/setPrinting.html = FAIL" from LayoutTests/platform/chromium/test_expectations.txt, run the test, and include test result in this change. > Tools/DumpRenderTree/chromium/LayoutTestController.h:552 > + Please don't include unnecessary change. > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1498 > int left = max(m_paintRect.x, clientRect.x); Please don't incude unrelated change. This kind of cleanup can be done by separate patch. (And such kind of cleanup is highly welcome!)

Comment on attachment 97851 [details] Potential patch for missing setPrinting in chromium r- at this time.

Created attachment 106632 [details] Patch

The patch I just uploaded requires changes to expected results on all platforms. I will not get to that until tomorrow, so DO NOT SUBMIT.

Comment on attachment 106632 [details] Patch Attachment 106632 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9607474

Created attachment 106742 [details] Patch

Created attachment 106767 [details] New expected image result This is a new expected result for this test case. The use of a border color rather than background color makes the test useful for browsers that do not print background colors by default. The path for this image is: LayoutTests/platform/mac/printing/setPrinting-expected.png

Created attachment 106768 [details] New expected text result This is a new expected result for this test case. The use of a border color rather than background color makes the test useful for browsers that do not print background colors by default. The path for this image is: LayoutTests/platform/mac/printing/setPrinting-expected.txt

Attachment 106768 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 106767 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 106742 [details] Patch Attachment 106742 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9621739 New failing tests: printing/setPrinting.html

Turns out that the patch I have up does not build on Mac because we are using a CGContext there for GraphicsContext, rather than a Skia context. So hold off on review until I get this sorted out.

Created attachment 108881 [details] Patch

Comment on attachment 108881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108881&action=review > Source/WebKit/chromium/src/WebFrameImpl.h:178 > + virtual WebCanvas* printPagesWithBoundaries(WebCanvas*, const WebSize&); I guess this method can be part of DRT instead of WebKit API. In general, test-specific API should be stayed in DRT if possible, we have some of such unfortunate API in WebKit though. Also, you need to cc fishd@ if you add new API to chromium WebKit interface. > Source/WebKit/chromium/src/WebFrameImpl.h:192 > + virtual WebString renderTreeAsText(bool showDebugInfo = false, bool showPrintedText = false) const; How about to define an enum instead of use bool? You can find two-sided enum example in AssertMatchingEnums.cpp

There are some problems with moving paintPagesWithBoundaries out of the WebKit API and into DRT. The ChromePrintContext class, that contains printing logic specific to Chrome, is defined in WebFrameImpl.cpp, so is only available in that file, or the class WebFrameImpl. While this could theoretically be worked around by either moving it out of the cpp file, it then leads to another issue ... We could choose to use a WebCore::PrintContext instead of a Chrome context, but in theory that may produce incorrect results and will not catch errors in the logic of ChromePrintContext. And including WebCore API in DRT means bringing in several WebCore header files. It doesn't help that PrintContext.h is not self contained. That is, PrintContext.h requires other declarations that are not included in its own header. So I see two possibilities: Move ChromePrintContext out of WebFrameImpl.cpp and deal with the includes, or leave paintPagesWithBoundaries in WebKit::WebWidget.h and WebKit::WebFrameImpl Regarding the new parameters for WebFrame::renderTreeAsText, we could require the caller to define an appropriate mask based on the declarations in WebCore::RenderTreeAsText, but that (a) requires the caller to have detailed knowledge of the flags and (b) requires headers from WebCore in DRT. So I would prefer to keep the bool, as it is obvious what it means and easy to use. Finally, I'll need to update this patch because the logic for scaling on the mac is different. Damn.

(In reply to comment #16) > There are some problems with moving paintPagesWithBoundaries out of the WebKit API and into DRT. The ChromePrintContext class, that contains printing logic specific to Chrome, is defined in WebFrameImpl.cpp, so is only available in that file, or the class WebFrameImpl. While this could theoretically be worked around by either moving it out of the cpp file, it then leads to another issue ... > > We could choose to use a WebCore::PrintContext instead of a Chrome context, but in theory that may produce incorrect results and will not catch errors in the logic of ChromePrintContext. > > And including WebCore API in DRT means bringing in several WebCore header files. It doesn't help that PrintContext.h is not self contained. That is, PrintContext.h requires other declarations that are not included in its own header. > > So I see two possibilities: Move ChromePrintContext out of WebFrameImpl.cpp and deal with the includes, or leave paintPagesWithBoundaries in WebKit::WebWidget.h and WebKit::WebFrameImpl > > Regarding the new parameters for WebFrame::renderTreeAsText, we could require the caller to define an appropriate mask based on the declarations in WebCore::RenderTreeAsText, but that (a) requires the caller to have detailed knowledge of the flags and (b) requires headers from WebCore in DRT. So I would prefer to keep the bool, as it is obvious what it means and easy to use. > > Finally, I'll need to update this patch because the logic for scaling on the mac is different. Damn. Your points are making sense. I'll keep this r- since you will have updated patch anyway.

Created attachment 110406 [details] Patch

Comment on attachment 110406 [details] Patch This patch addresses all expected result on all platforms I have access to. I moved the primary method to ChromePrintingContext, and I moved code dealing with resizing the canvas back into WebViewHost. I also added support for both WEBKIT_USE_CG and WEBKIT_USE_SKIA. I think it's really ready to go now.

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

Comment on attachment 110406 [details] Patch Attachment 110406 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10015856 New failing tests: printing/single-line-must-not-be-split-into-two-pages.html printing/setPrinting.html

Aww crap. I failed to notice that the linux expected results order is chromium-linux -> chromium-win -> chromium. I'll have to copy the chromium expected results to chromium-linux. New patch tomorrow AM. Please still review, as the failing tests will definitely get sorted.

Created attachment 110448 [details] Patch that moves the chromium results to chromium-linux, otherwise the same as the previous one.

Comment on attachment 110448 [details] Patch that moves the chromium results to chromium-linux, otherwise the same as the previous one. View in context: https://bugs.webkit.org/attachment.cgi?id=110448&action=review > Source/WebKit/chromium/public/WebFrame.h:542 > + virtual WebString renderTreeAsText(bool showDebugInfo = false, bool showPrintedText = false) const = 0; nit: normally, we try to avoid functions with strings of bool parameters. it causes the callsite to be pretty confusing: renderTreeAsText(true, false) it is not obvious what those parameters mean at the callsite. enums work better or an option struct with bool fields work better.

Created attachment 112569 [details] Patch

Comment on attachment 112569 [details] Patch This patch changes the control parameters for WebFrame::renderTreeAsText to use an enum as the reviewers suggested. The enum is defined in chromium/public/WebFrame.h and it explciitly does not match the WebCore enum because the necessary values differ. All changes to LayoutTests should be good to go.

Comment on attachment 112569 [details] Patch Having fishd's glance, now it lgtm now.

Comment on attachment 112569 [details] Patch Clearing flags on attachment: 112569 Committed r98634: <http://trac.webkit.org/changeset/98634>

All reviewed patches have been landed. Closing bug.