Attachment 340844 Details for Bug 179391 – Patch

[patch] Patch

bug-179391-20180521145725.patch (text/plain), 12.42 KB, created by Emilio Cobos Álvarez (:emilio) on 2018-05-21 05:57:27 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Emilio Cobos Álvarez (:emilio)

Created: 2018-05-21 05:57:27 PDT

Size: 12.42 KB

patch

obsolete

>Subversion Revision: 232013
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 4434c93435d9ae0fa4a8b8b89f16b1e90724690f..b16c57e192cb98429cf7d355fa9e8ae4834d35cb 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,8 +1,38 @@
>+2018-05-20  Emilio Cobos Ãlvarez  <emilio@crisal.io>
>+
>+        [cssom] Make CSSOM APIs always append to the declaration block.
>+        https://bugs.webkit.org/show_bug.cgi?id=179391
>+
>+        This aligns us with Firefox and Blink which have already implemented
>+        the change in [1] and [2] respectively.
>+
>+        See [3] and [4] for spec text and discussion, and [5] for the blik-dev
>+        intent to implement and ship thread for more context.
>+
>+        I found a couple bits that don't really make sense from
>+        addParsedProperty anymore, which will fix separately with separate
>+        tests.
>+
>+        [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1415330
>+        [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=782407
>+        [3]: https://drafts.csswg.org/cssom/#set-a-css-declaration,
>+        [4]: https://github.com/w3c/csswg-drafts/issues/1898
>+        [5]: https://groups.google.com/a/chromium.org/d/msg/blink-dev/lzBoa1EAQpI/strfpNczAwAJ
>+
>+        Tests: LayoutTests/imported/web-platform-tests/css/cssom/cssstyledeclaration-mutationrecord-001.html
>+               LayoutTests/imported/web-platform-tests/css/cssom/cssstyledeclaration-setter-order.html
>+
>+        * css/StyleProperties.cpp:
>+        (WebCore::MutableStyleProperties::setProperty):
>+        (WebCore::MutableStyleProperties::setCustomProperty):
>+        (WebCore::MutableStyleProperties::addParsedProperty):
>+        * css/StyleProperties.h:
>+
> 2018-05-19  Eric Carlson  <eric.carlson@apple.com>
> 
>         Improve NowPlaying "title"
>         https://bugs.webkit.org/show_bug.cgi?id=185680
>         <rdar://problem/40296700>
> 
>         Reviewed by Dean Jackson.
> 
>diff --git a/Source/WebCore/css/StyleProperties.cpp b/Source/WebCore/css/StyleProperties.cpp
>index 30dbeb42b6b5a67a06fa9c65e0fdfc6deccf9086..fd99625c0555df51b02bd7b326bc95e3ff1720d9 100644
>--- a/Source/WebCore/css/StyleProperties.cpp
>+++ b/Source/WebCore/css/StyleProperties.cpp
>@@ -758,17 +758,16 @@ bool MutableStyleProperties::setProperty(CSSPropertyID propertyID, const String&
>     // Setting the value to an empty string just removes the property in both IE and Gecko.
>     // Setting it to null seems to produce less consistent results, but we treat it just the same.
>     if (value.isEmpty())
>         return removeProperty(propertyID);
> 
>     parserContext.mode = cssParserMode();
> 
>     // When replacing an existing property value, this moves the property to the end of the list.
>-    // Firefox preserves the position, and MSIE moves the property to the beginning.
>     return CSSParser::parseValue(*this, propertyID, value, important, parserContext) == CSSParser::ParseResult::Changed;
> }
> 
> bool MutableStyleProperties::setProperty(CSSPropertyID propertyID, const String& value, bool important)
> {
>     CSSParserContext parserContext(cssParserMode());
>     return setProperty(propertyID, value, important, parserContext);
> }
>@@ -777,17 +776,16 @@ bool MutableStyleProperties::setCustomProperty(const String& propertyName, const
> {
>     // Setting the value to an empty string just removes the property in both IE and Gecko.
>     // Setting it to null seems to produce less consistent results, but we treat it just the same.
>     if (value.isEmpty())
>         return removeCustomProperty(propertyName);
> 
>     parserContext.mode = cssParserMode();
>     // When replacing an existing property value, this moves the property to the end of the list.
>-    // Firefox preserves the position, and MSIE moves the property to the beginning.
>     return CSSParser::parseCustomPropertyValue(*this, propertyName, value, important, parserContext) == CSSParser::ParseResult::Changed;
> }
> 
> void MutableStyleProperties::setProperty(CSSPropertyID propertyID, RefPtr<CSSValue>&& value, bool important)
> {
>     StylePropertyShorthand shorthand = shorthandForProperty(propertyID);
>     if (!shorthand.length()) {
>         setProperty(CSSProperty(propertyID, WTFMove(value), important));
>@@ -795,49 +793,30 @@ void MutableStyleProperties::setProperty(CSSPropertyID propertyID, RefPtr<CSSVal
>     }
> 
>     removePropertiesInSet(shorthand.properties(), shorthand.length());
> 
>     for (unsigned i = 0; i < shorthand.length(); ++i)
>         m_propertyVector.append(CSSProperty(shorthand.properties()[i], value.copyRef(), important));
> }
> 
>-bool MutableStyleProperties::setProperty(const CSSProperty& property, CSSProperty* slot)
>+void MutableStyleProperties::setProperty(const CSSProperty& property)
> {
>-    if (!removeShorthandProperty(property.id())) {
>-        CSSProperty* toReplace = slot;
>-        if (!slot) {
>-            if (property.id() == CSSPropertyCustom) {
>-                if (property.value())
>-                    toReplace = findCustomCSSPropertyWithName(downcast<CSSCustomPropertyValue>(*property.value()).name());
>-            } else
>-                toReplace = findCSSPropertyWithID(property.id());
>-        }
>-        
>-        if (toReplace) {
>-            if (*toReplace == property)
>-                return false;
>-
>-            *toReplace = property;
>-            return true;
>-        }
>-    }
>-
>+    removeProperty(property.id());
>     m_propertyVector.append(property);
>-    return true;
> }
> 
>-bool MutableStyleProperties::setProperty(CSSPropertyID propertyID, CSSValueID identifier, bool important)
>+void MutableStyleProperties::setProperty(CSSPropertyID propertyID, CSSValueID identifier, bool important)
> {
>-    return setProperty(CSSProperty(propertyID, CSSValuePool::singleton().createIdentifierValue(identifier), important));
>+    setProperty(CSSProperty(propertyID, CSSValuePool::singleton().createIdentifierValue(identifier), important));
> }
> 
>-bool MutableStyleProperties::setProperty(CSSPropertyID propertyID, CSSPropertyID identifier, bool important)
>+void MutableStyleProperties::setProperty(CSSPropertyID propertyID, CSSPropertyID identifier, bool important)
> {
>-    return setProperty(CSSProperty(propertyID, CSSValuePool::singleton().createIdentifierValue(identifier), important));
>+    setProperty(CSSProperty(propertyID, CSSValuePool::singleton().createIdentifierValue(identifier), important));
> }
> 
> bool MutableStyleProperties::parseDeclaration(const String& styleDeclaration, CSSParserContext context)
> {
>     auto oldProperties = WTFMove(m_propertyVector);
>     m_propertyVector.clear();
> 
>     context.mode = cssParserMode();
>@@ -858,22 +837,29 @@ bool MutableStyleProperties::addParsedProperties(const ParsedPropertyVector& pro
>             anyChanged = true;
>     }
> 
>     return anyChanged;
> }
> 
> bool MutableStyleProperties::addParsedProperty(const CSSProperty& property)
> {
>+    // FIXME: This stopped looking at !important for non-custom properties in
>+    // https://webkit.org/b/159959, which is what some callers, mainly
>+    // mergeAndOverrideOnConflict, assume this function does.
>     if (property.id() == CSSPropertyCustom) {
>-        if ((property.value() && !customPropertyIsImportant(downcast<CSSCustomPropertyValue>(*property.value()).name())) || property.isImportant())
>-            return setProperty(property);
>-        return false;
>+        if (!property.value())
>+            return false;
>+        if (!property.isImportant() && customPropertyIsImportant(downcast<CSSCustomPropertyValue>(*property.value()).name()))
>+            return false;
>+        setProperty(property);
>+        return true;
>     }
>-    return setProperty(property);
>+    setProperty(property);
>+    return true;
> }
> 
> String StyleProperties::asText() const
> {
>     StringBuilder result;
> 
>     int positionXPropertyIndex = -1;
>     int positionYPropertyIndex = -1;
>diff --git a/Source/WebCore/css/StyleProperties.h b/Source/WebCore/css/StyleProperties.h
>index 2661ec1121108a635065dc6a250145d6d5649a39..b6ade6e90a3c56ea3f59fc59c0924bf4c0e1f63b 100644
>--- a/Source/WebCore/css/StyleProperties.h
>+++ b/Source/WebCore/css/StyleProperties.h
>@@ -223,19 +223,19 @@ public:
>     bool addParsedProperty(const CSSProperty&);
> 
>     // These expand shorthand properties into multiple properties.
>     bool setProperty(CSSPropertyID, const String& value, bool important, CSSParserContext);
>     bool setProperty(CSSPropertyID, const String& value, bool important = false);
>     void setProperty(CSSPropertyID, RefPtr<CSSValue>&&, bool important = false);
> 
>     // These do not. FIXME: This is too messy, we can do better.
>-    bool setProperty(CSSPropertyID, CSSValueID identifier, bool important = false);
>-    bool setProperty(CSSPropertyID, CSSPropertyID identifier, bool important = false);
>-    bool setProperty(const CSSProperty&, CSSProperty* slot = nullptr);
>+    void setProperty(CSSPropertyID, CSSValueID identifier, bool important = false);
>+    void setProperty(CSSPropertyID, CSSPropertyID identifier, bool important = false);
>+    void setProperty(const CSSProperty&);
> 
>     bool removeProperty(CSSPropertyID, String* returnText = nullptr);
>     void removeBlockProperties();
>     bool removePropertiesInSet(const CSSPropertyID* set, unsigned length);
> 
>     void mergeAndOverrideOnConflict(const StyleProperties&);
> 
>     void clear();
>diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
>index d26a4a08b6064aca6719db3584802ca154a0d224..32ad9512b994ca32fb2c1500db1d4f577c0e8c09 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,8 +1,20 @@
>+2018-05-20  Emilio Cobos Ãlvarez  <emilio@crisal.io>
>+
>+        [cssom] Make CSSOM APIs always append to the declaration block.
>+        https://bugs.webkit.org/show_bug.cgi?id=179391
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Rebaselined tests to account for the progression.
>+
>+        * web-platform-tests/css/cssom/cssstyledeclaration-mutationrecord-001-expected.txt:
>+        * web-platform-tests/css/cssom/cssstyledeclaration-setter-order-expected.txt:
>+
> 2018-05-20  Emilio Cobos Ãlvarez  <emilio@crisal.io>
> 
>         Update CSSOM WPT tests.
>         https://bugs.webkit.org/show_bug.cgi?id=185805
> 
>         They've been moved under the css/ directory.
> 
>         This has been done with:
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-mutationrecord-001-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-mutationrecord-001-expected.txt
>index f327d4ac80b2a11d629765380382bde5c1abcff9..5eacaa76ef82ba261054e2468167cac239484745 100644
>--- a/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-mutationrecord-001-expected.txt
>+++ b/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-mutationrecord-001-expected.txt
>@@ -1,5 +1,3 @@
> 
>-Harness Error (TIMEOUT), message = null
>-
>-NOTRUN CSSStyleDeclaration.setPropertyValue queues a mutation record, even if not mutated 
>+PASS CSSStyleDeclaration.setPropertyValue queues a mutation record, even if not mutated 
> 
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-order-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-order-expected.txt
>index eb066fd1415408933d41e30424b68b794200b3c0..057696adbf58c102e8bb93b120c6ec6dc692137b 100644
>--- a/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-order-expected.txt
>+++ b/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-order-expected.txt
>@@ -1,6 +1,6 @@
> 
>-FAIL setProperty with existing longhand should change order assert_array_equals: Property order should match after setting existing property property 0, expected "bottom" but got "top"
>-FAIL invoke property setter with existing longhand should change order assert_array_equals: Property order should match after setting existing property property 0, expected "bottom" but got "top"
>-FAIL setProperty with existing shorthand should change order assert_array_equals: Property order should match after setting an existing shorthand property 0, expected "top" but got "margin-top"
>-FAIL invoke property setter with existing shorthand should change order assert_array_equals: Property order should match after setting an existing shorthand property 0, expected "top" but got "margin-top"
>+PASS setProperty with existing longhand should change order 
>+PASS invoke property setter with existing longhand should change order 
>+PASS setProperty with existing shorthand should change order 
>+PASS invoke property setter with existing shorthand should change order 
>

Actions: View | Formatted Diff | Diff

Attachments on bug 179391: 340796 | 340797 | 340801 | 340844 | 340852 | 340853 | 340854 | 340856 | 340859 | 340866 | 340961 | 340965 | 340967 | 340971 | 340972