Attachment 369076 Details for Bug 195264 – Patch

[patch] Patch

patch.txt (text/plain), 18.45 KB, created by Nikita Vasilyev on 2019-05-04 17:01:03 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Nikita Vasilyev

Created: 2019-05-04 17:01:03 PDT

Size: 18.45 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 24b7d8c6f58..1e15a4d1c63 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-05-04  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-04-03  Myles C. Maxfield  <mmaxfield@apple.com>
> 
>         -apple-trailing-word is needed for browser detection
>diff --git a/LayoutTests/inspector/unit-tests/array-utilities-expected.txt b/LayoutTests/inspector/unit-tests/array-utilities-expected.txt
>index f52c69c9544..88e6b4eb1f4 100644
>--- a/LayoutTests/inspector/unit-tests/array-utilities-expected.txt
>+++ b/LayoutTests/inspector/unit-tests/array-utilities-expected.txt
>@@ -83,6 +83,16 @@ PASS: shallowEqual of non-arrays should be false.
> ["b","a"], ["a","c"] => [["b",-1],["a",0],["c",1]]
> ["b","a"], ["a","b"] => [["a",0],["b",0]]
> ["a","b","c"], ["a","d","c"] => [["a",0],["b",-1],["d",1],["c",0]]
>+["a","b","c"], ["c","b","a"] => [["c",0],["b",0],["a",0]]
>+
>+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",0],["b",0],["b",-1],["a",1]]
>+["a","b","b","c"], ["c","b","b","b","a"] => [["c",0],["b",0],["b",0],["b",1],["c",-1],["a",1]]
>+["a","b","b","b","c"], ["c","b","b","a"] => [["c",0],["b",0],["b",0],["b",-1],["a",0]]
>+["a","a","b","b","c","c"], ["b","b","c","c","a","a"] => [["b",0],["b",0],["c",0],["c",0],["a",0],["a",0]]
> 
> -- 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 6809f8dab28..dd7349c8dba 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,47 @@
>+2019-05-04  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.hash):
>+        (WI.CSSProperty.prototype.get modified):
>+        (WI.CSSProperty.prototype.set modified):
>+        (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-04-03  Devin Rousso  <drousso@apple.com>
> 
>         Web Inspector: Single click on links in non-read-only TextEditors should not follow links
>diff --git a/Source/WebInspectorUI/UserInterface/Base/Utilities.js b/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>index 174faabfad3..0dbe239462d 100644
>--- a/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>+++ b/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>@@ -501,15 +501,32 @@ Object.defineProperty(Array, "shallowEqual",
> 
> Object.defineProperty(Array, "diffArrays",
> {
>-    value(initialArray, currentArray, onEach)
>-    {
>-        let initialSet = new Set(initialArray);
>-        let currentSet = new Set(currentArray);
>+    value(initialArray, currentArray, onEach, hashFunction = (x) => x)
>+    {
>+        function createUsageCountMap(array, hashFunction) {
>+            let map = new Map();
>+            for (let item of array) {
>+                let hash = hashFunction(item);
>+                let count = map.get(hash) || 0;
>+                map.set(hash, count + 1);
>+            }
>+            return map;
>+        }
>+
>+        function decrementUsageCount(map, key) {
>+            let count = map.get(key);
>+            if (count)
>+                map.set(key, count - 1);
>+        }
>+
>+        let initialMap = createUsageCountMap(initialArray, hashFunction);
>+        let currentMap = createUsageCountMap(currentArray, hashFunction);
>         let indexInitial = 0;
>         let indexCurrent = 0;
>         let deltaInitial = 0;
>         let deltaCurrent = 0;
> 
>+
>         let i = 0;
>         while (true) {
>             if (indexInitial >= initialArray.length || indexCurrent >= currentArray.length)
>@@ -517,11 +534,17 @@ Object.defineProperty(Array, "diffArrays",
> 
>             let initial = initialArray[indexInitial];
>             let current = currentArray[indexCurrent];
>+            let initialHash = hashFunction(initial);
>+            let currentHash = hashFunction(current);
> 
>-            if (initial === current)
>+            if (initialHash === currentHash) {
>                 onEach(current, 0);
>-            else if (currentSet.has(initial)) {
>-                if (initialSet.has(current)) {
>+                decrementUsageCount(initialMap, initialHash);
>+                decrementUsageCount(currentMap, currentHash);
>+            } else if (currentMap.get(initialHash) > 0) {
>+                decrementUsageCount(currentMap, initialHash);
>+                if (initialMap.get(currentHash) > 0) {
>+                    decrementUsageCount(initialMap, currentHash);
>                     // Moved.
>                     onEach(current, 0);
>                 } else {
>@@ -533,12 +556,12 @@ Object.defineProperty(Array, "diffArrays",
>             } else {
>                 // Removed.
>                 onEach(initial, -1);
>-                if (!initialSet.has(current)) {
>-                    // Added.
>-                    onEach(current, 1);
>-                } else {
>+                if (initialMap.get(currentHash) > 0) {
>                     --i;
>                     ++deltaInitial;
>+                } else {
>+                    // Added.
>+                    onEach(current, 1);
>                 }
>             }
> 
>@@ -556,7 +579,6 @@ Object.defineProperty(Array, "diffArrays",
>             // Added.
>             onEach(currentArray[i], 1);
>         }
>-
>     }
> });
> 
>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..1f6ace2fb03 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);
>     }
>@@ -48,6 +49,11 @@ WI.CSSProperty = class CSSProperty extends WI.Object
>         return name.startsWith("--");
>     }
> 
>+    static hash(property)
>+    {
>+        return `${property.enabled ? "" : "*"}${property.name}:${property.rawValue}`;
>+    }
>+
>     // Public
> 
>     get ownerStyle()
>@@ -184,7 +190,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 +392,26 @@ WI.CSSProperty = class CSSProperty extends WI.Object
>         return this._hasOtherVendorNameOrKeyword;
>     }
> 
>+    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)
>@@ -437,6 +472,7 @@ WI.CSSProperty = class CSSProperty extends WI.Object
> 
>         let propertyWasRemoved = !newText;
>         this._ownerStyle.shiftPropertiesAfter(this, lineDelta, columnDelta, propertyWasRemoved);
>+        this._ownerStyle.updatePropertiesModifiedState();
>     }
> 
>     _prependSemicolonIfNeeded()
>@@ -456,30 +492,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..7cd86c89bc7 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,29 @@ 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;
>+        Array.diffArrays(initialCSSProperties, cssProperties, (property, action) => {
>+            if (action !== 0)
>+                hasModified = true;
>+
>+            property.modified = action === 1;
>+        }, WI.CSSProperty.hash);
>+
>+        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..e88a72ee977 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js
>@@ -149,28 +149,25 @@ WI.ChangesDetailsSidebarPanel = class ChangesDetailsSidebarPanel extends WI.DOMD
> 
>         selectorLineElement.append(" {\n");
> 
>-        let appendProperty = (cssProperty, className) => {
>-            let propertyLineElement = ruleElement.appendChild(document.createElement("div"));
>-            propertyLineElement.classList.add("css-property-line", className);
>-            let stylePropertyView = new WI.SpreadsheetStyleProperty(null, cssProperty, {readOnly: true});
>-            propertyLineElement.append(WI.indentString(), stylePropertyView.element, "\n");
>-        };
>-
>         let initialCSSProperties = style.initialState.visibleProperties;
>         let cssProperties = style.visibleProperties;
> 
>         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");
>+            let className = "";
>+            if (action === 1)
>+                className = "added";
>             else if (action === -1)
>-                appendProperty(cssProperty, "removed");
>-        });
>+                className = "removed";
>+            else
>+                className = "unchanged";
>+
>+            let propertyLineElement = ruleElement.appendChild(document.createElement("div"));
>+            propertyLineElement.classList.add("css-property-line", className);
>+
>+            const delegate = null;
>+            let stylePropertyView = new WI.SpreadsheetStyleProperty(delegate, cssProperty, {readOnly: true});
>+            propertyLineElement.append(WI.indentString(), stylePropertyView.element, "\n");
>+        }, WI.CSSProperty.hash);
> 
>         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.

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