Bug 116438

Summary: [EFL] Platform support for WebSpeech feature.
Product: WebKit Reporter: Krzysztof Czech <k.czech>
Component: WebKit EFLAssignee: Krzysztof Czech <k.czech>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, bunhere, cdumez, cgarcia, commit-queue, gyuyoung.kim, gyuyoung.kim, jinwoo7.song, lucas.de.marchi, rakuco, rego, rniwa, sergio, tripta.g
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 133938    
Bug Blocks: 116314    
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
patch
none
patch
none
patch
none
patch - attempt to check GTK bots
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
patch - attempt to check GTK bots
none
patch - attempt to check gtk-wk2 ews none

Description Krzysztof Czech 2013-05-20 06:34:31 PDT
First part is to enable specific WebSpeech flags and stub out required API.
Comment 1 Krzysztof Czech 2014-05-23 06:46:08 PDT
Created attachment 231961 [details]
patch
Comment 2 WebKit Commit Bot 2014-05-23 07:58:49 PDT
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.
Comment 3 Krzysztof Czech 2014-06-10 07:55:41 PDT
Created attachment 232786 [details]
patch
Comment 4 Krzysztof Czech 2014-06-11 08:24:58 PDT
Created attachment 232865 [details]
patch
Comment 5 Krzysztof Czech 2014-06-11 08:27:57 PDT
Created attachment 232866 [details]
patch
Comment 6 Build Bot 2014-06-11 11:38:45 PDT
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
Comment 7 Build Bot 2014-06-11 11:38:50 PDT
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 8 Grzegorz Czajkowski 2014-06-12 00:33:57 PDT
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.
Comment 9 Krzysztof Czech 2014-06-12 02:57:28 PDT
Created attachment 232939 [details]
patch
Comment 10 Krzysztof Czech 2014-06-12 03:00:12 PDT
(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.
Comment 11 Krzysztof Czech 2014-06-12 03:01:00 PDT
Created attachment 232940 [details]
patch
Comment 12 Grzegorz Czajkowski 2014-06-12 05:14:51 PDT
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 13 Gyuyoung Kim 2014-06-13 00:50:27 PDT
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 14 Gyuyoung Kim 2014-06-13 05:44:44 PDT
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.
Comment 15 Krzysztof Czech 2014-06-13 06:22:51 PDT
(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.
Comment 16 Krzysztof Czech 2014-06-13 08:16:38 PDT
Created attachment 233051 [details]
patch
Comment 17 Krzysztof Czech 2014-06-13 08:17:09 PDT
Applied all suggestions
Comment 18 Gyuyoung Kim 2014-06-15 07:15:23 PDT
Comment on attachment 233051 [details]
patch

LGTM. r=me.
Comment 19 Gyuyoung Kim 2014-06-15 07:17:44 PDT
Comment on attachment 233051 [details]
patch

LGTM. r=me.
Comment 20 WebKit Commit Bot 2014-06-16 01:26:13 PDT
Comment on attachment 233051 [details]
patch

Clearing flags on attachment: 233051

Committed r170003: <http://trac.webkit.org/changeset/170003>
Comment 21 WebKit Commit Bot 2014-06-16 01:26:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Manuel Rego Casasnovas 2014-06-16 03:21:40 PDT
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
Comment 23 WebKit Commit Bot 2014-06-16 05:12:00 PDT
Re-opened since this is blocked by bug 133938
Comment 24 Carlos Garcia Campos 2014-06-16 05:43:11 PDT
(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.
Comment 25 Gyuyoung Kim 2014-06-16 06:06:32 PDT
(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.
Comment 26 Krzysztof Czech 2014-06-17 08:15:32 PDT
Created attachment 233229 [details]
patch - attempt to check GTK bots
Comment 27 Krzysztof Czech 2014-06-17 08:17:53 PDT
(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 28 Build Bot 2014-06-17 10:26:14 PDT
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
Comment 29 Build Bot 2014-06-17 10:26:21 PDT
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
Comment 30 Krzysztof Czech 2014-06-18 03:25:17 PDT
Created attachment 233304 [details]
patch - attempt to check GTK bots
Comment 31 Carlos Garcia Campos 2014-06-20 05:39:49 PDT
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.
Comment 32 Krzysztof Czech 2014-06-20 05:56:39 PDT
Created attachment 233423 [details]
patch - attempt to check gtk-wk2 ews
Comment 33 Krzysztof Czech 2014-06-20 06:49:18 PDT
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 34 Csaba Osztrogonác 2014-06-23 03:19:41 PDT
Comment on attachment 233423 [details]
patch - attempt to check gtk-wk2 ews

rs=me
Comment 35 WebKit Commit Bot 2014-06-23 05:14:02 PDT
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>
Comment 36 WebKit Commit Bot 2014-06-23 05:14:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Tripta 2016-06-06 22:02:00 PDT
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.