Make setPrinting(false,...) restore the previous used media type

[Kenneth Rohde Christiansen]

Created attachment 49312 [details]
Patch

setPrinting(false,...) doesn't restore the previous used media type, but always sets it to "screen".

[Darin Adler]

Comment on attachment 49312 [details]
Patch

> +    static String prevMediaType = "screen";

Using a global for this is not a great solution. We would like to be able to print two different web pages at the same time some day, and restore each to the correct media type.

We don't have to leave the interface to setPrinting alone -- we can add a new argument or return value. Or even make a larger change to give it a more sensible interface.

Otherwise this seems OK.

[Kenneth Rohde Christiansen]

Ig I add a class variable for now, would that be OK?

[Kenneth Rohde Christiansen]

An option would be to make setMediaType with no arguments, return the previous type, what do you think about that?

or remove this responsibility from setPrinting all together, and deal with it outside. Though that is somewhat more difficult as this is propagated to sub frames.

[Kenneth Rohde Christiansen]

Darin,

I was thinking about change setPrinting into a

enterPrintingMode(minimumPageWidth, maximumPageWidth, adjustViewSize)

and a

leavePrintingMode(adjustViewSize),

but I do not understand how you are using it, for instance in WebKit/mac/WebView/WebHTMLView.mm I find code like

1028     for (unsigned i = 0; i < count; ++i)
1029         [[descendantWebHTMLViews objectAtIndex:i] _setPrinting:YES minimumPageWidth:0.0f maximumPageWidth:0.0f adjustViewSize:NO];

so the above might not cover your use cases.

When leaving printing mode, you normally call with 0 and min and max width, so is this supposed to correspond to a normal invalidation plus forceLayout?

What is the usecase for not calling adjustViewSize (thus adjusting the content size)?

[Kenneth Rohde Christiansen]

Created attachment 49325 [details]
Patch 2

Use an instance variable

[Kenneth Rohde Christiansen]

Darin, could you review the new patch?

[Darin Adler]

(In reply to comment #6)
> Darin, could you review the new patch?

Sure. I'll get to it as soon as I can.

[Eric Seidel (no email)]

Comment on attachment 49325 [details]
Patch 2

The patch doesn't apply, meaning we have no EWS results and it can't be commit-queue'd.  Could we fix the patch so that we can have EWS results (thus making the reviewer's life easier).

[Eric Seidel (no email)]

Comment on attachment 49325 [details]
Patch 2

Without EWS results I'm not comfortable reviewing this.  r-  Please upload a new patch which applies.

[Kenneth Rohde Christiansen]

Created attachment 50810 [details]
rebased patch

[Antonio Gomes]

(In reply to comment #10)
> Created an attachment (id=50810) [details]
> rebased patch

hey kenneth. I have a suggestion:

I do not think you have to implement your own 'log' and 'shouldBeEqual' functions, as they are already defined. Just do

<script src="../../js/resources/js-test-pre.js"></script>
<script type="application/javascript">
   (...) your script.
</script>
<script src="../js/resources/js-test-post.js"></script>

[Antonio Gomes]

Hum .. in fact, I just grepped the code, and a lot of layout tests are reimplementing their own shouldBeEqualXXX function. I can create a patch afterwards this one lands fixing them all, so my suggestion can be a followup and ignored for now.

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> Hum .. in fact, I just grepped the code, and a lot of layout tests are
> reimplementing their own shouldBeEqualXXX function. I can create a patch
> afterwards this one lands fixing them all, so my suggestion can be a followup
> and ignored for now.

Thanks for the hint, I will keep that in mind. If you update it in a future patch, that is fine with me.

[Antonio Gomes]

(In reply to comment #10)
> Created an attachment (id=50810) [details]
> rebased patch

kenneth, also would m_previousPrintMediaType be clearer than m_prePrintMediaType?

[Kenneth Rohde Christiansen]

Eric, could you review it now?

[Darin Adler]

Comment on attachment 50810 [details]
rebased patch

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

[Kenneth Rohde Christiansen]

Thanks for the comments Darin!

[Kenneth Rohde Christiansen]

Created attachment 50995 [details]
Patch addressing the comments

[WebKit Review Bot]

Attachment 50995 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/page/FrameView.cpp:806:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/FrameView.cpp:810:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/FrameView.cpp:814:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Created attachment 50996 [details]
Patch, fixing style issues

[Darin Adler]

Hooking up DumpRenderTree to test this on Mac should be very easy. -[WebView setMediaStyle:] is part of the public API to WebKit on Mac OS X.

[WebKit Commit Bot]

Comment on attachment 50996 [details]
Patch, fixing style issues

Clearing flags on attachment: 50996

Committed r56209: <http://trac.webkit.org/changeset/56209>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.