Bug 42764

Summary: Rename CSSStyleSelector to StyleResolver.
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: CSSAssignee: Alexis Menard (darktears) <menard>
Status: RESOLVED FIXED    
Severity: Normal CC: arv, eric.carlson, eric, feature-media-reviews, hyatt, kling, koivisto, macpherson, menard, mitz, rakuco, rolandsteiner, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 84734, 84765, 84814    
Bug Blocks: 46590    
Attachments:
Description Flags
Patch
none
Patch none

Dimitri Glazkov (Google)
Reported 2010-07-21 09:48:21 PDT
Rename CSSStyleSelector to RenderStyleResolver.
Attachments
Patch (785.99 KB, patch)
2010-07-21 09:51 PDT, Dimitri Glazkov (Google) 		no flags
DetailsFormatted DiffDiff
Patch (292.29 KB, patch)
2012-04-24 09:27 PDT, Alexis Menard (darktears) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dimitri Glazkov (Google)
Comment 1 2010-07-21 09:51:54 PDT
Created attachment 62201 [details] Patch
WebKit Review Bot
Comment 2 2010-07-21 09:54:32 PDT
Attachment 62201 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/css/SVGCSSStyleSelector.cpp:32: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/css/RenderStyleResolver.h:83: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 94 files If any of these errors are false positives, please file a bug against check-webkit-style.
mitz
Comment 3 2010-07-21 09:57:16 PDT
Comment on attachment 62201 [details] Patch Please explain the motivation and the benefit of this patch.
Dimitri Glazkov (Google)
Comment 4 2010-07-21 10:02:30 PDT
(In reply to comment #3) > (From update of attachment 62201 [details]) > Please explain the motivation and the benefit of this patch. Ah, good point. I should've included a more detailed ChangeLog entry. Looking at CSSStyleSelector, I understand that its primary use is to determine the RenderStyle for an element (CSSStyleSelector::[pseudo]styleForFoo) -- and a bunch of helpers around that. So RenderStyleResolver seems to fit better. Plus, it seems to me that the name is a bit confusing, since CSSStyleSelector could be interpreted as an encapsulation of CSS Selector, which it's not.
Dimitri Glazkov (Google)
Comment 5 2010-07-21 10:05:40 PDT
Another possibility is CSSStyleResolver.
mitz
Comment 6 2010-07-22 09:54:57 PDT
I’d be fine with CSSStyleResolver, but I’d like to know what Dave Hyatt thinks.
Antti Koivisto
Comment 7 2011-12-04 08:14:10 PST
StyleResolver perhaps. I think CSS prefix should mainly be reserved for cssom types. CSSStyleApplyProperty needs a new name too. StyleBuilder might be good.
Dimitri Glazkov (Google)
Comment 8 2011-12-04 08:16:26 PST
(In reply to comment #7) > StyleResolver perhaps. I think CSS prefix should mainly be reserved for cssom types. > > CSSStyleApplyProperty needs a new name too. StyleBuilder might be good. I like StyleResolver. And StyleBuilder, for that matter.
Roland Steiner
Comment 9 2011-12-04 20:48:30 PST
@_@ I can haz review for <style scoped> really, really fast, plz?!?
Alexis Menard (darktears)
Comment 10 2012-04-24 09:27:31 PDT
Created attachment 138578 [details] Patch
Alexis Menard (darktears)
Comment 11 2012-04-24 09:32:32 PDT
(In reply to comment #10) > Created an attachment (id=138578) [details] > Patch As requested I'll make child bugs for each of the patches.
Eric Seidel (no email)
Comment 12 2012-04-24 10:58:22 PDT
I think it's more interesting to split out the logic into a new class instead of renaming the existing one. CSSStyleSelector has a whole bunch of per-element and per-selector state. None of which the pure "resolver" would need. One of my goals after I finally land seamless is to rip a bunch of that state out to simplify the object.
Antti Koivisto
Comment 13 2012-04-24 11:10:47 PDT
(In reply to comment #12) > I think it's more interesting to split out the logic into a new class instead of renaming the existing one. CSSStyleSelector has a whole bunch of per-element and per-selector state. None of which the pure "resolver" would need. One of my goals after I finally land seamless is to rip a bunch of that state out to simplify the object. Sure but that is a completely orthogonal task. We still don't want to have silly names around.
Alexis Menard (darktears)
Comment 14 2012-04-25 15:40:59 PDT
All patches have landed. Done!
Note You need to log in before you can comment on or make changes to this bug.