Bug 156901

Summary: Drop [UsePointersEvenForNonNullableObjectArguments] from SpeechSynthesis
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, esprehn+autocc, kondapallykalyan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 156844    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description youenn fablet 2016-04-22 02:21:18 PDT
Drop [UsePointersEvenForNonNullableObjectArguments] from SpeechSynthesis
Comment 1 youenn fablet 2016-04-22 02:24:58 PDT
Created attachment 277030 [details]
Patch
Comment 2 Darin Adler 2016-04-22 07:39:04 PDT
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.
Comment 3 youenn fablet 2016-04-24 04:54:43 PDT
(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.
Comment 4 youenn fablet 2016-04-25 02:05:14 PDT
Created attachment 277233 [details]
Patch
Comment 5 Darin Adler 2016-04-25 08:56:01 PDT
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.
Comment 6 Darin Adler 2016-04-25 08:58:17 PDT
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.
Comment 7 youenn fablet 2016-04-25 09:42:45 PDT
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.
Comment 8 Darin Adler 2016-04-25 15:52:53 PDT
(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.
Comment 9 youenn fablet 2016-04-26 05:20:21 PDT
Created attachment 277358 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2016-04-26 06:20:12 PDT
Comment on attachment 277358 [details]
Patch for landing

Clearing flags on attachment: 277358

Committed r200080: <http://trac.webkit.org/changeset/200080>
Comment 11 WebKit Commit Bot 2016-04-26 06:20:16 PDT
All reviewed patches have been landed.  Closing bug.