First part is to enable specific WebSpeech flags and stub out required API.
Created attachment 231961 [details] patch
Attachment 231961 [details] did not pass style-queue: ERROR: Source/WebCore/PlatformEfl.cmake:447: Use lowercase command "list" [command/lowercase] [5] ERROR: Source/WebCore/PlatformEfl.cmake:458: Use lowercase command "list" [command/lowercase] [5] ERROR: Source/cmake/OptionsEfl.cmake:294: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 232786 [details] patch
Created attachment 232865 [details] patch
Created attachment 232866 [details] patch
Comment on attachment 232866 [details] patch Attachment 232866 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5232664806162432 New failing tests: media/W3C/video/src/src_reflects_attribute_not_source_elements.html
Created attachment 232877 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 232866 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=232866&action=review Looks nice for me. Added some comments before formal review. > Source/WebCore/PlatformEfl.cmake:454 > + Modules/speech/DOMWindowSpeechSynthesis.cpp > + Modules/speech/SpeechSynthesis.cpp > + Modules/speech/SpeechSynthesisEvent.cpp > + Modules/speech/SpeechSynthesisUtterance.cpp > + Modules/speech/SpeechSynthesisVoice.cpp > + platform/PlatformSpeechSynthesisUtterance.cpp > + platform/PlatformSpeechSynthesizer.cpp > + platform/PlatformSpeechSynthesisVoice.cpp Since those are not EFL specific they should be added to common Source/WebCore/CMakeLists.txt. I believe that it will bring benefits for other CMake ports. > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' BY APPLE? Please use BSD license. > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:31 > +#include <PlatformSpeechSynthesisUtterance.h> > +#include <PlatformSpeechSynthesisVoice.h> > +#include <PlatformSpeechSynthesizer.h> Do we need to include them if !ENABLE(SPEECH_SYNTHESIS) ? > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:39 > +{ It's good practice to call notImplemented() for stubs. Please consider adding it for below methods as well. > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.h:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' Ditto. > Source/cmake/OptionsEfl.cmake:90 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SPEECH_SYNTHESIS ON) I am wondering whether we are ready to turn it on? Maybe it's better to enable it after adding implementation in platform/efl/PlatformSpeechSynthesisProviderEfl.cpp ? IMHO, it doesn't bring significant benefits now.
Created attachment 232939 [details] patch
(In reply to comment #8) > (From update of attachment 232866 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232866&action=review > > Looks nice for me. Added some comments before formal review. > > > Source/WebCore/PlatformEfl.cmake:454 > > + Modules/speech/DOMWindowSpeechSynthesis.cpp > > + Modules/speech/SpeechSynthesis.cpp > > + Modules/speech/SpeechSynthesisEvent.cpp > > + Modules/speech/SpeechSynthesisUtterance.cpp > > + Modules/speech/SpeechSynthesisVoice.cpp > > + platform/PlatformSpeechSynthesisUtterance.cpp > > + platform/PlatformSpeechSynthesizer.cpp > > + platform/PlatformSpeechSynthesisVoice.cpp > > Since those are not EFL specific they should be added to common Source/WebCore/CMakeLists.txt. I believe that it will bring benefits for other CMake ports. Sounds good. > > > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:13 > > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' > > BY APPLE? Please use BSD license. Done. > > > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:31 > > +#include <PlatformSpeechSynthesisUtterance.h> > > +#include <PlatformSpeechSynthesisVoice.h> > > +#include <PlatformSpeechSynthesizer.h> > > Do we need to include them if !ENABLE(SPEECH_SYNTHESIS) ? Nice catch. Done > > > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:39 > > +{ > > It's good practice to call notImplemented() for stubs. Please consider adding it for below methods as well. > > > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.h:13 > > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' > > Ditto. Done > > > Source/cmake/OptionsEfl.cmake:90 > > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SPEECH_SYNTHESIS ON) > > I am wondering whether we are ready to turn it on? Maybe it's better to enable it after adding implementation in platform/efl/PlatformSpeechSynthesisProviderEfl.cpp ? IMHO, it doesn't bring significant benefits now. It makes sense.
Created attachment 232940 [details] patch
Comment on attachment 232940 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=232940&action=review Thanks for updates. LGTM! > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.h:30 > +#include <wtf/PassRefPtr.h> > +#include <wtf/Vector.h> Nit: before landing, they may be guarded by SPEECH_SYNTHESIS as well.
Comment on attachment 232940 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=232940&action=review > Source/WebCore/platform/efl/PlatformSpeechSynthesizerEfl.cpp:41 > + m_platformSpeechWrapper = std::make_unique<PlatformSpeechSynthesisProviderEfl>(this); How about initializing this variable as below ? PlatformSpeechSynthesizer::PlatformSpeechSynthesizer(PlatformSpeechSynthesizerClient* client) : m_voiceListIsInitialized(false) : m_platformSpeechWrapper(std::make_unique<PlatformSpeechSynthesisProviderEfl>(this)) , m_speechSynthesizerClient(client) > Source/WebCore/platform/efl/PlatformSpeechSynthesizerEfl.cpp:50 > + if (m_platformSpeechWrapper) m_platformSpeechWrapper is unique pointer. So, I think we don't need to check if it is null.
Comment on attachment 232940 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=232940&action=review >> Source/WebCore/platform/efl/PlatformSpeechSynthesizerEfl.cpp:50 >> + if (m_platformSpeechWrapper) > > m_platformSpeechWrapper is unique pointer. So, I think we don't need to check if it is null. Additionally it looks there is no chance it can be null.
(In reply to comment #14) > (From update of attachment 232940 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232940&action=review > > >> Source/WebCore/platform/efl/PlatformSpeechSynthesizerEfl.cpp:50 > >> + if (m_platformSpeechWrapper) > > > > m_platformSpeechWrapper is unique pointer. So, I think we don't need to check if it is null. > > Additionally it looks there is no chance it can be null. I guess unique_ptr as it self would not be null, but dereferenced pointer to managed object could be. It should not happen hopefully. I will prepare a new patch, thanks.
Created attachment 233051 [details] patch
Applied all suggestions
Comment on attachment 233051 [details] patch LGTM. r=me.
Comment on attachment 233051 [details] patch Clearing flags on attachment: 233051 Committed r170003: <http://trac.webkit.org/changeset/170003>
All reviewed patches have been landed. Closing bug.
This is breaking GTK+ build with the following error: In file included from DerivedSources/webkitdom/WebKitDOMDOMWindowPrivate.h:25:0, from ../../Source/WebCore/bindings/gobject/WebKitDOMPrivate.cpp:45: DerivedSources/webkitdom/WebKitDOMDOMWindow.h:774:40: error: WebKitDOMSpeechSynthesis does not name a type
Re-opened since this is blocked by bug 133938
(In reply to comment #22) > This is breaking GTK+ build with the following error: > In file included from DerivedSources/webkitdom/WebKitDOMDOMWindowPrivate.h:25:0, > from ../../Source/WebCore/bindings/gobject/WebKitDOMPrivate.cpp:45: > DerivedSources/webkitdom/WebKitDOMDOMWindow.h:774:40: error: WebKitDOMSpeechSynthesis does not name a type This is because you need to add the idl file to the list of GObject DOM bindings idl files in PlatformGTK.cmake.
(In reply to comment #24) > (In reply to comment #22) > > This is breaking GTK+ build with the following error: > > In file included from DerivedSources/webkitdom/WebKitDOMDOMWindowPrivate.h:25:0, > > from ../../Source/WebCore/bindings/gobject/WebKitDOMPrivate.cpp:45: > > DerivedSources/webkitdom/WebKitDOMDOMWindow.h:774:40: error: WebKitDOMSpeechSynthesis does not name a type > > This is because you need to add the idl file to the list of GObject DOM bindings idl files in PlatformGTK.cmake. Sorry for breaking GTK port. We will try to take care of GTK port further.
Created attachment 233229 [details] patch - attempt to check GTK bots
(In reply to comment #24) > (In reply to comment #22) > > This is breaking GTK+ build with the following error: > > In file included from DerivedSources/webkitdom/WebKitDOMDOMWindowPrivate.h:25:0, > > from ../../Source/WebCore/bindings/gobject/WebKitDOMPrivate.cpp:45: > > DerivedSources/webkitdom/WebKitDOMDOMWindow.h:774:40: error: WebKitDOMSpeechSynthesis does not name a type > > This is because you need to add the idl file to the list of GObject DOM bindings idl files in PlatformGTK.cmake. Thanks, I already added some idls. Hopefully it's enough.
Comment on attachment 233229 [details] patch - attempt to check GTK bots Attachment 233229 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4599252558807040 New failing tests: media/W3C/video/readyState/readyState_during_canplay.html
Created attachment 233241 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 233304 [details] patch - attempt to check GTK bots
Comment on attachment 233304 [details] patch - attempt to check GTK bots View in context: https://bugs.webkit.org/attachment.cgi?id=233304&action=review It seems the build failure is because the gtk-doc generator doesn't find a symbols file, this shouldn't be a problem now if you add the idls to the unstable list. > Source/WebCore/PlatformGTK.cmake:463 > + Modules/speech/SpeechSynthesis.idl > + Modules/speech/SpeechSynthesisEvent.idl > + Modules/speech/SpeechSynthesisUtterance.idl > + Modules/speech/SpeechSynthesisVoice.idl This has changed today, now we have GObjectDOMBindingsStable_IDL_FILES and GObjectDOMBindingsUnstable_IDL_FILES, please make sure these idls are added to GObjectDOMBindingsUnstable_IDL_FILES.
Created attachment 233423 [details] patch - attempt to check gtk-wk2 ews
Comment on attachment 233423 [details] patch - attempt to check gtk-wk2 ews EWS look good for EFL and GTK. I guess finally it could land.
Comment on attachment 233423 [details] patch - attempt to check gtk-wk2 ews rs=me
Comment on attachment 233423 [details] patch - attempt to check gtk-wk2 ews Clearing flags on attachment: 233423 Committed r170288: <http://trac.webkit.org/changeset/170288>
Hello Mr. Carlos Garcia Campos In reference to comment number #24 I am trying to add Web Speech feature to Webkit version number 150045. I am getting the same error: In file included from DerivedSources/webkitdom/WebKitDOMDOMWindowPrivate.h:25:0, from ../../Source/WebCore/bindings/gobject/WebKitDOMPrivate.cpp:45: DerivedSources/webkitdom/WebKitDOMDOMWindow.h:774:40: error: WebKitDOMSpeechSynthesis does not name a type In webkit version 150045 the file PlatformGTK.cmake does not exist. I have tried adding the .idl files in DerivedSources.pri, but it did not work. Could you please help.