Move pageScaleFactor code from Frame.{h|cpp} to Page.{h|cpp}

[Fady Samuel]

Move pageScaleFactor code from Frame.{h|cpp} to Page.{h|cpp}

[Fady Samuel]

Created attachment 105725 [details]
Patch

[Fady Samuel]

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.

[Simon Fraser (smfr)]

What happens when a page goes into and out of the page cache? Seems like we'd want to preserve the scale.

[Early Warning System Bot]

Comment on attachment 105725 [details]
Patch

Attachment 105725 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9572400

[Beth Dakin]

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

[WebKit Review Bot]

Comment on attachment 105725 [details]
Patch

Attachment 105725 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9572459

[Gustavo Noronha (kov)]

Comment on attachment 105725 [details]
Patch

Attachment 105725 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9564832

[Fady Samuel]

Created attachment 105789 [details]
Patch

[Darin Adler]

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

[Adam Roben (:aroben)]

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.

[Fady Samuel]

Created attachment 106172 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106172 [details]
Patch

Attachment 106172 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9586531

[Fady Samuel]

Created attachment 106181 [details]
Patch

[Darin Adler]

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?

[Fady Samuel]

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?

[Fady Samuel]

Created attachment 106199 [details]
Patch

[Fady Samuel]

Darin,

I've addressed all your points except the helper method at the moment. Could you please clarify? Thanks.

[WebKit Review Bot]

Comment on attachment 106199 [details]
Patch

Attachment 106199 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9584622

[Fady Samuel]

Created attachment 106222 [details]
Patch

[Fady Samuel]

Created attachment 106232 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106232 [details]
Patch

Attachment 106232 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9582916

[Fady Samuel]

Created attachment 106596 [details]
Patch

[Simon Fraser (smfr)]

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.

[Adam Roben (:aroben)]

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

[Fady Samuel]

(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!

[Fady Samuel]

Created attachment 106778 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 106778 [details]
Patch

Is there a reason why you haven't added the helper function that Darin recommended?

[Simon Fraser (smfr)]

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()?

[Fady Samuel]

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.

[Fady Samuel]

Created attachment 106783 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106783 [details]
Patch

Attachment 106783 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9622421

[Gustavo Noronha (kov)]

Comment on attachment 106783 [details]
Patch

Attachment 106783 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9623391

[Fady Samuel]

Created attachment 106804 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106804 [details]
Patch

Attachment 106804 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9623456

[WebKit Review Bot]

Comment on attachment 106804 [details]
Patch

Attachment 106804 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9624396

[Fady Samuel]

Created attachment 106893 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106893 [details]
Patch

Clearing flags on attachment: 106893

Committed r94889: <http://trac.webkit.org/changeset/94889>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.