Move pageScaleFactor code from Frame.{h|cpp} to Page.{h|cpp}
Created attachment 105725 [details] Patch
This is based on a comment aroben added to Page.h last week. I'm not yet done refactoring (will try to finish tonight) but I thought I'd let you guys take a look at a work in progress to give me early feedback. Thanks.
What happens when a page goes into and out of the page cache? Seems like we'd want to preserve the scale.
Comment on attachment 105725 [details] Patch Attachment 105725 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9572400
(In reply to comment #3) > What happens when a page goes into and out of the page cache? Seems like we'd want to preserve the scale. Yes, our current implementation preserves scale for a page in the page cache, and we definitely want to maintain that.
Comment on attachment 105725 [details] Patch Attachment 105725 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9572459
Comment on attachment 105725 [details] Patch Attachment 105725 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9564832
Created attachment 105789 [details] Patch
Comment on attachment 105789 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105789&action=review review- because there seem to be many missing Page null checks > Source/WebCore/css/CSSStyleSelector.cpp:1173 > - documentStyle->setPageScaleTransform(frame ? frame->pageScaleFactor() : 1); > + documentStyle->setPageScaleTransform(document->page()->pageScaleFactor()); document->page() can be 0; this is not safe without a null check > Source/WebCore/html/HTMLBodyElement.cpp:271 > + Page* page = document->page(); > + float zoomFactor = frame->pageZoomFactor() * page->pageScaleFactor(); Unlike frame, page can be 0, so this is not safe without a null check (unless you can somehow prove that it is). > Source/WebCore/html/HTMLBodyElement.cpp:300 > + view->setScrollPosition(IntPoint(static_cast<int>(scrollLeft * frame->pageZoomFactor() * page->pageScaleFactor()), view->scrollY())); Unlike frame, page can be 0, so this is not safe without a null check (unless you can somehow prove that it is). > Source/WebCore/html/HTMLBodyElement.cpp:323 > + view->setScrollPosition(IntPoint(view->scrollX(), static_cast<int>(scrollTop * frame->pageZoomFactor() * page->pageScaleFactor()))); Unlike frame, page can be 0, so this is not safe without a null check (unless you can somehow prove that it is). > Source/WebCore/loader/HistoryController.cpp:89 > + item->setPageScaleFactor(m_frame->page()->pageScaleFactor()); Unlike frame, page can be 0, so this is not safe without a null check (unless you can somehow prove that it is).
Comment on attachment 105789 [details] Patch I agree with all the comments made so far. We should ensure that this doesn't change our behavior wrt. scaled pages going into/out of the page cache.
Created attachment 106172 [details] Patch
Comment on attachment 106172 [details] Patch Attachment 106172 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9586531
Created attachment 106181 [details] Patch
Comment on attachment 106181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106181&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1173 > - documentStyle->setPageScaleTransform(frame ? frame->pageScaleFactor() : 1); > + documentStyle->setPageScaleTransform(document->page() ? document->page()->pageScaleFactor() : 1.0f); Why the 1.0f? It wasn’t needed before, isn’t needed now, is inconsistent with the line of code above, and unnecessarily ties the code to a specific type. Seeing all these call sites makes me think we need a helper functions somewhere to implement the default of 1 so we don’t have to have explicit null checks repeated everywhere. Given what I see at all the call sites, the helper function could take a Document* and would make the code a lot easier to read, eliminating many local variables and extra lines of code. It could just be in the Page.h header: float pageScaleFactor(Document*); And then implemented in Page.cpp in the obvious way. > Source/WebCore/html/HTMLBodyElement.cpp:271 > + float pageScaleFactor = page ? page->pageScaleFactor() : 1.0f; Our style is to just use "1" in cases like this instead of "1.0f". > Source/WebCore/html/HTMLBodyElement.cpp:301 > + float pageScaleFactor = page ? page->pageScaleFactor() : 1.0f; Same "1" thing. > Source/WebCore/page/FrameView.cpp:1341 > - float pageScaleFactor = m_frame->pageScaleFactor(); > + float pageScaleFactor = m_frame->page()->pageScaleFactor(); What guarantees page() is non-zero here? > Source/WebCore/page/FrameView.cpp:1376 > - float pageScaleFactor = m_frame->pageScaleFactor(); > + float pageScaleFactor = m_frame->page()->pageScaleFactor(); What guarantees page() is non-zero here? > Source/WebCore/page/Page.cpp:616 > + Document* document = mainFrame()->document(); > + if (!document) > + return; I know the old code checked for a document of 0, but it can never be zero. > Source/WebCore/page/Page.cpp:631 > + if (FrameView* view = mainFrame()->view()) { Would probably be better to just get the view from the view() function on Document. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1254 > Page* page = frame ? frame->page() : 0; > - if (page->mainFrame()->pageScaleFactor() != 1) > + if (page->pageScaleFactor() != 1) Makes no sense that this does not check for zero. The line above goes to the trouble of checking the frame for zero, but then goes ahead and does a null-dereference in that case anyway! > Source/WebCore/rendering/RenderView.cpp:231 > + float pageScaleFactor = document()->page()->pageScaleFactor(); What guarantees that page() won’t be 0 here? > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:788 > + coreFrame->page()->scalePage(scalefactor, origin); What guarantees page() is non-zero here?
Thanks for catching these Darin!!! :) I don't understand your suggestion about the helper method. page might be null, and yet the helper method is in page? Should it be a static method of Page, then? Fady (In reply to comment #14) > (From update of attachment 106181 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106181&action=review > > > Source/WebCore/css/CSSStyleSelector.cpp:1173 > > - documentStyle->setPageScaleTransform(frame ? frame->pageScaleFactor() : 1); > > + documentStyle->setPageScaleTransform(document->page() ? document->page()->pageScaleFactor() : 1.0f); > > Why the 1.0f? It wasn’t needed before, isn’t needed now, is inconsistent with the line of code above, and unnecessarily ties the code to a specific type. > > Seeing all these call sites makes me think we need a helper functions somewhere to implement the default of 1 so we don’t have to have explicit null checks repeated everywhere. Given what I see at all the call sites, the helper function could take a Document* and would make the code a lot easier to read, eliminating many local variables and extra lines of code. > > It could just be in the Page.h header: > > float pageScaleFactor(Document*); > > And then implemented in Page.cpp in the obvious way. > > > Source/WebCore/html/HTMLBodyElement.cpp:271 > > + float pageScaleFactor = page ? page->pageScaleFactor() : 1.0f; > > Our style is to just use "1" in cases like this instead of "1.0f". > > > Source/WebCore/html/HTMLBodyElement.cpp:301 > > + float pageScaleFactor = page ? page->pageScaleFactor() : 1.0f; > > Same "1" thing. > > > Source/WebCore/page/FrameView.cpp:1341 > > - float pageScaleFactor = m_frame->pageScaleFactor(); > > + float pageScaleFactor = m_frame->page()->pageScaleFactor(); > > What guarantees page() is non-zero here? > > > Source/WebCore/page/FrameView.cpp:1376 > > - float pageScaleFactor = m_frame->pageScaleFactor(); > > + float pageScaleFactor = m_frame->page()->pageScaleFactor(); > > What guarantees page() is non-zero here? > > > Source/WebCore/page/Page.cpp:616 > > + Document* document = mainFrame()->document(); > > + if (!document) > > + return; > > I know the old code checked for a document of 0, but it can never be zero. > > > Source/WebCore/page/Page.cpp:631 > > + if (FrameView* view = mainFrame()->view()) { > > Would probably be better to just get the view from the view() function on Document. > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1254 > > Page* page = frame ? frame->page() : 0; > > - if (page->mainFrame()->pageScaleFactor() != 1) > > + if (page->pageScaleFactor() != 1) > > Makes no sense that this does not check for zero. The line above goes to the trouble of checking the frame for zero, but then goes ahead and does a null-dereference in that case anyway! > > > Source/WebCore/rendering/RenderView.cpp:231 > > + float pageScaleFactor = document()->page()->pageScaleFactor(); > > What guarantees that page() won’t be 0 here? > > > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:788 > > + coreFrame->page()->scalePage(scalefactor, origin); > > What guarantees page() is non-zero here?
Created attachment 106199 [details] Patch
Darin, I've addressed all your points except the helper method at the moment. Could you please clarify? Thanks.
Comment on attachment 106199 [details] Patch Attachment 106199 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9584622
Created attachment 106222 [details] Patch
Created attachment 106232 [details] Patch
Comment on attachment 106232 [details] Patch Attachment 106232 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9582916
Created attachment 106596 [details] Patch
Comment on attachment 106596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106596&action=review > Source/WebCore/loader/HistoryController.cpp:90 > + item->setPageScaleFactor(page ? page->pageScaleFactor() : 1.0f); Should be 1, not 1.0f > Source/WebCore/page/Page.cpp:633 > + if (FrameView* view = document->view()) { > + if (document->renderer() && document->renderer()->needsLayout() && view->didFirstLayout()) > + view->layout(); > + view->setScrollPosition(origin); > + } Why does this still do the layout if the scale didn't change? > Source/WebCore/page/Page.h:248 > + void scalePage(float scale, const LayoutPoint& origin); > + float pageScaleFactor() const { return m_pageScaleFactor; } It bugs me that these names are not symmetrical. Either call the first one setPageScaleFactor(), or call the second one pageScale(). > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:594 > + Page* page = frame()->page(); > + float pageScaleFactor = page ? page->pageScaleFactor() : 1; All this null checking is really tedious. Maybe Frame should have a convenience method that just turns around and calls Page, doing the null check.
(In reply to comment #15) > I don't understand your suggestion about the helper method. page might be null, and yet the helper method is in page? Should it be a static method of Page, then? I believe Darin was suggesting a free function (i.e., not a member function). The function would be declared in Page.h but would not be a member of the Page class.
(In reply to comment #23) > (From update of attachment 106596 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106596&action=review > > > Source/WebCore/loader/HistoryController.cpp:90 > > + item->setPageScaleFactor(page ? page->pageScaleFactor() : 1.0f); > > Should be 1, not 1.0f > > > Source/WebCore/page/Page.cpp:633 > > + if (FrameView* view = document->view()) { > > + if (document->renderer() && document->renderer()->needsLayout() && view->didFirstLayout()) > > + view->layout(); > > + view->setScrollPosition(origin); > > + } > > Why does this still do the layout if the scale didn't change? > > > Source/WebCore/page/Page.h:248 > > + void scalePage(float scale, const LayoutPoint& origin); > > + float pageScaleFactor() const { return m_pageScaleFactor; } > > It bugs me that these names are not symmetrical. Either call the first one setPageScaleFactor(), or call the second one pageScale(). > > > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:594 > > + Page* page = frame()->page(); > > + float pageScaleFactor = page ? page->pageScaleFactor() : 1; > > All this null checking is really tedious. Maybe Frame should have a convenience method that just turns around and calls Page, doing the null check. Ok, I'll change scalePage to setPageScaleFactor, and I'll clean up all the null checking with a helper method. Thanks!
Created attachment 106778 [details] Patch
Comment on attachment 106778 [details] Patch Is there a reason why you haven't added the helper function that Darin recommended?
Comment on attachment 106778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106778&action=review > Source/WebCore/dom/Range.cpp:2032 > + if (Page* page = document->page()) > + pageScale = page->pageScaleFactor(); Why not use Frame::pageScaleFactor()? > Source/WebCore/loader/HistoryController.cpp:90 > + Page* page = m_frame->page(); > + item->setPageScaleFactor(page ? page->pageScaleFactor() : 1); Why not use Frame::pageScaleFactor()? > Source/WebCore/page/Page.cpp:635 > + if (FrameView* view = document->view()) { > + if (view->scrollPosition() != origin) { > + if (document->renderer() && document->renderer()->needsLayout() && view->didFirstLayout()) > + view->layout(); > + view->setScrollPosition(origin); > + } > + } Why do layout if the scale didn't change? > Source/WebCore/rendering/RenderView.cpp:232 > + Page* page = document()->page(); > + float pageScaleFactor = page ? page->pageScaleFactor() : 1; Why not use Frame::pageScaleFactor()?
Comment on attachment 106778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106778&action=review >> Source/WebCore/dom/Range.cpp:2032 >> + pageScale = page->pageScaleFactor(); > > Why not use Frame::pageScaleFactor()? More code? >> Source/WebCore/loader/HistoryController.cpp:90 >> + item->setPageScaleFactor(page ? page->pageScaleFactor() : 1); > > Why not use Frame::pageScaleFactor()? Fixed. >> Source/WebCore/page/Page.cpp:635 >> + } > > Why do layout if the scale didn't change? Fixed.
Created attachment 106783 [details] Patch
Comment on attachment 106783 [details] Patch Attachment 106783 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9622421
Comment on attachment 106783 [details] Patch Attachment 106783 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9623391
Created attachment 106804 [details] Patch
Comment on attachment 106804 [details] Patch Attachment 106804 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9623456
Comment on attachment 106804 [details] Patch Attachment 106804 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9624396
Created attachment 106893 [details] Patch
Comment on attachment 106893 [details] Patch Clearing flags on attachment: 106893 Committed r94889: <http://trac.webkit.org/changeset/94889>
All reviewed patches have been landed. Closing bug.