Bug 188300

Created attachment 346469 [details] Patch

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

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

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

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

Created attachment 346616 [details] Patch for landing

Created attachment 346619 [details] Patch for landing

Comment on attachment 346619 [details] Patch for landing Clearing flags on attachment: 346619 Committed r234596: <https://trac.webkit.org/changeset/234596>

All reviewed patches have been landed. Closing bug.

<rdar://problem/42962870>