Drop [UsePointersEvenForNonNullableObjectArguments] from SpeechSynthesis
Created attachment 277030 [details] Patch
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.
(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.
Created attachment 277233 [details] Patch
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 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.
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.
(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.
Created attachment 277358 [details] Patch for landing
Comment on attachment 277358 [details] Patch for landing Clearing flags on attachment: 277358 Committed r200080: <http://trac.webkit.org/changeset/200080>
All reviewed patches have been landed. Closing bug.