Bug 48385 - Add WebKit SPI to scale a WebView
Summary: Add WebKit SPI to scale a WebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-26 15:45 PDT by Beth Dakin
Modified: 2010-10-27 14:49 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.39 KB, patch)
2010-10-26 15:45 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
New patch (16.79 KB, patch)
2010-10-27 09:29 PDT, Beth Dakin
darin: review+
Details | Formatted Diff | Diff
Patch (15.38 KB, patch)
2010-10-27 13:57 PDT, Beth Dakin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2010-10-26 15:45:56 PDT
Created attachment 71952 [details]
Patch

Add WebKit SPI to scale a WebView.
Comment 1 Simon Fraser (smfr) 2010-10-26 16:54:23 PDT
Comment on attachment 71952 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71952&action=review

r- because I think this isn't quite right yet.

> WebCore/ChangeLog:5
> +        Reviewed by NOBODY (OOPS!).
> +
> +        This patch adds SPI to Mac WebKit that scales the page by the given 

You should file a bugzilla or put a radar number here.

> WebCore/page/Frame.cpp:988
> +    root->setStyle(newStyle.release());

It worries me that this style might get clobbered by something else. I thought that styleWillChange() was the right place to "fix up" the style, but that doesn't seem to be the case.

> WebCore/page/Frame.cpp:992
> +    m_pageScaleFactor = scale;
> +    m_pageScaleOrigin = origin;
> +

I don't see m_pageScaleOrigin being used anywhere.

> WebKit/mac/WebView/WebViewData.h:84
> +    float viewScaleFactor;
> +

Do you really need to store the scale in two places (here and Frame)?

> WebKitTools/DumpRenderTree/mac/DumpRenderTreeDraggingInfo.mm:108
> -#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> +#if 0

I don't think you want to commit this.
Comment 2 Simon Fraser (smfr) 2010-10-26 17:02:11 PDT
> > WebCore/ChangeLog:5
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        This patch adds SPI to Mac WebKit that scales the page by the given 
> 
> You should file a bugzilla or put a radar number here.

Er, mention this bug! :)
Comment 3 Eric Seidel (no email) 2010-10-26 19:46:17 PDT
Attachment 71952 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4748030
Comment 4 Beth Dakin 2010-10-27 09:29:09 PDT
Created attachment 72044 [details]
New patch

Here is a new patch that addresses all of Simon's concerns and takes a stab at the WebKit2 SPI.
Comment 5 Early Warning System Bot 2010-10-27 09:41:43 PDT
Attachment 72044 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4846040
Comment 6 Darin Adler 2010-10-27 11:44:15 PDT
Comment on attachment 72044 [details]
New patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72044&action=review

> WebCore/page/Frame.cpp:163
> +    , m_pageScaleFactor(1)

I’m disappointed to see more being added to Frame. I’ve been trying to cut down on operations in Frame, and would normally look for a different home for such a thing. On the other hand, I was unsuccessful at moving the page and text zoom factor out.

If this does have to be on the central “god object” maybe it makes more sense to put it on Page than on Frame.

> WebCore/page/Frame.cpp:967
> +    if (m_pageScaleFactor == scale)
> +        return;

Will this do the right thing in the case where the scale factor is the same, but the origin is different?

> WebCore/page/Frame.cpp:979
> +    // Update the scroll position to the origin of the scale.
> +    if (FrameView* view = this->view())
> +        view->setScrollPosition(IntPoint(origin.x(), origin.y()));

I really don’t understand why this is the right thing to do and why we need a single function call that does both of these things? Can’t we set the scroll position another way.
Comment 7 Beth Dakin 2010-10-27 13:57:25 PDT
Created attachment 72083 [details]
Patch
Comment 8 Beth Dakin 2010-10-27 14:00:14 PDT
Thanks for the review, Darin! I thought I would post a new patch with some of the tweaks I made.

(In reply to comment #6)
> (From update of attachment 72044 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72044&action=review
> 
> > WebCore/page/Frame.cpp:163
> > +    , m_pageScaleFactor(1)
> 
> I’m disappointed to see more being added to Frame. I’ve been trying to cut down on operations in Frame, and would normally look for a different home for such a thing. On the other hand, I was unsuccessful at moving the page and text zoom factor out.
> 
> If this does have to be on the central “god object” maybe it makes more sense to put it on Page than on Frame.

This is the one change I did not make. I kept the scale factor on the Frame because zoom is there too. But I don't think there would be a problem moving it to Page, and you could convince me to do that.

> 
> > WebCore/page/Frame.cpp:967
> > +    if (m_pageScaleFactor == scale)
> > +        return;
> 
> Will this do the right thing in the case where the scale factor is the same, but the origin is different?
> 
> > WebCore/page/Frame.cpp:979
> > +    // Update the scroll position to the origin of the scale.
> > +    if (FrameView* view = this->view())
> > +        view->setScrollPosition(IntPoint(origin.x(), origin.y()));
> 
> I really don’t understand why this is the right thing to do and why we need a single function call that does both of these things? Can’t we set the scroll position another way.

I decided to get rid of the whole origin concept for the time being. We might want to add it back in later, but for now, I think we should make it the caller's problem after all. We can add new SPI or tweak this SPI to handle to origin if we decide it doesn't make sense for the caller.
Comment 9 Darin Adler 2010-10-27 14:10:38 PDT
Comment on attachment 72083 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72083&action=review

I noticed that the Qt EWS had a failure; you should check into that.

> WebCore/page/Frame.cpp:977
> +    Document* document = this->document();
> +    if (!document)
> +        return;
> +
> +    m_pageScaleFactor = scale;

We should probably set m_pageScaleFactor even before checking the document for 0.

> WebCore/page/Frame.cpp:987
> +    document->recalcStyle(Node::Force);
> +
> +    if (FrameView* view = this->view()) {
> +        if (document->renderer() && document->renderer()->needsLayout() && view->didFirstLayout())
> +            view->layout();
> +    }

Why do we need to do all this synchronously? Can’t we use the normal machinery to do the style and layout instead of forcing it immediately? This is probably OK for now, but I think we can tighten it up later.
Comment 10 Beth Dakin 2010-10-27 14:49:10 PDT
Thanks Darin! Fixed with r70716.