CSSValue: reorder ClassType enum to allow faster comparisons, add COMPILE_ASSERT on class size.
Created attachment 116162 [details] Patch
Comment on attachment 116162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116162&action=review Good idea, I'd like one more iteration though. > Source/WebCore/css/CSSValue.h:97 > + /* Primitive class types must appear before PrimitiveClass */ Coding style, we use C++ comments (//), and end them in periods. > Source/WebCore/css/CSSValue.h:103 > + /* Image generator classes */ Ditto. > Source/WebCore/css/CSSValue.h:109 > + /* Timing function classes */ Ditto. > Source/WebCore/css/CSSValue.h:114 > + /* Other class types */ Ditto. > Source/WebCore/css/CSSValue.h:139 > + /* List class types must appear after ValueListClass */ Ditto. > Source/WebCore/css/CSSValue.h:145 > + /* Do not append non-list class types here */ Ditto. > Source/WebCore/css/CSSValue.h:157 > + COMPILE_ASSERT((sizeof(CSSValue) - sizeof(RefCounted<CSSValue>)) <= 4, CSS_value_packs_into_four_bytes); This should be in CSSValue.cpp instead. > Source/WebCore/css/CSSValue.h:173 > - unsigned m_primitiveUnitType : 7; // CSSPrimitiveValue::UnitTypes > + unsigned char m_primitiveUnitType : 7; // CSSPrimitiveValue::UnitTypes I suppose you're making this change because of MSVC? At least note it in the ChangeLog as it's unrelated to the rest of the patch.
Created attachment 116285 [details] Patch
Patch addresses those issues. I've changed back from unsigned char to unsigned - since we are only trying to pack 15 bits into 32 (behavior due to extending the 32-bit RefPtr), we probably don't care too much that MSVC doesn't pack it tightly.
(In reply to comment #4) > Patch addresses those issues. I've changed back from unsigned char to unsigned - since we are only trying to pack 15 bits into 32 (behavior due to extending the 32-bit RefPtr), we probably don't care too much that MSVC doesn't pack it tightly. s/RefPtr/RefCounted
Created attachment 116317 [details] Patch
Comment on attachment 116317 [details] Patch r=me
Comment on attachment 116317 [details] Patch Rejecting attachment 116317 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: Value.o CXX(target) out/Debug/obj.target/webcore_remaining/Source/WebCore/css/CSSValue.o CXX(target) out/Debug/obj.target/webcore_remaining/Source/WebCore/css/CSSValueList.o Source/WebCore/css/CSSValue.cpp:62: error: size of array 'CSS_value_packs_into_four_bytes' is negative CXX(target) out/Debug/obj.target/webcore_remaining/Source/WebCore/css/CSSWrapShapes.o make: *** [out/Debug/obj.target/webcore_remaining/Source/WebCore/css/CSSValue.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/10645027
Created attachment 116472 [details] Patch for landing
Comment on attachment 116472 [details] Patch for landing Clearing flags on attachment: 116472 Committed r101119: <http://trac.webkit.org/changeset/101119>
All reviewed patches have been landed. Closing bug.