Bug 67625 – Clean up CSSPropertyTextDecoration implementation and ETextDecoration usage.

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

[Luke Macpherson]

Clean up CSSPropertyTextDecoration implementation and ETextDecoration usage.

[Luke Macpherson]

Created attachment 106381 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 3 in 6 files


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

[WebKit Review Bot]

Comment on attachment 106381 [details]
Patch

Attachment 106381 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9592716

[Early Warning System Bot]

Comment on attachment 106381 [details]
Patch

Attachment 106381 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9591742

[Gyuyoung Kim]

Comment on attachment 106381 [details]
Patch

Attachment 106381 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9595437

[Luke Macpherson]

Created attachment 106388 [details]
Patch

[Early Warning System Bot]

Comment on attachment 106388 [details]
Patch

Attachment 106388 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9594508

[WebKit Review Bot]

Comment on attachment 106388 [details]
Patch

Attachment 106388 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9595443

[Gyuyoung Kim]

Comment on attachment 106388 [details]
Patch

Attachment 106388 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9590838

[Luke Macpherson]

Created attachment 106390 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/style/RenderStyleConstants.h:343:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 1 in 6 files


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

[Luke Macpherson]

The style error is because it's picking up on the operator| function name.

[Ryosuke Niwa]

Comment on attachment 106390 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.h:195
> -        unsigned _text_decorations : 4;
> +        ETextDecoration _text_decorations : ETextDecorationBits;

This is going to hit a bug in Visual C++.

> Source/WebCore/rendering/style/RenderStyleConstants.h:339
> +static const size_t ETextDecorationBits = 4;

I like this.

[Luke Macpherson]

Comment on attachment 106390 [details]
Patch

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

>> Source/WebCore/rendering/style/RenderStyle.h:195
>> +        ETextDecoration _text_decorations : ETextDecorationBits;
> 
> This is going to hit a bug in Visual C++.

For my own education, could I have a pointer to the details? The build bot seems happy, but I'm not familiar with Visual C++.

[Darin Adler]

Microsoft’s compiler creates signed bitfields for enum types, and that causes problems with bitfields that are perfectly sized to hold an enum. Because of that, WebKit uses the type "unsigned" for bitfields instead of the actual enum type.

I found an old bug report about this <http://connect.microsoft.com/VisualStudio/feedback/details/100849/enum-bitfield-treated-incorrectly-as-integer> but haven’t found any definitive information that is newer.

[Darin Adler]

We discovered this the hard way, after a lot of debugging, when porting Safari and WebKit to Windows.

[Luke Macpherson]

(In reply to comment #16)
> We discovered this the hard way, after a lot of debugging, when porting Safari and WebKit to Windows.

Interesting, thanks for the explanation.
Did anyone try the workaround suggested in that link?
For example:
enum ETextDecoration : unsigned {
    TDNONE = 0x0 , UNDERLINE = 0x1, OVERLINE = 0x2, LINE_THROUGH= 0x4, BLINK = 0x8
};

[Luke Macpherson]

(In reply to comment #17)
> (In reply to comment #16)
> > We discovered this the hard way, after a lot of debugging, when porting Safari and WebKit to Windows.
> 
> Interesting, thanks for the explanation.
> Did anyone try the workaround suggested in that link?
> For example:
> enum ETextDecoration : unsigned {
>     TDNONE = 0x0 , UNDERLINE = 0x1, OVERLINE = 0x2, LINE_THROUGH= 0x4, BLINK = 0x8
> };

Hmm, seems gcc doesn't allow that anyway, so back to unsigned storage it is. It would have been nice not to have to sacrifice type safety.

[Luke Macpherson]

Created attachment 107954 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/style/RenderStyleConstants.h:343:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 1 in 6 files


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

[Luke Macpherson]

Just wanted to ping this patch. Still hoping for a review.

[Eric Seidel (no email)]

Comment on attachment 107954 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyleConstants.h:339
> +static const size_t ETextDecorationBits = 4;

If we don't need this broken out like this, I think it's better to not use a constant here.  It makes it more difficult to tell how large RenderSTyle is.

> Source/WebCore/rendering/style/StyleVisualData.h:52
> +    unsigned textDecoration : ETextDecorationBits; // Text decorations defined *only* by this element.

I see, you want to re-use it here.  OK.

[WebKit Review Bot]

Comment on attachment 107954 [details]
Patch

Clearing flags on attachment: 107954

Committed r97314: <http://trac.webkit.org/changeset/97314>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.