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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied 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 193454: Patch
https://bugs.webkit.org/attachment.cgi?id=193454&action=review

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


This is looking a lot better.  Below are a large number of minor comments. 
Thanks for iterating on this patch.

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

As mentioned in Comment #14, we should probably just return a static object
here instead of creating a new one each time.  That's how we implement most of
these subobjects on Platform.  (If you replied to Comment #14 already and I
missed it, I apologize.)

> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:62
> +    void assign(const WebSpeechSynthesisUtterance&);
> +    void reset();
> +    bool isNull() const { return m_private.isNull(); }
> +
> +    WebString text() const;
> +    WebString lang() const;
> +    WebString voice() const;
> +
> +    // As defined in:
https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html
> +    float volume() const; // 0...1, 1 is default
> +    float rate() const; // 0.1...10, 1 is default
> +    float pitch() const; // 0...2, 1 is default
> +
> +    double startTime() const; // In seconds.

We probably need WEBKIT_EXPORT macros for all these functions.

> Source/Platform/chromium/public/WebSpeechSynthesizerClient.h:46
> +    virtual void wordBoundaryEventOccurred(const
WebSpeechSynthesisUtterance&, int charIndex) = 0;
> +    virtual void sentenceBoundaryEventOccurred(const
WebSpeechSynthesisUtterance&, int charIndex) = 0;

int -> size_t ?

> Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:78
> -    const PlatformSpeechSynthesisUtterance& platformUtterance() const {
return m_platformUtterance; }
> +    PassRefPtr<PlatformSpeechSynthesisUtterance> platformUtterance() const {
return m_platformUtterance; }

Normally we just return PlatformSpeechSynthesisUtterance* in cases like this
where we're not passing ownership of the object.

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

I see that you've declared this function, but I can't find anywhere that calls
it.  Is there some mechanism to ensure that the m_client pointer doesn't point
to an object that has already been freed?

> Source/WebCore/platform/chromium/PlatformSpeechSynthesizerChromium.cpp:35
> +#include "Document.h"
> +#include "Page.h"

These headers should not be included when in the WebCore/platform directory. 
They don't seem to be used for anything, which is good.  :)

> Source/WebCore/platform/chromium/PlatformSpeechSynthesizerChromium.cpp:55
> +    initializeVoiceList();

initializeVoiceList is declared virtual.  Calling virtual functions in
constructors sometimes trips me up because you get the base class version if
you haven't started the constructor for the subclass yet.  I looked briefly,
but that doesn't appear to be the case here.  I just wanted to mention it
because this line gave me a moment of doubt.