[webkit-reviews] review denied: [Bug 182230] [CSSOM View] Handle the scrollingElement in Element::scroll(Left/Top/Width/Height/To) : [Attachment 348772] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Frédéric Wang (:fredw)
<fred.wang at free.fr>'s request for review:
Bug 182230: [CSSOM View] Handle the scrollingElement in
Element::scroll(Left/Top/Width/Height/To)
https://bugs.webkit.org/show_bug.cgi?id=182230

Attachment 348772: Patch

https://bugs.webkit.org/attachment.cgi?id=348772&action=review




--- Comment #39 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 348772
  --> https://bugs.webkit.org/attachment.cgi?id=348772
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348772&action=review

> Source/WebCore/dom/Document.cpp:1466
> +    if (settings().CSSOMViewScrollingAPIEnabled() && inQuirksMode())

inQuirksModeI() is the more efficient check: put it first.

> Source/WebCore/dom/Document.h:453
>      WEBCORE_EXPORT Element* scrollingElement();
> +    // When calling from C++ code, use this method. scrollingElement() is
just for the web IDL implementation.
> +    Element* scrollingElementNoLayout();

It might be clearer to flip the naming here; rename scrollingElement() to
scrollingElementForAPI or scrollingElementUpdatingLayout() or something, and
make the no-layout version just be scrollingElement().

> Source/WebCore/dom/Element.cpp:817
> +static int adjustForZoom(int value, const Frame& frame)

This should say what is being adjusted. What is 'value'? Is it a scroll
position or offset?

> Source/WebCore/dom/Element.cpp:824
> +    // Needed because of truncation (rather than rounding) when scaling up.
> +    if (zoomFactor > 1)
> +	   value++;

Weird. Why not just floor/ceil after the division by zoomFactor?

> Source/WebCore/dom/Element.cpp:979
> +	   RefPtr<Frame> frame = document().frame();
> +	   if (!frame)
> +	       return 0;
> +	   RefPtr<FrameView> view = frame->view();
> +	   if (!view)
> +	       return 0;

This appears 6 times. Share it.