Bug 82335 – Introduce CSSParserMode in all classes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dirk Schulze]

Introduce CSSParserMode in all classes and get rid of the boolean 'strictParsing'.

[Dirk Schulze]

Created attachment 134173 [details]
Patch

Patch. Note: I get an alphabetic sorting problems. Seems to be a false positive.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 134173 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 134173 [details]
Patch

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

[Alexis Menard (darktears)]

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.

[Dirk Schulze]

(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.

[Darin Adler]

(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.

[Alexis Menard (darktears)]

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.

[Build Bot]

Comment on attachment 134173 [details]
Patch

Attachment 134173 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12142835

[Alexis Menard (darktears)]

(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.

[WebKit Review Bot]

Comment on attachment 134173 [details]
Patch

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

[Eric Seidel (no email)]

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. :(

[Dirk Schulze]

Created attachment 134217 [details]
Patch

Patch. Potential fix of chromium bot.

[Early Warning System Bot]

Comment on attachment 134217 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 134217 [details]
Patch

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

[Build Bot]

Comment on attachment 134217 [details]
Patch

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

[Dirk Schulze]

Created attachment 134294 [details]
Patch

Patch. Not for review. Test if it fixes Qt

[Dirk Schulze]

Created attachment 134312 [details]
Patch

Patch. Not for review. Test if it fixes Qt

[Antti Koivisto]

(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.

[Alexis Menard (darktears)]

(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.

[Dirk Schulze]

(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.

[Dirk Schulze]

Created attachment 134317 [details]
Patch

Patch. Not for review. Test if it fixes Qt

[WebKit Review Bot]

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.

[Dirk Schulze]

Created attachment 134321 [details]
Patch

Patch. Not for review. Test if it fixes Qt 2.

[WebKit Review Bot]

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.

[Dirk Schulze]

Created attachment 134333 [details]
Patch

Patch. Not for review. Test if it fixes Qt 3.

[Dirk Schulze]

Created attachment 134334 [details]
Patch

Patch. Not for review. Test if it fixes Qt 3.

[WebKit Review Bot]

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.

[Dirk Schulze]

Created attachment 134353 [details]
Patch

[Ryosuke Niwa]

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.

[Dirk Schulze]

Created attachment 134450 [details]
Patch

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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() ?

[Dirk Schulze]

Committed r112537: <http://trac.webkit.org/changeset/112537>

[Dirk Schulze]

(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.