Attachment 370902 Details for Bug 195264 – Patch

[patch] Patch

patch.txt (text/plain), 21.05 KB, created by Nikita Vasilyev on 2019-05-29 17:22:10 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Nikita Vasilyev

Created: 2019-05-29 17:22:10 PDT

Size: 21.05 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 60a6b0287a3..cbc05e3ca0e 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-05-29  Nikita Vasilyev  <nvasilyev@apple.com>
>+
>+        Web Inspector: CSS Changes: modifications aren't shared for rules that match multiple elements
>+        https://bugs.webkit.org/show_bug.cgi?id=195264
>+        <rdar://problem/48550023>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Test arrays with repeating items for Array.diffArrays.
>+
>+        * inspector/unit-tests/array-utilities-expected.txt:
>+        * inspector/unit-tests/array-utilities.html:
>+
> 2019-05-17  Joonghun Park  <pjh0718@gmail.com>
> 
>         Implement CSS `display: flow-root` (modern clearfix)
>diff --git a/LayoutTests/inspector/unit-tests/array-utilities-expected.txt b/LayoutTests/inspector/unit-tests/array-utilities-expected.txt
>index f52c69c9544..9cc6c96a99f 100644
>--- a/LayoutTests/inspector/unit-tests/array-utilities-expected.txt
>+++ b/LayoutTests/inspector/unit-tests/array-utilities-expected.txt
>@@ -81,8 +81,18 @@ PASS: shallowEqual of non-arrays should be false.
> ["b","a"], ["a"] => [["b",-1],["a",0]]
> ["b","a"], ["a","c"] => [["b",-1],["a",0],["c",1]]
> ["b","a"], ["a","c"] => [["b",-1],["a",0],["c",1]]
>-["b","a"], ["a","b"] => [["a",0],["b",0]]
>+["b","a"], ["a","b"] => [["a",1],["b",0],["a",-1]]
> ["a","b","c"], ["a","d","c"] => [["a",0],["b",-1],["d",1],["c",0]]
>+["a","b","c"], ["c","b","a"] => [["c",1],["b",1],["a",0],["b",-1],["c",-1]]
>+
>+Repeating items:
>+["a"], ["a","a"] => [["a",0],["a",1]]
>+["a","a"], ["a"] => [["a",0],["a",-1]]
>+["a","a"], ["a","a"] => [["a",0],["a",0]]
>+["b","a","b"], ["a","b","a"] => [["a",1],["b",0],["a",0],["b",-1]]
>+["a","b","b","c"], ["c","b","b","b","a"] => [["a",-1],["c",1],["b",0],["b",0],["c",-1],["b",1],["a",1]]
>+["a","b","b","b","c"], ["c","b","b","a"] => [["a",-1],["c",1],["b",0],["b",0],["b",-1],["c",-1],["a",1]]
>+["a","a","b","b","c","c"], ["b","b","c","c","a","a"] => [["a",-1],["a",-1],["b",0],["b",0],["c",0],["c",0],["a",1],["a",1]]
> 
> -- Running test case: Array.prototype.lastValue
> PASS: lastValue of a nonempty array should be the last value.
>diff --git a/LayoutTests/inspector/unit-tests/array-utilities.html b/LayoutTests/inspector/unit-tests/array-utilities.html
>index d15afecee7d..c3282a778c0 100644
>--- a/LayoutTests/inspector/unit-tests/array-utilities.html
>+++ b/LayoutTests/inspector/unit-tests/array-utilities.html
>@@ -175,6 +175,16 @@ function test()
>             diff(["b", "a"], ["a", "c"]);
>             diff(["b", "a"], ["a", "b"]);
>             diff(["a", "b", "c"], ["a", "d", "c"]);
>+            diff(["a", "b", "c"], ["c", "b", "a"]);
>+
>+            InspectorTest.log("\nRepeating items:");
>+            diff(["a"], ["a", "a"]);
>+            diff(["a", "a"], ["a"]);
>+            diff(["a", "a"], ["a", "a"]);
>+            diff(["b", "a", "b"], ["a", "b", "a"]);
>+            diff(["a", "b", "b", "c"], ["c", "b", "b", "b", "a"]);
>+            diff(["a", "b", "b", "b", "c"], ["c", "b", "b", "a"]);
>+            diff(["a", "a", "b", "b", "c", "c"], ["b", "b", "c", "c", "a", "a"]);
> 
>             return true;
>         }
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index d76efb6430c..caeedad7bab 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,47 @@
>+2019-05-29  Nikita Vasilyev  <nvasilyev@apple.com>
>+
>+        Web Inspector: CSS Changes: modifications aren't shared for rules that match multiple elements
>+        https://bugs.webkit.org/show_bug.cgi?id=195264
>+        <rdar://problem/48550023>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch fixes several cases when the diff was incorrect.
>+
>+        1. Perform diff based on CSSProperty content (name, value, and enabled property) instead
>+           of strict equality of CSSProperty instances.
>+
>+        2. Copy all initial CSSProperty instances of CSSStyleDeclaration on 1st edit.
>+           This removes the need to update `properties` on every single edit.
>+
>+        3. Do full diff to display modified property markers (green background) in Rules panel.
>+           This fixes a few cases when the markers were inaccurate. E.g. a newly added property
>+           matches removed property - no need to show the green background.
>+
>+        * UserInterface/Base/Utilities.js:
>+        (Array.diffArrays):
>+        Allow repeating items in the arrays.
>+
>+        * UserInterface/Controllers/CSSManager.js:
>+        (WI.CSSManager.prototype.getModifiedStyle):
>+        (WI.CSSManager.prototype.removeModifiedStyle):
>+        * UserInterface/Models/CSSProperty.js:
>+        (WI.CSSProperty):
>+        (WI.CSSProperty.prototype.get modified):
>+        (WI.CSSProperty.prototype.set modified):
>+        (WI.CSSProperty.prototype.equals):
>+        (WI.CSSProperty.prototype.clone):
>+        (WI.CSSProperty.prototype._updateOwnerStyleText):
>+        (WI.CSSProperty.prototype._markModified):
>+        * UserInterface/Models/CSSStyleDeclaration.js:
>+        (WI.CSSStyleDeclaration.prototype.markModified):
>+        (WI.CSSStyleDeclaration.prototype.updatePropertiesModifiedState):
>+        * UserInterface/Views/ChangesDetailsSidebarPanel.js:
>+        (WI.ChangesDetailsSidebarPanel.prototype._createRuleElement):
>+        * UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:
>+        (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.layout):
>+        * UserInterface/Views/SpreadsheetStyleProperty.js:
>+
> 2019-05-17  Devin Rousso  <drousso@apple.com>
> 
>         Web Inspector: Timelines: CPU/memory timeline bars sometimes don't draw correctly and jump around on scrolling
>diff --git a/Source/WebInspectorUI/UserInterface/Base/Utilities.js b/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>index 2bd4fad60cf..e4fa9e52c8d 100644
>--- a/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>+++ b/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>@@ -509,62 +509,83 @@ Object.defineProperty(Array, "shallowEqual",
> 
> Object.defineProperty(Array, "diffArrays",
> {
>-    value(initialArray, currentArray, onEach)
>+    value(initialArray, currentArray, onEach, comparator)
>     {
>-        let initialSet = new Set(initialArray);
>-        let currentSet = new Set(currentArray);
>-        let indexInitial = 0;
>-        let indexCurrent = 0;
>-        let deltaInitial = 0;
>-        let deltaCurrent = 0;
>+        "use strict";
> 
>-        let i = 0;
>-        while (true) {
>-            if (indexInitial >= initialArray.length || indexCurrent >= currentArray.length)
>-                break;
>+        function defaultComparator(initial, current) {
>+            return initial === current;
>+        }
>+        comparator = comparator || defaultComparator;
> 
>-            let initial = initialArray[indexInitial];
>-            let current = currentArray[indexCurrent];
>-
>-            if (initial === current)
>-                onEach(current, 0);
>-            else if (currentSet.has(initial)) {
>-                if (initialSet.has(current)) {
>-                    // Moved.
>-                    onEach(current, 0);
>-                } else {
>-                    // Added.
>-                    onEach(current, 1);
>-                    --i;
>-                    ++deltaCurrent;
>-                }
>-            } else {
>-                // Removed.
>-                onEach(initial, -1);
>-                if (!initialSet.has(current)) {
>-                    // Added.
>-                    onEach(current, 1);
>-                } else {
>-                    --i;
>-                    ++deltaInitial;
>+        // Find the shortest prefix of matching items in both arrays.
>+        //
>+        //    initialArray = ["a", "b", "b", "c"]
>+        //    currentArray = ["c", "b", "b", "a"]
>+        //    findShortestEdit() // [1, 1]
>+        //
>+        function findShortestEdit() {
>+            let deletionCount = initialArray.length;
>+            let additionCount = currentArray.length;
>+            let editCount = deletionCount + additionCount;
>+            for (let i = 0; i < initialArray.length; ++i) {
>+                if (i > editCount)
>+                    break;
>+                for (let j = 0; j < currentArray.length; ++j) {
>+                    let newEditCount = i + j;
>+                    if (newEditCount > editCount) {
>+                        // Break since any possible edits at this point are going to be longer than the one already found.
>+                        break;
>+                    }
>+
>+                    if (comparator(initialArray[i], currentArray[j])) {
>+                        // A candidate for the shortest edit found.
>+                        if (newEditCount < editCount) {
>+                            editCount = newEditCount;
>+                            deletionCount = i;
>+                            additionCount = j;
>+                        }
>+                        break;
>+                    }
>                 }
>             }
>-
>-            ++i;
>-            indexInitial = i + deltaInitial;
>-            indexCurrent = i + deltaCurrent;
>+            return [deletionCount, additionCount];
>         }
> 
>-        for (let i = indexInitial; i < initialArray.length; ++i) {
>-            // Removed.
>-            onEach(initialArray[i], -1);
>+        function commonPrefixLength(listA, listB) {
>+            let shorterListLength = Math.min(listA.length, listB.length);
>+            let i = 0;
>+            while (i < shorterListLength) {
>+                if (!comparator(listA[i], listB[i]))
>+                    break;
>+                ++i;
>+            }
>+            return i;
>         }
> 
>-        for (let i = indexCurrent; i < currentArray.length; ++i) {
>-            // Added.
>-            onEach(currentArray[i], 1);
>+        function fireOnEach(count, diffAction, array) {
>+            for (let i = 0; i < count; ++i)
>+                onEach(array[i], diffAction);
>         }
> 
>+        while (initialArray.length || currentArray.length) {
>+            // Remove common prefix.
>+            let prefixLength = commonPrefixLength(initialArray, currentArray);
>+            if (prefixLength) {
>+                fireOnEach(prefixLength, 0, initialArray);
>+                initialArray = initialArray.slice(prefixLength);
>+                currentArray = currentArray.slice(prefixLength);
>+            }
>+
>+            if (!initialArray.length && !currentArray.length)
>+                break;
>+
>+            let [deletionCount, additionCount] = findShortestEdit();
>+            fireOnEach(deletionCount, -1, initialArray);
>+            fireOnEach(additionCount, 1, currentArray);
>+            initialArray = initialArray.slice(deletionCount);
>+            currentArray = currentArray.slice(additionCount);
>+        }
>     }
> });
> 
>diff --git a/Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js b/Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js
>index a622d0e5296..a2fbe91eac2 100644
>--- a/Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js
>+++ b/Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js
>@@ -431,6 +431,16 @@ WI.CSSManager = class CSSManager extends WI.Object
>         this._modifiedStyles.set(style.stringId, style);
>     }
> 
>+    getModifiedStyle(style)
>+    {
>+        return this._modifiedStyles.get(style.stringId);
>+    }
>+
>+    removeModifiedStyle(style)
>+    {
>+        this._modifiedStyles.delete(style.stringId);
>+    }
>+
>     // Protected
> 
>     mediaQueryResultChanged()
>diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>index 2c10895cc78..82082cfc153 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>@@ -33,6 +33,7 @@ WI.CSSProperty = class CSSProperty extends WI.Object
>         this._index = index;
>         this._overridingProperty = null;
>         this._initialState = null;
>+        this._modified = false;
> 
>         this.update(text, name, value, priority, enabled, overridden, implicit, anonymous, valid, styleSheetTextRange, true);
>     }
>@@ -184,7 +185,16 @@ WI.CSSProperty = class CSSProperty extends WI.Object
> 
>     get modified()
>     {
>-        return !!this._initialState;
>+        return this._modified;
>+    }
>+
>+    set modified(value)
>+    {
>+        if (this._modified === value)
>+            return;
>+
>+        this._modified = value;
>+        this.dispatchEventToListeners(WI.CSSProperty.Event.ModifiedChanged);
>     }
> 
>     get name()
>@@ -377,6 +387,37 @@ WI.CSSProperty = class CSSProperty extends WI.Object
>         return this._hasOtherVendorNameOrKeyword;
>     }
> 
>+    equals(property)
>+    {
>+        if (property === this)
>+            return true;
>+
>+        if (!property)
>+            return false;
>+
>+        return this._name === property.name && this._rawValue === property.rawValue && this._enabled === property.enabled;
>+    }
>+
>+    clone()
>+    {
>+        let cssProperty = new WI.CSSProperty(
>+            this._index,
>+            this._text,
>+            this._name,
>+            this._rawValue,
>+            this._priority,
>+            this._enabled,
>+            this._overridden,
>+            this._implicit,
>+            this._anonymous,
>+            this._valid,
>+            this._styleSheetTextRange);
>+
>+        cssProperty.ownerStyle = this._ownerStyle;
>+
>+        return cssProperty;
>+    }
>+
>     // Private
> 
>     _updateStyleText(forceRemove = false)
>@@ -393,8 +434,6 @@ WI.CSSProperty = class CSSProperty extends WI.Object
> 
>     _updateOwnerStyleText(oldText, newText, forceRemove = false)
>     {
>-        console.assert(this.modified, "CSSProperty was modified without saving initial state.");
>-
>         if (oldText === newText) {
>             if (forceRemove) {
>                 const lineDelta = 0;
>@@ -437,6 +476,7 @@ WI.CSSProperty = class CSSProperty extends WI.Object
> 
>         let propertyWasRemoved = !newText;
>         this._ownerStyle.shiftPropertiesAfter(this, lineDelta, columnDelta, propertyWasRemoved);
>+        this._ownerStyle.updatePropertiesModifiedState();
>     }
> 
>     _prependSemicolonIfNeeded()
>@@ -456,30 +496,13 @@ WI.CSSProperty = class CSSProperty extends WI.Object
> 
>     _markModified()
>     {
>-        if (this.modified)
>-            return;
>-
>-        this._initialState = new WI.CSSProperty(
>-            this._index,
>-            this._text,
>-            this._name,
>-            this._rawValue,
>-            this._priority,
>-            this._enabled,
>-            this._overridden,
>-            this._implicit,
>-            this._anonymous,
>-            this._valid,
>-            this._styleSheetTextRange);
>-
>-        if (this._ownerStyle) {
>+        if (this._ownerStyle)
>             this._ownerStyle.markModified();
>-            this._initialState.ownerStyle = this._ownerStyle.initialState;
>-        }
>     }
> };
> 
> WI.CSSProperty.Event = {
>     Changed: "css-property-changed",
>+    ModifiedChanged: "css-property-modified-changed",
>     OverriddenStatusChanged: "css-property-overridden-status-changed"
> };
>diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>index 74b694f2212..e03160439f0 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>@@ -365,9 +365,11 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
> 
>     markModified()
>     {
>-        let properties = this._initialState ? this._initialState.properties : this._properties;
>-
>         if (!this._initialState) {
>+            let visibleProperties = this.visibleProperties.map((property) => {
>+                return property.clone();
>+            });
>+
>             this._initialState = new WI.CSSStyleDeclaration(
>                 this._nodeStyles,
>                 this._ownerStyleSheet,
>@@ -376,12 +378,10 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
>                 this._node,
>                 this._inherited,
>                 this._text,
>-                [], // Passing CSS properties here would change their ownerStyle.
>+                visibleProperties,
>                 this._styleSheetTextRange);
>         }
> 
>-        this._initialState.properties = properties.map((property) => { return property.initialState || property });
>-
>         WI.cssManager.addModifiedStyle(this);
>     }
> 
>@@ -417,6 +417,36 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
>         this._visibleProperties = null;
>     }
> 
>+    updatePropertiesModifiedState()
>+    {
>+        if (!this._initialState)
>+            return;
>+
>+        if (this._type === WI.CSSStyleDeclaration.Type.Computed)
>+            return;
>+
>+        let initialCSSProperties = this._initialState.visibleProperties;
>+        let cssProperties = this.visibleProperties;
>+
>+        let hasModified = false;
>+
>+        function onEach(cssProperty, action) {
>+            if (action !== 0)
>+                hasModified = true;
>+
>+            cssProperty.modified = action === 1;
>+        }
>+
>+        function comparator(a, b) {
>+            return a.equals(b);
>+        }
>+
>+        Array.diffArrays(initialCSSProperties, cssProperties, onEach, comparator);
>+
>+        if (!hasModified)
>+            WI.cssManager.removeModifiedStyle(this);
>+    }
>+
>     // Protected
> 
>     get nodeStyles()
>diff --git a/Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js b/Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js
>index 8fef7aff5a6..7759a0b8c8d 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js
>@@ -149,28 +149,31 @@ WI.ChangesDetailsSidebarPanel = class ChangesDetailsSidebarPanel extends WI.DOMD
> 
>         selectorLineElement.append(" {\n");
> 
>-        let appendProperty = (cssProperty, className) => {
>+        let initialCSSProperties = style.initialState.visibleProperties;
>+        let cssProperties = style.visibleProperties;
>+
>+        function onEach(cssProperty, action) {
>+            let className = "";
>+            if (action === 1)
>+                className = "added";
>+            else if (action === -1)
>+                className = "removed";
>+            else
>+                className = "unchanged";
>+
>             let propertyLineElement = ruleElement.appendChild(document.createElement("div"));
>             propertyLineElement.classList.add("css-property-line", className);
>-            let stylePropertyView = new WI.SpreadsheetStyleProperty(null, cssProperty, {readOnly: true});
>+
>+            const delegate = null;
>+            let stylePropertyView = new WI.SpreadsheetStyleProperty(delegate, cssProperty, {readOnly: true});
>             propertyLineElement.append(WI.indentString(), stylePropertyView.element, "\n");
>-        };
>+        }
> 
>-        let initialCSSProperties = style.initialState.visibleProperties;
>-        let cssProperties = style.visibleProperties;
>+        function comparator(a, b) {
>+            return a.equals(b);
>+        }
> 
>-        Array.diffArrays(initialCSSProperties, cssProperties, (cssProperty, action) => {
>-            if (action === 0) {
>-                if (cssProperty.modified) {
>-                    appendProperty(cssProperty.initialState, "removed");
>-                    appendProperty(cssProperty, "added");
>-                } else
>-                    appendProperty(cssProperty, "unchanged");
>-            } else if (action === 1)
>-                appendProperty(cssProperty, "added");
>-            else if (action === -1)
>-                appendProperty(cssProperty, "removed");
>-        });
>+        Array.diffArrays(initialCSSProperties, cssProperties, onEach, comparator);
> 
>         let closeBraceElement = document.createElement("span");
>         closeBraceElement.className = "close-brace";
>diff --git a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js
>index 4361f8892bf..e82da88c485 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js
>@@ -82,6 +82,9 @@ WI.SpreadsheetCSSStyleDeclarationEditor = class SpreadsheetCSSStyleDeclarationEd
> 
>         this.element.removeChildren();
> 
>+        if (this._style)
>+            this._style.updatePropertiesModifiedState();
>+
>         let properties = this.propertiesToRender;
>         this.element.classList.toggle("no-properties", !properties.length);
> 
>diff --git a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>index 93d4d16b295..6cbc629b264 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>@@ -53,6 +53,7 @@ WI.SpreadsheetStyleProperty = class SpreadsheetStyleProperty extends WI.Object
> 
>         if (!this._readOnly) {
>             this._element.tabIndex = -1;
>+            property.addEventListener(WI.CSSProperty.Event.ModifiedChanged, this.updateStatus, this);
> 
>             this._element.addEventListener("blur", (event) => {
>                 // Keep selection after tabbing out of Web Inspector window and back.

Flags:

hi: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 195264: 366548 | 366552 | 366558 | 366561 | 366564 | 366568 | 366569 | 366672 | 366678 | 366683 | 368625 | 369076 | 369077 | 369201 | 369999 | 370198 | 370376 | 370902 | 371081