Attachment 346512 Details for Bug 188300 – Adresses some of the review comments

[patch] Adresses some of the review comments

0001-Bug-188300-Make-two-arguments-versions-of-scrollBy-s.patch (text/plain), 9.47 KB, created by Frédéric Wang (:fredw) on 2018-08-03 12:11:08 PDT

(hide)

Description:

Filename:

MIME Type:

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

Created: 2018-08-03 12:11:08 PDT

Size: 9.47 KB

patch

obsolete

>From a3ff6534ae0420b6c443a530ec477522eb0bd1d4 Mon Sep 17 00:00:00 2001
>From: Frederic Wang <fwang@igalia.com>
>Date: Fri, 3 Aug 2018 15:49:26 +0200
>Subject: [PATCH xserver 1/2] Bug 188300 - Make two-arguments versions of
> scrollBy/scrollTo depend on the one-argument versions
>
>---
> Source/WebCore/ChangeLog              | 39 ++++++++++++++++++++
> Source/WebCore/dom/Element.cpp        | 26 ++++++-------
> Source/WebCore/page/DOMWindow.cpp     | 53 +++++++++------------------
> Source/WebCore/page/ScrollToOptions.h |  8 ++++
> 4 files changed, 78 insertions(+), 48 deletions(-)
>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 0234a6a8edd..5e4a42dacd9 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 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 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-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 4549e875e29..786bb5ea6d5 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.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 +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 813fcaf5525..e4c498afb50 100644
>--- a/Source/WebCore/page/DOMWindow.cpp
>+++ b/Source/WebCore/page/DOMWindow.cpp
>@@ -1574,45 +1574,26 @@ 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;
>-
>-    document()->updateLayoutIgnorePendingStylesheets();
>-
>-    FrameView* view = m_frame->view();
>-    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 = options;
>+    scrollToOptions.normalizeNonFiniteCoordinatesOrFallBackTo(0, 0);
>+    scrollToOptions.left.value() += scrollLeft();
>+    scrollToOptions.top.value() += scrollTop();
>+    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 +1602,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 84a9d9ddb28..8318ccdcae7 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 {
>@@ -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;
>+    }
> };
> 
> }
>-- 
>2.17.0
>

Actions: View | Formatted Diff | Diff

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