Bug 121357 – Get rid of ref-counting on RenderWidget.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Andreas Kling]

Let's see if we can do this.

[Andreas Kling]

Created attachment 211648 [details]
Hack for EWS

[Andreas Kling]

Created attachment 211649 [details]
Hack for EWS

[Andreas Kling]

Created attachment 211684 [details]
Patch

[Darin Adler]

Comment on attachment 211684 [details]
Patch

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-grant.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.

[Andreas Kling]

Committed r155796: <http://trac.webkit.org/changeset/155796>