Created attachment 71653 [details] Simple test case See the attached printtes.html file. Relevant body: <body onload="window.print()"> <img src="http://code.google.com/p/chromium/logo?cct=1287781185"> <H1>Hello, world.</h1> </body> If you load that in any Chromium build after r58142 (webkit rev 66547), no print dialog comes up. If you remove the <img> tag, the print dialog displays. I found that Chromium 57968 (webkit 66396, 30 Aug) works as expected. I'm guessing this is related to the HTML5 parser. This regression breaks Print All from Gmail in Chrome 7 (517), and I'd like to get an update out in the next couple of weeks.
This bug isn't unique to Chromium, so it shouldn't have [chromium] in title. Removing it.
Created attachment 73359 [details] Patch v1
Attachment 73359 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/loader/DocumentLoader.cpp', u'WebCore/loader/DocumentLoader.h', u'WebCore/loader/FrameLoader.cpp', u'WebCore/manual-tests/print-onload-with-image.html', u'WebCore/page/DOMWindow.cpp', u'WebCore/page/DOMWindow.h']" exit_code: 1 WebCore/manual-tests/print-onload-with-image.html:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. Total errors found: 13 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 73362 [details] Patch v2
Created attachment 73363 [details] Patch v3
Sorry for spamming patches. I've just made minor fixes. I think this issue was introduced in https://bugs.webkit.org/show_bug.cgi?id=43658 and Darin has already mentioned the possibility of this issue. Sorry for my wrong review and thanks Darin for mentioning the issues.
Comment on attachment 73363 [details] Patch v3 I'm not an expert here, but this patch looks reasonable.
> I'm not an expert here, but this patch looks reasonable. Generally, that would mean a need to wait for an expert, wouldn't it? :)
> Generally, that would mean a need to wait for an expert, wouldn't it? :) If you're interested in reviewing the patch, please be my guest. :)
Unfortunately, I have no idea about how window.print works.
(In reply to comment #10) > Unfortunately, I have no idea about how window.print works. Do you know who a good expert to ask would be?
Thanks for reviewing and comments. All changes in my patch are suggested by Darin in Bug 43658 (if I didn't misread his comments), so it would be great if Darin has a chance to see my change. I'll wait for a few days anyway :)
Comment on attachment 73363 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=73363&action=review > WebCore/loader/DocumentLoader.cpp:312 > +void DocumentLoader::printWhenFinishedLoading() It was much better to have this deferred print capability in a higher level class, DOMWindow I believe. While I applaud the desire to do the printing at the right time, this should be done by having an appropriate hook to say the document is done loading, not by building the print function directly into DocumentLoader.
I agree with Darin, printing from DocumentLoader seems like bad abstraction.
Comment on attachment 73363 [details] Patch v3 Okiedokes.
Created attachment 73580 [details] Patch v4
Thanks Darin and Eric for your suggestion. I've updated my patch.
Attachment 73580 [details] did not build on chromium: Build output: http://queues.webkit.org/results/5621011
Created attachment 73581 [details] Patch v5
Comment on attachment 73581 [details] Patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=73581&action=review Looks like a good approach. I have a couple of reservations with the patch. > WebCore/loader/DocumentLoader.cpp:360 > + bool oldIsLoading = m_loading; A better name for this local variable is “wasLoading”. > WebCore/page/DOMWindow.cpp:898 > if (m_frame->loader()->isLoading()) { > - m_printDeferred = true; > + m_shouldPrintWhenFinishedLoading = true; > return; > } It’s not good that the deferral mechanism uses FrameLoader’s definition of isLoading, but the actual trigger for printing uses the DocumentLoader definition. Is there a way to make the two of them consistent? > WebCore/page/DOMWindow.cpp:1573 > + if (m_shouldPrintWhenFinishedLoading) > + print(); I think we should set m_shouldPrintWhenFinishedLoading to false here, even though we don’t have to. That could prevent double printing if something goes wrong elsewhere.
Committed r71892: <http://trac.webkit.org/changeset/71892>
Thanks for the great comments! I've landed the change with fixes. > It’s not good that the deferral mechanism uses FrameLoader’s definition of isLoading, but the actual trigger for printing uses the DocumentLoader definition. Is there a way to make the two of them consistent? We can use m_frame->loader()->activeDocumentLoader()->isLoading() instead of m_frame->loader()->isLoading() in DOMWindow.cpp so we explicitly use DocumentLoader.