Bug 217980 – Merge WorkerScriptController and WorkletScriptController into WorkerOrWorkletScriptController

• Component: Web Audio
• Status: RESOLVED FIXED

[Chris Dumez]

Merge WorkerScriptController and WorkletScriptController into WorkerOrWorkletScriptController, to share more code.

[Chris Dumez]

Created attachment 411903 [details]
Patch

[Chris Dumez]

Created attachment 411905 [details]
Patch

[Darin Adler]

Comment on attachment 411905 [details]
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?

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 411905 [details]
> 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?

Will add "Copy" suffix.

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

I am actually not familiar with this code. However, looking at it, it seems that calling clear() on the DOMGuardedObjects is not a complex operation and from my glancing at it, I do not believe it can cause objects to get added to the list. Pretty much all clear() does it grab a lock and remove the object from the list.

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

Correct, I merely moved it.

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

Ok, will do.

[Chris Dumez]

Created attachment 411925 [details]
Patch

[Chris Dumez]

Created attachment 411927 [details]
Patch

[Chris Dumez]

Created attachment 411928 [details]
Patch

[EWS]

Committed r268775: <https://trac.webkit.org/changeset/268775>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411928 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70508220>