Bug 156901 – Drop [UsePointersEvenForNonNullableObjectArguments] from SpeechSynthesis

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Drop [UsePointersEvenForNonNullableObjectArguments] from SpeechSynthesis

[youenn fablet]

Created attachment 277030 [details]
Patch

[Darin Adler]

Comment on attachment 277030 [details]
Patch

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

> Source/WebCore/Modules/speech/SpeechSynthesis.idl:35
> -    void speak(SpeechSynthesisUtterance utterance);
> +    // FIXME: utterance should not be nullable.
> +    void speak(SpeechSynthesisUtterance? utterance);

Maybe we can change this one right now rather than leaving the FIXME. I don’t think any of the ports is currently shipping with this feature turned on, so it seems we could fix this now.

[youenn fablet]

(In reply to comment #2)
> Comment on attachment 277030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277030&action=review
> 
> > Source/WebCore/Modules/speech/SpeechSynthesis.idl:35
> > -    void speak(SpeechSynthesisUtterance utterance);
> > +    // FIXME: utterance should not be nullable.
> > +    void speak(SpeechSynthesisUtterance? utterance);
> 
> Maybe we can change this one right now rather than leaving the FIXME. I
> don’t think any of the ports is currently shipping with this feature turned
> on, so it seems we could fix this now.

Thanks for the review.
I'll update it accordingly.

[youenn fablet]

Created attachment 277233 [details]
Patch

[Darin Adler]

Comment on attachment 277233 [details]
Patch

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

> Source/WebCore/Modules/speech/SpeechSynthesis.h:54
> -    void speak(SpeechSynthesisUtterance*);
> +    void speak(Ref<SpeechSynthesisUtterance>&&);

Does anyone calling this hand over ownership of the utterance? I ask because the JavaScript language binding isn’t in a position to do that, so normally we would just use SpeechSynthesisUtterance&, unless there is a client that can move in an existing smart pointer.

I suggest changing this to a plain old reference instead of a Ref unless there is a client that is actually transferring ownership.

[Darin Adler]

Comment on attachment 277233 [details]
Patch

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

> Source/WebCore/Modules/speech/SpeechSynthesis.cpp:193
> +    ASSERT(utterance);

We should return and change all these functions to take PlatformSpeechSynthesisUtterance&. I believe there is no reason for them to take over ownership of the passed-in utterance, and there is also no need to call them with a null.

[youenn fablet]

Thanks for the review.

> > Source/WebCore/Modules/speech/SpeechSynthesis.h:54
> > -    void speak(SpeechSynthesisUtterance*);
> > +    void speak(Ref<SpeechSynthesisUtterance>&&);
> 
> Does anyone calling this hand over ownership of the utterance? I ask because
> the JavaScript language binding isn’t in a position to do that, so normally
> we would just use SpeechSynthesisUtterance&, unless there is a client that
> can move in an existing smart pointer.
> 
> I suggest changing this to a plain old reference instead of a Ref unless
> there is a client that is actually transferring ownership.

Some other APIs, like MediaStream::addTrack use Ref<>&&/RefPtr<>&& even though they only have JS binding code as client AFAIK.
speak is similar to addToSpeakingQueue, hence why I used Ref<>&&.

Anyway, I can make the change.

> > Source/WebCore/Modules/speech/SpeechSynthesis.cpp:193
> > +    ASSERT(utterance);
> 
> We should return and change all these functions to take
> PlatformSpeechSynthesisUtterance&. I believe there is no reason for them to
> take over ownership of the passed-in utterance, and there is also no need to
> call them with a null.

Agreed.

[Darin Adler]

(In reply to comment #7)
> Some other APIs, like MediaStream::addTrack use Ref<>&&/RefPtr<>&& even
> though they only have JS binding code as client AFAIK.
> speak is similar to addToSpeakingQueue, hence why I used Ref<>&&.

Generally, I don’t think we want the extra visual and conceptual complexity of passing a Ref&& or RefPtr&& unless the optimization of transferring ownership by moving is actually paying off.

So I would suggest using references for all of these, and not Ref&&.

A counter argument would be that these operations *could* take ownership and so we are ready for future callers that are in a position to hand over ownership. But I don’t think that’s sufficient reason to do it that way.

[youenn fablet]

Created attachment 277358 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 277358 [details]
Patch for landing

Clearing flags on attachment: 277358

Committed r200080: <http://trac.webkit.org/changeset/200080>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.