Simplify and streamline some Color-related code to prepare for some Color/ExtendedCol...
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Dec 2017 17:26:58 +0000 (17:26 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Dec 2017 17:26:58 +0000 (17:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180569

Reviewed by Sam Weinig.

* accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::colorValue const): Use valueAsColor instead of
having custom code here to parse the color string.

* css/CSSGradientValue.cpp:
(WebCore::interpolate): Deleted.
(WebCore::CSSGradientValue::computeStops): Call blend instead of interpolate. The only
difference is that the interpolate function truncated when converting from floating point
to integer, and the blend function rounds instead.

* css/StyleResolver.cpp:
(WebCore::StyleResolver::colorFromPrimitiveValue const): Removed unneeded special case
for identifier of 0, since StyleColor::colorFromKeyword already handles that correctly.
Also got rid of unneded local variable "state".

* html/ColorInputType.cpp:
(WebCore::isValidSimpleColor): Rewrote to take a StringView instead of a String and
to stay with a single loop since this does not need the extra efficiency of a separate
8-bit and 16-bit character version. Renamed to more closely match what the specification
calls this algorithm.
(WebCore::parseSimpleColorValue): Added. To be used instead of relying on the behavior of
the Color constructor that takes a String, so we can remove that later.
(WebCore::ColorInputType::sanitizeValue const): Updated for name change.
(WebCore::ColorInputType::valueAsColor const): Use parseSimpleColorValue instead of the
Color constructor that takes a string.
(WebCore::ColorInputType::typeMismatchFor const): Updated for name change.
(WebCore::ColorInputType::selectColor): Updated to take a StringView instead of a Color.
Note that this function is used for testing only.

* html/ColorInputType.h: Marked everything final instead of override. Updated the
selectColor function to take a StringView instead of a Color.

* html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::selectColor): Take a StringView instead of a Color.
* html/HTMLInputElement.h: Ditto.

* html/InputType.cpp:
(WebCore::InputType::selectColor): Take a StringView instead of a Color.
* html/InputType.h: Ditto.

* testing/Internals.cpp:
(WebCore::Internals::selectColorInColorChooser): Pass the string directly instead of
constructing a Color with it.
(WebCore::Internals::setViewBaseBackgroundColor): Instead of pasring the passed in string
with the Color constructor, implemented the two names that are actually used with this
function in tests: "transparent" and "white".

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@225680 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/accessibility/AccessibilityNodeObject.cpp
Source/WebCore/css/CSSGradientValue.cpp
Source/WebCore/css/StyleResolver.cpp
Source/WebCore/html/ColorInputType.cpp
Source/WebCore/html/ColorInputType.h
Source/WebCore/html/HTMLInputElement.cpp
Source/WebCore/html/HTMLInputElement.h
Source/WebCore/html/InputType.cpp
Source/WebCore/html/InputType.h
Source/WebCore/testing/Internals.cpp

index 358abe1..745c81d 100644 (file)
@@ -1,3 +1,57 @@
+2017-12-07  Darin Adler  <darin@apple.com>
+
+        Simplify and streamline some Color-related code to prepare for some Color/ExtendedColor work
+        https://bugs.webkit.org/show_bug.cgi?id=180569
+
+        Reviewed by Sam Weinig.
+
+        * accessibility/AccessibilityNodeObject.cpp:
+        (WebCore::AccessibilityNodeObject::colorValue const): Use valueAsColor instead of
+        having custom code here to parse the color string.
+
+        * css/CSSGradientValue.cpp:
+        (WebCore::interpolate): Deleted.
+        (WebCore::CSSGradientValue::computeStops): Call blend instead of interpolate. The only
+        difference is that the interpolate function truncated when converting from floating point
+        to integer, and the blend function rounds instead.
+
+        * css/StyleResolver.cpp:
+        (WebCore::StyleResolver::colorFromPrimitiveValue const): Removed unneeded special case
+        for identifier of 0, since StyleColor::colorFromKeyword already handles that correctly.
+        Also got rid of unneded local variable "state".
+
+        * html/ColorInputType.cpp:
+        (WebCore::isValidSimpleColor): Rewrote to take a StringView instead of a String and
+        to stay with a single loop since this does not need the extra efficiency of a separate
+        8-bit and 16-bit character version. Renamed to more closely match what the specification
+        calls this algorithm.
+        (WebCore::parseSimpleColorValue): Added. To be used instead of relying on the behavior of
+        the Color constructor that takes a String, so we can remove that later.
+        (WebCore::ColorInputType::sanitizeValue const): Updated for name change.
+        (WebCore::ColorInputType::valueAsColor const): Use parseSimpleColorValue instead of the
+        Color constructor that takes a string.
+        (WebCore::ColorInputType::typeMismatchFor const): Updated for name change.
+        (WebCore::ColorInputType::selectColor): Updated to take a StringView instead of a Color.
+        Note that this function is used for testing only.
+
+        * html/ColorInputType.h: Marked everything final instead of override. Updated the
+        selectColor function to take a StringView instead of a Color.
+
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::selectColor): Take a StringView instead of a Color.
+        * html/HTMLInputElement.h: Ditto.
+
+        * html/InputType.cpp:
+        (WebCore::InputType::selectColor): Take a StringView instead of a Color.
+        * html/InputType.h: Ditto.
+
+        * testing/Internals.cpp:
+        (WebCore::Internals::selectColorInColorChooser): Pass the string directly instead of
+        constructing a Color with it.
+        (WebCore::Internals::setViewBaseBackgroundColor): Instead of pasring the passed in string
+        with the Color constructor, implemented the two names that are actually used with this
+        function in tests: "transparent" and "white".
+
 2017-12-08  Konstantin Tokarev  <annulen@yandex.ru>
 
         Unreviewed, fix wrong letter case in #import HTMLIframeElement.h
