[Webkit-unassigned] [Bug 25703] Stack overflow crash rendering element with mega-huge number of background layers
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jun 2 06:28:42 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=25703
--- Comment #8 from hans at chromium.org 2010-06-02 06:28:41 PST ---
(In reply to comment #7)
> (From update of attachment 57333 [details])
Thanks for the review.
> WebKit uses 4 space indent.
Done. Sorry about that.
>
> 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.
Added. Are these mandatory, or is it more for things that really need commenting, such as the magic number 8 below?
>
> > + * 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".)
Added comment in the ChangeLog about this.
>
>
> > + 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]"
Done.
>
>
> > 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).
Done.
> The * is in the wrong place (should be "FillLayer* layer").
Done.
>
> Doesn't ~FillLayer have the same problem?
Yes it does; thanks for spotting it. I also found that the FillLayer copy constructor, equality operator and containsImage() share the problem. Those are now fixed, but I have to say that it takes away quite a bit of the elegance that the recursive versions had. Suggestions for nicer ways to implement it are very welcome.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list