Created attachment 63790 [details] Patch to fix the bug. This is http://code.google.com/p/chromium/issues/detail?id=25027 I've included a minimal test case above. The expect result is a printed page with the words "Print me". The current result with Chrome and Safari is a blank page. I believe the solution is to defer printing until the page has finished loading. Attached is a patch to do this.
Attachment 63790 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/loader/FrameLoader.cpp:2231: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/page/DOMWindow.h:93: print_deferred is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/page/DOMWindow.h:401: m_print_deferred is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 63790 [details] did not build on qt: Build output: http://queues.webkit.org/results/3598897
Created attachment 63791 [details] Patch to fix the bug. (try 2)
Attachment 63791 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/loader/FrameLoader.cpp:2234: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 63790 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3668045
Hamaji might be able to review. He also might have some ideas on how to write a layout test for this. There are some general notes about layout tests here: http://trac.webkit.org/wiki/CreatingLayoutTests http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
Comment on attachment 63791 [details] Patch to fix the bug. (try 2) It needs a ChangeLog and a layout test (if possible). Tony's right; Hamaji-san is the right person to help here.
Is this change compatible with what other browsers do? Is it expected that if the script calls print() multiple times while loading, printing will only happen once at most. What happens if the script has called print() while loading, and before the page is done loading, the user closes the page or navigates to another page?
(In reply to comment #8) > Is this change compatible with what other browsers do? Is it expected that if the script calls print() multiple times while loading, printing will only happen once at most. What happens if the script has called print() while loading, and before the page is done loading, the user closes the page or navigates to another page? I tried changing the test page to call print() multiple times. On Windows, we have the following behavior: IE 8: prompts once always FF 3.6: prompts once always Chrome 5: prompts once if you hit cancel, multiple prompts if you hit ok Safari 4: prompts multiple times always Opera 10.60: does not prompt at all Based on this, I'd say it's ok if the WebKit based browser match IE/FF. It doesn't make sense to me to prompt multiple times on initial load. If you know of any cases where one would want to do that, please let me know. For the question of what happens when a script calls printing while the page is loading, but then the user navigates away, I made a simple html page that calls print(), followed by a few MBs of text. I restricted the bandwidth on the web server to 10 kbps to simulate slow rendering and found the following: IE 8: If you navigate away before the page loads, the print prompt never shows up. FF 3.6: If you navigate away, FF will pop up a print prompt and print out what it has loaded - which can be blank or a partial page. This behavior is also a bit weird because you could have closed the last FF window, but you'd get a lone print dialog. There's no clear defined behavior here either. For WebKit, with my patch, we'd match IE's behavior. Since the page never finished loading, we wouldn't get a print prompt. It appears I'd need to add a layout test to LayoutTests/printing ? I'll take a look there.
Created attachment 63966 [details] Defer printing patch 3 - add a changelog entry and pass check-webkit-style. Just a clean up patch, no real code change. I looked at the printing layout tests and it's not obvious to me how we can test the behavior in this bug. I'm open to suggestions.
Comment on attachment 63966 [details] Defer printing patch 3 - add a changelog entry and pass check-webkit-style. Oops. Sorry for the latency. I think the test method I've added cannot help testing window.print(). I think we can add WebCore/manual-tests for this test. This change looks good to me as the new behavior sounds more compatible to other browsers. I hesitate to r+ this change now because this patch is related to loader which I don't know well. WebCore/loader/FrameLoader.cpp:2233 + window->print(); A question: don't we need to reset m_printDeferred?
Created attachment 64371 [details] Defer printing patch 3 - add a manual test. (In reply to comment #11) > (From update of attachment 63966 [details]) > Oops. Sorry for the latency. I think the test method I've added cannot help testing window.print(). I think we can add WebCore/manual-tests for this test. Ok, I added a copy of 25027.htm to manual-tests. See try 4. > This change looks good to me as the new behavior sounds more compatible to other browsers. I hesitate to r+ this change now because this patch is related to loader which I don't know well. > > WebCore/loader/FrameLoader.cpp:2233 > + window->print(); > A question: don't we need to reset m_printDeferred? DOMWindow::print() resets m_printDeferred.
Comment on attachment 64371 [details] Defer printing patch 3 - add a manual test. Looks good. As I wrote in Comment 11, I'm not sure I'm a good reviewer for loader code. Could you wait for a few days before you land this to see if no one objects? > DOMWindow::print() resets m_printDeferred. Ah, I see. Thanks for clarification!
Comment on attachment 64371 [details] Defer printing patch 3 - add a manual test. Clearing flags on attachment: 64371 Committed r66428: <http://trac.webkit.org/changeset/66428>
All reviewed patches have been landed. Closing bug.
I see multiple problems with this patch: It’s not good how this patch spreads the feature over two classes. The code in both FrameLoader and DOMWindow is inelegant. All the code should be in one class, with the other class calling over as needed. That way you would not have to expose a printDeferred function. The name printDeferred is not a good boolean function or data member name. It sounds like a verb "print deferred". I would name it "should print when finished loading": m_shouldPrintWhenFinishedLoading Also, I think FrameLoader::finishedLoading may not be the right place to call this. That's the function called when the main resources is loaded, not when the entire page is loaded.
(In reply to comment #16) > I see multiple problems with this patch: > > It’s not good how this patch spreads the feature over two classes. The code in both FrameLoader and DOMWindow is inelegant. All the code should be in one class, with the other class calling over as needed. That way you would not have to expose a printDeferred function. > > The name printDeferred is not a good boolean function or data member name. It sounds like a verb "print deferred". I would name it "should print when finished loading": > > m_shouldPrintWhenFinishedLoading > > Also, I think FrameLoader::finishedLoading may not be the right place to call this. That's the function called when the main resources is loaded, not when the entire page is loaded. I'd be happy to fix this to address your concerns. Should I post a follow up patch to this bug / a separate bug, or revert? If FrameLoader::finishedLoading is the wrong place, then do you know where the right place is?
(In reply to comment #17) > I'd be happy to fix this to address your concerns. Should I post a follow up patch to this bug / a separate bug, or revert? I think a separate bug is the cleanest way to attack it. > If FrameLoader::finishedLoading is the wrong place, then do you know where the right place is? It would take a little thought to be sure. I would want it to be a place that exactly corresponded to whatever test we're doing to check if it should be deferred. So if the test is "isLoading" then it should be the place that makes isLoading false. Or if the test is "did not fire load event yet", then it should be the place that the load event is fired. Sorry, I don’t have time to do the research myself right now.
> I tried changing the test page to call print() multiple times. On Windows, we have the following behavior: > > IE 8: prompts once always > FF 3.6: prompts once always <...> > Based on this, I'd say it's ok if the WebKit based browser match IE/FF. This is not what I see in Firefox 3.6 on Mac or Windows. What I see is that window.print() is always synchronous in Firefox, just like in Safari 5 - as evidenced by the included test case, which fails in Firefox. So, this patch has changed WebKit from matching Firefox to matching IE. What am I missing? Why does the original issue with Bank of America not occur with Firefox?
(In reply to comment #19) > > I tried changing the test page to call print() multiple times. On Windows, we have the following behavior: > > > > IE 8: prompts once always > > FF 3.6: prompts once always > <...> > > Based on this, I'd say it's ok if the WebKit based browser match IE/FF. > > This is not what I see in Firefox 3.6 on Mac or Windows. What I see is that window.print() is always synchronous in Firefox, just like in Safari 5 - as evidenced by the included test case, which fails in Firefox. > > So, this patch has changed WebKit from matching Firefox to matching IE. What am I missing? Why does the original issue with Bank of America not occur with Firefox? Which test case did you try exactly? On Windows XP, with Firefox 3.6.12 and a CutePDF dummy printer, I loaded up http://leiz.org/chromium/25027_test2.html, which calls window.print() 3 times, but only pops up the print dialog once. screen capture of what I described above: http://leiz.org/chromium/25027_ff.avi
I've been testing with local files, and your test case also results in three synchronous print dialogs when loaded from a local file. Can you confirm that the regression test that went to manual-tests fails in Firefox for you? Looks like Firefox has a special case for file:///, or maybe it just has some kind of timing issue. Interesting.
(In reply to comment #21) > I've been testing with local files, and your test case also results in three synchronous print dialogs when loaded from a local file. Can you confirm that the regression test that went to manual-tests fails in Firefox for you? > > Looks like Firefox has a special case for file:///, or maybe it just has some kind of timing issue. Interesting. Yes, I do see the different behavior from Firefox when using a file:// url rather than http://. Perhaps some kind of safeguard against malicious webpages? The regression test did not fail for me with Firefox. When I loaded print-before-load.html, the CutePDF 'printer' output had the text from the webpage. Same for both file:// and http://.
> The regression test did not fail for me with Firefox. That's still strange, but is less important than the fact that Firefox also delays printing web pages until the page is loaded.
(In reply to comment #23) > That's still strange, but is less important than the fact that Firefox also delays printing web pages until the page is loaded. As far as I can tell from <http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#printing>, the user agent may delay printing, but if it does so, it should still print the page as it was when print() was called. At any rate, it may not defer the printing prompt. It appears as if Firefox (and perhaps WebKit browsers after this fix) or not in compliance with HTML.
s/or not/are not/
Then that means IE / FF / Webkit all violates the WHATWG spec, but at least they are consistent with respect to each other.
I've sent an e-mail to WHATWG about this.