WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
156901
Drop [UsePointersEvenForNonNullableObjectArguments] from SpeechSynthesis
https://bugs.webkit.org/show_bug.cgi?id=156901
Summary
Drop [UsePointersEvenForNonNullableObjectArguments] from SpeechSynthesis
youenn fablet
Reported
2016-04-22 02:21:18 PDT
Drop [UsePointersEvenForNonNullableObjectArguments] from SpeechSynthesis
Attachments
Patch
(1.59 KB, patch)
2016-04-22 02:24 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(13.44 KB, patch)
2016-04-25 02:05 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(13.26 KB, patch)
2016-04-26 05:20 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2016-04-22 02:24:58 PDT
Created
attachment 277030
[details]
Patch
Darin Adler
Comment 2
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.
youenn fablet
Comment 3
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.
youenn fablet
Comment 4
2016-04-25 02:05:14 PDT
Created
attachment 277233
[details]
Patch
Darin Adler
Comment 5
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.
Darin Adler
Comment 6
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.
youenn fablet
Comment 7
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.
Darin Adler
Comment 8
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.
youenn fablet
Comment 9
2016-04-26 05:20:21 PDT
Created
attachment 277358
[details]
Patch for landing
WebKit Commit Bot
Comment 10
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
>
WebKit Commit Bot
Comment 11
2016-04-26 06:20:16 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug