Attachment 358205 Details for Bug 192739 – Patch

[patch] Patch

patch.txt (text/plain), 7.66 KB, created by Nikita Vasilyev on 2019-01-02 14:18:03 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Nikita Vasilyev

Created: 2019-01-02 14:18:03 PST

Size: 7.66 KB

patch

obsolete

>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index d0d8c6c2cb9..e194b71639c 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,40 @@
>+2019-01-02  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!).
>+
>+        Style updates are asynchronous. For example, there are two updates, A and B:
>+
>+          A:  [------]
>+          B:    [--]
>+
>+        B may finish before A, even though B started after A.
>+
>+        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.
>+
> 2018-12-21  Devin Rousso  <drousso@apple.com>
> 
>         Web Inspector: Styles Redesign: remove unused CSS style icons
>diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>index 4ba36efc8c3..b61f3d77a23 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>@@ -383,7 +383,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..da3d05930b3 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,9 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
>     update(text, properties, styleSheetTextRange, options = {})
>     {
>         let dontFireEvents = options.dontFireEvents || false;
>-        let suppressLock = options.suppressLock || false;
>+        let isLocked = !options.suppressLock || this._locked;
> 
>-        if (this._locked && !suppressLock && text !== this._text)
>+        if (isLocked && this._updatesInProgressCount > 0 && text !== this._text)
>             return;
> 
>         text = text || "";
>@@ -168,7 +169,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 (!isLocked) {
>                 // We shouldn't have any added or removed properties in this case.
>                 console.assert(!addedProperties.length && !removedProperties.length);
>             }
>@@ -201,7 +202,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
>         return this._text;
>     }
> 
>-    set text(text)
>+    setText(text)
>     {
>         if (this._text === text)
>             return;
>@@ -213,11 +214,30 @@ 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;
>+
>+        // Updates are asynchronous.
>+        //   A:  [------]
>+        //   B:    [--]
>+        // B may finish before A, even though B started after A.
>+        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()
>diff --git a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>index f44b05628a1..32a8b939ba2 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>@@ -449,21 +449,26 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
>         return result.promise;
>     }
> 
>-    changeStyleText(style, text)
>+    changeStyleText(style, text, callback = null)
>     {
>         if (!style.ownerStyleSheet || !style.styleSheetTextRange)
>             return;
> 
>         text = text || "";
> 
>-        function styleChanged(error, stylePayload)
>-        {
>-            if (error)
>+        let didSetStyleText = (error, stylePayload) => {
>+            if (error) {
>+                callback(error);
>                 return;
>-            this.refresh();
>-        }
>+            }
>+
>+            this.refresh().then(() => {
>+                if (callback)
>+                    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 a41f95dfd64..bf3e43f9e0a 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>@@ -71,8 +71,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)

Flags:

nvasilyev: review-
nvasilyev: commit-queue-

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