Changeset 240946 in webkit


Ignore:
Timestamp:
Feb 4, 2019 3:30:49 PM (5 years ago)
Author:
Nikita Vasilyev
Message:

Web Inspector: Styles: fix race conditions when editing
​https://bugs.webkit.org/show_bug.cgi?id=192739
<rdar://problem/46752925>

Reviewed by Devin Rousso.

Source/WebInspectorUI:

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.

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 nameTextField): Removed.
(WI.SpreadsheetStyleProperty.prototype.get valueTextField): Removed.
Drive-by: remove unused code.

LayoutTests:

  • inspector/css/modify-css-property-expected.txt:
  • inspector/css/modify-css-property-race-expected.txt: Added.
  • inspector/css/modify-css-property-race.html: Added.
  • inspector/css/modify-css-property.html:
Location:
trunk
Files:
2 added
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r240941 r240946  
     12019-02-04  Nikita Vasilyev  <nvasilyev@apple.com>
     2
     3        Web Inspector: Styles: fix race conditions when editing
     4        https://bugs.webkit.org/show_bug.cgi?id=192739
     5        <rdar://problem/46752925>
     6
     7        Reviewed by Devin Rousso.
     8
     9        * inspector/css/modify-css-property-expected.txt:
     10        * inspector/css/modify-css-property-race-expected.txt: Added.
     11        * inspector/css/modify-css-property-race.html: Added.
     12        * inspector/css/modify-css-property.html:
     13
    1142019-02-04  Simon Fraser  <simon.fraser@apple.com>
    215

  • trunk/LayoutTests/inspector/css/modify-css-property-expected.txt

    r239251 r240946  
    55-- Running test case: Update value when CSSStyleDeclaration is not locked
    66PASS: "font-size" property value should update immediately.
    7 PASS: Style declaration text should stay unchanged.
     7PASS: Style declaration text should update immediately.
    88
    99-- Running test case: Update value when CSSStyleDeclaration is locked
    … …  
    1111PASS: Style declaration text should update immediately.
    1212
     13-- Running test case: ModifyCSSProperty.PropertiesChangedEvent
     14PASS: Style declaration is unlocked.
     15PASS: "width" property value should update to "200px".
     16PASS: Inline style declaration text should update when not locked.
     17
    1318-- Running test case: Update inline style value when CSSStyleDeclaration locked and not locked
    1419PASS: Style declaration text should update immediately.
    15 PASS: Style declaration text should stay "width: 64px".
    16 PASS: "width" property value should update to "200px".
     20PASS: Style declaration text should update immediately.
     21PASS: Style declaration is unlocked.
     22PASS: "width" property value should update to "128px".
    1723PASS: Inline style declaration text should update when not locked.
     24
     25-- Running test case: ModifyCSSProperty.ConsequentValueChanges
     26PASS: Style declaration text should contain most recent value.
    1827
    1928-- Running test case: ModifyCSSProperty.CommentOutAndUncommentPropertyWithNewlines

  • trunk/LayoutTests/inspector/css/modify-css-property.html

    r240314 r240946  
    4545            InspectorTest.expectEqual(getProperty("font-size").rawValue, "10px", `"font-size" property value should update immediately.`);
    4646
    47             InspectorTest.expectEqual(styleDeclaration.text, `font-size: 12px; color: antiquewhite`, `Style declaration text should stay unchanged.`);
     47            InspectorTest.expectEqual(styleDeclaration.text, `font-size: 10px; color: antiquewhite`, `Style declaration text should update immediately.`);
    4848
    4949            resolve();
    … …  
    9595
    9696    suite.addTestCase({
    97         name: "Update inline style value when CSSStyleDeclaration locked and not locked",
     97        name: "ModifyCSSProperty.PropertiesChangedEvent",
    9898        test(resolve, reject) {
    9999            let getInlineStyleDeclaration = () => {
    … …  
    108108            let getProperty = (propertyName) => {
    109109                let styleDeclaration = getInlineStyleDeclaration();
     110                for (let property of styleDeclaration.properties) {
     111                    if (property.name === propertyName)
     112                        return property;
     113                }
     114                InspectorTest.fail("No property found.");
     115                resolve();
     116            };
     117
     118            let styleDeclaration = getInlineStyleDeclaration();
     119
     120            styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).then((event) => {
     121                InspectorTest.expectThat(!styleDeclaration.locked, `Style declaration is unlocked.`);
     122                InspectorTest.expectEqual(getProperty("width").rawValue, "200px", `"width" property value should update to "200px".`);
     123                InspectorTest.expectEqual(styleDeclaration.text, `width: 200px;`, `Inline style declaration text should update when not locked.`);
     124                resolve();
     125            });
     126
     127            styleDeclaration.locked = true;
     128            // WI.CSSStyleDeclaration.Event.PropertiesChanged event should not fire when the style declaration is locked.
     129            InspectorTest.evaluateInPage(`makeNarrow()`);
     130
     131            styleDeclaration.locked = false;
     132            InspectorTest.evaluateInPage(`makeWide()`);
     133        }
     134    });
     135
     136    suite.addTestCase({
     137        name: "Update inline style value when CSSStyleDeclaration locked and not locked",
     138        test(resolve, reject) {
     139            let getInlineStyleDeclaration = () => {
     140                for (let styleDeclaration of nodeStyles.orderedStyles) {
     141                    if (styleDeclaration.type === styleDeclaration.constructor.Type.Inline)
     142                        return styleDeclaration;
     143                }
     144                InspectorTest.fail("No declaration found.");
     145                resolve();
     146            };
     147
     148            let getProperty = (propertyName) => {
     149                let styleDeclaration = getInlineStyleDeclaration();
    110150                for (let property of styleDeclaration.enabledProperties) {
    111151                    if (property.name === propertyName)
    … …  
    118158            let styleDeclaration = getInlineStyleDeclaration();
    119159
    120             styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged)
    121                 .then((event) => {
    122                     InspectorTest.expectEqual(getProperty("width").rawValue, "200px", `"width" property value should update to "200px".`);
    123                     InspectorTest.expectEqual(styleDeclaration.text, `width: 200px;`, `Inline style declaration text should update when not locked.`);
    124                 })
    125                 .then(resolve, reject);
     160            WI.CSSStyleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).then((event) => {
     161                InspectorTest.expectThat(!styleDeclaration.locked, `Style declaration is unlocked.`);
     162                InspectorTest.expectEqual(getProperty("width").rawValue, "128px", `"width" property value should update to "128px".`);
     163                InspectorTest.expectEqual(styleDeclaration.text, `width: 128px;`, `Inline style declaration text should update when not locked.`);
     164                resolve();
     165            });
    126166
    127167            styleDeclaration.locked = true;
    … …  
    134174            styleDeclaration.locked = false;
    135175            getProperty("width").rawValue = "128px";
    136             InspectorTest.expectEqual(styleDeclaration.text, `width: 64px;`, `Style declaration text should stay "width: 64px".`);
     176            InspectorTest.expectEqual(styleDeclaration.text, `width: 128px;`, `Style declaration text should update immediately.`);
    137177
    138178            InspectorTest.evaluateInPage(`makeWide()`);
     179        }
     180    });
     181
     182    suite.addTestCase({
     183        name: "ModifyCSSProperty.ConsequentValueChanges",
     184        test(resolve, reject) {
     185            let getInlineStyleDeclaration = () => {
     186                for (let styleDeclaration of nodeStyles.orderedStyles) {
     187                    if (styleDeclaration.type === styleDeclaration.constructor.Type.Inline)
     188                        return styleDeclaration;
     189                }
     190                InspectorTest.fail("No declaration found.");
     191                resolve();
     192            };
     193
     194            let getProperty = (propertyName) => {
     195                let styleDeclaration = getInlineStyleDeclaration();
     196                for (let property of styleDeclaration.properties) {
     197                    if (property.name === propertyName)
     198                        return property;
     199                }
     200                InspectorTest.fail("No property found.");
     201                resolve();
     202            };
     203
     204            let styleDeclaration = getInlineStyleDeclaration();
     205            styleDeclaration.locked = false;
     206
     207            for (let i = 0; i <= 20; ++i)
     208                getProperty("width").rawValue = i + "px";
     209
     210            InspectorTest.expectEqual(styleDeclaration.text, `width: 20px;`, `Style declaration text should contain most recent value.`);
     211
     212            resolve();
    139213        }
    140214    });

  • trunk/Source/WebInspectorUI/ChangeLog

    r240884 r240946  
     12019-02-04  Nikita Vasilyev  <nvasilyev@apple.com>
     2
     3        Web Inspector: Styles: fix race conditions when editing
     4        https://bugs.webkit.org/show_bug.cgi?id=192739
     5        <rdar://problem/46752925>
     6
     7        Reviewed by Devin Rousso.
     8
     9        Editing CSS property in the style editor syncronously updates CSSStyleDeclaration on the front-end
     10        and asyncronously updates the backend by calling CSSAgent.setStyleText. After the new style text is applied
     11        on the backend, CSSStyleDeclaration (on the front-end) gets updated.
     12
     13        Unsure there's no race conditions by introducing `_updatesInProgressCount`:
     14
     15          - Increment it before calling CSSAgent.setStyleText.
     16          - Decrement it after CSSAgent.setStyleText is finished.
     17
     18        Prevent updates of CSSStyleDeclaration when _updatesInProgressCount isn't 0.
     19
     20        * UserInterface/Models/CSSProperty.js:
     21        (WI.CSSProperty.prototype._updateOwnerStyleText):
     22        * UserInterface/Models/CSSStyleDeclaration.js:
     23        (WI.CSSStyleDeclaration):
     24        (WI.CSSStyleDeclaration.prototype.set text): Removed.
     25        (WI.CSSStyleDeclaration.prototype.setText): Added.
     26        Change the setter to a method since it has side effects including an asynchronous backend call.
     27
     28        * UserInterface/Models/DOMNodeStyles.js:
     29        (WI.DOMNodeStyles.prototype.changeStyleText):
     30
     31        * UserInterface/Views/SpreadsheetStyleProperty.js:
     32        (WI.SpreadsheetStyleProperty.prototype.get nameTextField): Removed.
     33        (WI.SpreadsheetStyleProperty.prototype.get valueTextField): Removed.
     34        Drive-by: remove unused code.
     35
    1362019-02-01  Devin Rousso  <drousso@apple.com>
    237

  • trunk/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js

    r240559 r240946  
    381381        }
    382382
     383        console.assert(this._ownerStyle);
     384        if (!this._ownerStyle)
     385            return;
     386
    383387        this._prependSemicolonIfNeeded();
    384388

  • trunk/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js

    r240559 r240946  
    4242
    4343        this._initialState = null;
     44        this._updatesInProgressCount = 0;
    4445        this._locked = false;
    4546        this._pendingProperties = [];
    … …  
    107108    {
    108109        let dontFireEvents = options.dontFireEvents || false;
    109         let suppressLock = options.suppressLock || false;
    110 
    111         if (this._locked && !suppressLock && text !== this._text)
     110
     111        // When two consequent setText calls happen (A and B), only update when the last call (B) is finished.
     112        //               Front-end:   A B
     113        //                Back-end:       A B
     114        // _updatesInProgressCount: 0 1 2 1 0
     115        //                                  ^
     116        //                                  update only happens here
     117        if (this._updatesInProgressCount > 0 && !options.forceUpdate) {
     118            if (WI.settings.enableStyleEditingDebugMode.value && text !== this._text)
     119                console.warn("Style modified while editing:", text);
     120
     121            return;
     122        }
     123
     124        // Allow updates from the backend when text matches because `properties` may contain warnings that need to be shown.
     125        if (this._locked && !options.forceUpdate && text !== this._text)
    112126            return;
    113127
    … …  
    200214            text = trimmedText;
    201215
    202         // Update text immediately when it was modified via the styles sidebar.
    203         if (this._locked)
    204             this._text = text;
    205 
    206         this._nodeStyles.changeStyleText(this, text);
     216        this._text = text;
     217        ++this._updatesInProgressCount;
     218
     219        let timeoutId = setTimeout(() => {
     220            console.error("Timed out when setting style text:", text);
     221            styleTextDidChange();
     222        }, 2000);
     223
     224        let styleTextDidChange = () => {
     225            if (!timeoutId)
     226                return;
     227
     228            clearTimeout(timeoutId);
     229            timeoutId = null;
     230            this._updatesInProgressCount = Math.max(0, this._updatesInProgressCount - 1);
     231        };
     232
     233        this._nodeStyles.changeStyleText(this, text, styleTextDidChange);
    207234    }
    208235
    … …  
    323350            this._properties[index].index = index;
    324351
    325         this.update(this._text, this._properties, this._styleSheetTextRange, {dontFireEvents: true, suppressLock: true});
     352        this.update(this._text, this._properties, this._styleSheetTextRange, {dontFireEvents: true, forceUpdate: true});
    326353
    327354        return property;

  • trunk/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js

    r240314 r240946  
    450450    }
    451451
    452     changeStyleText(style, text)
    453     {
    454         if (!style.ownerStyleSheet || !style.styleSheetTextRange)
     452    changeStyleText(style, text, callback)
     453    {
     454        if (!style.ownerStyleSheet || !style.styleSheetTextRange) {
     455            callback();
    455456            return;
     457        }
    456458
    457459        text = text || "";
    458460
    459         function styleChanged(error, stylePayload)
    460         {
    461             if (error)
     461        let didSetStyleText = (error, stylePayload) => {
     462            if (error) {
     463                callback(error);
    462464                return;
    463             this.refresh();
    464         }
    465 
    466         CSSAgent.setStyleText(style.id, text, styleChanged.bind(this));
     465            }
     466
     467            this.refresh().then(callback);
     468        };
     469
     470        CSSAgent.setStyleText(style.id, text, didSetStyleText);
    467471    }
    468472

  • trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js

    r240741 r240946  
    7676    get element() { return this._element; }
    7777    get property() { return this._property; }
    78     get nameTextField() { return this._nameTextField; }
    79     get valueTextField() { return this._valueTextField; }
    8078    get enabled() { return this._property.enabled; }
    8179
Note: See TracChangeset for help on using the changeset viewer.