Bug 48195

Summary:

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

Product:

WebKit

Reporter:

Mark Larson (Google) <mal>

Component:

DOM

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, ap, darin, dglazkov, eric, hamaji, tonyg, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

All

Attachments:

Description	Flags
Simple test case	none
Patch v1	none
Patch v2	none
Patch v3	none
Patch v4	none
Patch v5	darin: review+

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.