[webkit-reviews] review granted: [Bug 217980] Merge WorkerScriptController and WorkletScriptController into WorkerOrWorkletScriptController : [Attachment 411905] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 217980: Merge WorkerScriptController and WorkletScriptController into
WorkerOrWorkletScriptController
https://bugs.webkit.org/show_bug.cgi?id=217980

Attachment 411905: Patch

https://bugs.webkit.org/attachment.cgi?id=411905&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 411905
  --> https://bugs.webkit.org/attachment.cgi?id=411905
Patch

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

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:299
> +    auto guardedObjects = m_guardedObjects;

We are copying this so it does the right thing even if some objects are removed
while we iterate. Three questions:

1) Can we make clearer that this is a copy, perhaps by including copy in the
name of the local variable or adding a comment?
2) Is it OK to call clear if this object is removed and then perhaps reused in
some way?
3) Is it OK that this does not clear objects that are added during the
iteration?

Looks like this code was just moved, so none of this is new.

> Source/WebCore/bindings/js/JSDOMGlobalObject.h:87
> +    JSC::JSProxy* proxy() const { ASSERT(m_proxy); return m_proxy.get(); }

If we can assert this, it seems we should return a reference, not a pointer.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:253
>  JSWindowProxy* JSDOMWindowBase::proxy() const

Reference?