See https://bugs.webkit.org/show_bug.cgi?id=107382 for discussion; the idea is to move platform-specific code to the platform directory. Questions: * Is it okay to have a method defined in a header file, but have the method implemented differently in different platform source files, without subclassing? * If not, is there a preferred style for factory classes? * What's a good example of a module that implements the cross-platform abstraction correctly, in terms of code style? Ideally, pick something that's NOT a core WebCore feature (i.e. some platforms might disable it) because core features tend to be much more complicated or have other requirements. Finally, could we either document the best practices somewhere, or file bugs to refactor all of the modules that violate this rule? As it is, WebCore has several examples of both approaches so there's no way for a contributor to know that one style is preferred.
> See https://bugs.webkit.org/show_bug.cgi?id=107382 for discussion; the idea is to move platform-specific code to the platform directory. > > Questions: > * Is it okay to have a method defined in a header file, but have the method implemented differently in different platform source files, without subclassing? Yes. This is a common pattern we use to avoid introducing unnecessary virtual functions. > * If not, is there a preferred style for factory classes? N/A > * What's a good example of a module that implements the cross-platform abstraction correctly, in terms of code style? Ideally, pick something that's NOT a core WebCore feature (i.e. some platforms might disable it) because core features tend to be much more complicated or have other requirements. Sam might have some ideas here, but each feature has its own quirks, and I'm not sure off-hand of one that's a particularly good showpiece. One that we've done relatively recently is MEDIA_STREAM. Unfortunately, there aren't really non-Chromium implementations of MEDIA_STREAM, and that feature is likely slightly different from what you want because so much of the "guts" is in the WebRTC library. WEB_AUDIO might be a good example. I'm pretty sure I know what Sam has in mind, and I'm happy to help by reviewing patches. Perhaps we can make SpeechSynthesis a good example for others to follow. > Finally, could we either document the best practices somewhere, or file bugs to refactor all of the modules that violate this rule? As it is, WebCore has several examples of both approaches so there's no way for a contributor to know that one style is preferred. I would like to clean up all the features to use a consistent approach, but it's a big unsexy project and I haven't managed to convince anyone to do it (yet). :)
The general approach is that SpeechSynthesis.cpp in Source/WebCore/Modules/speech/SpeedSynthesis.cpp should be implemented in terms of a PlatformSpeechSynthesis.h header from Source/WebCore/platform/PlatformSpeechSynthesis.h. We'd then have two cpp files that implement the PlatformSpeechSynthesis.h interface, SpeechSynthesisChromium.cpp and SpeechSynthesisMac.cpp (in the appropriate folder). SpeechSynthesisChromium.cpp would implement the PlatformSpeechSynthesis API in terms of the Chromium WebKit API (i.e., in terms of an API we define in Source/Platform/chromium/public). SpeechSynthesisMac.cpp would implement the PlatformSpeechSynthesis API in terms of Mac OS X APIs. (Other ports could implement the PlatformSpeechSynthesis API in terms of facilities available on their platform.) The idea behind this approach is that SpeechSynthesis.cpp should be in charge of the "webby" parts of the feature whereas PlatformSpeechSynthesis should be a simple abstraction over the multitude of libraries that you could use to implement the web feature. I can be tricky to design a PlatformSpeechSynthesis that works for many platforms, but we try to do our best with the examples we have. In general, I would recommend making the Chromium Platform API similar to the PlatformSpeechSynthesis API to make it as easy as possible to implement the latter in terms of the former. I suspect we'll have less flexibility in the design of the Mac OS X API, so that often drives the design of the platform abstraction somewhat.
Created attachment 184309 [details] patch to check on whether this is the right approach
Comment on attachment 184309 [details] patch to check on whether this is the right approach Hi Adam, does this look like the right approach? The platformObject will need to call back into the WebCore/SpeechSynthesis object in order to post notifications and such, hence the need to pass in the core object to the platform object
Comment on attachment 184309 [details] patch to check on whether this is the right approach View in context: https://bugs.webkit.org/attachment.cgi?id=184309&action=review > Source/WebCore/Modules/speech/SpeechSynthesis.cpp:42 > + m_platformSpeechSynthesis = adoptRef(new PlatformSpeechSynthesis(this)); We should check with Sam, but my guess is that he won't be happy with passing |this| to the platform object because it introduces a dependency from WebCore/platform to the rest of WebCore.
(In reply to comment #5) > (From update of attachment 184309 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184309&action=review > > > Source/WebCore/Modules/speech/SpeechSynthesis.cpp:42 > > + m_platformSpeechSynthesis = adoptRef(new PlatformSpeechSynthesis(this)); > > We should check with Sam, but my guess is that he won't be happy with passing |this| to the platform object because it introduces a dependency from WebCore/platform to the rest of WebCore. That's one reason why I liked have one SpeechSynthesis object with certain methods implemented in the platform versions. The issue is that when a speech job finishes, we need to inform SpeechSynthesis so that it can fire back the SpeechSynthesisEvent. Do you have any thoughts how that should be factored?
> Do you have any thoughts how that should be factored? I think we should get Sam's feedback.
Comment on attachment 184309 [details] patch to check on whether this is the right approach View in context: https://bugs.webkit.org/attachment.cgi?id=184309&action=review >>> Source/WebCore/Modules/speech/SpeechSynthesis.cpp:42 >>> + m_platformSpeechSynthesis = adoptRef(new PlatformSpeechSynthesis(this)); >> >> We should check with Sam, but my guess is that he won't be happy with passing |this| to the platform object because it introduces a dependency from WebCore/platform to the rest of WebCore. > > That's one reason why I liked have one SpeechSynthesis object with certain methods implemented in the platform versions. > > The issue is that when a speech job finishes, we need to inform SpeechSynthesis so that it can fire back the SpeechSynthesisEvent. > > Do you have any thoughts how that should be factored? Yes, this will definitely be a two-way API in other places too - for example, in the Chromium implementation, voices can be added and deleted dynamically, so the platform will need to be able to call a method on the WebCore SpeechSynthesis object to tell it to update its list of voices.
Created attachment 184630 [details] patch with header file
To do a better review of this, I'd like to understand what parts that the underlying platform will be providing. But without that, here's a sketch of a design that generally works: in WebCore/platform/speech/ class PlatformSpeechSynthesisVoice { public: ... }; class PlatformSpeechSynthesizerClient { public: virtual void voicesDidChange(); ... }; class PlatformSpeechSynthesizer { public: PlatformSpeechSynthesize(PlatformSpeechSynthesizerClient*); ... } in WebCore/Modules/speech class SpeechSynthesis : PlatformSpeechSynthesizerClient, RefPtr<SpeechSynthesis> { public: ... private: PlatformSpeechSynthesizer m_platformSpeechSynthesizer; }; class SpeechSynthesisVoice : RefCounted<SpeechSynthesisVoice> { public: .... private: PlatformSpeechSynthesisVoice m_platformVoice; }
(In reply to comment #10) > To do a better review of this, I'd like to understand what parts that the underlying platform will be providing. But without that, here's a sketch of a design that generally works: > > in WebCore/platform/speech/ > > class PlatformSpeechSynthesisVoice { > public: > ... > }; > > class PlatformSpeechSynthesizerClient { > public: > virtual void voicesDidChange(); > ... > }; > > class PlatformSpeechSynthesizer { > public: > PlatformSpeechSynthesize(PlatformSpeechSynthesizerClient*); > ... > } > > > in WebCore/Modules/speech > > class SpeechSynthesis : PlatformSpeechSynthesizerClient, RefPtr<SpeechSynthesis> { > public: > ... > > private: > PlatformSpeechSynthesizer m_platformSpeechSynthesizer; > }; > > class SpeechSynthesisVoice : RefCounted<SpeechSynthesisVoice> { > public: > .... > > private: > PlatformSpeechSynthesisVoice m_platformVoice; > } The platform needs to provide the data for each voice (name, identifier, etc). This information is static for the voice and doesn't really change. (As such, I don't think it's necessary at this point to have a PlatformSpeechSyntVoice object because we can populate a SpeechSynthVoice object with all the necessary data.. On the mac, there's not a 'voice object' either. ) The platform also needs to provide - a way to start, pause, resume, stop speaking - a way to query the state of whether speaking is happening or is paused - a way to set rate/pitch/volume The platform will also provide callbacks for when things happen. These need to get turned into events - speaking job started/finished/had error - speaking job reached a certain mark (word, sentence) re-org forthcoming...
Created attachment 184981 [details] refactor idea How does this look?
Comment on attachment 184981 [details] refactor idea My only question is what if a particular platform needs member variables - are we planning to have platform-specific sections in PlatformSpeechSynthesizer with ifdefs, or should those platforms subclass? Other than that, the design looks fine.
(In reply to comment #13) > (From update of attachment 184981 [details]) > My only question is what if a particular platform needs member variables - are we > planning to have platform-specific sections in PlatformSpeechSynthesizer with > ifdefs, or should those platforms subclass? > > Other than that, the design looks fine. I don't know the answer, but Accessibility related code has ifdefs for platform variables (as a point of reference)
Sam and Adam, any other thoughts on this? Let's go ahead with this refactoring if there are no objections, I'd like to start the Chromium port. The only way to truly validate the design is to finish the code!
Attachment 184981 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/Modules/speech/SpeechSynthesis.cpp', u'Source/WebCore/Modules/speech/SpeechSynthesis.h', u'Source/WebCore/Modules/speech/mac/SpeechSynthesisMac.mm', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/PlatformSpeechSynthesizer.h', u'Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm']" exit_code: 1 Source/WebCore/platform/PlatformSpeechSynthesizer.h:36: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/Modules/speech/SpeechSynthesis.h:33: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 184981 [details] refactor idea View in context: https://bugs.webkit.org/attachment.cgi?id=184981&action=review > Source/WebCore/platform/PlatformSpeechSynthesizer.h:41 > + virtual void didStartSpeaking(SpeechSynthesisUtterance*) = 0; > + virtual void didFinishSpeaking(SpeechSynthesisUtterance*) = 0; > + virtual void speakingErrorOccurred(SpeechSynthesisUtterance*) = 0; These are still a layering violation. Code in WebCore/platform is not allowed to depend on types outside of WebCore/platform. Here, you've got a dependency on http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h which isn't part of the platform abstraction. > Source/WebCore/platform/PlatformSpeechSynthesizer.h:48 > + PlatformSpeechSynthesizer(PlatformSpeechSynthesizerClient*); Please mark one-argument constructors "explicit"
Comment on attachment 184981 [details] refactor idea View in context: https://bugs.webkit.org/attachment.cgi?id=184981&action=review >> Source/WebCore/platform/PlatformSpeechSynthesizer.h:41 >> + virtual void speakingErrorOccurred(SpeechSynthesisUtterance*) = 0; > > These are still a layering violation. Code in WebCore/platform is not allowed to depend on types outside of WebCore/platform. Here, you've got a dependency on http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h which isn't part of the platform abstraction. Isn't this how other "Client" platform objects operate? like ScrollbarThemeClient.h for instance Can you give a little more guidance how this should work? The platform code needs to tell the WebCore code that you started speaking, for instance. Thanks
So if I understand correctly, we want a PlatformSpeechSynthesisUtterance that actually has the data fields. Each SpeechSynthesisUtterance will own a PlatformSpeechSynthesisUtterance and the getters/setters will just modify the owned PlatformSpeechSynthesisUtterance.
Comment on attachment 184981 [details] refactor idea View in context: https://bugs.webkit.org/attachment.cgi?id=184981&action=review >>> Source/WebCore/platform/PlatformSpeechSynthesizer.h:41 >>> + virtual void speakingErrorOccurred(SpeechSynthesisUtterance*) = 0; >> >> These are still a layering violation. Code in WebCore/platform is not allowed to depend on types outside of WebCore/platform. Here, you've got a dependency on http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h which isn't part of the platform abstraction. > > Isn't this how other "Client" platform objects operate? like ScrollbarThemeClient.h for instance > > Can you give a little more guidance how this should work? The platform code needs to tell the WebCore code that you started speaking, for instance. > > Thanks You need to pass PlatformSpeechSynthesisUtterance* here.
Comment on attachment 184981 [details] refactor idea View in context: https://bugs.webkit.org/attachment.cgi?id=184981&action=review > Source/WebCore/platform/PlatformSpeechSynthesizer.h:50 > + void getVoiceList(Vector<RefPtr<SpeechSynthesisVoice> >&); This should be a Vector<RefPtr<PlatformSpeechSynthesisVoice> > and you just return it. There shouldn't be a good reason to use the get() pattern here. > Source/WebCore/platform/PlatformSpeechSynthesizer.h:51 > + void speak(SpeechSynthesisUtterance*); This should take a PlatformSpeechSynthesisUtterance > Source/WebCore/Modules/speech/SpeechSynthesis.cpp:69 > + m_platformSpeechSynthesizer.speak(utterance); This should probably do something like. m_platformSpeechSynthesizer.speak(utterance.platformUtterence());
Created attachment 186430 [details] patch I think this is more in line with what people were thinking. Once I started implementing, it became clear what Sam was trying to achieve with his comments in terms of layering abstraction
Attachment 186430 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/speech/DOMWindowSpeechSynthesis.cpp', u'Source/WebCore/Modules/speech/SpeechSynthesis.cpp', u'Source/WebCore/Modules/speech/SpeechSynthesis.h', u'Source/WebCore/Modules/speech/SpeechSynthesisUtterance.cpp', u'Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h', u'Source/WebCore/Modules/speech/SpeechSynthesisVoice.cpp', u'Source/WebCore/Modules/speech/SpeechSynthesisVoice.h', u'Source/WebCore/Modules/speech/mac/SpeechSynthesisMac.mm', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/PlatformSpeechSynthesis.h', u'Source/WebCore/platform/PlatformSpeechSynthesisUtterance.cpp', u'Source/WebCore/platform/PlatformSpeechSynthesisUtterance.h', u'Source/WebCore/platform/PlatformSpeechSynthesisVoice.cpp', u'Source/WebCore/platform/PlatformSpeechSynthesisVoice.h', u'Source/WebCore/platform/PlatformSpeechSynthesizer.cpp', u'Source/WebCore/platform/PlatformSpeechSynthesizer.h', u'Source/WebCore/platform/mac/PlatformSpeechSynthesisMac.mm', u'Source/WebCore/platform/mac/PlatformSpeechSynthesizerMac.mm']" exit_code: 1 Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:60: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/Modules/speech/SpeechSynthesis.h:33: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/Modules/speech/SpeechSynthesis.h:33: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/Modules/speech/SpeechSynthesis.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/PlatformSpeechSynthesizer.h:37: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/Modules/speech/SpeechSynthesisVoice.h:40: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/Modules/speech/SpeechSynthesisVoice.h:49: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/platform/PlatformSpeechSynthesisVoice.cpp:46: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebCore/Modules/speech/SpeechSynthesisVoice.cpp:33: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/Modules/speech/SpeechSynthesisVoice.cpp:38: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/platform/PlatformSpeechSynthesisVoice.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 11 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 186432 [details] patch
(In reply to comment #22) > Created an attachment (id=186430) [details] > patch > > I think this is more in line with what people were thinking. Once I started implementing, it became clear what Sam was trying to achieve with his comments in terms of layering abstraction Any comments on this patch?
> Any comments on this patch? @Sam: ^^^
Comment on attachment 186432 [details] patch Clearing flags on attachment: 186432 Committed r142320: <http://trac.webkit.org/changeset/142320>
All reviewed patches have been landed. Closing bug.