Bug 37203 - Add layoutTestController.setPrinting()
Summary: Add layoutTestController.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: Nobody
URL:
Keywords:
Depends on:
Blocks: 20011
  Show dependency treegraph
 
Reported: 2010-04-07 05:31 PDT by Shinichiro Hamaji
Modified: 2010-04-30 22:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1 (23.95 KB, patch)
2010-04-07 05:37 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v2 (23.80 KB, patch)
2010-04-27 12:00 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v3 (23.73 KB, patch)
2010-04-27 12:14 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
fix for the buildbot failure (3.94 KB, patch)
2010-04-29 04:09 PDT, Shinichiro Hamaji
darin: review+
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-04-07 05:31:39 PDT
After this function is called, DRT should dump render tree with printing mode enabled. I proposed printToPNG in Bug 20011, but I want to add this function instead. I've noticed we don't need PNG images to test media queries, which means we can test media queries without pixel tests.

I'm planning to add pixel dump support after this bug. It will paint all pages into one PNG image with page separators.
Comment 1 Shinichiro Hamaji 2010-04-07 05:37:13 PDT
Created attachment 52730 [details]
Patch v1
Comment 2 Shinichiro Hamaji 2010-04-07 05:39:33 PDT
As I wrote in the ChangeLog, the pixel test of printing/media-queries-print.html is currently failing so the media-queries-print-expected.png has a red box and the text isn't italic. I'll fix this in a separated patch.
Comment 3 Shinichiro Hamaji 2010-04-20 06:33:46 PDT
Ping?
Comment 4 Shinichiro Hamaji 2010-04-27 07:58:05 PDT
Ping again?
Comment 5 Dimitri Glazkov (Google) 2010-04-27 08:56:08 PDT
I like the idea. Unless anyone has objections, I'll r+.
Comment 6 Eric Seidel (no email) 2010-04-27 10:03:54 PDT
Comment on attachment 52730 [details]
Patch v1

I think it's very confusing that it only works for dumps and not printing.

WebCore/rendering/RenderTreeAsText.cpp:635
 +      if (printContext.get())
I don't think you need the .get() here.

WebCore/rendering/RenderTreeAsText.cpp:636
 +          printContext->end();
Very sad that there is no way to have these auto-end.

WebCore/rendering/RenderTreeAsText.cpp:625
 +      if (RenderObject* o = frame->contentRenderer()) {
Having to indent here is less readable, but I understand why you did it. :(

WebCore/rendering/RenderTreeAsText.cpp:636
 +          printContext->end();
Does destructing the PrintContext auto-end it?

WebCore/rendering/RenderTreeAsText.h:43
 +      RenderAsTextInPrintingMode = 1 << 4 // Dump the tree in printing mode.
I'm not sure the "In" is necessary in this name.  But it's OK either way.

WebCore/rendering/RenderTreeAsText.cpp:619
 +          printContext->begin(pageWidthInPixels);
I'm not sure the extra parameter for the width is useful.  We should probably just use the frame width.
Comment 7 Shinichiro Hamaji 2010-04-27 12:00:40 PDT
Created attachment 54440 [details]
Patch v2
Comment 8 Darin Adler 2010-04-27 12:03:39 PDT
Comment on attachment 54440 [details]
Patch v2

> -- (NSString *)renderTreeAsExternalRepresentation;
> +- (NSString *)renderTreeAsExternalRepresentation:(BOOL)isPrinting;

The Objective-C way to name a function like this is:

    renderTreeAsExternalRepresentationForPrinting:(BOOL)forPrinting;

You have to make sure you name the arguments.
Comment 9 Darin Adler 2010-04-27 12:04:21 PDT
Comment on attachment 54440 [details]
Patch v2

> -            resultString = [mainFrame renderTreeAsExternalRepresentation];
> +            if (gLayoutTestController->isPrinting())
> +                resultString = [mainFrame renderTreeAsExternalRepresentation:YES];
> +            else
> +                resultString = [mainFrame renderTreeAsExternalRepresentation:NO];

I would write:

    resultString = [mainFrame renderTreeAsExternalRepresentationForPrinting:gLayoutTestController->isPrinting()];
Comment 10 Shinichiro Hamaji 2010-04-27 12:08:40 PDT
Thanks for your review!

(In reply to comment #6)
> (From update of attachment 52730 [details])
> I think it's very confusing that it only works for dumps and not printing.

So, do you think it's better not to enable media-queries-print.html in this bug to avoid confusion?
Comment 11 Shinichiro Hamaji 2010-04-27 12:14:47 PDT
Created attachment 54441 [details]
Patch v3
Comment 12 Darin Adler 2010-04-27 12:21:42 PDT
Comment on attachment 54441 [details]
Patch v3

>  PrintContext::~PrintContext()
>  {
> -    ASSERT(!m_isPrinting);
> +    if (m_isPrinting)
> +        end();
>      m_pageRects.clear();
>  }

Can you remove any of the explicit end() calls now?

r=me
Comment 13 Shinichiro Hamaji 2010-04-27 23:44:33 PDT
> Can you remove any of the explicit end() calls now?

Yes. I'll remove them in separate patch.
Comment 14 Shinichiro Hamaji 2010-04-28 00:30:33 PDT
Committed r58386: <http://trac.webkit.org/changeset/58386>
Comment 15 WebKit Review Bot 2010-04-28 00:40:25 PDT
http://trac.webkit.org/changeset/58386 might have broken Chromium Win Release and Chromium Linux Release
Comment 16 Shinichiro Hamaji 2010-04-28 00:49:40 PDT
Committed r58389: <http://trac.webkit.org/changeset/58389>
Comment 17 Shinichiro Hamaji 2010-04-28 02:58:33 PDT
Committed r58396: <http://trac.webkit.org/changeset/58396>
Comment 18 WebKit Review Bot 2010-04-28 03:27:03 PDT
http://trac.webkit.org/changeset/58396 might have broken SnowLeopard Intel Release (Tests)
Comment 19 Shinichiro Hamaji 2010-04-28 03:29:48 PDT
Committed r58398: <http://trac.webkit.org/changeset/58398>
Comment 20 Shinichiro Hamaji 2010-04-28 04:15:36 PDT
(In reply to comment #19)
> Committed r58398: <http://trac.webkit.org/changeset/58398>

I disabled the test as it failed on the buildbot. I'm guessing the value of domWindow()->screen()->width() depends on the size of physical screen or something , but I'm not sure. I'll investigate more.
Comment 21 Shinichiro Hamaji 2010-04-29 04:09:29 PDT
Created attachment 54688 [details]
fix for the buildbot failure
Comment 22 Shinichiro Hamaji 2010-04-29 04:14:31 PDT
(In reply to comment #21)
> Created an attachment (id=54688) [details]
> fix for the buildbot failure

The buildbot failure happened because my machine's domWindow()->screen()->width() was different from the builtbot's. I should have just used contentRenderer()->width().
Comment 23 Darin Adler 2010-04-29 13:30:07 PDT
Comment on attachment 54688 [details]
fix for the buildbot failure

> -        printContext.begin(frame->domWindow()->screen()->width());
> +        printContext.begin(frame->contentRenderer()->width());

What guarantees that contentRenderer is not zero?
Comment 24 Shinichiro Hamaji 2010-04-30 22:34:44 PDT
Committed r58629: <http://trac.webkit.org/changeset/58629>
Comment 25 Shinichiro Hamaji 2010-04-30 22:39:43 PDT
> What guarantees that contentRenderer is not zero?

I've added a NULL check before this line. Thanks for your review!