We currently have a special StylePropertySet constructor that deals with eliminating duplicate properties (with !important taken into account.) This logic should really live in the parser. And let's make it more efficient while we're at it.
Created attachment 142881 [details] Proposed patch Note: The PrimitiveSet class could be generalized into a fabulous container class if we figure out some great use-case for it.
Comment on attachment 142881 [details] Proposed patch Attachment 142881 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12739027 New failing tests: inspector/styles/styles-new-API.html
Created attachment 142882 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 142881 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=142881&action=review Nice, except... > Source/WebCore/css/CSSParser.cpp:1113 > +template<unsigned size> > +class PrimitiveSet { I don't quite understand the purpose of this type. Can't it be replaced with a BitVector of size numCSSProperties? The only benefit I see is that you avoid zeroing the memory. However zeroing 38 bytes is not that costly. With this the lookup and setting is slower (two reads/writes). Valgrind and pals are also likely to go crazy.
Cool, I think this will make the variables patch cleaner too by moving the variable de-duping logic into the parser.
(In reply to comment #4) > (From update of attachment 142881 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142881&action=review > > Nice, except... > > > Source/WebCore/css/CSSParser.cpp:1113 > > +template<unsigned size> > > +class PrimitiveSet { > > I don't quite understand the purpose of this type. Can't it be replaced with a BitVector of size numCSSProperties? The only benefit I see is that you avoid zeroing the memory. However zeroing 38 bytes is not that costly. With this the lookup and setting is slower (two reads/writes). Valgrind and pals are also likely to go crazy. Yeah, I kinda got tunnel-visioned on finding a use for this rad set implementation trick.. Will replace it by BitVectors.
Hmm, BitVector could really use templated inline capacity. Otherwise it is probably better to do without to avoid the mallocs.
Created attachment 143040 [details] Patch v2 Same thing with a fixed-size bit array instead. Again, this could be generalized for WTF but I'm not sure that's useful just yet.
Attachment 143040 [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/css/CSSParser.cpp:1114: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 143040 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=143040&action=review > Source/WebCore/css/CSSParser.cpp:1113 > +template<unsigned fixedSize> > +class FixedBitArray I would use it if it was in WTF. >> Source/WebCore/css/CSSParser.cpp:1114 >> +{ > > This { should be at the end of the previous line [whitespace/braces] [4] By Toutatis, the bot is right! > Source/WebCore/css/CSSParser.cpp:1121 > + void quickSet(unsigned index) The quickFoo naming makes little sense with this type as there are no slow versions. For compatibility I guess? > Source/WebCore/css/CSSParser.cpp:1124 > + m_data[index / 8] |= 1 << (index & 7); It would look more consistent if you used >> instead of / (here and elsewhere).
Comment on attachment 143040 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=143040&action=review > Source/WebCore/css/CSSParser.cpp:1116 > + FixedBitArray() "Array" implies "Fixed"
Created attachment 143060 [details] Landalicious patch
Comment on attachment 143060 [details] Landalicious patch Clearing flags on attachment: 143060 Committed r117809: <http://trac.webkit.org/changeset/117809>
All reviewed patches have been landed. Closing bug.