[webkit-reviews] review granted: [Bug 111695] Implement Web Speech Synthesis for Chromium : [Attachment 193842] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dominic Mazzoni
<dmazzoni at google.com>'s request for review:
Bug 111695: Implement Web Speech Synthesis for Chromium
https://bugs.webkit.org/show_bug.cgi?id=111695

Attachment 193842: Patch
https://bugs.webkit.org/attachment.cgi?id=193842&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=193842&action=review


> Source/Platform/chromium/public/Platform.h:122
> +    virtual WebSpeechSynthesizer*
speechSynthesizer(WebSpeechSynthesizerClient*) { return 0; }

If you prefer the "create" style, feel free to change this back before landing.


> Source/WebCore/platform/PlatformSpeechSynthesisUtterance.h:74
> +    void setClient(PlatformSpeechSynthesisUtteranceClient* client) {
m_client = client; }

I think you mentioned above that you aren't using this function yet.  We should
wait to add it until we actually need it for something.

> Source/WebCore/platform/PlatformSpeechSynthesisUtterance.h:79
>      PlatformSpeechSynthesisUtteranceClient* m_client;

It wasn't clear to me whether we're using m_client at all yet.	If we're not
using it, we should remove it.	We can always add it when it's needed.

> Source/WebCore/platform/PlatformSpeechSynthesizer.cpp:37
> +    PlatformSpeechSynthesizer* synthesizer = new
PlatformSpeechSynthesizer(client);
> +    synthesizer->initializeVoiceList();
> +    return adoptPtr(synthesizer);

This is really a minor point, but the idiom we prefer is to call adoptPtr
immediately:

OwnPtr<PlatformSpeechSynthesizer> synthesizer = adoptPtr(new
PlatformSpeechSynthesizer(client));
synthesizer->initializeVoiceList();
return synthesizer.release();

This idiom is better because it's easy to see that the code doesn't leak (i.e.,
we call adoptPtr immediately after calling new).  If someone adds an early
return to this function, they won't introduce a leak by mistake.

> Source/WebCore/platform/PlatformSpeechSynthesizer.h:97
> +    WebKit::WebSpeechSynthesizer* m_webSpeechSynthesizer;

If we're using a static object for the WebKit::WebSpeechSynthesizer, there's no
reason to keep a raw pointer to the object here.  If you decide to switch back
to the createFoo style, then we'll want this to be an OwnPtr.

In case you haven't noticed, raw pointers scary me.  ;)