Bug 46152 – [Chromium] Need setPrinting

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Shinichiro Hamaji]

In Bug 20011, we implemented layoutTestController.setPrinting but chromium port doesn't have it yet.

[Stephen Chenney]

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.

[Hajime Morrita]

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

[Hajime Morrita]

Comment on attachment 97851 [details]
Potential patch for missing setPrinting in chromium

r- at this time.

[Stephen Chenney]

Created attachment 106632 [details]
Patch

[Stephen Chenney]

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.

[WebKit Review Bot]

Comment on attachment 106632 [details]
Patch

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

[Stephen Chenney]

Created attachment 106742 [details]
Patch

[Stephen Chenney]

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

[Stephen Chenney]

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

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Stephen Chenney]

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.

[Stephen Chenney]

Created attachment 108881 [details]
Patch

[Hajime Morrita]

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

[Stephen Chenney]

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.

[Hajime Morrita]

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

[Stephen Chenney]

Created attachment 110406 [details]
Patch

[Stephen Chenney]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Stephen Chenney]

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.

[Stephen Chenney]

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

[Darin Fisher (:fishd, Google)]

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.

[Stephen Chenney]

Created attachment 112569 [details]
Patch

[Stephen Chenney]

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.

[Hajime Morrita]

Comment on attachment 112569 [details]
Patch

Having fishd's glance, now it lgtm now.

[WebKit Review Bot]

Comment on attachment 112569 [details]
Patch

Clearing flags on attachment: 112569

Committed r98634: <http://trac.webkit.org/changeset/98634>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.