[webkit-reviews] review denied: [Bug 25703] Stack overflow crash rendering element with mega-huge number of background layers : [Attachment 57333] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 1 10:22:15 PDT 2010
David Levin <levin at chromium.org> has denied 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 57333: Patch
https://bugs.webkit.org/attachment.cgi?id=57333&action=review
------- Additional Comments from David Levin <levin at chromium.org>
WebKit uses 4 space indent.
Note when I made comments below, I often only pointed out one instance of the
issue but it may occur in several places, so please check your code throughout
for the same issue.
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> + Test: css3/many-background-layers.html
> +
> + * rendering/RenderBox.cpp:
> + (WebCore::RenderBox::paintFillLayers):
Note that this is where per function comments go.
> + * rendering/style/FillLayer.h:
> + (WebCore::FillLayer::hasImage):
> + (WebCore::FillLayer::hasFixedImage):
> +
> 2010-05-27 Kwang Yul Seo <skyul at company100.net>
>
> Reviewed by Darin Adler.
> diff --git a/WebCore/rendering/RenderBox.cpp
b/WebCore/rendering/RenderBox.cpp
> void RenderBox::paintFillLayers(const PaintInfo& paintInfo, const Color& c,
const FillLayer* fillLayer, int tx, int ty, int width, int height,
CompositeOperator op, RenderObject* backgroundObject)
> {
> - if (!fillLayer)
> - return;
> + WTF::Vector<const FillLayer*, 8> layers;
Why 8? (Ideally the ChangeLog would have a comment about this function and the
magic number "8".)
> + for (size_t i = layers.size(); i > 0; --i)
> + paintFillLayer(paintInfo, c, layers[i-1], tx, ty, width, height, op,
backgroundObject);
There should be spaces around operators: "layers[i - 1]"
> diff --git a/WebCore/rendering/style/FillLayer.h
b/WebCore/rendering/style/FillLayer.h
> bool hasImage() const
> {
> - if (m_image)
> + for (const FillLayer *layer = this; layer; layer = layer->m_next)
This should have {} around the body (see
http://webkit.org/coding/coding-style.html).
The * is in the wrong place (should be "FillLayer* layer").
Doesn't ~FillLayer have the same problem?
More information about the webkit-reviews
mailing list