[webkit-reviews] review granted: [Bug 188300] Make two-arguments versions of scrollBy/scrollTo depend on the one-argument versions : [Attachment 346469] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Frédéric Wang (:fredw)
<fred.wang at free.fr>'s request for review:
Bug 188300: Make two-arguments versions of scrollBy/scrollTo depend on the
one-argument versions
https://bugs.webkit.org/show_bug.cgi?id=188300

Attachment 346469: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 346469
  --> https://bugs.webkit.org/attachment.cgi?id=346469
Patch

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

Patch seems OK as is, but some of the code seems to be getting a little less
clear. Please consider some of my observations to decide if you want to do
things differently.

>> Source/WebCore/ChangeLog:16
>> +	    avoid possible behavior change.
> 
> This was added in https://trac.webkit.org/changeset/170063/webkit where not
test is added and where it is explained the change is not observable. So I
guess we can just make DOMWindow::scrollBy call DOMWindow::scrollTo in order to
share more code (maybe in another patch)? Any concern?

Yes, I think having that optimization for scrollBy too is likely fine. If it’s
not observable. Which is not a matter of opinion, but rather something we can
test and prove.

> Source/WebCore/dom/Element.cpp:698
> +    scrollToOptions.left = scrollLeft() + scrollToOptions.left.value();
> +    scrollToOptions.top = scrollTop() + scrollToOptions.top.value();

This can be written with +=:

    scrollToOptions.left.value() += scrollLeft();
    scrollToOptions.top.value() += scrollTop();

But see below for other suggestions about how to make this better.

> Source/WebCore/dom/Element.cpp:725
> +	   adjustForAbsoluteZoom(renderer->scrollLeft(), *renderer),
> +	   adjustForAbsoluteZoom(renderer->scrollTop(), *renderer)

We now do these computations unconditionally, which is less efficient than the
code before, which previously computed these only if needed. Not a big deal,
but a bit irritating.

> Source/WebCore/page/ScrollToOptions.h:31
> +#include <math.h>

To call std::isfinite, the correct header is <cmath>, not <math.h>.

> Source/WebCore/page/ScrollToOptions.h:40
> +    void normalizeNonFiniteCoordinatesOrFallbackTo(double x, double y)

"fall back to" is three words, and is not "fallback to", so the "b" in this
function name should be capitalized

Is this best as a member function?

- I think that adding this member function makes the ScrollToOptions struct
itself less minimal and harder to understand. Before this change, the struct is
simple; this starts mixing policy in with what was previously a pure data
holder. A trivial way to avoid that is to use a separate function. It can still
be an inline function and can still be in a header.

- I think this would be clearer as a function that takes an argument and
returns a result rather than something that modifies a structure in place.
You’ll notice that each of the call sites for this has to copy the structure
first before calling it; this is a hint that this would be helpful.

- For return value, I am a bit disappointed that we end up using
ScrollToOptions itself, losing the type information that the values are no
longer optional. It’s like if std::optional::value_or returned a std::optional.
Would be nice if we could return something like FloatPoint instead; but if we
really must leave these as doubles in internal computation then I suppose we
would need a DoublePoint.

- If we did start using something like FloatPoint, the scrolling math could
start being a lot clearer. We would make scrollTop/Left pair in a FloatPoint or
FloatSize and could write simpler code.

- Returning something other than a ScrollToOptions would make things a bit
inconvenient, I guess, when we are writing code paths that normalize and
dealing with optional twice. But that’s something unfortunate introduced in
this refactoring. We are now doing a bit of extra work and have lost a bit of
clarity by storing known non-optional things as optional and then calling other
functions that re-check to see if they are missing, even computing values that
are thrown away.

- It’s annoying to have substantial amounts of code that needs to deal
separately with x and y rather than using functions that can deal with multiple
dimensions at once; when things are still optional I suppose that’s likely
needed since each dimension has to be separately optional, but generally code
bloats and has opportunities for errors (oops, x instead of y) when we have to
separately write out both dimensions.

> Source/WebCore/page/ScrollToOptions.h:45
> +    };

Unnecessary semicolon.