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

Description Dimitri Glazkov (Google) 2010-07-21 09:48:21 PDT
Rename CSSStyleSelector to RenderStyleResolver.
Comment 1 Dimitri Glazkov (Google) 2010-07-21 09:51:54 PDT
Created attachment 62201 [details]
Patch
Comment 2 WebKit Review Bot 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.
Comment 3 mitz 2010-07-21 09:57:16 PDT
Comment on attachment 62201 [details]
Patch

Please explain the motivation and the benefit of this patch.
Comment 4 Dimitri Glazkov (Google) 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.
Comment 5 Dimitri Glazkov (Google) 2010-07-21 10:05:40 PDT
Another possibility is CSSStyleResolver.
Comment 6 mitz 2010-07-22 09:54:57 PDT
I’d be fine with CSSStyleResolver, but I’d like to know what Dave Hyatt thinks.
Comment 7 Antti Koivisto 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.
Comment 8 Dimitri Glazkov (Google) 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.
Comment 9 Roland Steiner 2011-12-04 20:48:30 PST
@_@ I can haz review for <style scoped> really, really fast, plz?!?
Comment 10 Alexis Menard (darktears) 2012-04-24 09:27:31 PDT
Created attachment 138578 [details]
Patch
Comment 11 Alexis Menard (darktears) 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.
Comment 12 Eric Seidel (no email) 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.
Comment 13 Antti Koivisto 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.
Comment 14 Alexis Menard (darktears) 2012-04-25 15:40:59 PDT
All patches have landed. Done!