[Webkit-unassigned] [Bug 43716] Migrating m_continuation to a HashMap

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43716


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #63892|review?                     |review-
               Flag|                            |




--- Comment #6 from Darin Adler <darin at apple.com>  2010-08-29 12:19:47 PST ---
(From update of attachment 63892)
> +     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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.