Bug 202840 – Web Inspector: Convert CSSRule selectorText setter to setSelectorText method because it's asynchronous

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Currently, selectorText setter has a rather unpredictable behaviour:

    cssRule.selectorText // "foo"
    cssRule.selectorText = "bar";
    cssRule.selectorText // Still "foo"!

Internally, selectorText setter is asyncronous. Since it's a setter, it doesn't have a callback.

My patch converts the setter to setSelectorText method that returns a promise.

[Nikita Vasilyev]

Created attachment 380723 [details]
Patch

[EWS Watchlist]

Comment on attachment 380723 [details]
Patch

Attachment 380723 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13120434

New failing tests:
fast/workers/worker-cloneport.html
js/dom/try-catch-crash.html

[EWS Watchlist]

Created attachment 380758 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Nikita Vasilyev]

Comment on attachment 380723 [details]
Patch

Unrelated crash.

[Matt Baker]

Comment on attachment 380723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380723&action=review

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:71
> +        return this._nodeStyles.changeRuleSelector(this, selectorText).then(this._selectorResolved.bind(this), this._selectorRejected);

This can just be:

return this._nodeStyles.changeRuleSelector(this, selectorText).then(this._selectorResolved.bind(this));

Here's a little stand alone example showing why:

function changeRuleSelector(rule, selectorText) {
    return selectorText ? Promise.resolve() : Promise.reject("Uh oh")
}

function setSelectorText(text) {
    function resolved() {
        // Do some stuff with resolved text. No need to return a Promise.
    }
    return changeRuleSelector(this, text).then(resolved);
}

setSelectorText("Some text").then(() => {
    console.log("success!");
}).catch((error) => {
    console.error("Error: " + error);
});

// Outputs "success!"

setSelectorText(null).then(() => {
    console.log("success!");
}).catch((error) => {
    console.error("Error: " + error);
});

// Outputs "Error: Uh oh"

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:116
> +        return Promise.reject(error);

This method is no longer needed.

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:147
> +        return Promise.resolve();

This can be removed.

[Nikita Vasilyev]

Created attachment 380907 [details]
Patch

[Matt Baker]

Comment on attachment 380907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380907&action=review

r=me

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:114
> +    // This method is only needed to be called for CSS rules that don't match the selected DOM node.

// This method only needs to be called

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:115
> +    // CSS rules that do match the selected DOM node get updated by WI.DOMNodeStyles.prototype._parseRulePayload.

// CSS rules that match

[Nikita Vasilyev]

Created attachment 381036 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 381036 [details]
Patch

Clearing flags on attachment: 381036

Committed r251168: <https://trac.webkit.org/changeset/251168>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56313428>