Add WebKit SPI to scale a WebView

• Component: WebKit API
• Status: RESOLVED FIXED

[Beth Dakin]

Created attachment 71952 [details]
Patch

Add WebKit SPI to scale a WebView.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

> > 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! :)

[Eric Seidel (no email)]

Attachment 71952 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4748030

[Beth Dakin]

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.

[Early Warning System Bot]

Attachment 72044 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4846040

[Darin Adler]

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.

[Beth Dakin]

Created attachment 72083 [details]
Patch

[Beth Dakin]

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.

[Darin Adler]

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.

[Beth Dakin]

Thanks Darin! Fixed with r70716.