Summary: | Rename CSSStyleSelector to StyleResolver. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||||
Component: | CSS | Assignee: | 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
Dimitri Glazkov (Google)
2010-07-21 09:48:21 PDT
Created attachment 62201 [details]
Patch
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 on attachment 62201 [details]
Patch
Please explain the motivation and the benefit of this patch.
(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. Another possibility is CSSStyleResolver. I’d be fine with CSSStyleResolver, but I’d like to know what Dave Hyatt thinks. StyleResolver perhaps. I think CSS prefix should mainly be reserved for cssom types. CSSStyleApplyProperty needs a new name too. StyleBuilder might be good. (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. @_@ I can haz review for <style scoped> really, really fast, plz?!? Created attachment 138578 [details]
Patch
(In reply to comment #10) > Created an attachment (id=138578) [details] > Patch As requested I'll make child bugs for each of the patches. 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. (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. All patches have landed. Done! |