Introduce CSSParserMode in all classes and get rid of the boolean 'strictParsing'.
Created attachment 134173 [details] Patch Patch. Note: I get an alphabetic sorting problems. Seems to be a false positive.
Attachment 134173 [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/html/HTMLLinkElement.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 134173 [details] Patch Attachment 134173 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12142819
Comment on attachment 134173 [details] Patch Attachment 134173 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12145831
Comment on attachment 134173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134173&action=review You need to update COMPILE_ASSERT(sizeof(StylePropertySet) == sizeof(SameSizeAsStylePropertySet), style_property_set_should_stay_small); as you modified the members. It will grow the size of the object though. > Source/WebCore/css/MediaList.cpp:110 > + CSSParser parser; Globally I liked the fact that when I see the old code I would read : "The parser is in strict mode and will not use some other mode from somewhere else". So I would like better CSSParser parser(CSSStrictMode) and not the default value that you get from the constructor automatically. That said it's just my personal taste.
(In reply to comment #5) > (From update of attachment 134173 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134173&action=review > > You need to update COMPILE_ASSERT(sizeof(StylePropertySet) == sizeof(SameSizeAsStylePropertySet), style_property_set_should_stay_small); as you modified the members. It will grow the size of the object though. Can you be more explicit please? Where should I update it and why? In order to get Qt build? > > > Source/WebCore/css/MediaList.cpp:110 > > + CSSParser parser; > > Globally I liked the fact that when I see the old code I would read : "The parser is in strict mode and will not use some other mode from somewhere else". So I would like better CSSParser parser(CSSStrictMode) and not the default value that you get from the constructor automatically. That said it's just my personal taste.
(In reply to comment #1) > Note: I get an alphabetic sorting problems. Seems to be a false positive. Not a false positive. CSS sorts before Cached because we use case sensitive sorting.
Comment on attachment 134173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134173&action=review > Source/WebCore/css/StylePropertySet.h:138 > + CSSParserMode m_cssParserMode; You replace here one bolean represented with bitfield by a an enum represented by an int the class gets bigger. That's what the compile check is enforcing.
Comment on attachment 134173 [details] Patch Attachment 134173 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12142835
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 134173 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=134173&action=review > > > > You need to update COMPILE_ASSERT(sizeof(StylePropertySet) == sizeof(SameSizeAsStylePropertySet), style_property_set_should_stay_small); as you modified the members. It will grow the size of the object though. > Can you be more explicit please? Where should I update it and why? In order to get Qt build? It's not specific to Qt the compile check is run on all platforms. > > > > > > Source/WebCore/css/MediaList.cpp:110 > > > + CSSParser parser; > > > > Globally I liked the fact that when I see the old code I would read : "The parser is in strict mode and will not use some other mode from somewhere else". So I would like better CSSParser parser(CSSStrictMode) and not the default value that you get from the constructor automatically. That said it's just my personal taste.
Comment on attachment 134173 [details] Patch Attachment 134173 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12141835
Comment on attachment 134173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134173&action=review > Source/WebCore/css/CSSStyleSheet.h:152 > + CSSParserMode m_cssParserMode; This will make MSVC sad. It also is larger than 1 bit. It's in fact, 32 bits. MSVC uses signed enums, GCC makes them unsigned. WE have to store them as unsigned m_foo : 1; instead. :(
Created attachment 134217 [details] Patch Patch. Potential fix of chromium bot.
Comment on attachment 134217 [details] Patch Attachment 134217 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12140951
Comment on attachment 134217 [details] Patch Attachment 134217 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12143975
Comment on attachment 134217 [details] Patch Attachment 134217 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12141962
Created attachment 134294 [details] Patch Patch. Not for review. Test if it fixes Qt
Created attachment 134312 [details] Patch Patch. Not for review. Test if it fixes Qt
(In reply to comment #5) > You need to update COMPILE_ASSERT(sizeof(StylePropertySet) == sizeof(SameSizeAsStylePropertySet), style_property_set_should_stay_small); as you modified the members. It will grow the size of the object though. No. You need to ensure that the type doesn't grow.
(In reply to comment #19) > (In reply to comment #5) > > You need to update COMPILE_ASSERT(sizeof(StylePropertySet) == sizeof(SameSizeAsStylePropertySet), style_property_set_should_stay_small); as you modified the members. It will grow the size of the object though. > > No. You need to ensure that the type doesn't grow. Yes maybe I was not clear with the "It will grow the size of the object though" which I was almost sure Andreas and Antti will complain about.
(In reply to comment #19) > (In reply to comment #5) > > You need to update COMPILE_ASSERT(sizeof(StylePropertySet) == sizeof(SameSizeAsStylePropertySet), style_property_set_should_stay_small); as you modified the members. It will grow the size of the object though. > > No. You need to ensure that the type doesn't grow. Yes, the last patch tries to do that. I just have to merge it with TOT. After that it will hopefully work.
Created attachment 134317 [details] Patch Patch. Not for review. Test if it fixes Qt
Attachment 134317 [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:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 134321 [details] Patch Patch. Not for review. Test if it fixes Qt 2.
Attachment 134321 [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:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 134333 [details] Patch Patch. Not for review. Test if it fixes Qt 3.
Created attachment 134334 [details] Patch Patch. Not for review. Test if it fixes Qt 3.
Attachment 134334 [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:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 134353 [details] Patch
Comment on attachment 134353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134353&action=review > Source/WebCore/css/CSSImportRule.cpp:72 > + bool strict = cssParserMode == CSSStrictMode; It seems like this boolean is redundant. You can move this condition to enforceMIMEType. > Source/WebCore/css/CSSParser.cpp:329 > +static inline bool isStrictParserMode(CSSParserMode cssParserMode) > +{ > + // FIXME: SVG presnetation attribute values should be parsed in strict mode as well. > + return cssParserMode == CSSStrictMode; > +} It seems like this function should be next to where CSSParserMode is defined. > Source/WebCore/css/CSSParserMode.h:34 > -enum CSSParserMode { > - CSSQuirksMode, > +enum { > + CSSQuirksMode = 1, This doesn't look right. Why does CSSQuirksMode need to be 1? Also why do we need to convert CSSParserMode to an unsigned char? That should be a trick only used wherever bit fields are defined. If we use unsigned char everywhere, then compiler won't warn us when we add new values and forgot to add it to a switch statement. r- because of this. > Source/WebCore/css/CSSParserMode.h:49 > +inline CSSParserMode inCSSParserMode(CSSParserMode cssParserMode) > +{ > + return static_cast<CSSParserMode>(cssParserMode); > +} This function should be unnecessary.
Created attachment 134450 [details] Patch
Comment on attachment 134450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134450&action=review > Source/WebCore/css/CSSStyleSheet.h:155 > - bool m_strictParsing : 1; > + CSSParserMode m_cssParserMode; Oops, this is going to increase the size of he class :( You should change the type of all bitfield member variables including this one to unsigned. But given that we shouldn't have so many StyleSheet objects per page, it can be done in a follow up patch. We should also add a compile assert :) > Source/WebCore/css/StylePropertySet.cpp:50 > + : m_hasCSSOMWrapper(false) > { > + setCSSParserMode(cssParserMode); I think it makes more sense to set the value in the initialization list. > Source/WebCore/css/StylePropertySet.cpp:57 > + setCSSParserMode(CSSStrictMode); Ditto. > Source/WebCore/css/StylePropertySet.cpp:64 > + : m_hasCSSOMWrapper(false) > { > + setCSSParserMode(cssParserMode); Ditto. > Source/WebCore/css/StylePropertySet.h:90 > + void setCSSParserMode(CSSParserMode cssParserMode) { m_cssParserMode = static_cast<unsigned char>(cssParserMode); } I don't think you need an explicit cast here. > Source/WebCore/dom/Document.cpp:2689 > - m_pageUserSheet->parseString(userSheetText, !inQuirksMode()); > + m_pageUserSheet->parseString(userSheetText, toCSSParserMode(!inQuirksMode())); Maybe it's better to call it strictToCSSParserMode to clarify the fact the boolean must be true iff it's in strict mode?
Comment on attachment 134450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134450&action=review > Source/WebKit/chromium/src/WebDocument.cpp:183 > - parsedSheet->parseString(sourceCode, !document->inQuirksMode()); > + parsedSheet->parseString(sourceCode, toCSSParserMode(!document->inQuirksMode())); Maybe we want document->cssParserMode() ?
Committed r112537: <http://trac.webkit.org/changeset/112537>
(In reply to comment #32) > (From update of attachment 134450 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134450&action=review > > > Source/WebCore/css/CSSStyleSheet.h:155 > > - bool m_strictParsing : 1; > > + CSSParserMode m_cssParserMode; > > Oops, this is going to increase the size of he class :( > You should change the type of all bitfield member variables including this one to unsigned. > But given that we shouldn't have so many StyleSheet objects per page, it can be done in a follow up patch. > We should also add a compile assert :) I work on that on a different bug report: bug 82759.