WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
82335
Introduce CSSParserMode in all classes
https://bugs.webkit.org/show_bug.cgi?id=82335
Summary
Introduce CSSParserMode in all classes
Dirk Schulze
Reported
2012-03-27 07:01:43 PDT
Introduce CSSParserMode in all classes and get rid of the boolean 'strictParsing'.
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
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Dirk Schulze
Comment 1
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.
WebKit Review Bot
Comment 2
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.
Early Warning System Bot
Comment 3
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
Early Warning System Bot
Comment 4
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
Alexis Menard (darktears)
Comment 5
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.
Dirk Schulze
Comment 6
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.
Darin Adler
Comment 7
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.
Alexis Menard (darktears)
Comment 8
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.
Build Bot
Comment 9
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
Alexis Menard (darktears)
Comment 10
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.
WebKit Review Bot
Comment 11
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
Eric Seidel (no email)
Comment 12
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. :(
Dirk Schulze
Comment 13
2012-03-27 21:52:50 PDT
Created
attachment 134217
[details]
Patch Patch. Potential fix of chromium bot.
Early Warning System Bot
Comment 14
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
Early Warning System Bot
Comment 15
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
Build Bot
Comment 16
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
Dirk Schulze
Comment 17
2012-03-28 07:46:43 PDT
Created
attachment 134294
[details]
Patch Patch. Not for review. Test if it fixes Qt
Dirk Schulze
Comment 18
2012-03-28 09:22:35 PDT
Created
attachment 134312
[details]
Patch Patch. Not for review. Test if it fixes Qt
Antti Koivisto
Comment 19
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.
Alexis Menard (darktears)
Comment 20
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.
Dirk Schulze
Comment 21
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.
Dirk Schulze
Comment 22
2012-03-28 09:46:04 PDT
Created
attachment 134317
[details]
Patch Patch. Not for review. Test if it fixes Qt
WebKit Review Bot
Comment 23
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.
Dirk Schulze
Comment 24
2012-03-28 10:04:58 PDT
Created
attachment 134321
[details]
Patch Patch. Not for review. Test if it fixes Qt 2.
WebKit Review Bot
Comment 25
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.
Dirk Schulze
Comment 26
2012-03-28 10:43:08 PDT
Created
attachment 134333
[details]
Patch Patch. Not for review. Test if it fixes Qt 3.
Dirk Schulze
Comment 27
2012-03-28 10:44:31 PDT
Created
attachment 134334
[details]
Patch Patch. Not for review. Test if it fixes Qt 3.
WebKit Review Bot
Comment 28
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.
Dirk Schulze
Comment 29
2012-03-28 11:39:26 PDT
Created
attachment 134353
[details]
Patch
Ryosuke Niwa
Comment 30
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.
Dirk Schulze
Comment 31
2012-03-28 17:06:17 PDT
Created
attachment 134450
[details]
Patch
Ryosuke Niwa
Comment 32
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?
Ryosuke Niwa
Comment 33
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() ?
Dirk Schulze
Comment 34
2012-03-29 09:13:38 PDT
Committed
r112537
: <
http://trac.webkit.org/changeset/112537
>
Dirk Schulze
Comment 35
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
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug