[webkit-reviews] review granted: [Bug 121357] Get rid of ref-counting on RenderWidget. : [Attachment 211684] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <akling at apple.com>'s
request for review:
Bug 121357: Get rid of ref-counting on RenderWidget.
https://bugs.webkit.org/show_bug.cgi?id=121357

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=211684&action=review


> Source/WTF/wtf/WeakPtr.h:103
> +    bool operator!() const { return !m_ref; }

Agent "!" (He comes as no surprise)
<http://www.againwiththecomics.com/2010/03/best-comics-ever-doom-patrol-by-gran
t.html>

> Source/WebCore/page/FrameView.cpp:-131
> -// The maximum number of updateEmbeddedObjects iterations that should be
done before returning.
> -static const unsigned maxUpdateEmbeddedObjectsIterations = 2;

I think the only reason you were able to remove this is that now we are willing
to infinite loop!

> Source/WebCore/page/FrameView.cpp:2689
> +    // CAUTION: It's possible the renderer was destroyed again, since
loading a plugin
> +    //	   may execute arbitrary JavaScript.

Not a fan of this kind of indenting. Just do it all on one line!

Not sure CAUTION is an improvement. Not sure what I need to be careful about.

What does "destroyed again" mean?

> Source/WebCore/page/FrameView.cpp:2703
> +    while (!m_embeddedObjectsToUpdate->isEmpty()) {

This idiom can result in an infinite loop where we keep getting an object from
the set and it keeps re-adding itself; not sure if it's possible in practice,
but in theory I think it is. Unless you did something to stop it. Is there any
way to protect ourselves from this? Maybe the "use ListHashSet and add a null
to mark where to stop" technique used in DocumentEventQueue?

> Source/WebCore/page/FrameView.cpp:2707
> +	   auto it = m_embeddedObjectsToUpdate->begin();
> +	   RenderEmbeddedObject* embeddedObject = *it;
> +	   m_embeddedObjectsToUpdate->remove(it);
> +	   updateEmbeddedObject(*embeddedObject);

We need a HashSet member function that does this, maybe called takeAny, since
it takes the first thing from the set. If we had it, it would be a one liner
here:

    updateEmbeddedObject(*m_embeddedObjectsToUpdate->takeAny());

> Source/WebCore/page/FrameView.cpp:4253
> +static Vector<RefPtr<Widget>> collectWidgets(const HashSet<Widget*>& set)

Name should make it clear it protects and refs them! Or “collects and
protects”.

> Source/WebCore/page/FrameView.cpp:4267
> +    for (auto it = protectedWidgets.begin(), end = protectedWidgets.end();
it != end; ++it) {

This is vector. Should just iterate like this:

    for (unsigned i = 0, size = protectedWidgets.size(); i < size; ++i)

> Source/WebCore/page/FrameView.cpp:4279
> +	   Widget& widget = **it;
> +	   widget.notifyWidget(notification);

Why the local variable?

> Source/WebCore/page/FrameView.h:440
> +    void updateWidgetPositions();

Why public?

> Source/WebCore/rendering/RenderObject.h:945
> +    void destroy();

Maybe even inline at the call site?

> Source/WebCore/rendering/RenderWidget.cpp:93
> +    , m_weakFactory(this)

Should call it m_weakPtrFactory. I even thought the class was named WeakFactory
when I wrote this comment, but I checked, and it’s named that way too.

> Source/WebCore/rendering/RenderWidget.cpp:141
> +    // CAUTION: This call *may* cause this renderer to disappear from
underneath...

I don’t think CAUTION helps. Otherwise, I love this comment.

> Source/WebCore/rendering/RenderWidget.cpp:149
> +	   // CAUTION: This call *may* cause this renderer to disappear from
underneath...

I don’t think CAUTION helps. Otherwise, I love this comment.

> Source/WebCore/rendering/RenderWidget.cpp:193
> +	   widgetRendererMap().set(m_widget.get(), this);

Why set instead of add? It's true that set will overwrite the old value if
there was one, but add is more efficient if add is what you need. Is there a
real case where we need the set behavior here?

> Source/WebCore/rendering/RenderWidget.cpp:345
> +    WeakPtr<RenderWidget> weakThis(createWeakPtr());

I think I like the = syntax better than the () syntax in cases like this.