Bug 99239 – NamedFlowCollection should be a ContextDestructionObserver

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[WebKit Review Bot]

NamedFlowCollection should be a ContextDestructionObserver
Requested by abarth on #webkit.

[Hanyee Kim]

Created attachment 187076 [details]
Patch

[Mihnea Ovidenie]

I think an explanation about why this change is needed would be helpful.

[Adam Barth]

Comment on attachment 187076 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187076&action=review

> Source/WebCore/dom/NamedFlowCollection.cpp:43
>  NamedFlowCollection::NamedFlowCollection(Document* doc)

doc -> document

> Source/WebCore/dom/NamedFlowCollection.cpp:45
> +    , m_document(doc)

You don't need the m_document variable anymore.  ContextDestructionObserver already keep that information for you.

> Source/WebCore/dom/NamedFlowCollection.cpp:107
> -void NamedFlowCollection::documentDestroyed()
> +void NamedFlowCollection::contextDestroyed()
>  {
>      m_document = 0;
>  }

Once you remove m_document, you can delete this function.

[Adam Barth]

> I think an explanation about why this change is needed would be helpful.

This class just re-implements ContextDestructionObserver manually.  Instead of re-implementing ContextDestructionObserver, we can just subclass it.

[Adam Barth]

We're trying to remove as many of the raw Document* member variables as we can from WebCore because those raw pointers often lead to memory corruption.  This case doesn't cause memory corruption because NamedFlowCollection clears the pointer when the Document is destroyed.  However, it's easier to audit these cases if we let ContextDestructionObserver do that work automatically.

[Hanyee Kim]

Your comment is helpful to understand about ContextDestructionContext perfectly.
Thanks for reviewing!!
I will upload new patch.

[Hanyee Kim]

Created attachment 187226 [details]
Patch

[Adam Barth]

Comment on attachment 187226 [details]
Patch

looks great

[WebKit Review Bot]

Comment on attachment 187226 [details]
Patch

Clearing flags on attachment: 187226

Committed r142223: <http://trac.webkit.org/changeset/142223>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.