Bug 124056 – Make RenderBlockRareData into a hash.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

Make RenderBlockRareData into a hash to get those 4 bytes back.

[Dave Hyatt]

Created attachment 216413 [details]
Patch to move into map (not ready for review)

[Build Bot]

Comment on attachment 216413 [details]
Patch to move into map (not ready for review)

Attachment 216413 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22738544

[Build Bot]

Comment on attachment 216413 [details]
Patch to move into map (not ready for review)

Attachment 216413 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22778562

New failing tests:
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-circle-001.html
fast/shapes/shape-inside/shape-inside-bottom-edge.html
fast/shapes/parsing/parsing-shape-lengths.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-000.html
fast/shapes/shape-inside/shape-inside-calc-crash.html
fast/regions/shape-inside/shape-inside-on-additional-regions.html
fast/shapes/shape-outside-floats/shape-outside-dynamic-shape-image-threshold.html
fast/shapes/shape-inside/floats/shape-inside-left-float-in-lower-left-triangle-block-content.html
http/tests/css/css-image-valued-shape.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-002.html
http/tests/security/shape-image-cors.html
fast/shapes/shape-inside/floats/shape-inside-left-float-in-lower-right-triangle-inline-content.html
fast/shapes/parsing/parsing-shape-outside.html
fast/regions/shape-inside/shape-inside-on-multiple-autoheight-regions.html
fast/regions/shape-inside/shape-inside-on-first-region-inline-content.html
fast/shapes/shape-inside/floats/shape-inside-left-float-in-lower-left-triangle-inline-content.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-004.html
fast/shapes/shape-outside-floats/shape-outside-animation.html
fast/shapes/shape-outside-floats/shape-outside-dynamic-shape-margin.html
fast/shapes/shape-inside/floats/shape-inside-left-float-in-lower-right-triangle-block-content.html
fast/shapes/parsing/parsing-shape-inside.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-005.html
fast/shapes/shape-inside/shape-inside-box-sizing.html
fast/shapes/shape-inside/shape-inside-animation.html
fast/regions/shape-inside/shape-inside-on-first-region-block-content.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-006.html
fast/shapes/css-shapes-enabled.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-003.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-circle-000.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-001.html

[Build Bot]

Created attachment 216455 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Dave Hyatt]

Created attachment 216587 [details]
Patch

[WebKit Commit Bot]

Attachment 216587 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h', u'Source/WebCore/rendering/RenderBlockFlow.cpp', u'Source/WebCore/rendering/RenderBlockFlow.h']" exit_code: 1
Source/WebCore/rendering/RenderBlockFlow.h:525:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 216587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216587&action=review

> Source/WebCore/rendering/RenderBlock.cpp:129
> +typedef WTF::HashMap<const RenderBlock*, OwnPtr<RenderBlockRareData>> RenderBlockRareDataMap;

I would remove the WTF:: from HashMap (its not needed) and make it a std::unique_ptr<RenderBlockRareData>.

[Dave Hyatt]

Created attachment 216591 [details]
Patch

[Anders Carlsson]

Comment on attachment 216591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216591&action=review

> Source/WebCore/rendering/RenderBlock.cpp:225
> +    if (hasRareData())
> +        gRareDataMap->take(this);

I’d just do an if (g_rareDataMap) g_rareDataMap->remove(this) here to avoid two hash lookups (one for the hasRareData and another for the actual remove).

> Source/WebCore/rendering/RenderBlock.cpp:1416
> +static RenderBlockRareData* ensureRareData(const RenderBlock* block)

I think this should return a reference since it can never be null.

[Build Bot]

Comment on attachment 216591 [details]
Patch

Attachment 216591 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22908437

[Build Bot]

Comment on attachment 216591 [details]
Patch

Attachment 216591 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22868501

New failing tests:
fast/block/margin-collapse/webkit-margin-collapse-floats.html

[Build Bot]

Created attachment 216597 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 216591 [details]
Patch

Attachment 216591 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22539439

New failing tests:
js/basic-set.html
fast/block/margin-collapse/webkit-margin-collapse-floats.html

[Build Bot]

Created attachment 216602 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Dave Hyatt]

Created attachment 216681 [details]
Address the typo in RenderBlockFlow::setMustDiscardMarginBefore

Note I had made hasRareData() private in RenderBlock to avoid this mistake, but right now RenderBlockFlow is a friend of RenderBlock, so it was able to use hasRareData without it being a compile error. Annoying. Once we finish the refactoring, we'll want to make sure to remove the friend connection.

[Build Bot]

Comment on attachment 216681 [details]
Address the typo in RenderBlockFlow::setMustDiscardMarginBefore

Attachment 216681 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22719636

[Dave Hyatt]

Landed in r159150.