[webkit-reviews] review denied: [Bug 106847] Stub out WebSpeech synthesis : [Attachment 182923] initial stubbing out of JS objects and code

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 106847: Stub out WebSpeech synthesis
https://bugs.webkit.org/show_bug.cgi?id=106847

Attachment 182923: initial stubbing out of JS objects and code
https://bugs.webkit.org/attachment.cgi?id=182923&action=review

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


> Source/WebCore/WebCore.xcodeproj/project.pbxproj:26207
>				FD537353137B651800008DCE /* ZeroPole.h in
Headers */,
> +				AA2A5ACE16A485FD00975A25 /*
SpeechSynthesisVoice.h in Headers */,

Would you be willing to sort the xcodeproj file when you land this patch?  We
try to keep the file sorted so that folks have fewer merge conflicts.

> Source/WebCore/Configurations/FeatureDefines.xcconfig:157
> +ENABLE_WEB_SPEECH_SYNTHESIS = ;

nit: I probably would have just called this ENABLE_SPEECH_SYNTHESIS (the "web"
doesn't really mean much in this context).

> Source/WebCore/Modules/speech/DOMWindowSpeechSynthesis.idl:29
> +] interface DOMWindowSpeechSynthesis {

It's interesting that you've created a new interface for synthesis.  I would
have expected you to add this code to DOMWindowSpeech.idl, but I agree that
this approach is cleaner.

> Source/WebCore/Modules/speech/DOMWindowSpeechSynthesis.idl:33
> +    readonly attribute SpeechSynthesis speechSynthesis;
> +    attribute SpeechSynthesisEventConstructor SpeechSynthesisEvent;
> +    attribute SpeechSynthesisUtteranceConstructor SpeechSynthesisUtterance;

Here you've chosen to use unprefixed names.  That's different from what we did
with ENABLE(SCRIPTED_SPEECH), but it's also a fine approach.

> Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:39
> +    static PassRefPtr<SpeechSynthesisUtterance> create(String text);

String -> const String&

There are many occurrences of this issue in this patch.  Generally speaking,
when passing a String as a parameter to a function, you almost always want to
use const String& to avoid ref churn.

> Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:60
> +    EventListener* onstart() { return
getAttributeEventListener(eventNames().startEvent); }
> +    void setOnstart(PassRefPtr<EventListener> listener) {
setAttributeEventListener(eventNames().startEvent, listener); }

I believe we have a macro that does this work for you.	See, for example,
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.h#L264

DEFINE_ATTRIBUTE_EVENT_LISTENER(abort);

> Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:86
> +    SpeechSynthesisUtterance(String text);

Please mark one-argument constructors "explicit".

> Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:97
> +    ScriptExecutionContext* m_scriptExecutionContext;

This is very unlikely to the the right thing to do.  Instead, you probably want
to inherit from ContextDestructionObserver so that your ScriptExecutionContext
pointer won't be left dangling when the ScriptExecutionContext is destroyed.

> Source/WebCore/Modules/speech/SpeechSynthesisVoice.h:43
> +    String voiceURI() const { return m_voiceURI; }
> +    String name() const { return m_name; }
> +    String lang() const { return m_lang; }

These should all return const String& because this object hold the underlying
String object (again to avoid ref churn)

> Source/WebCore/Modules/speech/DOMWindowSpeechSynthesis.h:51
> +    DOMWindow* m_window;

It's unlikely that you should hold onto a DOMWindow*.  Again, there's a risk
that the DOMWindow pointer will be left dangling.  Given that you're inheriting
from DOMWindowProperty, you already have an m_associatedDOMWindow member
variable whose lifetime is managed automatically.

> Source/WebCore/Modules/speech/SpeechSynthesisVoiceList.idl:28
> +] interface SpeechSynthesisVoiceList {

Rather than creating a fake JavaScript array, we should probably change the
spec to use sequence<SpeechSynthesisVoice>.  The platform as a whole is moving
away from these fake arrays because don't work correctly from the perspective
of a JavaScript developer.