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.
Created attachment 52730 [details] Patch v1
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.
Ping?
Ping again?
I like the idea. Unless anyone has objections, I'll r+.
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.
Created attachment 54440 [details] Patch v2
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 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()];
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?
Created attachment 54441 [details] Patch v3
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
> Can you remove any of the explicit end() calls now? Yes. I'll remove them in separate patch.
Committed r58386: <http://trac.webkit.org/changeset/58386>
http://trac.webkit.org/changeset/58386 might have broken Chromium Win Release and Chromium Linux Release
Committed r58389: <http://trac.webkit.org/changeset/58389>
Committed r58396: <http://trac.webkit.org/changeset/58396>
http://trac.webkit.org/changeset/58396 might have broken SnowLeopard Intel Release (Tests)
Committed r58398: <http://trac.webkit.org/changeset/58398>
(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.
Created attachment 54688 [details] fix for the buildbot failure
(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 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?
Committed r58629: <http://trac.webkit.org/changeset/58629>
> What guarantees that contentRenderer is not zero? I've added a NULL check before this line. Thanks for your review!