Bug 32799 - Cleanup: Rename CSS list-style-type markers to conform to WebKit Code Style Guidelines
Summary: Cleanup: Rename CSS list-style-type markers to conform to WebKit Code Style G...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-20 12:26 PST by Daniel Bates
Modified: 2009-12-22 23:31 PST (History)
7 users (show)

See Also:


Attachments
Patch (18.48 KB, patch)
2009-12-20 12:41 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with style-bot fixes (maybe we should do these fixes in a separate bug?) (28.89 KB, patch)
2009-12-20 12:55 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (18.74 KB, patch)
2009-12-20 22:01 PST, Daniel Bates
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2009-12-20 12:26:16 PST
Following up on a suggestion by Darin <https://bugs.webkit.org/show_bug.cgi?id=32138#c5>, we should rename the enums in RenderStyleConstants::EListStyleType to conform to the WebKit Code Style Guidelines.
Comment 1 Daniel Bates 2009-12-20 12:41:09 PST
Created attachment 45288 [details]
Patch

No functionality was changed. So, no tests included in patch.
Comment 2 WebKit Review Bot 2009-12-20 12:41:34 PST
Attachment 45288 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSPrimitiveValueMappings.h:933:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListItem.cpp:54:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderListMarker.cpp:343:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:574:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:696:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:776:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:793:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:839:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 8
Comment 3 Daniel Bates 2009-12-20 12:55:12 PST
Created attachment 45289 [details]
Patch with style-bot fixes (maybe we should do these fixes in a separate bug?)

An updated patch with more style corrections to make the style-bot happy.

I think I recall seeing a webkit-dev post about allowing switch indents when there are many cases. Maybe we can disagree with the style-bot (and not use this attachment) since the cleanup requires us to touch some switch statements that have many cases?

Also, if we do decide to go with the style-bot's suggestions then we may want to do these changes as part of a separate bug(s).
Comment 4 Darin Adler 2009-12-20 21:06:21 PST
Comment on attachment 45288 [details]
Patch

I think that DecimalListStyle would be better than LDecimal and NoneListStyle better than LNone.
Comment 5 Daniel Bates 2009-12-20 22:01:44 PST
Created attachment 45306 [details]
Patch

Updated patch based on Darin's suggestion to substitute DecimalListStyle for LDecimal and NoneListStyle for LNone.
Comment 6 WebKit Review Bot 2009-12-20 22:02:55 PST
Attachment 45306 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSPrimitiveValueMappings.h:939:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:343:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:574:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:696:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:776:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:793:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:839:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 7
Comment 7 Eric Seidel (no email) 2009-12-20 22:39:38 PST
Comment on attachment 45306 [details]
Patch

Maybe more should ahve the ListStyle postfix, but this seems better than what we had before.  r=me.
Comment 8 Daniel Bates 2009-12-22 23:31:04 PST
Committed in <http://trac.webkit.org/changeset/52521>.