48195 – REGRESSION: window.print in onload doesn't fire if there's an img

RESOLVED FIXED 48195

REGRESSION: window.print in onload doesn't fire if there's an img

https://bugs.webkit.org/show_bug.cgi?id=48195

Summary REGRESSION: window.print in onload doesn't fire if there's an img

Mark Larson (Google)

Reported 2010-10-23 14:55:41 PDT

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.

Attachments
Simple test case (258 bytes, text/html) 2010-10-23 14:55 PDT, Mark Larson (Google)	no flags	Details
Patch v1 (7.12 KB, patch) 2010-11-09 04:53 PST, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Patch v2 (7.02 KB, patch) 2010-11-09 04:59 PST, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Patch v3 (7.01 KB, patch) 2010-11-09 05:00 PST, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Patch v4 (5.39 KB, patch) 2010-11-10 21:24 PST, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Patch v5 (5.60 KB, patch) 2010-11-10 22:35 PST, Shinichiro Hamaji	darin: review+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2010-10-23 17:12:45 PDT

This bug isn't unique to Chromium, so it shouldn't have [chromium] in title. Removing it.

Shinichiro Hamaji

Comment 2 2010-11-09 04:53:51 PST

Created attachment 73359 [details] Patch v1

WebKit Review Bot

Comment 3 2010-11-09 04:56:55 PST

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.

Shinichiro Hamaji

Comment 4 2010-11-09 04:59:16 PST

Created attachment 73362 [details] Patch v2

Shinichiro Hamaji

Comment 5 2010-11-09 05:00:29 PST

Created attachment 73363 [details] Patch v3

Shinichiro Hamaji

Comment 6 2010-11-09 05:04:37 PST

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.

Adam Barth

Comment 7 2010-11-09 22:51:38 PST

Comment on attachment 73363 [details] Patch v3 I'm not an expert here, but this patch looks reasonable.

Alexey Proskuryakov

Comment 8 2010-11-09 23:03:23 PST

> 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? :)

Adam Barth

Comment 9 2010-11-09 23:25:43 PST

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

Alexey Proskuryakov

Comment 10 2010-11-09 23:36:35 PST

Unfortunately, I have no idea about how window.print works.

Adam Barth

Comment 11 2010-11-09 23:49:47 PST

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

Shinichiro Hamaji

Comment 12 2010-11-10 00:35:32 PST

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 :)

Darin Adler

Comment 13 2010-11-10 08:59:24 PST

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.

Eric Seidel (no email)

Comment 14 2010-11-10 09:26:09 PST

I agree with Darin, printing from DocumentLoader seems like bad abstraction.

Adam Barth

Comment 15 2010-11-10 10:32:36 PST

Comment on attachment 73363 [details] Patch v3 Okiedokes.

Shinichiro Hamaji

Comment 16 2010-11-10 21:24:57 PST

Created attachment 73580 [details] Patch v4

Shinichiro Hamaji

Comment 17 2010-11-10 21:26:35 PST

Thanks Darin and Eric for your suggestion. I've updated my patch.

WebKit Review Bot

Comment 18 2010-11-10 21:58:59 PST

Attachment 73580 [details] did not build on chromium: Build output: http://queues.webkit.org/results/5621011

Shinichiro Hamaji

Comment 19 2010-11-10 22:35:57 PST

Created attachment 73581 [details] Patch v5

Darin Adler

Comment 20 2010-11-11 07:29:52 PST

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.

Shinichiro Hamaji

Comment 21 2010-11-11 22:52:10 PST

Committed r71892: <http://trac.webkit.org/changeset/71892>

Shinichiro Hamaji

Comment 22 2010-11-11 22:56:51 PST

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.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P1

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware PC

OS All

Product WebKit

Component DOM

Assignee

Adam Barth

Reported

2010-10-23 14:55 PDT

Modified

2010-11-11 22:56 PST History

CC List

8 users Show

URL

Keywords

Depends on

Blocks