Bug 202840 - Web Inspector: Convert CSSRule selectorText setter to setSelectorText method because it's asynchronous
Summary: Web Inspector: Convert CSSRule selectorText setter to setSelectorText method ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-10 22:41 PDT by Nikita Vasilyev
Modified: 2019-10-15 16:40 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.86 KB, patch)
2019-10-10 22:46 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (14.12 MB, application/zip)
2019-10-11 09:05 PDT, EWS Watchlist
no flags Details
Patch (6.38 KB, patch)
2019-10-14 12:26 PDT, Nikita Vasilyev
mattbaker: review+
Details | Formatted Diff | Diff
Patch (6.37 KB, patch)
2019-10-15 16:10 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-10-10 22:41:27 PDT
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.
Comment 1 Nikita Vasilyev 2019-10-10 22:46:45 PDT
Created attachment 380723 [details]
Patch
Comment 2 EWS Watchlist 2019-10-11 09:05:43 PDT
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
Comment 3 EWS Watchlist 2019-10-11 09:05:50 PDT
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
Comment 4 Nikita Vasilyev 2019-10-11 09:28:01 PDT
Comment on attachment 380723 [details]
Patch

Unrelated crash.
Comment 5 Matt Baker 2019-10-11 18:09:45 PDT
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.
Comment 6 Nikita Vasilyev 2019-10-14 12:26:32 PDT
Created attachment 380907 [details]
Patch
Comment 7 Matt Baker 2019-10-15 15:46:29 PDT
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
Comment 8 Nikita Vasilyev 2019-10-15 16:10:19 PDT
Created attachment 381036 [details]
Patch
Comment 9 WebKit Commit Bot 2019-10-15 16:39:13 PDT
Comment on attachment 381036 [details]
Patch

Clearing flags on attachment: 381036

Committed r251168: <https://trac.webkit.org/changeset/251168>
Comment 10 WebKit Commit Bot 2019-10-15 16:39:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-10-15 16:40:20 PDT
<rdar://problem/56313428>