Attachment 346469 Details for Bug 188300 – Patch

[patch] Patch

bug-188300-20180803122854.patch (text/plain), 9.12 KB, created by Frédéric Wang (:fredw) on 2018-08-03 03:28:56 PDT

(hide)

Description:

Filename:

MIME Type:

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

Created: 2018-08-03 03:28:56 PDT

Size: 9.12 KB

patch

obsolete

>Subversion Revision: 234540
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 0234a6a8eddfc436b79e7bfc25424045e4f0e2b5..ef0a3b03d79e3bcb5ad0b1b215437a9cca6bcf63 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,41 @@
>+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 NOBODY (OOPS!).
>+
>+        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 moved to a
>+        common ScrollToOptions::normalizeNonFiniteCoordinatesOrFallbackTo. Note that
>+        DOMWindow::scrollTo has an optimization when scrolling to position (0, 0) but this is absent
>+        from DOMWindow::scrollBy, so this patch does not merge their implementations in order to
>+        avoid possible behavior change.
>+
>+        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 rely on normalizeNonFiniteCoordinatesOrFallbackTo.
>+        (WebCore::Element::scrollTo): Rely on normalizeNonFiniteCoordinatesOrFallbackTo.
>+        (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 rely on normalizeNonFiniteCoordinatesOrFallbackTo.
>+        (WebCore::DOMWindow::scrollTo const): Ditto.
>+        * page/ScrollToOptions.h: Add math.h 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-03  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         [WPE] Use WPE key symbols and new API instead of xkbcommon and the key mapper
>diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
>index 4549e875e297846b33748b5d9ce737b178b90810..a3778021c073be1df43ae17d5d9efcddc274ac8b 100644
>--- a/Source/WebCore/dom/Element.cpp
>+++ b/Source/WebCore/dom/Element.cpp
>@@ -692,17 +692,16 @@ 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 = options;
>+    scrollToOptions.normalizeNonFiniteCoordinatesOrFallbackTo(0, 0);
>+    scrollToOptions.left = scrollLeft() + scrollToOptions.left.value();
>+    scrollToOptions.top = scrollTop() + scrollToOptions.top.value();
>+    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 +719,13 @@ 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 = options;
>+    scrollToOptions.normalizeNonFiniteCoordinatesOrFallbackTo(
>+        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 813fcaf55253075f19b8122cf01c6a9434833fb1..d4ebf217255d296122c21549455e80ea53178f84 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));
>+    ScrollToOptions scrollToOptions = options;
>+    scrollToOptions.normalizeNonFiniteCoordinatesOrFallbackTo(0, 0);
>+    IntSize scaledOffset(view->mapFromCSSToLayoutUnits(scrollToOptions.left.value()), view->mapFromCSSToLayoutUnits(scrollToOptions.top.value()));
>     view->setContentsScrollPosition(view->contentsScrollPosition() + scaledOffset);
> }
> 
>-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,18 @@ 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 = options;
>+    scrollToOptions.normalizeNonFiniteCoordinatesOrFallbackTo(
>+        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..7372a7bd4db2125a8026b66e768af64e07a898c4 100644
>--- a/Source/WebCore/page/ScrollToOptions.h
>+++ b/Source/WebCore/page/ScrollToOptions.h
>@@ -28,6 +28,7 @@
> 
> #pragma once
> 
>+#include <math.h>
> #include <wtf/Optional.h>
> 
> namespace WebCore {
>@@ -35,6 +36,13 @@ namespace WebCore {
> struct ScrollToOptions {
>     std::optional<double> left;
>     std::optional<double> top;
>+
>+    void normalizeNonFiniteCoordinatesOrFallbackTo(double x, double y)
>+    {
>+        // Normalize non-finite values (https://drafts.csswg.org/cssom-view/#normalize-non-finite-values).
>+        left = left ? (std::isfinite(left.value()) ? left.value() : 0) : x;
>+        top = top ? (std::isfinite(top.value()) ? top.value() : 0) : y;
>+    };
> };
> 
> }

Actions: View | Formatted Diff | Diff

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