Bug 188300 – Make two-arguments versions of scrollBy/scrollTo depend on the one-argument versions

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Frédéric Wang (:fredw)]

This will help for bug 188043, since the two-version parameters still need to use "Auto" behavior and hence resolves the CSS behavior value:

https://drafts.csswg.org/cssom-view/#dom-window-scroll
https://drafts.csswg.org/cssom-view/#dom-element-scroll

[Frédéric Wang (:fredw)]

Created attachment 346469 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 346469 [details]
Patch

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

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

[Darin Adler]

Comment on attachment 346469 [details]
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.

[Frédéric Wang (:fredw)]

Comment on attachment 346469 [details]
Patch

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

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

OK, I think I can integrate that change in this no-behavior-change patch.

>> Source/WebCore/dom/Element.cpp:725
>> +        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.

I was irritated too. However, I was thinking that if normalizeNonFiniteCoordinatesOrFallBackTo is inlined then the default values don't need to be computed and we don't lose in efficiency.

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

Thanks for the suggestions, I was also considering some of these things. I'll experiment that again next Monday, but in any case I guess I'll try to avoid potential behavior changes in this patch and hence stick to doubles.

[Frédéric Wang (:fredw)]

Created attachment 346512 [details]
Adresses some of the review comments

[EWS Watchlist]

Comment on attachment 346469 [details]
Patch

Attachment 346469 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8759529

New failing tests:
legacy-animation-engine/imported/blink/transitions/unprefixed-transform.html

[EWS Watchlist]

Created attachment 346583 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Frédéric Wang (:fredw)]

Created attachment 346616 [details]
Patch for landing

[Frédéric Wang (:fredw)]

Created attachment 346619 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 346619 [details]
Patch for landing

Clearing flags on attachment: 346619

Committed r234596: <https://trac.webkit.org/changeset/234596>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42962870>