Bug 35308 - Make setPrinting(false,...) restore the previous used media type
: Make setPrinting(false,...) restore the previous used media type
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC All
: P3 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-02-23 11:40 PST by
Modified: 2010-03-18 22:11 PST (History)


Attachments
Patch (9.53 KB, patch)
2010-02-23 11:40 PST, Kenneth Rohde Christiansen
darin: review-
darin: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch 2 (12.52 KB, patch)
2010-02-23 13:55 PST, Kenneth Rohde Christiansen
eric: review-
eric: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
rebased patch (12.62 KB, patch)
2010-03-16 11:10 PST, Kenneth Rohde Christiansen
darin: review-
Review Patch | Details | Formatted Diff | Diff
Patch addressing the comments (15.68 KB, patch)
2010-03-17 19:31 PST, Kenneth Rohde Christiansen
no flags Review Patch | Details | Formatted Diff | Diff
Patch, fixing style issues (15.69 KB, patch)
2010-03-17 19:37 PST, Kenneth Rohde Christiansen
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-02-23 11:40:18 PST
Created an attachment (id=49312) [details]
Patch

setPrinting(false,...) doesn't restore the previous used media type, but always sets it to "screen".
------- Comment #1 From 2010-02-23 11:43:49 PST -------
(From update of attachment 49312 [details])
> +    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.
------- Comment #2 From 2010-02-23 11:49:33 PST -------
Ig I add a class variable for now, would that be OK?
------- Comment #3 From 2010-02-23 11:59:42 PST -------
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.
------- Comment #4 From 2010-02-23 13:25:01 PST -------
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)?
------- Comment #5 From 2010-02-23 13:55:42 PST -------
Created an attachment (id=49325) [details]
Patch 2

Use an instance variable
------- Comment #6 From 2010-02-26 05:21:24 PST -------
Darin, could you review the new patch?
------- Comment #7 From 2010-02-26 08:22:21 PST -------
(In reply to comment #6)
> Darin, could you review the new patch?

Sure. I'll get to it as soon as I can.
------- Comment #8 From 2010-03-05 14:52:28 PST -------
(From update of attachment 49325 [details])
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).
------- Comment #9 From 2010-03-15 15:53:02 PST -------
(From update of attachment 49325 [details])
Without EWS results I'm not comfortable reviewing this.  r-  Please upload a new patch which applies.
------- Comment #10 From 2010-03-16 11:10:33 PST -------
Created an attachment (id=50810) [details]
rebased patch
------- Comment #11 From 2010-03-16 11:43:53 PST -------
(In reply to comment #10)
> Created an attachment (id=50810) [details] [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>
------- Comment #12 From 2010-03-16 11:52:55 PST -------
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.
------- Comment #13 From 2010-03-16 12:28:16 PST -------
(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.
------- Comment #14 From 2010-03-16 13:14:16 PST -------
(In reply to comment #10)
> Created an attachment (id=50810) [details] [details]
> rebased patch

kenneth, also would m_previousPrintMediaType be clearer than m_prePrintMediaType?
------- Comment #15 From 2010-03-16 13:16:38 PST -------
Eric, could you review it now?
------- Comment #16 From 2010-03-16 13:26:42 PST -------
(From update of attachment 50810 [details])
> -    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-.
------- Comment #17 From 2010-03-16 13:32:30 PST -------
Thanks for the comments Darin!
------- Comment #18 From 2010-03-17 19:31:44 PST -------
Created an attachment (id=50995) [details]
Patch addressing the comments
------- Comment #19 From 2010-03-17 19:32:32 PST -------
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.
------- Comment #20 From 2010-03-17 19:37:55 PST -------
Created an attachment (id=50996) [details]
Patch, fixing style issues
------- Comment #21 From 2010-03-18 13:50:27 PST -------
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.
------- Comment #22 From 2010-03-18 22:11:15 PST -------
(From update of attachment 50996 [details])
Clearing flags on attachment: 50996

Committed r56209: <http://trac.webkit.org/changeset/56209>
------- Comment #23 From 2010-03-18 22:11:21 PST -------
All reviewed patches have been landed.  Closing bug.