[webkit-reviews] review denied: [Bug 67250] Move pageScaleFactor code from Frame.{h|cpp} to Page.{h|cpp} : [Attachment 106181] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Fady Samuel <fsamuel at chromium.org>'s
request for review:
Bug 67250: Move pageScaleFactor code from Frame.{h|cpp} to Page.{h|cpp}
https://bugs.webkit.org/show_bug.cgi?id=67250

Attachment 106181: Patch
https://bugs.webkit.org/attachment.cgi?id=106181&action=review

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