[webkit-reviews] review denied: [Bug 25703] Stack overflow crash rendering element with mega-huge number of background layers : [Attachment 59116] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Hans Wennborg <hans at chromium.org>'s
request for review:
Bug 25703: Stack overflow crash rendering element with mega-huge number of
background layers
https://bugs.webkit.org/show_bug.cgi?id=25703

Attachment 59116: Patch
https://bugs.webkit.org/attachment.cgi?id=59116&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=59116&action=review

R- for readability.

> WebCore/rendering/RenderBox.cpp:761
> +    Vector<const FillLayer*, 8> layers; // Situations with more than 8
layers are extremely rare.

Do we have data to support this claim?

> WebCore/rendering/style/FillLayer.cpp:53
> +FillLayer::FillLayer(const FillLayer& o, bool shallowCopy)

Please don't use "o" as a variable name.  It looks too much like 0.

> WebCore/rendering/style/FillLayer.cpp:81
> +    const FillLayer* otherLayer = &o;

Perhaps "otherLayer" is a better name for o.

> WebCore/rendering/style/FillLayer.cpp:84
> +	   thisLayer->m_next = new FillLayer(*otherLayer->m_next, true);

passing explicit true/false is hard to read.  :(

> WebCore/rendering/style/FillLayer.cpp:96
> +	   delete layer;

Perhaps this should be an OwnPtr ?  Manual new/delete is bad times.