Let's start by adding the necessary stub files and making it compile when the ENABLE_SPEECH_SYNTHESIS flag is enabled.
Created attachment 183635 [details] Patch
What is the intent of SpeechSynthesisChromium.cpp? Why would there be something Chromium specific at this layer?
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
(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
(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.
(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
(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.
Created attachment 183687 [details] Patch
(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.
> 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.
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.
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.
Comment on attachment 183687 [details] Patch Clearing flags on attachment: 183687 Committed r140297: <http://trac.webkit.org/changeset/140297>
All reviewed patches have been landed. Closing bug.