Attachment 346616 Details for Bug 188300 – Patch for landing

[patch] Patch for landing

bug-188300-20180806092958.patch (text/plain), 9.34 KB, created by Frédéric Wang (:fredw) on 2018-08-06 00:29:59 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Frédéric Wang (:fredw)

Created: 2018-08-06 00:29:59 PDT

Size: 9.34 KB

patch

obsolete

>Subversion Revision: 234586
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 35b4a0bee7cbfc70b72a03107b6fe4225ea06ee0..87b362aabb81d75b78ca60ded01339d2067da873 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,42 @@
>+2018-08-03  Frederic Wang  <fwang@igalia.com>
>+
>+        Make two-arguments versions of scrollBy/scrollTo depend on the one-argument versions
>+        https://bugs.webkit.org/show_bug.cgi?id=188300
>+
>+        Reviewed by Darin Adler.
>+
>+        This patch refactors a bit the scrollBy/scrollTo code, so that the two-arguments versions
>+        share the same code path as the more generic one-argument versions. In particular, this
>+        helps to implement the ScrollBehavior option (bug 188043) since the one-argument versions
>+        will require to distinguish between smooth and instant scrolling. The logic to normalize
>+        non finite left/right values or to use a fallback when they are absent is also factored out
>+        into ScrollToOptions. Additionally, this patch makes DOMWindow::scrollBy rely on
>+        DOMWindow::scrollTo, the only change being an optimization when scrolling to position (0, 0)
>+        added in #170063, which is not observable.
>+
>+        References:
>+        https://drafts.csswg.org/cssom-view/#dom-element-scroll
>+        https://drafts.csswg.org/cssom-view/#dom-element-scrollby
>+        https://drafts.csswg.org/cssom-view/#dom-window-scroll
>+        https://drafts.csswg.org/cssom-view/#dom-window-scrollby
>+
>+        No new tests, behavior is unchanged.
>+
>+        * dom/Element.cpp:
>+        (WebCore::Element::scrollBy): Make two-parameter version depends on one-parameter version
>+        and rewrite the normalize / fallback logic.
>+        (WebCore::Element::scrollTo): Rewrite the normalize / fallback logic.
>+        (WebCore::normalizeNonFiniteValue): Deleted. The logic is moved to ScrollToOptions.
>+        * page/DOMWindow.cpp:
>+        (WebCore::DOMWindow::scrollBy const): Make two-parameter version depends on one-parameter
>+        version and rewrite the normalize / fallback logic. Also implements the one-parameter version
>+        from scrollTo.
>+        (WebCore::DOMWindow::scrollTo const): Make two-parameter version depends on one-parameter
>+        version and rewrite the normalize / fallback logic.
>+        * page/ScrollToOptions.h: Add <cmath> to use std::isfinite
>+        (WebCore::ScrollToOptions::normalizeNonFiniteCoordinatesOrFallBackTo): New function to
>+        normalize left/right values or fallback to the specified value if it is missing.
>+
> 2018-08-05  Yusuke Suzuki  <utatane.tea@gmail.com>
> 
>         Add support for microtasks in workers
>diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
>index 79cecb585374b0c4f2cd46301bb73370eb2fdd65..b1b3eddda85f32031e233c209b0d9d77ee90b9fd 100644
>--- a/Source/WebCore/dom/Element.cpp
>+++ b/Source/WebCore/dom/Element.cpp
>@@ -692,17 +692,15 @@ void Element::scrollIntoViewIfNotVisible(bool centerIfNotVisible)
> 
> void Element::scrollBy(const ScrollToOptions& options)
> {
>-    return scrollBy(options.left.value_or(0), options.top.value_or(0));
>-}
>-
>-static inline double normalizeNonFiniteValue(double f)
>-{
>-    return std::isfinite(f) ? f : 0;
>+    ScrollToOptions scrollToOptions = normalizeNonFiniteCoordinatesOrFallBackTo(options, 0, 0);
>+    scrollToOptions.left.value() += scrollLeft();
>+    scrollToOptions.top.value() += scrollTop();
>+    return scrollTo(scrollToOptions);
> }
> 
> void Element::scrollBy(double x, double y)
> {
>-    scrollTo(scrollLeft() + normalizeNonFiniteValue(x), scrollTop() + normalizeNonFiniteValue(y));
>+    scrollBy({ x, y });
> }
> 
> void Element::scrollTo(const ScrollToOptions& options, ScrollClamping clamping)
>@@ -720,12 +718,12 @@ void Element::scrollTo(const ScrollToOptions& options, ScrollClamping clamping)
>     if (!renderer || !renderer->hasOverflowClip())
>         return;
> 
>-    // Normalize non-finite values for left and top dictionary members of options, if present.
>-    double x = options.left ? normalizeNonFiniteValue(options.left.value()) : adjustForAbsoluteZoom(renderer->scrollLeft(), *renderer);
>-    double y = options.top ? normalizeNonFiniteValue(options.top.value()) : adjustForAbsoluteZoom(renderer->scrollTop(), *renderer);
>-
>-    renderer->setScrollLeft(clampToInteger(x * renderer->style().effectiveZoom()), clamping);
>-    renderer->setScrollTop(clampToInteger(y * renderer->style().effectiveZoom()), clamping);
>+    ScrollToOptions scrollToOptions = normalizeNonFiniteCoordinatesOrFallBackTo(options,
>+        adjustForAbsoluteZoom(renderer->scrollLeft(), *renderer),
>+        adjustForAbsoluteZoom(renderer->scrollTop(), *renderer)
>+    );
>+    renderer->setScrollLeft(clampToInteger(scrollToOptions.left.value() * renderer->style().effectiveZoom()), clamping);
>+    renderer->setScrollTop(clampToInteger(scrollToOptions.top.value() * renderer->style().effectiveZoom()), clamping);
> }
> 
> void Element::scrollTo(double x, double y)
>diff --git a/Source/WebCore/page/DOMWindow.cpp b/Source/WebCore/page/DOMWindow.cpp
>index ff371715c0835dfbbeaaf5cf27eca8b459dda5c8..7421e29fc842780e20a8e15771a2f3a862c3380e 100644
>--- a/Source/WebCore/page/DOMWindow.cpp
>+++ b/Source/WebCore/page/DOMWindow.cpp
>@@ -1574,12 +1574,12 @@ double DOMWindow::devicePixelRatio() const
>     return page->deviceScaleFactor();
> }
> 
>-void DOMWindow::scrollBy(const ScrollToOptions& options) const
>+void DOMWindow::scrollBy(double x, double y) const
> {
>-    return scrollBy(options.left.value_or(0), options.top.value_or(0));
>+    return scrollBy({ x, y });
> }
> 
>-void DOMWindow::scrollBy(double x, double y) const
>+void DOMWindow::scrollBy(const ScrollToOptions& options) const
> {
>     if (!isCurrentlyDisplayedInFrame())
>         return;
>@@ -1590,29 +1590,18 @@ void DOMWindow::scrollBy(double x, double y) const
>     if (!view)
>         return;
> 
>-    // Normalize non-finite values (https://drafts.csswg.org/cssom-view/#normalize-non-finite-values).
>-    x = std::isfinite(x) ? x : 0;
>-    y = std::isfinite(y) ? y : 0;
>-
>-    IntSize scaledOffset(view->mapFromCSSToLayoutUnits(x), view->mapFromCSSToLayoutUnits(y));
>-    view->setContentsScrollPosition(view->contentsScrollPosition() + scaledOffset);
>+    ScrollToOptions scrollToOptions = normalizeNonFiniteCoordinatesOrFallBackTo(options, 0, 0);
>+    scrollToOptions.left.value() += view->contentsScrollPosition().x();
>+    scrollToOptions.top.value() += view->contentsScrollPosition().y();
>+    return scrollTo(scrollToOptions);
> }
> 
>-void DOMWindow::scrollTo(const ScrollToOptions& options, ScrollClamping clamping) const
>+void DOMWindow::scrollTo(double x, double y, ScrollClamping clamping) const
> {
>-    if (!isCurrentlyDisplayedInFrame())
>-        return;
>-
>-    RefPtr<FrameView> view = m_frame->view();
>-    if (!view)
>-        return;
>-
>-    double x = options.left ? options.left.value() : view->contentsScrollPosition().x();
>-    double y = options.top ? options.top.value() : view->contentsScrollPosition().y();
>-    return scrollTo(x, y, clamping);
>+    return scrollTo({ x, y }, clamping);
> }
> 
>-void DOMWindow::scrollTo(double x, double y, ScrollClamping) const
>+void DOMWindow::scrollTo(const ScrollToOptions& options, ScrollClamping) const
> {
>     if (!isCurrentlyDisplayedInFrame())
>         return;
>@@ -1621,16 +1610,16 @@ void DOMWindow::scrollTo(double x, double y, ScrollClamping) const
>     if (!view)
>         return;
> 
>-    // Normalize non-finite values (https://drafts.csswg.org/cssom-view/#normalize-non-finite-values).
>-    x = std::isfinite(x) ? x : 0;
>-    y = std::isfinite(y) ? y : 0;
>+    ScrollToOptions scrollToOptions = normalizeNonFiniteCoordinatesOrFallBackTo(options,
>+        view->contentsScrollPosition().x(), view->contentsScrollPosition().y()
>+    );
> 
>-    if (!x && !y && view->contentsScrollPosition() == IntPoint(0, 0))
>+    if (!scrollToOptions.left.value() && !scrollToOptions.top.value() && view->contentsScrollPosition() == IntPoint(0, 0))
>         return;
> 
>     document()->updateLayoutIgnorePendingStylesheets();
> 
>-    IntPoint layoutPos(view->mapFromCSSToLayoutUnits(x), view->mapFromCSSToLayoutUnits(y));
>+    IntPoint layoutPos(view->mapFromCSSToLayoutUnits(scrollToOptions.left.value()), view->mapFromCSSToLayoutUnits(scrollToOptions.top.value()));
>     view->setContentsScrollPosition(layoutPos);
> }
> 
>diff --git a/Source/WebCore/page/ScrollToOptions.h b/Source/WebCore/page/ScrollToOptions.h
>index 84a9d9ddb2805d7f2b3943764a3190a9ccf5d0ff..87eeaa9cca9527681f383fc2b4d449f0d480e24f 100644
>--- a/Source/WebCore/page/ScrollToOptions.h
>+++ b/Source/WebCore/page/ScrollToOptions.h
>@@ -28,6 +28,7 @@
> 
> #pragma once
> 
>+#include <cmath>
> #include <wtf/Optional.h>
> 
> namespace WebCore {
>@@ -37,4 +38,19 @@ struct ScrollToOptions {
>     std::optional<double> top;
> };
> 
>+inline double normalizeNonFiniteValueOrFallBackTo(std::optional<double> value, double fallbackValue)
>+{
>+    // Normalize non-finite values (https://drafts.csswg.org/cssom-view/#normalize-non-finite-values).
>+    return value ? (std::isfinite(*value) ? *value : 0) : fallbackValue;
>+}
>+
>+// FIXME(https://webkit.org/b/88339): Consider using FloatPoint or DoublePoint for fallback and return values.
>+inline ScrollToOptions normalizeNonFiniteCoordinatesOrFallBackTo(const ScrollToOptions& value, double x, double y)
>+{
>+    ScrollToOptions options;
>+    options.left = normalizeNonFiniteValueOrFallBackTo(value.left, x);
>+    options.top = normalizeNonFiniteValueOrFallBackTo(value.top, y);
>+    return options;
>+}
>+
> }

Flags:

fred.wang: commit-queue+

Actions: View | Formatted Diff | Diff

Attachments on bug 188300: 346469 | 346512 | 346583 | 346616 | 346619