Bug 67250 - Move pageScaleFactor code from Frame.{h|cpp} to Page.{h|cpp}
Summary: Move pageScaleFactor code from Frame.{h|cpp} to Page.{h|cpp}
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-30 17:48 PDT by Fady Samuel
Modified: 2011-09-09 16:39 PDT (History)
9 users (show)

See Also:


Attachments
Patch (18.54 KB, patch)
2011-08-30 17:49 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (23.19 KB, patch)
2011-08-31 10:42 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (23.45 KB, patch)
2011-09-02 11:54 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (24.99 KB, patch)
2011-09-02 13:06 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (25.18 KB, patch)
2011-09-02 14:26 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (28.09 KB, patch)
2011-09-02 16:41 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (28.09 KB, patch)
2011-09-02 18:00 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (30.77 KB, patch)
2011-09-07 10:33 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (25.84 KB, patch)
2011-09-08 13:25 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (25.24 KB, patch)
2011-09-08 13:51 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (23.11 KB, patch)
2011-09-08 15:37 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (23.30 KB, patch)
2011-09-09 11:43 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2011-08-30 17:48:53 PDT
Move pageScaleFactor code from Frame.{h|cpp} to Page.{h|cpp}
Comment 1 Fady Samuel 2011-08-30 17:49:29 PDT
Created attachment 105725 [details]
Patch
Comment 2 Fady Samuel 2011-08-30 17:50:59 PDT
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.
Comment 3 Simon Fraser (smfr) 2011-08-30 17:52:42 PDT
What happens when a page goes into and out of the page cache? Seems like we'd want to preserve the scale.
Comment 4 Early Warning System Bot 2011-08-30 18:26:56 PDT
Comment on attachment 105725 [details]
Patch

Attachment 105725 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9572400
Comment 5 Beth Dakin 2011-08-30 18:28:20 PDT
(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 6 WebKit Review Bot 2011-08-31 00:09:54 PDT
Comment on attachment 105725 [details]
Patch

Attachment 105725 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9572459
Comment 7 Gustavo Noronha (kov) 2011-08-31 07:41:04 PDT
Comment on attachment 105725 [details]
Patch

Attachment 105725 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9564832
Comment 8 Fady Samuel 2011-08-31 10:42:59 PDT
Created attachment 105789 [details]
Patch
Comment 9 Darin Adler 2011-08-31 10:48:47 PDT
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 10 Adam Roben (:aroben) 2011-09-02 09:29:31 PDT
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.
Comment 11 Fady Samuel 2011-09-02 11:54:10 PDT
Created attachment 106172 [details]
Patch
Comment 12 WebKit Review Bot 2011-09-02 12:29:06 PDT
Comment on attachment 106172 [details]
Patch

Attachment 106172 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9586531
Comment 13 Fady Samuel 2011-09-02 13:06:13 PDT
Created attachment 106181 [details]
Patch
Comment 14 Darin Adler 2011-09-02 13:18:39 PDT
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?
Comment 15 Fady Samuel 2011-09-02 13:47:56 PDT
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?
Comment 16 Fady Samuel 2011-09-02 14:26:10 PDT
Created attachment 106199 [details]
Patch
Comment 17 Fady Samuel 2011-09-02 14:29:43 PDT
Darin,

I've addressed all your points except the helper method at the moment. Could you please clarify? Thanks.
Comment 18 WebKit Review Bot 2011-09-02 15:39:49 PDT
Comment on attachment 106199 [details]
Patch

Attachment 106199 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9584622
Comment 19 Fady Samuel 2011-09-02 16:41:08 PDT
Created attachment 106222 [details]
Patch
Comment 20 Fady Samuel 2011-09-02 18:00:30 PDT
Created attachment 106232 [details]
Patch
Comment 21 WebKit Review Bot 2011-09-02 21:01:49 PDT
Comment on attachment 106232 [details]
Patch

Attachment 106232 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9582916
Comment 22 Fady Samuel 2011-09-07 10:33:35 PDT
Created attachment 106596 [details]
Patch
Comment 23 Simon Fraser (smfr) 2011-09-08 10:55:05 PDT
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.
Comment 24 Adam Roben (:aroben) 2011-09-08 10:58:12 PDT
(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.
Comment 25 Fady Samuel 2011-09-08 11:18:15 PDT
(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!
Comment 26 Fady Samuel 2011-09-08 13:25:34 PDT
Created attachment 106778 [details]
Patch
Comment 27 Adam Roben (:aroben) 2011-09-08 13:27:29 PDT
Comment on attachment 106778 [details]
Patch

Is there a reason why you haven't added the helper function that Darin recommended?
Comment 28 Simon Fraser (smfr) 2011-09-08 13:29:27 PDT
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 29 Fady Samuel 2011-09-08 13:51:21 PDT
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.
Comment 30 Fady Samuel 2011-09-08 13:51:42 PDT
Created attachment 106783 [details]
Patch
Comment 31 WebKit Review Bot 2011-09-08 15:11:04 PDT
Comment on attachment 106783 [details]
Patch

Attachment 106783 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9622421
Comment 32 Gustavo Noronha (kov) 2011-09-08 15:24:09 PDT
Comment on attachment 106783 [details]
Patch

Attachment 106783 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9623391
Comment 33 Fady Samuel 2011-09-08 15:37:56 PDT
Created attachment 106804 [details]
Patch
Comment 34 WebKit Review Bot 2011-09-08 17:36:24 PDT
Comment on attachment 106804 [details]
Patch

Attachment 106804 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9623456
Comment 35 WebKit Review Bot 2011-09-08 18:10:38 PDT
Comment on attachment 106804 [details]
Patch

Attachment 106804 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9624396
Comment 36 Fady Samuel 2011-09-09 11:43:41 PDT
Created attachment 106893 [details]
Patch
Comment 37 WebKit Review Bot 2011-09-09 16:39:04 PDT
Comment on attachment 106893 [details]
Patch

Clearing flags on attachment: 106893

Committed r94889: <http://trac.webkit.org/changeset/94889>
Comment 38 WebKit Review Bot 2011-09-09 16:39:11 PDT
All reviewed patches have been landed.  Closing bug.