Bug 12228 - Cleanup assorted CSS classes
Summary: Cleanup assorted CSS classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-12 12:15 PST by Sam Weinig
Modified: 2007-01-14 17:26 PST (History)
0 users

See Also:


Attachments
cleanup patch (191.03 KB, patch)
2007-01-12 12:21 PST, Sam Weinig
darin: review+
Details | Formatted Diff | Diff
updated patch (217.65 KB, patch)
2007-01-13 12:18 PST, Sam Weinig
mitz: review+
Details | Formatted Diff | Diff
patch without white space changes (104.64 KB, patch)
2007-01-13 12:20 PST, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2007-01-12 12:15:40 PST
Patch forthcoming does cleanup of about half of the css files.
Comment 1 Sam Weinig 2007-01-12 12:21:46 PST
Created attachment 12395 [details]
cleanup patch
Comment 2 Darin Adler 2007-01-13 07:25:17 PST
Comment on attachment 12395 [details]
cleanup patch

The word "Hand" really shouldn't be capitalized in "shorthand".

Do we really want braces for the for statements that don't need them? I wish our style guidelines were clear on that.

If anything I'd like to see you applying even more of the m_ prefix. Hyatt was just talking about how it's hard to work on certain classes because it's so hard to tell the members apart from local variables.

Fine to land this.

r=me
Comment 3 Sam Weinig 2007-01-13 12:18:59 PST
Created attachment 12419 [details]
updated patch

This patch updates the previous version to address Darin's comments.  Specifically, the member variables in CSSSelector have been prefixed with m_ and uses of ShortHand have been replaced with Shorthand.
Comment 4 Sam Weinig 2007-01-13 12:20:40 PST
Created attachment 12420 [details]
patch without white space changes

This version is made using svn diff -x -w for easier reviewing.
Comment 5 Dave Hyatt 2007-01-13 13:11:39 PST
Comment on attachment 12395 [details]
cleanup patch

Yeah, please make sure not to capitalize the H in shorthand.
Comment 6 mitz 2007-01-13 14:49:21 PST
Comment on attachment 12420 [details]
patch without white space changes

Misplaced *:

+    DashboardRegion *getDashboardRegionValue () const { return m_type != CSS_DASHBOARD_REGION ? 0 : m_value.region; }

r=me
Comment 7 Sam Weinig 2007-01-14 17:26:21 PST
Landed in r18850.