Bug 82335 - Introduce CSSParserMode in all classes
Summary: Introduce CSSParserMode in all classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on: 82056
Blocks: 82759 46112
  Show dependency treegraph
 
Reported: 2012-03-27 07:01 PDT by Dirk Schulze
Modified: 2012-03-30 11:20 PDT (History)
7 users (show)

See Also:


Attachments
Patch (43.12 KB, patch)
2012-03-27 17:02 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (44.41 KB, patch)
2012-03-27 21:52 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (44.57 KB, patch)
2012-03-28 07:46 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (44.72 KB, patch)
2012-03-28 09:22 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (44.75 KB, patch)
2012-03-28 09:46 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (44.88 KB, patch)
2012-03-28 10:04 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (44.88 KB, patch)
2012-03-28 10:43 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (44.75 KB, patch)
2012-03-28 10:44 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (45.93 KB, patch)
2012-03-28 11:39 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (46.38 KB, patch)
2012-03-28 17:06 PDT, Dirk Schulze
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2012-03-27 07:01:43 PDT
Introduce CSSParserMode in all classes and get rid of the boolean 'strictParsing'.
Comment 1 Dirk Schulze 2012-03-27 17:02:08 PDT
Created attachment 134173 [details]
Patch

Patch. Note: I get an alphabetic sorting problems. Seems to be a false positive.
Comment 2 WebKit Review Bot 2012-03-27 17:06:25 PDT
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 3 Early Warning System Bot 2012-03-27 17:17:48 PDT
Comment on attachment 134173 [details]
Patch

Attachment 134173 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12142819
Comment 4 Early Warning System Bot 2012-03-27 17:21:05 PDT
Comment on attachment 134173 [details]
Patch

Attachment 134173 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12145831
Comment 5 Alexis Menard (darktears) 2012-03-27 17:25:38 PDT
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.
Comment 6 Dirk Schulze 2012-03-27 17:33:06 PDT
(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.
Comment 7 Darin Adler 2012-03-27 17:37:39 PDT
(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 8 Alexis Menard (darktears) 2012-03-27 17:38:38 PDT
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 9 Build Bot 2012-03-27 17:47:55 PDT
Comment on attachment 134173 [details]
Patch

Attachment 134173 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12142835
Comment 10 Alexis Menard (darktears) 2012-03-27 17:49:20 PDT
(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 11 WebKit Review Bot 2012-03-27 18:15:40 PDT
Comment on attachment 134173 [details]
Patch

Attachment 134173 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12141835
Comment 12 Eric Seidel (no email) 2012-03-27 20:56:23 PDT
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. :(
Comment 13 Dirk Schulze 2012-03-27 21:52:50 PDT
Created attachment 134217 [details]
Patch

Patch. Potential fix of chromium bot.
Comment 14 Early Warning System Bot 2012-03-27 22:06:44 PDT
Comment on attachment 134217 [details]
Patch

Attachment 134217 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12140951
Comment 15 Early Warning System Bot 2012-03-27 22:06:51 PDT
Comment on attachment 134217 [details]
Patch

Attachment 134217 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12143975
Comment 16 Build Bot 2012-03-28 00:10:13 PDT
Comment on attachment 134217 [details]
Patch

Attachment 134217 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12141962
Comment 17 Dirk Schulze 2012-03-28 07:46:43 PDT
Created attachment 134294 [details]
Patch

Patch. Not for review. Test if it fixes Qt
Comment 18 Dirk Schulze 2012-03-28 09:22:35 PDT
Created attachment 134312 [details]
Patch

Patch. Not for review. Test if it fixes Qt
Comment 19 Antti Koivisto 2012-03-28 09:27:12 PDT
(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.
Comment 20 Alexis Menard (darktears) 2012-03-28 09:29:25 PDT
(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.
Comment 21 Dirk Schulze 2012-03-28 09:44:01 PDT
(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.
Comment 22 Dirk Schulze 2012-03-28 09:46:04 PDT
Created attachment 134317 [details]
Patch

Patch. Not for review. Test if it fixes Qt
Comment 23 WebKit Review Bot 2012-03-28 09:49:22 PDT
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.
Comment 24 Dirk Schulze 2012-03-28 10:04:58 PDT
Created attachment 134321 [details]
Patch

Patch. Not for review. Test if it fixes Qt 2.
Comment 25 WebKit Review Bot 2012-03-28 10:10:06 PDT
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.
Comment 26 Dirk Schulze 2012-03-28 10:43:08 PDT
Created attachment 134333 [details]
Patch

Patch. Not for review. Test if it fixes Qt 3.
Comment 27 Dirk Schulze 2012-03-28 10:44:31 PDT
Created attachment 134334 [details]
Patch

Patch. Not for review. Test if it fixes Qt 3.
Comment 28 WebKit Review Bot 2012-03-28 10:50:12 PDT
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.
Comment 29 Dirk Schulze 2012-03-28 11:39:26 PDT
Created attachment 134353 [details]
Patch
Comment 30 Ryosuke Niwa 2012-03-28 16:08:24 PDT
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.
Comment 31 Dirk Schulze 2012-03-28 17:06:17 PDT
Created attachment 134450 [details]
Patch
Comment 32 Ryosuke Niwa 2012-03-28 21:56:45 PDT
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 33 Ryosuke Niwa 2012-03-28 21:57:25 PDT
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() ?
Comment 34 Dirk Schulze 2012-03-29 09:13:38 PDT
Committed r112537: <http://trac.webkit.org/changeset/112537>
Comment 35 Dirk Schulze 2012-03-30 11:20:25 PDT
(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.