Summary: | Add layoutTestController.setPrinting() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shinichiro Hamaji <hamaji> | ||||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric, hayato, webkit.review.bot, yuzo | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 20011 | ||||||||||||
Attachments: |
|
Description
Shinichiro Hamaji
2010-04-07 05:31:39 PDT
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!
|