[webkit-reviews] review denied: [Bug 99241] StyleSheetList should be a ContextDestructionObserver : [Attachment 188457] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 19 09:35:02 PDT 2014

Darin Adler <darin at apple.com> has denied Hanyee Kim <choco at company100.net>'s
request for review:
Bug 99241: StyleSheetList should be a ContextDestructionObserver

Attachment 188457: Patch

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

Seems OK, but I’m not sure this refactoring is sufficient improvement to be
worth taking the change. If we wanted to take the full benefit from this, I
think we’d want to remove the detachFromDocument function too. Also, I think
we’d probably want to inline the document() function.

> Source/WebCore/ChangeLog:11
> +	   This patch removes the raw pointer of Document in StyleSheetList. It

> +	   could be replaced with ContextDestructionObserver.
> +	   ContextDestructionObserver has the pointer and clears the pointer
> +	   automatically when the document is destroyed.

Why? If this is a cleanup we should do more to actually clean up.

> Source/WebCore/css/StyleSheetList.cpp:53
> +    ScriptExecutionContext* context =

We should call just scriptExecutionContext here, not explicitly

> Source/WebCore/css/StyleSheetList.h:36
> +class StyleSheetList : public RefCounted<StyleSheetList>, public
ContextDestructionObserver {

I think we’d want to use private inheritance here, not public.

> Source/WebCore/css/StyleSheetList.h:39
>      ~StyleSheetList();

We’d want to mark this virtual since it now will be implicitly virtual.

More information about the webkit-reviews mailing list