Bug 111695 – Implement Web Speech Synthesis for Chromium

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

The Chromium implementation will just expose an interface in WebKit to be implemented by the embedder. It will have just a bit of logic to create utterance ids for each utterance and then map utterance ids to back to utterance objects.

[Dominic Mazzoni]

Created attachment 191939 [details]
Patch

[chris fleizach]

Are you looking for review on this one or just whether the approach looks ok?

[Dominic Mazzoni]

FYI, the Chromium side of the implementation is here: https://codereview.chromium.org/12589005/

I still need to add preprocessor guards so this feature isn't enabled by default, so I know this isn't technically ready for a formal review - but other than that, please send detailed comments!

Please especially focus on issues like class names, scope, and ownership during your first pass - there are a lot of layers of abstraction here, and I'm sure it could be a lot more clear.

[Adam Barth]

Comment on attachment 191939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191939&action=review

> Source/WebCore/platform/PlatformSpeechSynthesis.h:43
> -    static PassRefPtr<PlatformSpeechSynthesis> create(SpeechSynthesis*);
> +    static PassRefPtr<PlatformSpeechSynthesis> create(SpeechSynthesis*, ScriptExecutionContext*);

Code in the platform directory can't depend on parts of WebCore outside the platform directory.

> Source/WebCore/platform/PlatformSpeechSynthesis.h:52
> -    SpeechSynthesis* m_speechSynthsis;
> +    SpeechSynthesis* m_speechSynthesis;
> +    ScriptExecutionContext* m_context;

These raw pointers look worrying.  What prevents us from using them after the objects they point to are freed?

