Summary: | Consider making CSSStyleSheet::rules() just an alias of CSSStyleSheet::cssRules(). | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Emilio Cobos Álvarez (:emilio) <emilio> | ||||||||
Component: | DOM | Assignee: | Tyler Wilcock <twilco.o> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | berto, cdumez, cgarcia, darin, emilio, esprehn+autocc, ews-watchlist, glenn, gustavo, gyuyoung.kim, kondapallykalyan, macpherson, menard, twilco.o, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Emilio Cobos Álvarez (:emilio)
2019-05-08 16:27:40 PDT
Created attachment 425139 [details]
Patch
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. Created attachment 426281 [details]
Patch
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 Created attachment 426302 [details]
Patch
Thanks for the review, Darin! I've done all those things and marked cq? now that EWS is green. 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. 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]. |