Clean up CSSPropertyTextDecoration implementation and ETextDecoration usage.
Created attachment 106381 [details] Patch
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.
Comment on attachment 106381 [details] Patch Attachment 106381 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9592716
Comment on attachment 106381 [details] Patch Attachment 106381 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9591742
Comment on attachment 106381 [details] Patch Attachment 106381 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9595437
Created attachment 106388 [details] Patch
Comment on attachment 106388 [details] Patch Attachment 106388 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9594508
Comment on attachment 106388 [details] Patch Attachment 106388 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9595443
Comment on attachment 106388 [details] Patch Attachment 106388 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9590838
Created attachment 106390 [details] Patch
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.
The style error is because it's picking up on the operator| function name.
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.
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++.
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.
We discovered this the hard way, after a lot of debugging, when porting Safari and WebKit to Windows.
(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 };
(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.
Created attachment 107954 [details] Patch
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.
Just wanted to ping this patch. Still hoping for a review.
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.
Comment on attachment 107954 [details] Patch Clearing flags on attachment: 107954 Committed r97314: <http://trac.webkit.org/changeset/97314>
All reviewed patches have been landed. Closing bug.