> Source/WebCore/platform/chromium/SpeechSynthesisController.h:54
> +class SpeechSynthesisController : public Supplement<Page> {

This is very unlikely to be the correct pattern to use for this functionality.  Instead, the speech synthesizer should be part of the Platform API (i.e., in Source/Platform/chromium/public).  There isn't any reason to have a Page dependency here for Chromium.

> Source/WebCore/platform/mac/PlatformSpeechSynthesisMac.mm:42
> -PlatformSpeechSynthesis::PlatformSpeechSynthesis(SpeechSynthesis* synthesis)
> +PlatformSpeechSynthesis::PlatformSpeechSynthesis(SpeechSynthesis* synthesis, ScriptExecutionContext*)

Notice that the apple-mac port doesn't have a Page dependency.

[Dominic Mazzoni]

Thanks for the feedback! I'm still unclear, though.

(In reply to comment #4)
> (From update of attachment 191939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191939&action=review
> 
> > Source/WebCore/platform/chromium/SpeechSynthesisController.h:54
> > +class SpeechSynthesisController : public Supplement<Page> {
> 
> This is very unlikely to be the correct pattern to use for this functionality.  Instead, the speech synthesizer should be part of the Platform API (i.e., in Source/Platform/chromium/public).  There isn't any reason to have a Page dependency here for Chromium.
...
> Notice that the apple-mac port doesn't have a Page dependency.

The difference is that the Mac port of speech synthesis implements all of the functionality within WebCore. For Chromium, all of the functionality is implemented by the embedder via a WebKit client API.

I used features like Speech Recognition, Geolocation, Notifications, and Device Orientation as a guide. In all of those cases, WebViewImpl uses the "provide" pattern to attach a client proxy to the page as a supplement.

How else can WebKit provide an implementation of an interface defined in WebCore, other than using a Page supplement? I couldn't find any examples that work another way.

Here's how I think I should fix the problem of Platform not depending on the rest of WebCore:
* Make PlatformSpeechSynthesizer into an abstract interface.
* Instead of WebCore/Modules/speech/SpeechSynthesis.cpp calling PlatformSpeechSynthesizer::Create, it should call Supplement<Page>::from() to get the platform speech synthesizer. Then WebKit/chromium/src/SpeechSynthesisClientProxy can implement PlatformSpeechSynthesizer directly, eliminating all of the code currently in WebCore/Platform/chromium.

This assumes that we could register the Mac implementation of PlatformSpeechSynthesizer as a page supplement as well.

[Adam Barth]

> The difference is that the Mac port of speech synthesis implements all of the functionality within WebCore.

Actually, it calls out to a library provided by the operating system.

> For Chromium, all of the functionality is implemented by the embedder via a WebKit client API.

Yes, that's the problem.  Instead, you should call out to the embedder via Chromium's Platform API (i.e., through an interface defined in https://trac.webkit.org/browser/trunk/Source/Platform/chromium/public/).

> I used features like Speech Recognition, Geolocation, Notifications, and Device Orientation as a guide. In all of those cases, WebViewImpl uses the "provide" pattern to attach a client proxy to the page as a supplement.

Those are not the correct examples to follow.  Some of them should be converted to use the Platform API and others should remain using the Client API.  The difference is that we ask the Client for advice or policy, but we ask the Platform to do work on our behalf.  In the case of synthesizing speech, we're clearly asking the embedder to do work for us.

> How else can WebKit provide an implementation of an interface defined in WebCore, other than using a Page supplement? I couldn't find any examples that work another way.

By using the Platform API.  Take a look at all the callers of WebKit::Platform::current().

> Here's how I think I should fix the problem of Platform not depending on the rest of WebCore:
> * Make PlatformSpeechSynthesizer into an abstract interface.
> * Instead of WebCore/Modules/speech/SpeechSynthesis.cpp calling PlatformSpeechSynthesizer::Create, it should call Supplement<Page>::from() to get the platform speech synthesizer. Then WebKit/chromium/src/SpeechSynthesisClientProxy can implement PlatformSpeechSynthesizer directly, eliminating all of the code currently in WebCore/Platform/chromium.

That's not the right approach.  The right approach is to move the API from the Client API to the Platform API.

> This assumes that we could register the Mac implementation of PlatformSpeechSynthesizer as a page supplement as well.

That's not the right design for apple-mac either.

[Dominic Mazzoni]

Created attachment 192957 [details]
Patch

[Dominic Mazzoni]

OK, I think this should be a lot closer to what you had in mind.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 192957 [details]
Patch

Attachment 192957 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17116191

[Adam Barth]

Comment on attachment 192957 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192957&action=review

> Source/Platform/chromium/public/Platform.h:121
> +    // May return null.
> +    virtual WebSpeechSynthesis* createSpeechSynthesis() { return 0; }

WebSpeechSynthesis -> WebSpeechSynthesizer ?

> Source/Platform/chromium/public/WebSpeechSynthesisDelegate.h:29
> +#include "../../../Platform/chromium/public/WebVector.h"

This can just be #include "WebVector.h"

> Source/Platform/chromium/public/WebSpeechSynthesisDelegate.h:45
> +    virtual void didStartSpeaking(int utteranceId) = 0;
> +    virtual void didPauseSpeaking(int utteranceId) = 0;
> +    virtual void didResumeSpeaking(int utteranceId) = 0;
> +    virtual void didFinishSpeaking(int utteranceId) = 0;
> +    virtual void speakingErrorOccurred(int utteranceId) = 0;
> +    virtual void wordBoundaryEventOccurred(int utteranceId, int charIndex) = 0;
> +    virtual void sentenceBoundaryEventOccurred(int utteranceId, int charIndex) = 0;

Why don't these operate in terms of a WebSpeechSynthesisUtterance*?  I don't understand why there's an ID involved.

> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:62
> +    const WebCore::PlatformSpeechSynthesisUtterance* m_private;
> +    int m_id;

What role does m_id play?

Also, what is the relationship between the lifetime of this object and m_private.

> Source/WebCore/platform/PlatformSpeechSynthesis.h:52
> +    ScriptExecutionContext* m_context;

Code in Source/WebCore/platform cannot depend on concepts like ScriptExecutionContext that exist outside the platform directory in WebCore.

I can't find any uses of m_context.  Maybe this is left over from the previous iteration?

> Source/WebCore/platform/PlatformSpeechSynthesisVoice.h:46
> -    
> +

This change seems spurious.

[Adam Barth]

Does WebSpeechSynthesis carry state on the Chromium side?

[Dominic Mazzoni]

(In reply to comment #12)
> Does WebSpeechSynthesis carry state on the Chromium side?

The renderer-side implementation in Chromium is stateless.

On the browser side, it has to keep track of the utterance id and the RenderProcessHost for the duration of the utterance, that's it.

[Adam Barth]

> > Does WebSpeechSynthesis carry state on the Chromium side?
> 
> The renderer-side implementation in Chromium is stateless.

In that case, we probably should just return a static object instead of allocating a new WebSpeechSynthesis each time.

> On the browser side, it has to keep track of the utterance id and the RenderProcessHost for the duration of the utterance, that's it.

Generally we try to keep these sorts of IDs out of the WebKit API if they're only needed because of multiprocess concerns.

[Build Bot]

Comment on attachment 192957 [details]
Patch

Attachment 192957 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17140109

[Build Bot]

Comment on attachment 192957 [details]
Patch

Attachment 192957 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17217035

[Dominic Mazzoni]

(In reply to comment #14)
> > On the browser side, it has to keep track of the utterance id and the RenderProcessHost for the duration of the utterance, that's it.
> 
> Generally we try to keep these sorts of IDs out of the WebKit API if they're only needed because of multiprocess concerns.

OK, I can make the delegate functions take a WebSpeechSynthesisUtterance directly. I think I'll have to make PlatformSpeechSynthesisUtterance ref-counted.

[Adam Barth]

We need to make sure that doesn't cause a problem with

    PlatformSpeechSynthesisUtteranceClient* m_client;

[Peter Beverloo (cr-android ews)]

Comment on attachment 192957 [details]
Patch

Attachment 192957 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17189390

[Peter Beverloo (cr-android ews)]

Comment on attachment 192957 [details]
Patch

Attachment 192957 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17149520

[WebKit Review Bot]

Comment on attachment 192957 [details]
Patch

Attachment 192957 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17193037

New failing tests:
fast/js/global-constructors.html
http/tests/security/isolatedWorld/all-window-prototypes.html
http/tests/security/isolatedWorld/all-window-properties.html
inspector/debugger/function-details.html
inspector/console/console-dir-global.html
inspector/debugger/debugger-completions-on-call-frame.html
fast/dom/everything-to-string.html

[WebKit Review Bot]

Comment on attachment 192957 [details]
Patch

Attachment 192957 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17060755

New failing tests:
html5lib/generated/run-tests16-data.html
fast/js/global-constructors.html
http/tests/security/isolatedWorld/all-window-prototypes.html
http/tests/security/isolatedWorld/all-window-properties.html
inspector/debugger/function-details.html
inspector/console/console-dir-global.html
inspector/debugger/debugger-completions-on-call-frame.html
fast/dom/everything-to-string.html

[Darin Fisher (:fishd, Google)]

Comment on attachment 192957 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192957&action=review

>> Source/Platform/chromium/public/Platform.h:121
>> +    virtual WebSpeechSynthesis* createSpeechSynthesis() { return 0; }
> 
> WebSpeechSynthesis -> WebSpeechSynthesizer ?

+1

> Source/Platform/chromium/public/WebSpeechSynthesis.h:40
> +    virtual void setDelegate(WebSpeechSynthesisDelegate*) = 0;

nit: we generally use the term "Client" instead of "Delegate" at the WebKit API level.  We also normally set clients at creation time, rather than make them settable later.  Do you need to be able to change the client at runtime?

> Source/Platform/chromium/public/WebSpeechSynthesis.h:41
> +    virtual void initializeVoiceList() = 0;

are their side-effects to this method?  if yes, then please add a comment, documenting those.  otherwise, if this is just something you need to do before calling speak, then maybe it should happen inside createSpeechSynthesizer?

> Source/Platform/chromium/public/WebSpeechSynthesis.h:42
> +    virtual void speak(const WebSpeechSynthesisUtterance&) = 0;

nit: you should document whether or not it is OK to issue multiple utterances at a time.

> Source/Platform/chromium/public/WebSpeechSynthesis.h:45
> +    virtual void cancel() = 0;

nit: is cancel really needed?  why not just destroy the WebSpeechSynthesizer instead?  (essentially, you have two ways of cancelling if you have an explicit cancel method.  what is the advantage of having two ways instead of one?)

> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:36
> +class WebSpeechSynthesisUtterance {

nit: Maybe this class could just be named "WebSpeechUtterance"?  I'm not sure the "Synthesis" bit improves the name.

> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:40
> +    WebString lang() const;

nit: lang -> language ... we try to avoid abbreviations.

> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:42
> +    float volume() const;

does it make sense to document the units here?  it seems like if you are implementing this API that you would have to study the caller to determine what these fields mean.  that is unfortunate.  comments would help.

> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:48
> +    WebSpeechSynthesisUtterance(const WebCore::PlatformSpeechSynthesisUtterance* u, int utteranceId)

same nit: not clear to me why "Synthesis" in PlatformSpeechSynthesisUtterance is better than PlatformSpeechUtterance

> Source/Platform/chromium/public/WebSpeechSynthesisVoice.h:44
> +

nit: define operator= as we normally do for such classes?

> Source/Platform/chromium/public/WebSpeechSynthesisVoice.h:45
> +    WEBKIT_EXPORT void assign(const WebString& voiceURI, const WebString& name, const WebString& lang, bool localService, bool isDefault);

nit: some of these parameters could benefit from comments so the API makes sense on its own.

nit: why not provide a constructor with all of these parameters?  note: it is more common to just add a bunch of setters on a class like this rather than an assign method with a zillion parameters.  the call site won't be very readable with two bool parameters, etc.

> Source/WebCore/platform/chromium/support/WebSpeechSynthesisDelegateImpl.cpp:46
> +    Vector<RefPtr<WebCore::PlatformSpeechSynthesisVoice> > outVoices;

nit: since you are inside the WebCore namespace, there is no need for WebCore::

[Dominic Mazzoni]

Comment on attachment 192957 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192957&action=review

>>> Source/Platform/chromium/public/Platform.h:121
>>> +    virtual WebSpeechSynthesis* createSpeechSynthesis() { return 0; }
>> 
>> WebSpeechSynthesis -> WebSpeechSynthesizer ?
> 
> +1

Done.

>> Source/Platform/chromium/public/WebSpeechSynthesis.h:40
>> +    virtual void setDelegate(WebSpeechSynthesisDelegate*) = 0;
> 
> nit: we generally use the term "Client" instead of "Delegate" at the WebKit API level.  We also normally set clients at creation time, rather than make them settable later.  Do you need to be able to change the client at runtime?

No problem, renamed Client and set at creation time now.

>> Source/Platform/chromium/public/WebSpeechSynthesis.h:41
>> +    virtual void initializeVoiceList() = 0;
> 
> are their side-effects to this method?  if yes, then please add a comment, documenting those.  otherwise, if this is just something you need to do before calling speak, then maybe it should happen inside createSpeechSynthesizer?

I renamed it updateVoiceList. The synthesizer can update the client with a voice list anytime it changes, but this allows the client to specifically request it.

>> Source/Platform/chromium/public/WebSpeechSynthesis.h:42
>> +    virtual void speak(const WebSpeechSynthesisUtterance&) = 0;
> 
> nit: you should document whether or not it is OK to issue multiple utterances at a time.

What if the answer is "the spec needs to be more clear"?

Currently, WebCore/Modules/speech will not issue multiple utterances at once. If it did, the backend implementation would handle it just fine, it supports queuing automatically.

I'd like to modify the spec to clarify what should happen if speech synthesizers from different DOM windows try to speak at once. I'd like it to be possible for them to queue or interrupt each other, but I'd be okay with more of an exclusive-lock type of approach, as long as the spec is crystal-clear.

I'm a little hesitant to document this too clearly when the spec is unclear. For now I made a note that it's okay to call with more than one utterance - that sound reasonable?

>> Source/Platform/chromium/public/WebSpeechSynthesis.h:45
>> +    virtual void cancel() = 0;
> 
> nit: is cancel really needed?  why not just destroy the WebSpeechSynthesizer instead?  (essentially, you have two ways of cancelling if you have an explicit cancel method.  what is the advantage of having two ways instead of one?)

Cancel is needed because I want to be able to still get the "speech finished" callback after I cancel the utterance. Also, it's really common to cancel and immediately speak something else instead, it doesn't make sense to recreate the whole synthesizer for that.

>> Source/Platform/chromium/public/WebSpeechSynthesisDelegate.h:29
>> +#include "../../../Platform/chromium/public/WebVector.h"
> 
> This can just be #include "WebVector.h"

Done

>> Source/Platform/chromium/public/WebSpeechSynthesisDelegate.h:45
>> +    virtual void sentenceBoundaryEventOccurred(int utteranceId, int charIndex) = 0;
> 
> Why don't these operate in terms of a WebSpeechSynthesisUtterance*?  I don't understand why there's an ID involved.

Refactored these to take a WebSpeechSynthesisUtterance object.

>> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:36
>> +class WebSpeechSynthesisUtterance {
> 
> nit: Maybe this class could just be named "WebSpeechUtterance"?  I'm not sure the "Synthesis" bit improves the name.

We already have these classes associated with speech recognition:

WebSpeechGrammar.h
WebSpeechInputController.h
WebSpeechInputListener.h
WebSpeechInputResult.h
WebSpeechRecognitionHandle.h
WebSpeechRecognitionParams.h
WebSpeechRecognitionResult.h
WebSpeechRecognizer.h
WebSpeechRecognizerClient.h

Personally, I don't think a casual non-speech developer should be expected to know that "grammar" only belongs to speech recognition and "utterance" only belongs to speech synthesis.

>> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:40
>> +    WebString lang() const;
> 
> nit: lang -> language ... we try to avoid abbreviations.

Done

>> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:42
>> +    float volume() const;
> 
> does it make sense to document the units here?  it seems like if you are implementing this API that you would have to study the caller to determine what these fields mean.  that is unfortunate.  comments would help.

Done. I linked to the spec also, is that okay? The comments are enough for a quick check but not enough to understand the nuances.

>> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:62
>> +    int m_id;
> 
> What role does m_id play?
> 
> Also, what is the relationship between the lifetime of this object and m_private.

No more m_id, and now PlatformSpeechSynthesisUtterance is ref-counted.

>> Source/Platform/chromium/public/WebSpeechSynthesisVoice.h:44
>> +
> 
> nit: define operator= as we normally do for such classes?

Done

>> Source/Platform/chromium/public/WebSpeechSynthesisVoice.h:45
>> +    WEBKIT_EXPORT void assign(const WebString& voiceURI, const WebString& name, const WebString& lang, bool localService, bool isDefault);
> 
> nit: some of these parameters could benefit from comments so the API makes sense on its own.
> 
> nit: why not provide a constructor with all of these parameters?  note: it is more common to just add a bunch of setters on a class like this rather than an assign method with a zillion parameters.  the call site won't be very readable with two bool parameters, etc.

I switched it to a bunch of setters, I like that better.

For the Platform class, I left both versions - is that okay? I didn't want to ruin the design decision of the mac port to set everything at once. It can be a good way to enforce that if you add a new parameter, all callers must set it.

>> Source/WebCore/platform/chromium/support/WebSpeechSynthesisDelegateImpl.cpp:46
>> +    Vector<RefPtr<WebCore::PlatformSpeechSynthesisVoice> > outVoices;
> 
> nit: since you are inside the WebCore namespace, there is no need for WebCore::

Done

[Dominic Mazzoni]

Created attachment 193382 [details]
Patch

[chris fleizach]

Comment on attachment 193382 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193382&action=review

> Source/WebCore/platform/PlatformSpeechSynthesis.h:52
> +    ScriptExecutionContext* m_context;

why do we need the ScriptContext here. i don't see it being used

> Source/WebCore/platform/PlatformSpeechSynthesizer.cpp:28
> +#include "ScriptExecutionContext.h"

i don't see this being referenced, might not need it

> Source/WebCore/platform/PlatformSpeechSynthesizer.h:55
> +class ScriptExecutionContext;

i don't see this being referenced, might not need it

> Source/WebCore/platform/mock/PlatformSpeechSynthesizerMock.cpp:54
> +    m_utterance.clear();

doesn't m_utterance = 0 also work?

> Source/WebCore/platform/mock/PlatformSpeechSynthesizerMock.cpp:67
>      ASSERT(!m_utterance);

does this assert need to check the .get()?

[Build Bot]

Comment on attachment 193382 [details]
Patch

Attachment 193382 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17015595

[Peter Beverloo (cr-android ews)]

Comment on attachment 193382 [details]
Patch

Attachment 193382 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17200283

[Peter Beverloo (cr-android ews)]

Comment on attachment 193382 [details]
Patch

Attachment 193382 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17215285

[Build Bot]

Comment on attachment 193382 [details]
Patch

Attachment 193382 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17230055

[Dominic Mazzoni]

Created attachment 193424 [details]
Patch

[Dominic Mazzoni]

Comment on attachment 193382 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193382&action=review

>> Source/WebCore/platform/PlatformSpeechSynthesizer.h:55
>> +class ScriptExecutionContext;
> 
> i don't see this being referenced, might not need it

Sorry, forgot to remove a few of these!

>> Source/WebCore/platform/mock/PlatformSpeechSynthesizerMock.cpp:54
>> +    m_utterance.clear();
> 
> doesn't m_utterance = 0 also work?

You're right.

>> Source/WebCore/platform/mock/PlatformSpeechSynthesizerMock.cpp:67
>>      ASSERT(!m_utterance);
> 
> does this assert need to check the .get()?

RefPtr implements operator! correctly, so this is fine.

[chris fleizach]

Comment on attachment 193382 [details]
Patch

you don't need to do in this patch, but you'll be able to move the layout tests from platform/mac/fast/speechsynthesis up a level and run them on chromium

[WebKit Review Bot]

Comment on attachment 193424 [details]
Patch

Attachment 193424 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17184215

[WebKit Review Bot]

Comment on attachment 193424 [details]
Patch

Attachment 193424 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17126547

[Peter Beverloo (cr-android ews)]

Comment on attachment 193424 [details]
Patch

Attachment 193424 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17131525

[Build Bot]

Comment on attachment 193424 [details]
Patch

Attachment 193424 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17220093

[Dominic Mazzoni]

Created attachment 193427 [details]
Patch

[Build Bot]

Comment on attachment 193427 [details]
Patch

Attachment 193427 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17118364

[Build Bot]

Comment on attachment 193427 [details]
Patch

Attachment 193427 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17134747

[Dominic Mazzoni]

Created attachment 193454 [details]
Patch

[Adam Barth]

Comment on attachment 193454 [details]
Patch

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.

[Dominic Mazzoni]

Comment on attachment 193454 [details]
Patch

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.

Thanks for your patience with me! I'm glad it's getting close.

>> Source/Platform/chromium/public/Platform.h:122
>> +    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.)

Sorry I neglected to reply before. There's one PlatformSpeechSynthesizer created per DOMWindow, so each one needs to have its own WebSpeechSynthesizer so that client messages get routed back to the same window. It looks like createAudioDevice works the same way.

I suppose another solution would be for each call to updateVoiceList() and speak() to take the client as an argument, or maybe interfaces like WebSpeechSynthesisVoiceListCallback and WebSpeechSynthesisSpeakCallback instead. That's fine if you prefer that, but I think this current solution (with create...) leads to less bookkeeping for the embedder.

>> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:62
>> +    double startTime() const; // In seconds.
> 
> We probably need WEBKIT_EXPORT macros for all these functions.

Done. What platform/configuration is this be needed for? I didn't get a compile error on Mac or Linux with Release or Debug.

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

Good catch. I used unsigned instead of size_t to match the rest of the WebCore platform interface.

>> Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:78
>> +    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.

Done

>> 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?

Good catch, I meant to do that. Added to the SpeechSynthesisUtterance destructor.

It may turn out that we don't need PlatformSpeechSynthesisUtteranceClient, it looks like we've implemented most of the spec without it.

>> Source/WebCore/platform/chromium/PlatformSpeechSynthesizerChromium.cpp:35
>> +#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.  :)

Fixed

>> 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.

I agree. I moved it to PlatformSpeechSynthesizer::create instead so it's called after the constructor and so that logic doesn't have to be duplicated across platforms.

[Dominic Mazzoni]

Created attachment 193842 [details]
Patch

[Adam Barth]

(In reply to comment #43)
> (From update of attachment 193454 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193454&action=review
> 
> >> Source/Platform/chromium/public/Platform.h:122
> >> +    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.)
> 
> Sorry I neglected to reply before. There's one PlatformSpeechSynthesizer created per DOMWindow, so each one needs to have its own WebSpeechSynthesizer so that client messages get routed back to the same window. It looks like createAudioDevice works the same way.

Oh, I misunderstood Comment #13 to mean that this object was completely stateless on the Chromium side, but it sounds like it has a routing ID or something.

> I suppose another solution would be for each call to updateVoiceList() and speak() to take the client as an argument, or maybe interfaces like WebSpeechSynthesisVoiceListCallback and WebSpeechSynthesisSpeakCallback instead. That's fine if you prefer that, but I think this current solution (with create...) leads to less bookkeeping for the embedder.

Having a createFoo function is fine.  We just skip it if there's no state on the Chromium side to associate with the created object.

> >> Source/Platform/chromium/public/WebSpeechSynthesisUtterance.h:62
> >> +    double startTime() const; // In seconds.
> > 
> > We probably need WEBKIT_EXPORT macros for all these functions.
> 
> Done. What platform/configuration is this be needed for? I didn't get a compile error on Mac or Linux with Release or Debug.

It's for the component build.  It instructs the compile to export the function from WebKit.dll so that other components can call the API.

> >> 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?
> 
> Good catch, I meant to do that. Added to the SpeechSynthesisUtterance destructor.
> 
> It may turn out that we don't need PlatformSpeechSynthesisUtteranceClient, it looks like we've implemented most of the spec without it.

In that case, we should probably leave it out until it's required.

> >> 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.
> 
> I agree. I moved it to PlatformSpeechSynthesizer::create instead so it's called after the constructor and so that logic doesn't have to be duplicated across platforms.

Great.

[Build Bot]

Comment on attachment 193842 [details]
Patch

Attachment 193842 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17202506

New failing tests:
platform/mac/fast/speechsynthesis/speech-synthesis-utterance-uses-voice.html
platform/mac/fast/speechsynthesis/speech-synthesis-voices.html

[Adam Barth]

Comment on attachment 193842 [details]
Patch

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.  ;)

[Dominic Mazzoni]

Comment on attachment 193842 [details]
Patch

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.

Done

>> 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.

This is now being used, we call setClient(0) when the SpeechSynthesisUtterance is destroyed.

>> 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.

It's being used by the speech synthesis client, to find the event target.

>> Source/WebCore/platform/PlatformSpeechSynthesizer.cpp:37
>> +    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.

Sounds good.

>> 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.  ;)

Done

[Dominic Mazzoni]

Created attachment 193935 [details]
Patch

[Adam Barth]

Comment on attachment 193935 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 193935 [details]
Patch

Clearing flags on attachment: 193935

Committed r146277: <http://trac.webkit.org/changeset/146277>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

Sorry, but this patch broke the Android build:
http://build.chromium.org/p/chromium.webkit/builders/Android%20Builder/builds/14160

The tree's been very red today and I need to try to keep it green to roll WebKit. Rolling this out.

[Kenneth Russell]

Reverted r146277 for reason:

Broke Chromium Android build

Committed r146280: <http://trac.webkit.org/changeset/146280>

[Kenneth Russell]

FYI, this also broke the Linux Debug build, so the fix should be easier to test:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/7288

[Dominic Mazzoni]

Created attachment 194282 [details]
Patch for landing

[chris fleizach]

Comment on attachment 194282 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=194282&action=review

> Source/WebCore/platform/PlatformSpeechSynthesisVoice.h:43
> +    void setVoiceURI(const String& voiceURI) { m_voiceURI =voiceURI; }

= sign missing space after it

[Dominic Mazzoni]

Comment on attachment 194282 [details]
Patch for landing

Thanks, will fix.

[Dominic Mazzoni]

Created attachment 194283 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 194283 [details]
Patch for landing

Clearing flags on attachment: 194283

Committed r146483: <http://trac.webkit.org/changeset/146483>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This doesn't compile:

FAILED: ../../Source/WebKit/chromium/third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/source/webcore/platform/chromium/support/webkit.webspeechsynthesizerclientimpl.o.d -DUSE_SKIA -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_THREADING -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1 -DENABLE_GOOGLE_NOW=1 -DENABLE_LANGUAGE_DETECTION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_MESSAGE_CENTER=1 -DENABLE_MANAGED_USERS=1 -DWEBKIT_IMPLEMENTATION=1 -DENABLE_3D_PLUGIN=1 -DENABLE_BATTERY_STATUS=0 -DENABLE_BLOB=1 -DENABLE_BLOB_SLICE=1 -DENABLE_CANVAS_PATH=1 -DENABLE_CANVAS_PROXY=1 -DENABLE_CHANNEL_MESSAGING=1 -DENABLE_CSP_NEXT=1 -DENABLE_CSS3_CONDITIONAL_RULES=0 -DENABLE_CSS3_TEXT=0 -DENABLE_CSS3_TEXT_LINE_BREAK=0 -DENABLE_CSS_BOX_DECORATION_BREAK=1 -DENABLE_CSS_COMPOSITING=0 -DENABLE_CSS_DEVICE_ADAPTATION=0 -DENABLE_CSS_EXCLUSIONS=1 -DENABLE_CSS_FILTERS=1 -DENABLE_CSS_IMAGE_SET=1 -DENABLE_CSS_IMAGE_RESOLUTION=0 -DENABLE_CSS_REGIONS=1 -DENABLE_CSS_SHADERS=1 -DENABLE_CSS_TRANSFORMS_ANIMATIONS_UNPREFIXED=0 -DENABLE_CSS_VARIABLES=1 -DENABLE_CSS_STICKY_POSITION=1 -DENABLE_CUSTOM_ELEMENTS=1 -DENABLE_CUSTOM_SCHEME_HANDLER=0 -DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_DATA_TRANSFER_ITEMS=1 -DENABLE_DETAILS_ELEMENT=1 -DENABLE_DEVICE_ORIENTATION=1 -DENABLE_DIALOG_ELEMENT=1 -DENABLE_DIRECTORY_UPLOAD=1 -DENABLE_DOM4_EVENTS_CONSTRUCTOR=1 -DENABLE_DOWNLOAD_ATTRIBUTE=1 -DENABLE_DRAGGABLE_REGION=1 -DENABLE_ENCRYPTED_MEDIA=1 -DENABLE_FILE_SYSTEM=1 -DENABLE_FILTERS=1 -DENABLE_FONT_LOAD_EVENTS=1 -DENABLE_FULLSCREEN_API=1 -DENABLE_GAMEPAD=1 -DENABLE_GEOLOCATION=1 -DENABLE_GESTURE_EVENTS=1 -DENABLE_ICONDATABASE=0 -DENABLE_IFRAME_SEAMLESS=1 -DENABLE_INDEXED_DATABASE=1 -DENABLE_INPUT_TYPE_DATE=1 -DENABLE_INPUT_TYPE_DATETIMELOCAL=1 -DENABLE_INPUT_TYPE_MONTH=1 -DENABLE_INPUT_TYPE_TIME=1 -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_JAVASCRIPT_I18N_API=1 -DENABLE_LEGACY_CSS_VENDOR_PREFIXES=0 -DENABLE_LEGACY_VIEWPORT_ADAPTION=1 -DENABLE_LEGACY_VENDOR_PREFIXES=0 -DENABLE_LEGACY_WEB_AUDIO=1 -DENABLE_LINK_PREFETCH=1 -DENABLE_LINK_PRERENDER=1 -DENABLE_MATHML=0 -DENABLE_MEDIA_SOURCE=1 -DENABLE_MEDIA_STATISTICS=1 -DENABLE_MEDIA_STREAM=1 -DENABLE_METER_ELEMENT=1 -DENABLE_MHTML=1 -DENABLE_MICRODATA=0 -DENABLE_MOUSE_CURSOR_SCALE=1 -DENABLE_NOSNIFF=1 -DENABLE_PAGE_VISIBILITY_API=1 -DENABLE_PERFORMANCE_TIMELINE=1 -DENABLE_POINTER_LOCK=1 -DENABLE_PROGRESS_ELEMENT=1 -DENABLE_PROXIMITY_EVENTS=0 -DENABLE_QUOTA=1 -DENABLE_REQUEST_ANIMATION_FRAME=1 -DENABLE_REQUEST_AUTOCOMPLETE=1 -DENABLE_RESOLUTION_MEDIA_QUERY=0 -DENABLE_RESOURCE_TIMING=1 -DENABLE_RUBY=1 -DENABLE_SANDBOX=1 -DENABLE_SCRIPTED_SPEECH=1 -DENABLE_SHADOW_DOM=1 -DENABLE_SMOOTH_SCROLLING=1 -DENABLE_SPEECH_SYNTHESIS=0 -DENABLE_SQL_DATABASE=1 -DENABLE_STYLE_SCOPED=1 -DENABLE_SUBPIXEL_LAYOUT=1 -DENABLE_SVG=1 -DENABLE_SVG_FONTS=1 -DENABLE_TEMPLATE_ELEMENT=1 -DENABLE_TEXT_AUTOSIZING=1 -DENABLE_THREADED_HTML_PARSER=1 -DENABLE_TOUCH_ADJUSTMENT=1 -DENABLE_TOUCH_EVENTS=1 -DENABLE_TOUCH_EVENT_TRACKING=1 -DENABLE_TOUCH_ICON_LOADING=0 -DENABLE_TOUCH_SLIDER=1 -DENABLE_USER_TIMING=1 -DENABLE_V8_SCRIPT_DEBUG_SERVER=1 -DENABLE_VIDEO=1 -DENABLE_VIDEO_TRACK=1 -DENABLE_VIEWPORT=1 -DENABLE_VIEW_MODE_CSS_MEDIA=1 -DENABLE_WEBGL=1 -DENABLE_WEB_SOCKETS=1 -DENABLE_WEB_TIMING=1 -DENABLE_WORKERS=1 -DENABLE_XHR_TIMEOUT=0 -DENABLE_XSLT=1 -DWTF_USE_LEVELDB=1 -DWTF_USE_BUILTIN_UTF8_CODEC=1 -DWTF_USE_OPENTYPE_SANITIZER=1 -DWTF_USE_RTL_SCROLLBAR=1 -DWTF_USE_SKIA_TEXT=1 -DWTF_USE_WEBP=1 -DWTF_USE_WEBKIT_IMAGE_DECODERS=1 -DENABLE_CALENDAR_PICKER=1 -DENABLE_DATALIST_ELEMENT=1 -DENABLE_INPUT_SPEECH=1 -DENABLE_INPUT_TYPE_COLOR=1 -DENABLE_INPUT_TYPE_WEEK=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1 -DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_MEDIA_CAPTURE=0 -DENABLE_NAVIGATOR_CONTENT_UTILS=1 -DENABLE_NOTIFICATIONS=1 -DENABLE_ORIENTATION_EVENTS=0 -DENABLE_PAGE_POPUP=1 -DENABLE_SHARED_WORKERS=1 -DENABLE_WEB_AUDIO=1 -DENABLE_8BIT_TEXTRUN=1 -DENABLE_BINDING_INTEGRITY=1 -DENABLE_3D_RENDERING=1 -DENABLE_ACCELERATED_2D_CANVAS=1 -DWTF_USE_ACCELERATED_COMPOSITING=1 -DENABLE_RUBBER_BANDING=1 -DWTF_USE_SKIA_ON_MAC_CHROMIUM=1 -DBUILDING_CHROMIUM__=1 -DWTF_USE_NEW_THEME=1 -DU_USING_ICU_NAMESPACE=0 -DSK_BUILD_NO_IMAGE_ENCODE -DSK_DEFERRED_CANVAS_USES_GPIPE=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DGR_AGGRESSIVE_SHADER_OPTS=1 -DSK_ENABLE_INST_COUNT=0 -DSK_USE_POSIX_THREADS -DU_STATIC_IMPLEMENTATION -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../../Source/WebKit/chromium/third_party/icu/public/common -I../../Source/WebKit/chromium/third_party/icu/public/i18n -I../../Source/WebKit/chromium/public -I../../Source/WebKit/chromium/src -I../../Source/WebKit/chromium/third_party/angle/include -I../../Source/WebKit/chromium/third_party/skia/include/utils -I../../Source/WebKit/chromium/public/mac -I../../Source/WebKit/chromium/third_party/khronos -I../../Source/WebKit/chromium/gpu -I../../Source/WebKit/chromium -I../../Source/Platform/chromium -Igen/webcore_headers -I../../Source/WebCore -I../../Source -I../../Source/WebCore/Modules/battery -I../../Source/WebCore/Modules/filesystem -I../../Source/WebCore/Modules/filesystem/chromium -I../../Source/WebCore/Modules/gamepad -I../../Source/WebCore/Modules/geolocation -I../../Source/WebCore/Modules/indexeddb -I../../Source/WebCore/Modules/indexeddb/chromium -I../../Source/WebCore/Modules/mediasource -I../../Source/WebCore/Modules/mediastream -I../../Source/WebCore/Modules/navigatorcontentutils -I../../Source/WebCore/Modules/notifications -I../../Source/WebCore/Modules/proximity -I../../Source/WebCore/Modules/quota -I../../Source/WebCore/Modules/speech -I../../Source/WebCore/Modules/webaudio -I../../Source/WebCore/Modules/webdatabase -I../../Source/WebCore/Modules/webdatabase/chromium -I../../Source/WebCore/Modules/websockets -I../../Source/WebCore/accessibility -I../../Source/WebCore/accessibility/chromium -I../../Source/WebCore/bindings -I../../Source/WebCore/bindings/generic -I../../Source/WebCore/bindings/v8 -I../../Source/WebCore/bindings/v8/custom -I../../Source/WebCore/bridge -I../../Source/WebCore/bridge/jni -I../../Source/WebCore/bridge/jni/v8 -I../../Source/WebCore/css -I../../Source/WebCore/dom -I../../Source/WebCore/dom/default -I../../Source/WebCore/dom/default/chromium -I../../Source/WebCore/editing -I../../Source/WebCore/fileapi -I../../Source/WebCore/history -I../../Source/WebCore/html -I../../Source/WebCore/html/canvas -I../../Source/WebCore/html/parser -I../../Source/WebCore/html/shadow -I../../Source/WebCore/html/track -I../../Source/WebCore/inspector -I../../Source/WebCore/loader -I../../Source/WebCore/loader/appcache -I../../Source/WebCore/loader/archive -I../../Source/WebCore/loader/archive/cf -I../../Source/WebCore/loader/archive/mhtml -I../../Source/WebCore/loader/cache -I../../Source/WebCore/loader/icon -I../../Source/WebCore/mathml -I../../Source/WebCore/page -I../../Source/WebCore/page/animation -I../../Source/WebCore/page/chromium -I../../Source/WebCore/page/scrolling -I../../Source/WebCore/page/scrolling/chromium -I../../Source/WebCore/platform -I../../Source/WebCore/platform/animation -I../../Source/WebCore/platform/audio -I../../Source/WebCore/platform/audio/chromium -I../../Source/WebCore/platform/chromium -I../../Source/WebCore/platform/chromium/support -I../../Source/WebCore/platform/graphics -I../../Source/WebCore/platform/graphics/chromium -I../../Source/WebCore/platform/graphics/chromium/cc -I../../Source/WebCore/platform/graphics/cpu/arm -I../../Source/WebCore/platform/graphics/cpu/arm/filters -I../../Source/WebCore/platform/graphics/filters -I../../Source/WebCore/platform/graphics/filters/skia -I../../Source/WebCore/platform/graphics/gpu -I../../Source/WebCore/platform/graphics/opentype -I../../Source/WebCore/platform/graphics/skia -I../../Source/WebCore/platform/graphics/transforms -I../../Source/WebCore/platform/image-decoders -I../../Source/WebCore/platform/image-decoders/bmp -I../../Source/WebCore/platform/image-decoders/gif -I../../Source/WebCore/platform/image-decoders/ico -I../../Source/WebCore/platform/image-decoders/jpeg -I../../Source/WebCore/platform/image-decoders/png -I../../Source/WebCore/platform/image-decoders/skia -I../../Source/WebCore/platform/image-decoders/webp -I../../Source/WebCore/platform/image-encoders/skia -I../../Source/WebCore/platform/leveldb -I../../Source/WebCore/platform/mediastream -I../../Source/WebCore/platform/mediastream/chromium -I../../Source/WebCore/platform/mock -I../../Source/WebCore/platform/network -I../../Source/WebCore/platform/network/chromium -I../../Source/WebCore/platform/sql -I../../Source/WebCore/platform/text -I../../Source/WebCore/platform/text/transcoder -I../../Source/WebCore/plugins -I../../Source/WebCore/plugins/chromium -I../../Source/WebCore/rendering -I../../Source/WebCore/rendering/mathml -I../../Source/WebCore/rendering/style -I../../Source/WebCore/rendering/svg -I../../Source/WebCore/storage -I../../Source/WebCore/svg -I../../Source/WebCore/svg/animation -I../../Source/WebCore/svg/graphics -I../../Source/WebCore/svg/graphics/filters -I../../Source/WebCore/svg/properties -I../../Source/ThirdParty/glu -I../../Source/WebCore/workers -I../../Source/WebCore/workers/chromium -I../../Source/WebCore/xml -I../../Source/WebCore/xml/parser -I../../Source/WebCore -I../../Source/WebCore/platform/audio/mac -I../../Source/WebCore/platform/cocoa -I../../Source/WebCore/platform/graphics/cg -I../../Source/WebCore/platform/graphics/cocoa -I../../Source/WebCore/platform/graphics/mac -I../../Source/WebCore/platform/mac -I../../Source/WebCore/platform/text/mac -I../../Source/WebCore/platform/graphics/harfbuzz -I../../Source/WebCore/platform/graphics/harfbuzz/ng -I../../Source/WebKit/chromium/third_party/apple_webkit -I../../Source/WebKit/mac/WebCoreSupport -Igen/webkit -Igen/webkit/bindings -I../../Source/WTF -I../../Source/JavaScriptCore -I../../Source/WebKit/chromium/third_party/qcms/src -I../../Source/WebKit/chromium/skia/config -I../../Source/WebKit/chromium/third_party/skia/src/core -I../../Source/WebKit/chromium/third_party/skia/include/config -I../../Source/WebKit/chromium/third_party/skia/include/core -I../../Source/WebKit/chromium/third_party/skia/include/effects -I../../Source/WebKit/chromium/third_party/skia/include/pdf -I../../Source/WebKit/chromium/third_party/skia/include/gpu -I../../Source/WebKit/chromium/third_party/skia/include/gpu/gl -I../../Source/WebKit/chromium/third_party/skia/include/pipe -I../../Source/WebKit/chromium/third_party/skia/include/ports -I../../Source/WebKit/chromium/third_party/skia/include/utils -I../../Source/WebKit/chromium/skia/ext -I../../Source/WebKit/chromium/third_party/skia/include/utils/mac -I../../Source/WebKit/chromium/third_party/npapi -I../../Source/WebKit/chromium/third_party/npapi/bindings -I../../Source/WebKit/chromium/v8/include -isysroot /Developer/SDKs/MacOSX10.6.sdk -O3 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.6 -arch i386 -Wglobal-constructors -Wall -Wendif-labels -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene -Wno-c++11-narrowing -Wno-reserved-user-defined-literal -Wno-char-subscripts -Wno-unused-function -Wno-covered-switch-default -Wstring-conversion -Wexit-time-destructors -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -fcolor-diagnostics -fno-strict-aliasing -std=gnu++11  -c ../../Source/WebCore/platform/chromium/support/WebSpeechSynthesizerClientImpl.cpp -o obj/source/webcore/platform/chromium/support/webkit.webspeechsynthesizerclientimpl.o
In file included from ../../Source/WebCore/platform/chromium/support/WebSpeechSynthesizerClientImpl.cpp:27:
../../Source/WebCore/platform/chromium/support/WebSpeechSynthesizerClientImpl.h:56:32: error: private field 'm_synthesizer' is not used [-Werror,-Wunused-private-field]
    PlatformSpeechSynthesizer* m_synthesizer;
                               ^
../../Source/WebCore/platform/chromium/support/WebSpeechSynthesizerClientImpl.h:57:38: error: private field 'm_client' is not used [-Werror,-Wunused-private-field]
    PlatformSpeechSynthesizerClient* m_client;

did this go through EWS?

[anton muhin]

Reverted r146483 for reason:

Breaks

Committed r146489: <http://trac.webkit.org/changeset/146489>

[Dominic Mazzoni]

Created attachment 194306 [details]
Patch

This should fix the compile error, but let's do a full EWS run before cq again

[WebKit Review Bot]

Comment on attachment 194306 [details]
Patch

Attachment 194306 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17290089

[WebKit Review Bot]

Comment on attachment 194306 [details]
Patch

Attachment 194306 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17196694

[Dominic Mazzoni]

Created attachment 194331 [details]
Patch

[Build Bot]

Comment on attachment 194331 [details]
Patch

Attachment 194331 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17291059

[Dominic Mazzoni]

Created attachment 194647 [details]
Patch

[WebKit Review Bot]

Comment on attachment 194647 [details]
Patch

Clearing flags on attachment: 194647

Committed r146724: <http://trac.webkit.org/changeset/146724>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.