Bug 48385

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.

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

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

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.

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

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.

Created attachment 72083 [details] Patch

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 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.

Thanks Darin! Fixed with r70716.