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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied  review:
Bug 111695: Implement Web Speech Synthesis for Chromium
https://bugs.webkit.org/show_bug.cgi?id=111695

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.