Bug 46152 - [Chromium] Need setPrinting
Summary: [Chromium] Need setPrinting
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Stephen Chenney
URL:
Keywords:
Depends on:
Blocks: 47578 49257
  Show dependency treegraph
 
Reported: 2010-09-20 18:21 PDT by Shinichiro Hamaji
Modified: 2011-10-27 13:54 PDT (History)
6 users (show)

See Also:


Attachments
Potential patch for missing setPrinting in chromium (15.95 KB, patch)
2011-06-20 13:24 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (11.68 KB, patch)
2011-09-07 13:59 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (16.20 KB, patch)
2011-09-08 08:30 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
New expected image result (39.12 KB, image/png)
2011-09-08 11:38 PDT, Stephen Chenney
no flags Details
New expected text result (796 bytes, text/plain)
2011-09-08 11:39 PDT, Stephen Chenney
no flags Details
Patch (330.60 KB, patch)
2011-09-27 12:31 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (1.78 MB, patch)
2011-10-10 14:45 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch that moves the chromium results to chromium-linux, otherwise the same as the previous one. (1.78 MB, patch)
2011-10-10 17:25 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (1.78 MB, patch)
2011-10-26 11:39 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinichiro Hamaji 2010-09-20 18:21:27 PDT
In Bug 20011, we implemented layoutTestController.setPrinting but chromium port doesn't have it yet.
Comment 1 Stephen Chenney 2011-06-20 13:24:06 PDT
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 2 Hajime Morrita 2011-06-27 02:29:45 PDT
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 3 Hajime Morrita 2011-06-27 02:30:22 PDT
Comment on attachment 97851 [details]
Potential patch for missing setPrinting in chromium

r- at this time.
Comment 4 Stephen Chenney 2011-09-07 13:59:43 PDT
Created attachment 106632 [details]
Patch
Comment 5 Stephen Chenney 2011-09-07 14:00:41 PDT
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 6 WebKit Review Bot 2011-09-07 14:23:16 PDT
Comment on attachment 106632 [details]
Patch

Attachment 106632 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9607474
Comment 7 Stephen Chenney 2011-09-08 08:30:13 PDT
Created attachment 106742 [details]
Patch
Comment 8 Stephen Chenney 2011-09-08 11:38:06 PDT
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
Comment 9 Stephen Chenney 2011-09-08 11:39:10 PDT
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
Comment 10 WebKit Review Bot 2011-09-08 11:42:31 PDT
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.
Comment 11 WebKit Review Bot 2011-09-08 11:43:41 PDT
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 12 WebKit Review Bot 2011-09-09 03:21:13 PDT
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
Comment 13 Stephen Chenney 2011-09-13 11:50:35 PDT
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.
Comment 14 Stephen Chenney 2011-09-27 12:31:16 PDT
Created attachment 108881 [details]
Patch
Comment 15 Hajime Morrita 2011-09-28 18:52:41 PDT
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
Comment 16 Stephen Chenney 2011-09-29 11:57:49 PDT
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.
Comment 17 Hajime Morrita 2011-10-05 22:14:57 PDT
(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.
Comment 18 Stephen Chenney 2011-10-10 14:45:51 PDT
Created attachment 110406 [details]
Patch
Comment 19 Stephen Chenney 2011-10-10 14:47:57 PDT
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.
Comment 20 WebKit Review Bot 2011-10-10 14:50:40 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 21 WebKit Review Bot 2011-10-10 16:06:46 PDT
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
Comment 22 Stephen Chenney 2011-10-10 16:51:32 PDT
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.
Comment 23 Stephen Chenney 2011-10-10 17:25:27 PDT
Created attachment 110448 [details]
Patch that moves the chromium results to chromium-linux, otherwise the same as the previous one.
Comment 24 Darin Fisher (:fishd, Google) 2011-10-24 10:34:01 PDT
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.
Comment 25 Stephen Chenney 2011-10-26 11:39:09 PDT
Created attachment 112569 [details]
Patch
Comment 26 Stephen Chenney 2011-10-26 11:47:38 PDT
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 27 Hajime Morrita 2011-10-27 11:23:51 PDT
Comment on attachment 112569 [details]
Patch

Having fishd's glance, now it lgtm now.
Comment 28 WebKit Review Bot 2011-10-27 13:54:25 PDT
Comment on attachment 112569 [details]
Patch

Clearing flags on attachment: 112569

Committed r98634: <http://trac.webkit.org/changeset/98634>
Comment 29 WebKit Review Bot 2011-10-27 13:54:35 PDT
All reviewed patches have been landed.  Closing bug.