[webkit-reviews] review granted: [Bug 48195] REGRESSION: window.print in onload doesn't fire if there's an img : [Attachment 73581] Patch v5

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 48195: REGRESSION: window.print in onload doesn't fire if there's an img
https://bugs.webkit.org/show_bug.cgi?id=48195

Attachment 73581: Patch v5
https://bugs.webkit.org/attachment.cgi?id=73581&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.