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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=99241

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

------- 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 =
ContextDestructionObserver::scriptExecutionContext();

We should call just scriptExecutionContext here, not explicitly
ContextDestructionObserver::scriptExecutionContext.

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