[webkit-reviews] review granted: [Bug 48385] Add WebKit SPI to scale a WebView : [Attachment 72044] New patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 48385: Add WebKit SPI to scale a WebView
https://bugs.webkit.org/show_bug.cgi?id=48385

Attachment 72044: New patch
https://bugs.webkit.org/attachment.cgi?id=72044&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.