Attachment 368625 Details for Bug 195264 – Patch

[patch] Patch

patch.txt (text/plain), 17.99 KB, created by Nikita Vasilyev on 2019-04-30 16:29:59 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Nikita Vasilyev

Created: 2019-04-30 16:29:59 PDT

Size: 17.99 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 24b7d8c6f58..bbac153e4be 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-04-30  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!).
>+
>+        * 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.html b/LayoutTests/inspector/unit-tests/array-utilities.html
>index d15afecee7d..0b449497295 100644
>--- a/LayoutTests/inspector/unit-tests/array-utilities.html
>+++ b/LayoutTests/inspector/unit-tests/array-utilities.html
>@@ -156,7 +156,7 @@ function test()
>         test() {
>             function diff(initial, current) {
>                 let actual = [];
>-                Array.diffArrays(initial, current, (value, changed) => {
>+                Array.diffArrays(initial, current, (x) => x, (value, changed) => {
>                     actual.push([value, changed]);
>                 });
> 
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 6809f8dab28..f8a5466da89 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,48 @@
>+2019-04-30  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.set initialState):
>+        (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..19b331393ba 100644
>--- a/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>+++ b/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>@@ -501,15 +501,31 @@ 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, hashFunction, onEach)
>+    {
>+        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;
>+        }
>+
>+        let initialMap = createUsageCountMap(initialArray, hashFunction);
>+        let currentMap = createUsageCountMap(currentArray, hashFunction);
>         let indexInitial = 0;
>         let indexCurrent = 0;
>         let deltaInitial = 0;
>         let deltaCurrent = 0;
> 
>+        function decrementUsageCount(map, key) {
>+            let count = map.get(key);
>+            if (count)
>+                map.set(key, count - 1);
>+        }
>+
>         let i = 0;
>         while (true) {
>             if (indexInitial >= initialArray.length || indexCurrent >= currentArray.length)
>@@ -517,28 +533,33 @@ 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, initialHash);
>+            } else if (currentMap.get(initialHash) > 0) {
>+                if (initialMap.get(currentHash) > 0) {
>                     // Moved.
>                     onEach(current, 0);
>+                    decrementUsageCount(initialMap, initialHash);
>                 } else {
>                     // Added.
>                     onEach(current, 1);
>-                    --i;
>-                    ++deltaCurrent;
>                 }
>+                --i;
>+                ++deltaCurrent;
>             } 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);
>                 }
>             }
> 
>@@ -548,15 +569,26 @@ Object.defineProperty(Array, "diffArrays",
>         }
> 
>         for (let i = indexInitial; i < initialArray.length; ++i) {
>-            // Removed.
>-            onEach(initialArray[i], -1);
>+            let initial = initialArray[i];
>+            let initialHash = hashFunction(initial);
>+            if (currentMap.get(initialHash) > 0)
>+                decrementUsageCount(currentMap, initialHash);
>+            else {
>+                // Removed.
>+                onEach(initialArray[i], -1);
>+            }
>         }
> 
>         for (let i = indexCurrent; i < currentArray.length; ++i) {
>-            // Added.
>-            onEach(currentArray[i], 1);
>+            let current = currentArray[i];
>+            let currentHash = hashFunction(current);
>+            if (initialMap.get(currentHash) > 0)
>+                decrementUsageCount(initialMap, currentHash);
>+            else {
>+                // Added.
>+                onEach(current, 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..83a3a3029e0 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>@@ -56,6 +56,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
>     // Public
> 
>     get initialState() { return this._initialState; }
>+    set initialState(state) { this._initialState = state; }
> 
>     get id()
>     {
>@@ -365,9 +366,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 +379,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 +418,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, WI.CSSProperty.hash, (property, action) => {
>+            if (action !== 0)
>+                hasModified = true;
>+
>+            property.modified = action === 1;
>+        });
>+
>+        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..58900f08b0b 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js
>@@ -149,27 +149,22 @@ 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");
>+        Array.diffArrays(initialCSSProperties, cssProperties, WI.CSSProperty.hash, (cssProperty, action) => {
>+            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);
>+            let stylePropertyView = new WI.SpreadsheetStyleProperty(null, cssProperty, {readOnly: true});
>+            propertyLineElement.append(WI.indentString(), stylePropertyView.element, "\n");
>         });
> 
>         let closeBraceElement = document.createElement("span");
>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+
hi: commit-queue-

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