[webkit-reviews] review granted: [Bug 43716] Use a HashMap for m_continuation to save memory : [Attachment 66184] Patch V1
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Dec 13 10:11:47 PST 2010
Darin Adler <darin at apple.com> has granted Kenichi Ishibashi
<bashi at google.com>'s request for review:
Bug 43716: Use a HashMap for m_continuation to save memory
https://bugs.webkit.org/show_bug.cgi?id=43716
Attachment 66184: Patch V1
https://bugs.webkit.org/attachment.cgi?id=66184&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=66184&action=review
> WebCore/rendering/RenderBlock.h:135
> + RenderBoxModelObject* continuation() const { return
RenderBoxModelObject::continuation(); }
> + void setContinuation(RenderBoxModelObject* continuation) {
RenderBoxModelObject::setContinuation(continuation); }
As I said before, this should be “using” rather than a forwarding inline
function.
> WebCore/rendering/RenderBoxModelObject.cpp:63
> +static ContinuationMap* gContinuationMap = 0;
The use of the “g” prefix here is not common in WebKit code. I did a search and
found that most global variables of this type do not use this sort of prefix.
> WebCore/rendering/RenderBoxModelObject.cpp:1796
> + if (!gContinuationMap)
> + gContinuationMap = new ContinuationMap;
> +
> + if (continuation)
> + gContinuationMap->set(this, continuation);
> + else
> + gContinuationMap->remove(this);
The creation of the map could be moved inside the non-zero case, since is no
need to create the map to remove something from it. Probably OK the way it is,
though, since we’d still need a null check of gContinuationMap.
> WebCore/rendering/RenderInline.h:63
> + RenderBoxModelObject* continuation() const { return
RenderBoxModelObject::continuation(); }
> + void setContinuation(RenderBoxModelObject* continuation) {
RenderBoxModelObject::setContinuation(continuation); }
Again, I think “using” is the way to go.
More information about the webkit-reviews
mailing list