Bug 107382 – Make SpeechSynthesis compile in the Chromium port

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

Let's start by adding the necessary stub files and making it compile when the ENABLE_SPEECH_SYNTHESIS flag is enabled.

[Dominic Mazzoni]

Created attachment 183635 [details]
Patch

[Sam Weinig]

What is the intent of SpeechSynthesisChromium.cpp?  Why would there be something Chromium specific at this layer?

[chris fleizach]

Comment on attachment 183635 [details]
Patch

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

> Source/WebCore/Modules/speech/chromium/SpeechSynthesisChromium.cpp:48
> +}

i think we'll be able to move some of these calls into the SpeechSynthesis.h (i've done that in my latest patch), but I can move that around as needed

> Source/WebCore/WebCore.gypi:901
> +            'Modules/speech/DOMWindowSpeechSynthesis.cpp',

this might not be in alpha order

> Source/WebKit/chromium/features.gypi:110
> +      'ENABLE_SPEECH_SYNTHESIS=0',

looks like this is not in alpha order. should be after enable_smooth

[Adam Barth]

(In reply to comment #2)
> What is the intent of SpeechSynthesisChromium.cpp?  Why would there be something Chromium specific at this layer?

Presumably for the same reason there's something Mac-specific: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/speech/mac/SpeechSynthesisMac.mm

[Sam Weinig]

(In reply to comment #4)
> (In reply to comment #2)
> > What is the intent of SpeechSynthesisChromium.cpp?  Why would there be something Chromium specific at this layer?
> 
> Presumably for the same reason there's something Mac-specific: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/speech/mac/SpeechSynthesisMac.mm

I see.  If there is something that needs to be platform specific, it should be in the platform directory.

[Dominic Mazzoni]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > What is the intent of SpeechSynthesisChromium.cpp?  Why would there be something Chromium specific at this layer?
> > 
> > Presumably for the same reason there's something Mac-specific: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/speech/mac/SpeechSynthesisMac.mm
> 
> I see.  If there is something that needs to be platform specific, it should be in the platform directory.

It seems like a pretty common pattern to have platform-specific subdirectories inside of WebCore modules, e.g.:

accessibility/[atk,chromium,mac,etc.]
editing/[blackberry,chromium,mac,qt,etc.]
history/[blackberry,mac,qt,etc.]
Modules/filesystem/chromium

Is this pattern discouraged now, and is there a meta-bug to refactor all of the existing cases? If not, I'd rather stay consistent with similar code, and in particular for an optional module like this, I think it's more readable to keep all of the platform implementations together.

- Dominic

[Sam Weinig]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #2)
> > > > What is the intent of SpeechSynthesisChromium.cpp?  Why would there be something Chromium specific at this layer?
> > > 
> > > Presumably for the same reason there's something Mac-specific: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/speech/mac/SpeechSynthesisMac.mm
> > 
> > I see.  If there is something that needs to be platform specific, it should be in the platform directory.
> 
> It seems like a pretty common pattern to have platform-specific subdirectories inside of WebCore modules, e.g.:
> 
> accessibility/[atk,chromium,mac,etc.]
> editing/[blackberry,chromium,mac,qt,etc.]
> history/[blackberry,mac,qt,etc.]
> Modules/filesystem/chromium
> 
> Is this pattern discouraged now, and is there a meta-bug to refactor all of the existing cases? If not, I'd rather stay consistent with similar code, and in particular for an optional module like this, I think it's more readable to keep all of the platform implementations together.
> 
> - Dominic

Its true that it is unfortunate that there are places where this is not true, but that should not stop us from getting it right going forward.

[Dominic Mazzoni]

Created attachment 183687 [details]
Patch

[Sam Weinig]

(In reply to comment #8)
> Created an attachment (id=183687) [details]
> Patch

You seemed to have ignored my feedback.  Platform specific parts should be implemented in WebCore/platform and the Mac part should fixed as well.

[Adam Barth]

> You seemed to have ignored my feedback.  Platform specific parts should be implemented in WebCore/platform and the Mac part should fixed as well.

Perhaps cfleizach would be willing to fix the Mac port.  It's unfortunate that you didn't give this feedback when the Mac code landed.

[Adam Barth]

Comment on attachment 183687 [details]
Patch

IMHO, this stub file is fine given that it's just fixing the compile.  We should refactor both the Chromium and Mac versions of this file before adding any actual code to either of them, however.

Sam is likely also asking for an extra layer of abstraction so that SpeechSynthesis.cpp can be implemented in terms of a common WebCore/platform API that's agnostic as to which platform backend is actually implementing the synthesis.

[Dominic Mazzoni]

I filed https://bugs.webkit.org/show_bug.cgi?id=107414 for the refactoring. Sam, please help address some of my questions there and then I'll be happy to work on this, both Chromium and Mac parts.

[WebKit Review Bot]

Comment on attachment 183687 [details]
Patch

Clearing flags on attachment: 183687

Committed r140297: <http://trac.webkit.org/changeset/140297>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.