Bug 197725 - Consider making CSSStyleSheet::rules() just an alias of CSSStyleSheet::cssRules().
Summary: Consider making CSSStyleSheet::rules() just an alias of CSSStyleSheet::cssRul...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-08 16:27 PDT by Emilio Cobos Álvarez (:emilio)
Modified: 2021-04-24 15:07 PDT (History)
15 users (show)

See Also:


Attachments
Patch (3.55 KB, patch)
2021-04-04 20:01 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (8.34 KB, patch)
2021-04-16 15:07 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (10.06 KB, patch)
2021-04-16 17:58 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emilio Cobos Álvarez (:emilio) 2019-05-08 16:27:40 PDT
Right now CSSStyleSheet.rules returns a StaticCSSRuleList. Blink got rid of this behavior, and I think WebKit could simplify their implementation too, so that it would match that and what I'm implementing in Firefox.

See https://github.com/w3c/csswg-drafts/pull/3900 / https://bugzilla.mozilla.org/show_bug.cgi?id=1545823
Comment 1 Tyler Wilcock 2021-04-04 20:01:08 PDT
Created attachment 425139 [details]
Patch
Comment 2 Darin Adler 2021-04-06 15:35:07 PDT
Comment on attachment 425139 [details]
Patch

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

> Source/WebCore/css/CSSStyleSheet.cpp:268
>  RefPtr<CSSRuleList> CSSStyleSheet::rules()
>  {
> -    if (!canAccessRules())
> -        return nullptr;
> -    // IE behavior.
> -    auto ruleList = StaticCSSRuleList::create();
> -    unsigned ruleCount = length();
> -    for (unsigned i = 0; i < ruleCount; ++i)
> -        ruleList->rules().append(item(i));
> -    return ruleList;
> +    return this->cssRules();
>  }

Four thoughts on this:

1) We should merge CSSStyleSheet::cssRulesForBindings with CSSStyleSheet::rulesForBindings, not just this lower layer.
2) We should use inlining, putting the forwarding in the header, if possible, and pay no runtime cost at all. So the function body of CSSStyleSheet::rulesForBindings should be in the header, not in the .cpp file.
3) We should delete CSSStyleSheet::rules entirely.
4) We can consider naming CSSStyleSheet::rulesForBindings to CSSStyleSheet::rules.
Comment 3 Tyler Wilcock 2021-04-16 15:07:28 PDT
Created attachment 426281 [details]
Patch
Comment 4 EWS Watchlist 2021-04-16 15:08:08 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 5 Tyler Wilcock 2021-04-16 17:58:47 PDT
Created attachment 426302 [details]
Patch
Comment 6 Tyler Wilcock 2021-04-17 07:11:08 PDT
Thanks for the review, Darin!  I've done all those things and marked cq? now that EWS is green.
Comment 7 Darin Adler 2021-04-17 15:18:34 PDT
Comment on attachment 426302 [details]
Patch

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

> Source/WebCore/css/CSSStyleSheet.h:67
> +    ExceptionOr<Ref<CSSRuleList>> rules() { return this->cssRulesForBindings(); }

The "this->" here is not needed.
Comment 8 EWS 2021-04-17 15:38:37 PDT
Committed r276209 (236691@main): <https://commits.webkit.org/236691@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426302 [details].
Comment 9 Radar WebKit Bug Importer 2021-04-24 15:07:14 PDT
<rdar://problem/77110091>