Bug 116438 – [EFL] Platform support for WebSpeech feature.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Krzysztof Czech]

First part is to enable specific WebSpeech flags and stub out required API.

[Krzysztof Czech]

Created attachment 231961 [details]
patch

[WebKit Commit Bot]

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.

[Krzysztof Czech]

Created attachment 232786 [details]
patch

[Krzysztof Czech]

Created attachment 232865 [details]
patch

[Krzysztof Czech]

Created attachment 232866 [details]
patch

[Build Bot]

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

[Build Bot]

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

[Grzegorz Czajkowski]

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.

[Krzysztof Czech]

Created attachment 232939 [details]
patch

[Krzysztof Czech]

(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.

[Krzysztof Czech]

Created attachment 232940 [details]
patch

[Grzegorz Czajkowski]

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.

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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.

[Krzysztof Czech]

(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.

[Krzysztof Czech]

Created attachment 233051 [details]
patch

[Krzysztof Czech]

Applied all suggestions

[Gyuyoung Kim]

Comment on attachment 233051 [details]
patch

LGTM. r=me.

[Gyuyoung Kim]

Comment on attachment 233051 [details]
patch

LGTM. r=me.

[WebKit Commit Bot]

Comment on attachment 233051 [details]
patch

Clearing flags on attachment: 233051

Committed r170003: <http://trac.webkit.org/changeset/170003>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Manuel Rego Casasnovas]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 133938

[Carlos Garcia Campos]

(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.

[Gyuyoung Kim]

(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.

[Krzysztof Czech]

Created attachment 233229 [details]
patch - attempt to check GTK bots

[Krzysztof Czech]

(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.

[Build Bot]

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

[Build Bot]

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

[Krzysztof Czech]

Created attachment 233304 [details]
patch - attempt to check GTK bots

[Carlos Garcia Campos]

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.

[Krzysztof Czech]

Created attachment 233423 [details]
patch - attempt to check gtk-wk2 ews

[Krzysztof Czech]

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.

[Csaba Osztrogonác]

Comment on attachment 233423 [details]
patch - attempt to check gtk-wk2 ews

rs=me

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tripta]

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.