[webkit-reviews] review denied: [Bug 43716] Migrating m_continuation to a HashMap : [Attachment 63892] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kenichi Ishibashi <bashi at google.com>'s
request for review:
Bug 43716: Migrating m_continuation to a HashMap
https://bugs.webkit.org/show_bug.cgi?id=43716

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +	RenderBoxModelObject* cont = continuation();
> +	if (cont) {
> +	    cont->destroy();
> +	    setContinuation(0);
> +	}

Please don't abbreviate continuation to "cont"; abbreviations are harder to
read and recognize than words, and can also be confusing and ambiguous. We
typically use this style in cases like this:

    if (RenderBoxModelObject* continuation = this->continuation()) {
	continuation->destroy();
	setContinuation(0);
    }

> +// The HashMap for storing continuation pointers.
> +// An inline can be split with blocks occuring in between the inline
content.
> +// When this occurs we need a pointer to the next object. We can basically
be
> +// split into a sequence of inlines and blocks. The continuation will either
be
> +// an anonymous block (that houses other blocks) or it will be an inline
flow.
> +// <b><i><p>Hello</p></i></b>. In this example the <i> will have a block as
> +// its continuation but the <b> will just have an inline as its
continuation.
> +typedef HashMap<const RenderBoxModelObject*, RenderBoxModelObject*>
ContinuationMap;
> +static ContinuationMap* gContinuationMap = 0;

I presume that the motivation for moving the continuation pointer into a map is
to save memory. Do you have any data on the memory and performance impact of
this change?

> --- a/WebCore/rendering/RenderInline.h
> +++ b/WebCore/rendering/RenderInline.h
> @@ -59,8 +59,8 @@ public:
>      InlineFlowBox* firstLineBox() const { return m_lineBoxes.firstLineBox();
}
>      InlineFlowBox* lastLineBox() const { return m_lineBoxes.lastLineBox(); }

>  
> -    RenderBoxModelObject* continuation() const { return m_continuation; }
> -    void setContinuation(RenderBoxModelObject* c) { m_continuation = c; }
> +    RenderBoxModelObject* continuation() const { return
RenderBoxModelObject::continuation(); }
> +    void setContinuation(RenderBoxModelObject* c) {
RenderBoxModelObject::setContinuation(c); }

Instead of forwarding functions, you can use the "using" statement:

    using RenderBoxModelObject::continuation;
    using RenderBoxModelObject::setContinuation;

I’d like to see a new patch that eliminates the abbreviations and is
accompanied by some performance data to show this is a good change.

I’m also not entirely sure that having this be a protected function in
RenderBoxModelObject that then is made public in both RenderInline and
RenderBlock is a helpful design. We may simply want it to be public in
RenderBoxModelObject instead. Hyatt can weigh in on that.