Bug 188300 - Make two-arguments versions of scrollBy/scrollTo depend on the one-argument versions
Summary: Make two-arguments versions of scrollBy/scrollTo depend on the one-argument v...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on:
Blocks: 188043 188343
  Show dependency treegraph
 
Reported: 2018-08-03 03:03 PDT by Frédéric Wang (:fredw)
Modified: 2018-08-06 02:58 PDT (History)
12 users (show)

See Also:


Attachments
Patch (9.12 KB, patch)
2018-08-03 03:28 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Adresses some of the review comments (9.47 KB, patch)
2018-08-03 12:11 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.94 MB, application/zip)
2018-08-04 01:21 PDT, Build Bot
no flags Details
Patch for landing (9.34 KB, patch)
2018-08-06 00:29 PDT, Frédéric Wang (:fredw)
fred.wang: commit-queue+
Details | Formatted Diff | Diff
Patch for landing (9.01 KB, patch)
2018-08-06 02:16 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-08-03 03:03:15 PDT
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
Comment 1 Frédéric Wang (:fredw) 2018-08-03 03:28:56 PDT
Created attachment 346469 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2018-08-03 03:43:16 PDT
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?
Comment 3 Darin Adler 2018-08-03 07:39:29 PDT
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.
Comment 4 Frédéric Wang (:fredw) 2018-08-03 08:09:20 PDT
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.
Comment 5 Frédéric Wang (:fredw) 2018-08-03 12:11:08 PDT
Created attachment 346512 [details]
Adresses some of the review comments
Comment 6 Build Bot 2018-08-04 01:21:40 PDT
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
Comment 7 Build Bot 2018-08-04 01:21:52 PDT
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
Comment 8 Frédéric Wang (:fredw) 2018-08-06 00:29:59 PDT
Created attachment 346616 [details]
Patch for landing
Comment 9 Frédéric Wang (:fredw) 2018-08-06 02:16:47 PDT
Created attachment 346619 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2018-08-06 02:56:14 PDT
Comment on attachment 346619 [details]
Patch for landing

Clearing flags on attachment: 346619

Committed r234596: <https://trac.webkit.org/changeset/234596>
Comment 11 WebKit Commit Bot 2018-08-06 02:56:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-08-06 02:58:19 PDT
<rdar://problem/42962870>