Bug 35308 - Make setPrinting(false,...) restore the previous used media type
: Make setPrinting(false,...) restore the previous used media type
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: PC All
: P3 Normal
Assigned To: Kenneth Rohde Christiansen
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-23 11:40 PST by Kenneth Rohde Christiansen
Modified: 2010-03-18 22:11 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2010-02-23 11:40:18 PST
Created attachment 49312 [details]
Patch

setPrinting(false,...) doesn't restore the previous used media type, but always sets it to "screen".
Comment 1 Darin Adler 2010-02-23 11:43:49 PST
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.
Comment 2 Kenneth Rohde Christiansen 2010-02-23 11:49:33 PST
Ig I add a class variable for now, would that be OK?
Comment 3 Kenneth Rohde Christiansen 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 Kenneth Rohde Christiansen 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 Kenneth Rohde Christiansen 2010-02-23 13:55:42 PST
Created attachment 49325 [details]
Patch 2

Use an instance variable
Comment 6 Kenneth Rohde Christiansen 2010-02-26 05:21:24 PST
Darin, could you review the new patch?
Comment 7 Darin Adler 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 Eric Seidel 2010-03-05 14:52:28 PST
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).
Comment 9 Eric Seidel 2010-03-15 15:53:02 PDT
Comment on attachment 49325 [details]
Patch 2

Without EWS results I'm not comfortable reviewing this.  r-  Please upload a new patch which applies.
Comment 10 Kenneth Rohde Christiansen 2010-03-16 11:10:33 PDT
Created attachment 50810 [details]
rebased patch
Comment 11 Antonio Gomes 2010-03-16 11:43:53 PDT
(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>
Comment 12 Antonio Gomes 2010-03-16 11:52:55 PDT
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 Kenneth Rohde Christiansen 2010-03-16 12:28:16 PDT
(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 Antonio Gomes 2010-03-16 13:14:16 PDT
(In reply to comment #10)
> Created an attachment (id=50810) [details]
> rebased patch

kenneth, also would m_previousPrintMediaType be clearer than m_prePrintMediaType?
Comment 15 Kenneth Rohde Christiansen 2010-03-16 13:16:38 PDT
Eric, could you review it now?
Comment 16 Darin Adler 2010-03-16 13:26:42 PDT
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-.
Comment 17 Kenneth Rohde Christiansen 2010-03-16 13:32:30 PDT
Thanks for the comments Darin!
Comment 18 Kenneth Rohde Christiansen 2010-03-17 19:31:44 PDT
Created attachment 50995 [details]
Patch addressing the comments
Comment 19 WebKit Review Bot 2010-03-17 19:32:32 PDT
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 Kenneth Rohde Christiansen 2010-03-17 19:37:55 PDT
Created attachment 50996 [details]
Patch, fixing style issues
Comment 21 Darin Adler 2010-03-18 13:50:27 PDT
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 WebKit Commit Bot 2010-03-18 22:11:15 PDT
Comment on attachment 50996 [details]
Patch, fixing style issues

Clearing flags on attachment: 50996

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