Bug 186015 – Modernize RenderStyleConstants.h - Part 3

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Modernize RenderStyleConstants.h - Part 3

[Sam Weinig]

Created attachment 341406 [details]
Patch

[EWS Watchlist]

Attachment 341406 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/TextDecorationPainter.cpp:252:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 341409 [details]
Patch

[EWS Watchlist]

Attachment 341409 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/TextDecorationPainter.cpp:252:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 341409 [details]
Patch

Attachment 341409 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7818329

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 341410 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Sam Weinig]

Created attachment 341414 [details]
Patch

[Sam Weinig]

The fact that you can't construct an OptionSet with a enum value that happens to be 0 is a bit annoying. I'm not clear on the reasoning for it.

[EWS Watchlist]

Attachment 341414 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/TextDecorationPainter.cpp:252:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 341414 [details]
Patch

Attachment 341414 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7820300

New failing tests:
accessibility/mac/css-speech-speak.html
fast/css/round-trip-stroke-width-using-computed-style.html

[EWS Watchlist]

Created attachment 341415 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Sam Weinig]

Created attachment 341416 [details]
Patch

[EWS Watchlist]

Attachment 341416 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/TextDecorationPainter.cpp:252:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 341416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341416&action=review

r=me with comments.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-1839
> -    if (speakAs & SpeakNormal)

Since `SpeakNormal` was 0 previously, I think this path was never executed (speakAs & SpeakNormal is always zero). Is the following code correct?

> Source/WebCore/css/CSSLineBoxContainValue.h:35
> +enum LineBoxContainFlags {

Should we change this to `enum class` too?

[Sam Weinig]

Created attachment 341432 [details]
Patch

[EWS Watchlist]

Attachment 341432 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/TextDecorationPainter.cpp:252:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

(In reply to Yusuke Suzuki from comment #14)
> Comment on attachment 341416 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341416&action=review
> 
> r=me with comments.
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-1839
> > -    if (speakAs & SpeakNormal)
> 
> Since `SpeakNormal` was 0 previously, I think this path was never executed
> (speakAs & SpeakNormal is always zero). Is the following code correct?

That code is funny. The added code doesn't actually change behavior, because the old code ended up doing the same thing at the end of the chain of if-statements if the list was empty. Anyway, no reason to have duplicate functionality so I removed that initial if.

> 
> > Source/WebCore/css/CSSLineBoxContainValue.h:35
> > +enum LineBoxContainFlags {
> 
> Should we change this to `enum class` too?

Yeah, I'll get there.

[WebKit Commit Bot]

Comment on attachment 341432 [details]
Patch

Clearing flags on attachment: 341432

Committed r232229: <https://trac.webkit.org/changeset/232229>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40588982>