| Summary: | AX: Crash at WebKit: WebKit::WebSpeechSynthesisClient::speak | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||
| Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | commit-queue, dbates, pvollan, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | Other | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
chris fleizach
2019-05-09 16:57:36 PDT
Created attachment 369546 [details]
patch
Comment on attachment 369546 [details]
patch
Nice! R=me.
Comment on attachment 369546 [details] patch Clearing flags on attachment: 369546 Committed r245178: <https://trac.webkit.org/changeset/245178> All reviewed patches have been landed. Closing bug. Comment on attachment 369546 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=369546&action=review > Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61 > + auto voiceURI = voice ? voice->voiceURI() : ""; Ok as is. Could be made a tiny bit more optimal (especially for dumb compiler) by hoisting the it condition into one of condition with an else branch. Are the string WTF string? Or const char? If the former, you can probably tighten it up a teensy bit more by using the null string instead of empty string BUT only do this if it's correct to do so. Otherwise, use emptyString() instead of "". (In reply to Daniel Bates from comment #7) > Comment on attachment 369546 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369546&action=review > > > Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61 > > + auto voiceURI = voice ? voice->voiceURI() : ""; > > Ok as is. Could be made a tiny bit more optimal (especially for dumb > compiler) by hoisting the it condition into one of condition with an else > branch. Are the string WTF string? Or const char? If the former, you can > probably tighten it up a teensy bit more by using the null string instead of > empty string BUT only do this if it's correct to do so. Otherwise, use > emptyString() instead of "". Hoist the if condition into one if condition... Comment on attachment 369546 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=369546&action=review >>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61 >>> + auto voiceURI = voice ? voice->voiceURI() : ""; >> >> Ok as is. Could be made a tiny bit more optimal (especially for dumb compiler) by hoisting the it condition into one of condition with an else branch. Are the string WTF string? Or const char? If the former, you can probably tighten it up a teensy bit more by using the null string instead of empty string BUT only do this if it's correct to do so. Otherwise, use emptyString() instead of "". > > Hoist the if condition into one if condition... will work on this when I get a chance. it means I'll have to have two lines of the form if () m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(), ... else m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(),... Comment on attachment 369546 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=369546&action=review >>>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61 >>>> + auto voiceURI = voice ? voice->voiceURI() : ""; >>> >>> Ok as is. Could be made a tiny bit more optimal (especially for dumb compiler) by hoisting the it condition into one of condition with an else branch. Are the string WTF string? Or const char? If the former, you can probably tighten it up a teensy bit more by using the null string instead of empty string BUT only do this if it's correct to do so. Otherwise, use emptyString() instead of "". >> >> Hoist the if condition into one if condition... > > will work on this when I get a chance. it means I'll have to have two lines of the form > > if () > m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(), ... > else > m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(),... I think our wires are crossed. Look at the lines you touched, they have “ voice ?” In the right hand side <—- extract that to avoid repetition. Comment on attachment 369546 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=369546&action=review > Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:60 > auto voice = utterance->voice(); Just looked up and saw this, left hand side ok as is. In my opinion auto* nicer and move this expression into the if () if you take my earlier advice so that we scope this var View in context: https://bugs.webkit.org/attachment.cgi?id=369546&action=review > Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:60 > auto voice = utterance->voice(); Just looked up and saw this, left hand side ok as is. In my opinion auto* nicer and move this expression into the if () if you take my earlier advice so that we scope this var Comment on attachment 369546 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=369546&action=review >>>>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61 >>>>> + auto voiceURI = voice ? voice->voiceURI() : ""; >>>> >>>> Ok as is. Could be made a tiny bit more optimal (especially for dumb compiler) by hoisting the it condition into one of condition with an else branch. Are the string WTF string? Or const char? If the former, you can probably tighten it up a teensy bit more by using the null string instead of empty string BUT only do this if it's correct to do so. Otherwise, use emptyString() instead of "". >>> >>> Hoist the if condition into one if condition... >> >> will work on this when I get a chance. it means I'll have to have two lines of the form >> >> if () >> m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(), ... >> else >> m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(),... > > I think our wires are crossed. > > Look at the lines you touched, they have “ voice ?” In the right hand side <—- extract that to avoid repetition. you mean like auto name = ""; if (voice) { name = voice->name() (In reply to chris fleizach from comment #13) > Comment on attachment 369546 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369546&action=review > > >>>>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61 > >>>>> + auto voiceURI = voice ? voice->voiceURI() : ""; > >>>> > >>>> Ok as is. Could be made a tiny bit more optimal (especially for dumb compiler) by hoisting the it condition into one of condition with an else branch. Are the string WTF string? Or const char? If the former, you can probably tighten it up a teensy bit more by using the null string instead of empty string BUT only do this if it's correct to do so. Otherwise, use emptyString() instead of "". > >>> > >>> Hoist the if condition into one if condition... > >> > >> will work on this when I get a chance. it means I'll have to have two lines of the form > >> > >> if () > >> m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(), ... > >> else > >> m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(),... > > > > I think our wires are crossed. > > > > Look at the lines you touched, they have “ voice ?” In the right hand side <—- extract that to avoid repetition. > > you mean like Almost except I suggest just declaring the vars, initialize inside of or else branch for chance at optimal assembly const char* name; if (auto* voice = ...) { name = voice->name(); ... } else { name = “”; ... } One question: > > auto name = ""; ^—— so lhs is const char* ? If that is a mistake and you meant lhs to be String then second question: can we use null string? If not then change “” to emptyString() in **my** code above. > if (voice) { > name = voice->name() |