Bug 37203 – Add layoutTestController.setPrinting()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Shinichiro Hamaji]

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.

[Shinichiro Hamaji]

Created attachment 52730 [details]
Patch v1

[Shinichiro Hamaji]

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.

[Shinichiro Hamaji]

Ping?

[Shinichiro Hamaji]

Ping again?

[Dimitri Glazkov (Google)]

I like the idea. Unless anyone has objections, I'll r+.

[Eric Seidel (no email)]

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.

[Shinichiro Hamaji]

Created attachment 54440 [details]
Patch v2

[Darin Adler]

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.

[Darin Adler]

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()];

[Shinichiro Hamaji]

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?

[Shinichiro Hamaji]

Created attachment 54441 [details]
Patch v3

[Darin Adler]

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

[Shinichiro Hamaji]

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

Yes. I'll remove them in separate patch.

[Shinichiro Hamaji]

Committed r58386: <http://trac.webkit.org/changeset/58386>

[WebKit Review Bot]

http://trac.webkit.org/changeset/58386 might have broken Chromium Win Release and Chromium Linux Release

[Shinichiro Hamaji]

Committed r58389: <http://trac.webkit.org/changeset/58389>

[Shinichiro Hamaji]

Committed r58396: <http://trac.webkit.org/changeset/58396>

[WebKit Review Bot]

http://trac.webkit.org/changeset/58396 might have broken SnowLeopard Intel Release (Tests)

[Shinichiro Hamaji]

Committed r58398: <http://trac.webkit.org/changeset/58398>

[Shinichiro Hamaji]

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

[Shinichiro Hamaji]

Created attachment 54688 [details]
fix for the buildbot failure

[Shinichiro Hamaji]

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

[Darin Adler]

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?

[Shinichiro Hamaji]

Committed r58629: <http://trac.webkit.org/changeset/58629>

[Shinichiro Hamaji]

> What guarantees that contentRenderer is not zero?

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