[Webkit-unassigned] [Bug 35308] Make setPrinting(false, ...) restore the previous used media type

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #50810|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #16 from Darin Adler <darin at apple.com>  2010-03-16 13:26:42 PST ---
(From update of attachment 50810)
> -    view()->setMediaType(printing ? "print" : "screen");
> +
> +    if (printing) {
> +        m_prePrintMediaType = view()->mediaType();
> +        view()->setMediaType("print");
> +    } else
> +        view()->setMediaType(m_prePrintMediaType.isNull() ? "screen" : m_prePrintMediaType);
> +

If you call setPrinting(true) twice, this will store "print" in
m_prePrintMediaType.

If you call setPrinting(false) and it's already false, this will change the
media type to "screen" or to whatever media type was used the last time you
used setPrinting, perhaps a long time ago.

Maybe there's a better way to handle those cases.

> +        String m_prePrintMediaType;

Not a great name. What's a "pre print media type"? Also, this should be stored
in the FrameView alongside the actual media type, not here in the Frame. We're
trying to cut down the Frame object down and give the specific responsibilities
to more-specific classes. This is a step in the wrong direction. One idea would
be m_mediaTypeWhenNotPrinting.

I would write it more like this:

    void FrameView::adjustMediaTypeForPrinting(bool printing)
    {
        if (printing) {
            if (m_mediaTypeWhenNotPrinting.isNull())
                m_mediaTypeWhenNotPrinting = mediaType();
            setMediaType("print");
        } else {
            if (!m_mediaTypeWhenNotPrinting.isNull())
                setMediaType(m_mediaTypeWhenNotPrinting);
            m_mediaTypeWhenNotPrinting = String();
        }
    }

And call that function from Frame::setPrinting.

Seems to me this could be tested in a platform-independent way using
window.print().

I'm tempted to say review+, since there's nothing here that *absolutely* is
wrong. It's a bit too Qt-specific right now and new data member is not in the
right place, and I'm not happy with the data member name. I guess that adds up
to review-.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.