RESOLVED FIXED 67625
Clean up CSSPropertyTextDecoration implementation and ETextDecoration usage.
https://bugs.webkit.org/show_bug.cgi?id=67625
Summary Clean up CSSPropertyTextDecoration implementation and ETextDecoration usage.
Luke Macpherson
Reported 2011-09-05 20:43:36 PDT
Clean up CSSPropertyTextDecoration implementation and ETextDecoration usage.
Attachments
Patch (8.44 KB, patch)
2011-09-05 20:50 PDT, Luke Macpherson
no flags
Patch (8.45 KB, patch)
2011-09-05 21:48 PDT, Luke Macpherson
no flags
Patch (8.62 KB, patch)
2011-09-05 22:48 PDT, Luke Macpherson
no flags
Patch (8.68 KB, patch)
2011-09-19 18:46 PDT, Luke Macpherson
no flags
Luke Macpherson
Comment 1 2011-09-05 20:50:16 PDT
WebKit Review Bot
Comment 2 2011-09-05 20:51:47 PDT
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 3 2011-09-05 21:09:54 PDT
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 4 2011-09-05 21:11:36 PDT
Gyuyoung Kim
Comment 5 2011-09-05 21:21:28 PDT
Luke Macpherson
Comment 6 2011-09-05 21:48:22 PDT
Early Warning System Bot
Comment 7 2011-09-05 22:00:39 PDT
WebKit Review Bot
Comment 8 2011-09-05 22:06:09 PDT
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 9 2011-09-05 22:13:35 PDT
Luke Macpherson
Comment 10 2011-09-05 22:48:42 PDT
WebKit Review Bot
Comment 11 2011-09-05 22:51:41 PDT
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
Comment 12 2011-09-05 22:55:13 PDT
The style error is because it's picking up on the operator| function name.
Ryosuke Niwa
Comment 13 2011-09-14 21:46:28 PDT
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 14 2011-09-15 20:48:22 PDT
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
Comment 15 2011-09-15 23:00:45 PDT
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
Comment 16 2011-09-15 23:01:33 PDT
We discovered this the hard way, after a lot of debugging, when porting Safari and WebKit to Windows.
Luke Macpherson
Comment 17 2011-09-19 17:41:18 PDT
(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
Comment 18 2011-09-19 17:47:39 PDT
(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
Comment 19 2011-09-19 18:46:20 PDT
WebKit Review Bot
Comment 20 2011-09-19 18:48:50 PDT
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
Comment 21 2011-10-05 15:26:25 PDT
Just wanted to ping this patch. Still hoping for a review.
Eric Seidel (no email)
Comment 22 2011-10-11 23:21:49 PDT
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 23 2011-10-12 15:50:25 PDT
Comment on attachment 107954 [details] Patch Clearing flags on attachment: 107954 Committed r97314: <http://trac.webkit.org/changeset/97314>
WebKit Review Bot
Comment 24 2011-10-12 15:50:30 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.