index e94aa94..f94167b 100644 (file)
@@ -1900,12 +1900,7 @@ void AccessibilityNodeObject::colorValue(int& r, int& g, int& b) const
     if (!is<HTMLInputElement>(node()))
         return;
 
-    auto& input = downcast<HTMLInputElement>(*node());
-    if (!input.isColorControl())
-        return;
-
-    // HTMLInputElement::value always returns a string parseable by Color().
-    Color color(input.value());
+    auto color = downcast<HTMLInputElement>(*node()).valueAsColor();
     r = color.red();
     g = color.green();
     b = color.blue();
index 215f278..eef295b 100644 (file)
@@ -119,22 +119,6 @@ Ref<CSSGradientValue> CSSGradientValue::gradientWithStylesResolved(const StyleRe
     return result;
 }
 
-static inline int interpolate(int min, int max, float position)
-{
-    return min + static_cast<int>(position * (max - min));
-}
-
-static inline Color interpolate(const Color& color1, const Color& color2, float position)
-{
-    // FIXME: ExtendedColor - Doesn't work with extended colors, and really should be a helper in Color.h, not here.
-    int red = interpolate(color1.red(), color2.red(), position);
-    int green = interpolate(color1.green(), color2.green(), position);
-    int blue = interpolate(color1.blue(), color2.blue(), position);
-    int alpha = interpolate(color1.alpha(), color2.alpha(), position);
-
-    return Color(red, green, blue, alpha);
-}
-
 class LinearGradientAdapter {
 public:
     explicit LinearGradientAdapter(Gradient::LinearData& data)
@@ -421,8 +405,9 @@ Gradient::ColorStopVector CSSGradientValue::computeStops(GradientAdapter& gradie
         // calculate colors
         for (size_t y = 0; y < 9; ++y) {
             float relativeOffset = (newStops[y].offset - offset1) / (offset2 - offset1);
-            float multiplier = powf(relativeOffset, logf(.5f) / logf(midpoint));
-            newStops[y].color = interpolate(color1, color2, multiplier);
+            float multiplier = std::pow(relativeOffset, std::log(.5f) / std::log(midpoint));
+            // FIXME: Why not premultiply here?
+            newStops[y].color = blend(color1, color2, multiplier, false /* do not premultiply */);
         }
 
         stops.remove(x);
index 13b0a4e..7fd25b8 100644 (file)
@@ -1800,8 +1800,7 @@ void StyleResolver::setFontSize(FontCascadeDescription& fontDescription, float s
 
 bool StyleResolver::colorFromPrimitiveValueIsDerivedFromElement(const CSSPrimitiveValue& value)
 {
-    int ident = value.valueID();
-    switch (ident) {
+    switch (value.valueID()) {
     case CSSValueWebkitText:
     case CSSValueWebkitLink:
     case CSSValueWebkitActivelink:
@@ -1817,26 +1816,23 @@ Color StyleResolver::colorFromPrimitiveValue(const CSSPrimitiveValue& value, boo
     if (value.isRGBColor())
         return value.color();
 
-    const State& state = m_state;
-    CSSValueID ident = value.valueID();
-    switch (ident) {
-    case 0:
-        return Color();
+    auto identifier = value.valueID();
+    switch (identifier) {
     case CSSValueWebkitText:
         return document().textColor();
     case CSSValueWebkitLink:
-        return (state.element()->isLink() && forVisitedLink) ? document().visitedLinkColor() : document().linkColor();
+        return (m_state.element()->isLink() && forVisitedLink) ? document().visitedLinkColor() : document().linkColor();
     case CSSValueWebkitActivelink:
         return document().activeLinkColor();
     case CSSValueWebkitFocusRingColor:
         return RenderTheme::focusRingColor();
     case CSSValueCurrentcolor:
         // Color is an inherited property so depending on it effectively makes the property inherited.
-        state.style()->setHasExplicitlyInheritedProperties();
-        return state.style()->color();
-    default: {
-        return StyleColor::colorFromKeyword(ident);
-    }
+        // FIXME: Setting the flag as a side effect of calling this function is a bit oblique. Can we do better?
+        m_state.style()->setHasExplicitlyInheritedProperties();
+        return m_state.style()->color();
+    default:
+        return StyleColor::colorFromKeyword(identifier);
     }
 }
 
index 8e152e3..d4df13c 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
- * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -45,7 +45,6 @@
 #include "HTMLInputElement.h"
 #include "HTMLOptionElement.h"
 #include "InputTypeNames.h"
-#include "RenderObject.h"
 #include "RenderView.h"
 #include "ScopedEventQueue.h"
 #include "ShadowRoot.h"
@@ -55,32 +54,28 @@ namespace WebCore {
 
 using namespace HTMLNames;
 
-static bool isValidSimpleColorString(const String& value)
+// https://html.spec.whatwg.org/multipage/infrastructure.html#valid-simple-colour
+static bool isValidSimpleColor(StringView string)
 {
-    // See https://html.spec.whatwg.org/multipage/infrastructure.html#valid-simple-colour
-
-    if (value.isEmpty())
-        return false;
-    if (value[0] != '#')
+    if (string.length() != 7)
         return false;
-    if (value.length() != 7)
+    if (string[0] != '#')
         return false;
-    if (value.is8Bit()) {
-        const LChar* characters = value.characters8();
-        for (unsigned i = 1, length = value.length(); i < length; ++i) {
-            if (!isASCIIHexDigit(characters[i]))
-                return false;
-        }
-    } else {
-        const UChar* characters = value.characters16();
-        for (unsigned i = 1, length = value.length(); i < length; ++i) {
-            if (!isASCIIHexDigit(characters[i]))
-                return false;
-        }
+    for (unsigned i = 1; i < 7; ++i) {
+        if (!isASCIIHexDigit(string[i]))
+            return false;
     }
     return true;
 }
 
+// https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-simple-colour-values
+static std::optional<RGBA32> parseSimpleColorValue(StringView string)
+{
+    if (!isValidSimpleColor(string))
+        return std::nullopt;
+    return makeRGB(toASCIIHexValue(string[1], string[2]), toASCIIHexValue(string[3], string[4]), toASCIIHexValue(string[5], string[6]));
+}
+
 ColorInputType::~ColorInputType()
 {
     endColorChooser();
@@ -108,7 +103,7 @@ String ColorInputType::fallbackValue() const
 
 String ColorInputType::sanitizeValue(const String& proposedValue) const
 {
-    if (!isValidSimpleColorString(proposedValue))
+    if (!isValidSimpleColor(proposedValue))
         return fallbackValue();
 
     return proposedValue.convertToASCIILowercase();
@@ -116,7 +111,7 @@ String ColorInputType::sanitizeValue(const String& proposedValue) const
 
 Color ColorInputType::valueAsColor() const
 {
-    return Color(element().value());
+    return parseSimpleColorValue(element().value()).value();
 }
 
 void ColorInputType::createShadowSubtree()
@@ -130,7 +125,7 @@ void ColorInputType::createShadowSubtree()
     colorSwatch->setPseudo(AtomicString("-webkit-color-swatch", AtomicString::ConstructFromLiteral));
     wrapperElement->appendChild(colorSwatch);
     element().userAgentShadowRoot()->appendChild(wrapperElement);
-    
+
     updateColorSwatch();
 }
 
@@ -176,7 +171,7 @@ bool ColorInputType::shouldRespectListAttribute()
 
 bool ColorInputType::typeMismatchFor(const String& value) const
 {
-    return !isValidSimpleColorString(value);
+    return !isValidSimpleColor(value);
 }
 
 bool ColorInputType::shouldResetOnDocumentActivation()
@@ -266,9 +261,10 @@ Vector<Color> ColorInputType::suggestions() const
     return suggestions;
 }
 
-void ColorInputType::selectColor(const Color& color)
+void ColorInputType::selectColor(StringView string)
 {
-    didChooseColor(color);
+    if (auto color = parseSimpleColorValue(string))
+        didChooseColor(color.value());
 }
 
 } // namespace WebCore
index 5c82d08..646d452 100644 (file)
@@ -45,26 +45,26 @@ public:
     virtual ~ColorInputType();
 
 private:
-    void didChooseColor(const Color&) override;
-    void didEndChooser() override;
-    IntRect elementRectRelativeToRootView() const override;
-    Color currentColor() override;
-    bool shouldShowSuggestions() const override;
-    Vector<Color> suggestions() const override;
-    bool isColorControl() const override;
-    const AtomicString& formControlType() const override;
-    bool supportsRequired() const override;
-    String fallbackValue() const override;
-    String sanitizeValue(const String&) const override;
-    void createShadowSubtree() override;
-    void setValue(const String&, bool valueChanged, TextFieldEventBehavior) override;
-    void handleDOMActivateEvent(Event&) override;
-    void detach() override;
-    bool shouldRespectListAttribute() override;
-    bool typeMismatchFor(const String&) const override;
-    bool shouldResetOnDocumentActivation() override;
-    Color valueAsColor() const override;
-    void selectColor(const Color&) override;
+    void didChooseColor(const Color&) final;
+    void didEndChooser() final;
+    IntRect elementRectRelativeToRootView() const final;
+    Color currentColor() final;
+    bool shouldShowSuggestions() const final;
+    Vector<Color> suggestions() const final;
+    bool isColorControl() const final;
+    const AtomicString& formControlType() const final;
+    bool supportsRequired() const final;
+    String fallbackValue() const final;
+    String sanitizeValue(const String&) const final;
+    void createShadowSubtree() final;
+    void setValue(const String&, bool valueChanged, TextFieldEventBehavior) final;
+    void handleDOMActivateEvent(Event&) final;
+    void detach() final;
+    bool shouldRespectListAttribute() final;
+    bool typeMismatchFor(const String&) const final;
+    bool shouldResetOnDocumentActivation() final;
+    Color valueAsColor() const final;
+    void selectColor(StringView) final;
 
     void endColorChooser();
     void updateColorSwatch();
index 3d86c7e..0bdccfb 100644 (file)
@@ -1605,12 +1605,13 @@ Color HTMLInputElement::valueAsColor() const
     return m_inputType->valueAsColor();
 }
 
-void HTMLInputElement::selectColor(const Color& color)
+void HTMLInputElement::selectColor(StringView color)
 {
     m_inputType->selectColor(color);
 }
 
 #if ENABLE(DATALIST_ELEMENT)
+
 RefPtr<HTMLElement> HTMLInputElement::list() const
 {
     return dataList();
@@ -1643,6 +1644,7 @@ void HTMLInputElement::listAttributeTargetChanged()
 {
     m_inputType->listAttributeTargetChanged();
 }
+
 #endif // ENABLE(DATALIST_ELEMENT)
 
 bool HTMLInputElement::isSteppable() const
index 4ec4946..815444b 100644 (file)
@@ -293,7 +293,7 @@ public:
     void cacheSelectionInResponseToSetValue(int caretOffset) { cacheSelection(caretOffset, caretOffset, SelectionHasNoDirection); }
 
     Color valueAsColor() const; // Returns transparent color if not type=color.
-    WEBCORE_EXPORT void selectColor(const Color&); // Does nothing if not type=color. Simulates user selection of color; intended for testing.
+    WEBCORE_EXPORT void selectColor(StringView); // Does nothing if not type=color. Simulates user selection of color; intended for testing.
 
     String defaultToolTip() const;
 
index 1203eda..1a6db49 100644 (file)
@@ -1142,7 +1142,7 @@ Color InputType::valueAsColor() const
     return Color::transparent;
 }
 
-void InputType::selectColor(const Color&)
+void InputType::selectColor(StringView)
 {
 }
 
index c739a8f..b685043 100644 (file)
@@ -272,7 +272,7 @@ public:
     virtual bool shouldAppearIndeterminate() const;
     virtual bool supportsSelectionAPI() const;
     virtual Color valueAsColor() const;
-    virtual void selectColor(const Color&);
+    virtual void selectColor(StringView);
 
     // Parses the specified string for the type, and return
     // the Decimal value for the parsing result if the parsing
index 7e4136f..12c662b 100644 (file)
@@ -1237,7 +1237,7 @@ String Internals::visiblePlaceholder(Element& element)
 
 void Internals::selectColorInColorChooser(HTMLInputElement& element, const String& colorValue)
 {
-    element.selectColor(Color(colorValue));
+    element.selectColor(colorValue);
 }
 
 ExceptionOr<Vector<String>> Internals::formControlStateOfPreviousHistoryItem()
@@ -1565,8 +1565,15 @@ ExceptionOr<void> Internals::setViewBaseBackgroundColor(const String& colorValue
     if (!document || !document->view())
         return Exception { InvalidAccessError };
 
-    document->view()->setBaseBackgroundColor(Color(colorValue));
-    return { };
+    if (colorValue == "transparent") {
+        document->view()->setBaseBackgroundColor(Color::transparent);
+        return { };
+    }
+    if (colorValue == "white") {
+        document->view()->setBaseBackgroundColor(Color::white);
+        return { };
+    }
+    return Exception { SyntaxError };
 }
 
 ExceptionOr<void> Internals::setPagination(const String& mode, int gap, int pageLength)