Attachment 358466 Details for Bug 192739 – Patch

[patch] Patch

patch.txt (text/plain), 15.90 KB, created by Nikita Vasilyev on 2019-01-06 12:34:49 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Nikita Vasilyev

Created: 2019-01-06 12:34:49 PST

Size: 15.90 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index f884bd836f6..1bfec960b9f 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-01-05  Nikita Vasilyev  <nvasilyev@apple.com>
>+
>+        Web Inspector: Styles: fix race conditions when editing
>+        https://bugs.webkit.org/show_bug.cgi?id=192739
>+        <rdar://problem/46752925>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * inspector/css/modify-css-property-expected.txt:
>+        * inspector/css/modify-css-property.html:
>+
> 2019-01-04  Devin Rousso  <drousso@apple.com>
> 
>         Web Inspector: Audit: disable breakpoints when running Audit
>diff --git a/LayoutTests/inspector/css/modify-css-property-expected.txt b/LayoutTests/inspector/css/modify-css-property-expected.txt
>index c5374e54c9e..0f17979a483 100644
>--- a/LayoutTests/inspector/css/modify-css-property-expected.txt
>+++ b/LayoutTests/inspector/css/modify-css-property-expected.txt
>@@ -4,18 +4,20 @@ Testing that CSSStyleDeclaration update immediately after modifying its properti
> == Running test suite: ModifyCSSProperty
> -- Running test case: Update value when CSSStyleDeclaration is not locked
> PASS: "font-size" property value should update immediately.
>-PASS: Style declaration text should stay unchanged.
>+PASS: Style declaration text should update immediately.
> 
> -- Running test case: Update value when CSSStyleDeclaration is locked
> PASS: "font-size" property value should update immediately.
> PASS: Style declaration text should update immediately.
> 
>--- Running test case: Update inline style value when CSSStyleDeclaration locked and not locked
>-PASS: Style declaration text should update immediately.
>-PASS: Style declaration text should stay "width: 64px".
>+-- Running test case: ModifyCSSProperty.PropertiesChangedEvent
> PASS: "width" property value should update to "200px".
> PASS: Inline style declaration text should update when not locked.
> 
>+-- Running test case: Update inline style value when CSSStyleDeclaration locked and not locked
>+PASS: Style declaration text should update immediately.
>+PASS: Style declaration text should update immediately.
>+
> -- Running test case: ModifyCSSProperty.CommentOutAndUncommentPropertyWithNewlines
> PASS: Commented out property should be disabled.
> PASS: Style declaration text should update immediately with uncommented property.
>diff --git a/LayoutTests/inspector/css/modify-css-property.html b/LayoutTests/inspector/css/modify-css-property.html
>index 52266588482..a70fe70e5fa 100644
>--- a/LayoutTests/inspector/css/modify-css-property.html
>+++ b/LayoutTests/inspector/css/modify-css-property.html
>@@ -44,7 +44,7 @@ function test() {
> 
>             InspectorTest.expectEqual(getProperty("font-size").rawValue, "10px", `"font-size" property value should update immediately.`);
> 
>-            InspectorTest.expectEqual(styleDeclaration.text, `font-size: 12px; color: antiquewhite`, `Style declaration text should stay unchanged.`);
>+            InspectorTest.expectEqual(styleDeclaration.text, `font-size: 10px; color: antiquewhite`, `Style declaration text should update immediately.`);
> 
>             resolve();
>         }
>@@ -93,6 +93,46 @@ function test() {
>         }
>     });
> 
>+    suite.addTestCase({
>+        name: "ModifyCSSProperty.PropertiesChangedEvent",
>+        test(resolve, reject) {
>+            let getInlineStyleDeclaration = () => {
>+                for (let styleDeclaration of nodeStyles.orderedStyles) {
>+                    if (styleDeclaration.type === styleDeclaration.constructor.Type.Inline)
>+                        return styleDeclaration;
>+                }
>+                InspectorTest.fail("No declaration found.");
>+                resolve();
>+            };
>+
>+            let getProperty = (propertyName) => {
>+                let styleDeclaration = getInlineStyleDeclaration();
>+                for (let property of styleDeclaration.properties) {
>+                    if (property.name === propertyName)
>+                        return property;
>+                }
>+                InspectorTest.fail("No property found.");
>+                resolve();
>+            };
>+
>+            let styleDeclaration = getInlineStyleDeclaration();
>+
>+            styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged)
>+                    .then((event) => {
>+                        InspectorTest.expectEqual(getProperty("width").rawValue, "200px", `"width" property value should update to "200px".`);
>+                        InspectorTest.expectEqual(styleDeclaration.text, `width: 200px;`, `Inline style declaration text should update when not locked.`);
>+                        resolve();
>+                    });
>+
>+            styleDeclaration.locked = true;
>+            // WI.CSSStyleDeclaration.Event.PropertiesChanged event should not fire when the style declaration is locked.
>+            InspectorTest.evaluateInPage(`makeNarrow()`);
>+
>+            styleDeclaration.locked = false;
>+            InspectorTest.evaluateInPage(`makeWide()`);
>+        }
>+    });
>+
>     suite.addTestCase({
>         name: "Update inline style value when CSSStyleDeclaration locked and not locked",
>         test(resolve, reject) {
>@@ -119,10 +159,8 @@ function test() {
> 
>             styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged)
>                 .then((event) => {
>-                    InspectorTest.expectEqual(getProperty("width").rawValue, "200px", `"width" property value should update to "200px".`);
>-                    InspectorTest.expectEqual(styleDeclaration.text, `width: 200px;`, `Inline style declaration text should update when not locked.`);
>-                })
>-                .then(resolve, reject);
>+                    InspectorTest.fail("PropertiesChanged event shouldn't fire while property is being edited by a user.");
>+                });
> 
>             styleDeclaration.locked = true;
>             getProperty("width").rawValue = "64px";
>@@ -133,9 +171,11 @@ function test() {
> 
>             styleDeclaration.locked = false;
>             getProperty("width").rawValue = "128px";
>-            InspectorTest.expectEqual(styleDeclaration.text, `width: 64px;`, `Style declaration text should stay "width: 64px".`);
>+            InspectorTest.expectEqual(styleDeclaration.text, `width: 128px;`, `Style declaration text should update immediately.`);
> 
>             InspectorTest.evaluateInPage(`makeWide()`);
>+
>+            resolve();
>         }
>     });
> 
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index f05867e772b..7cb375de296 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,44 @@
>+2019-01-05  Nikita Vasilyev  <nvasilyev@apple.com>
>+
>+        Web Inspector: Styles: fix race conditions when editing
>+        https://bugs.webkit.org/show_bug.cgi?id=192739
>+        <rdar://problem/46752925>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Editing CSS property in the style editor syncronously updates CSSStyleDeclaration on the front-end
>+        and asyncronously updates the backend by calling CSSAgent.setStyleText. After the new style text is applied
>+        on the backend, CSSStyleDeclaration (on the front-end) gets updated.
>+
>+        When two consequent style changes happen, only update CSSStyleDeclaration when the last call (B) is finished.
>+                      Front-end:   A B
>+                       Back-end:       A B
>+        _updatesInProgressCount: 0 1 2 1 0
>+                                         ^
>+                                         update only happens here
>+
>+        Unsure there's no race conditions by introducing `_updatesInProgressCount`:
>+
>+          - Increment it before calling CSSAgent.setStyleText.
>+          - Decrement it after CSSAgent.setStyleText is finished.
>+
>+        Prevent updates of CSSStyleDeclaration when _updatesInProgressCount isn't 0.
>+
>+        * UserInterface/Models/CSSProperty.js:
>+        (WI.CSSProperty.prototype._updateOwnerStyleText):
>+        * UserInterface/Models/CSSStyleDeclaration.js:
>+        (WI.CSSStyleDeclaration):
>+        (WI.CSSStyleDeclaration.prototype.set text): Removed.
>+        (WI.CSSStyleDeclaration.prototype.setText): Added.
>+        Change the setter to a method since it has side effects including an asynchronous backend call.
>+
>+        * UserInterface/Models/DOMNodeStyles.js:
>+        (WI.DOMNodeStyles.prototype.changeStyleText):
>+
>+        * UserInterface/Views/SpreadsheetStyleProperty.js:
>+        (WI.SpreadsheetStyleProperty.prototype.get property):
>+        Drive-by: remove unused code.
>+
> 2019-01-04  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: subclasses of WI.ClusterContentView don't save/restore content views after the initial view
>diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>index 4ba36efc8c3..62ca03b8ca0 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>@@ -358,6 +358,10 @@ WI.CSSProperty = class CSSProperty extends WI.Object
>             return;
>         }
> 
>+        console.assert(this._ownerStyle);
>+        if (!this._ownerStyle)
>+            return;
>+
>         this._prependSemicolonIfNeeded();
> 
>         let styleText = this._ownerStyle.text || "";
>@@ -383,7 +387,8 @@ WI.CSSProperty = class CSSProperty extends WI.Object
>         let columnDelta = newText.lastLine.length - oldText.lastLine.length;
>         this._styleSheetTextRange = this._styleSheetTextRange.cloneAndModify(0, 0, lineDelta, columnDelta);
> 
>-        this._ownerStyle.text = newStyleText;
>+        // This causes a backend call which is asynchronous yet changes in the model on the front-end are synchronous.
>+        this._ownerStyle.setText(newStyleText);
> 
>         let propertyWasRemoved = !newText;
>         this._ownerStyle.shiftPropertiesAfter(this, lineDelta, columnDelta, propertyWasRemoved);
>diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>index 62f11405844..e6bc1c670e9 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>@@ -40,6 +40,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
>         this._node = node || null;
>         this._inherited = inherited || false;
> 
>+        this._updatesInProgressCount = 0;
>         this._locked = false;
>         this._pendingProperties = [];
>         this._propertyNameMap = {};
>@@ -102,9 +103,23 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
>     update(text, properties, styleSheetTextRange, options = {})
>     {
>         let dontFireEvents = options.dontFireEvents || false;
>-        let suppressLock = options.suppressLock || false;
>+        let forceUpdate = options.forceUpdate || false;
>+
>+        // When two consequent setText calls happen (A and B), only update when the last call (B) is finished.
>+        //               Front-end:   A B
>+        //                Back-end:       A B
>+        // _updatesInProgressCount: 0 1 2 1 0
>+        //                                  ^
>+        //                                  update only happens here
>+        if (this._updatesInProgressCount > 0 && !forceUpdate) {
>+            if (WI.settings.enableStyleEditingDebugMode.value && text !== this._text)
>+                console.warn("Style modified while editing:", text);
> 
>-        if (this._locked && !suppressLock && text !== this._text)
>+            return;
>+        }
>+
>+        // Allow updates from the backend when text matches because `properties` may contain warnings that need to be shown.
>+        if (this._locked && !forceUpdate && text !== this._text)
>             return;
> 
>         text = text || "";
>@@ -168,7 +183,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
> 
>         // Don't fire the event if there is text and it hasn't changed.
>         if (oldText && this._text && oldText === this._text) {
>-            if (!this._locked || suppressLock) {
>+            if (!this._locked && !forceUpdate) {
>                 // We shouldn't have any added or removed properties in this case.
>                 console.assert(!addedProperties.length && !removedProperties.length);
>             }
>@@ -201,7 +216,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
>         return this._text;
>     }
> 
>-    set text(text)
>+    setText(text)
>     {
>         if (this._text === text)
>             return;
>@@ -213,11 +228,25 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
>         if (!trimmedText.length || this._type === WI.CSSStyleDeclaration.Type.Inline)
>             text = trimmedText;
> 
>-        // Update text immediately when it was modified via the styles sidebar.
>-        if (this._locked)
>-            this._text = text;
>+        this._text = text;
>+        this._updatesInProgressCount++;
>+
>+        let styleTextDidChange = () => {
>+            if (!timeoutId)
>+                return;
>+
>+            clearTimeout(timeoutId);
>+            timeoutId = null;
>+            this._updatesInProgressCount--;
>+            console.assert(this._updatesInProgressCount >= 0, "_updatesInProgressCount cannot be a negative number.");
>+        };
>+
>+        let timeoutId = setTimeout(() => {
>+            console.error("Timed out when setting style text:", text);
>+            styleTextDidChange();
>+        }, 1000);
> 
>-        this._nodeStyles.changeStyleText(this, text);
>+        this._nodeStyles.changeStyleText(this, text, styleTextDidChange);
>     }
> 
>     get properties()
>@@ -332,7 +361,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
>         for (let index = propertyIndex + 1; index < this._allProperties.length; index++)
>             this._allProperties[index].index = index;
> 
>-        this.update(this._text, this._allProperties, this._styleSheetTextRange, {dontFireEvents: true, suppressLock: true});
>+        this.update(this._text, this._allProperties, this._styleSheetTextRange, {dontFireEvents: true, forceUpdate: true});
> 
>         return property;
>     }
>diff --git a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>index f44b05628a1..4ecc8a7bfc0 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>@@ -449,21 +449,25 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
>         return result.promise;
>     }
> 
>-    changeStyleText(style, text)
>+    changeStyleText(style, text, callback)
>     {
>-        if (!style.ownerStyleSheet || !style.styleSheetTextRange)
>+        if (!style.ownerStyleSheet || !style.styleSheetTextRange) {
>+            callback();
>             return;
>+        }
> 
>         text = text || "";
> 
>-        function styleChanged(error, stylePayload)
>-        {
>-            if (error)
>+        let didSetStyleText = (error, stylePayload) => {
>+            if (error) {
>+                callback(error);
>                 return;
>-            this.refresh();
>-        }
>+            }
>+
>+            this.refresh().finally(callback);
>+        };
> 
>-        CSSAgent.setStyleText(style.id, text, styleChanged.bind(this));
>+        CSSAgent.setStyleText(style.id, text, didSetStyleText);
>     }
> 
>     // Private
>diff --git a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>index d454fa6020f..ebfdaa7b586 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>@@ -75,8 +75,6 @@ WI.SpreadsheetStyleProperty = class SpreadsheetStyleProperty extends WI.Object
> 
>     get element() { return this._element; }
>     get property() { return this._property; }
>-    get nameTextField() { return this._nameTextField; }
>-    get valueTextField() { return this._valueTextField; }
>     get enabled() { return this._property.enabled; }
> 
>     set index(index)

Actions: View | Formatted Diff | Diff

Attachments on bug 192739: 358205 | 358212 | 358216 | 358217 | 358219 | 358447 | 358450 | 358451 | 358453 | 358466 | 358857 | 360990 | 360994 | 361001 | 361007 | 361008 | 361104