Bug 137742 – [EFL] Utilize Eail library to support accessibility in EFL

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Krzysztof Czech]

Enlightenment Accessibility Implementation Library (EAIL) is a component that makes EFL widgets accessible through so called "accessibility stack" in Linux.
Accessibility stack is made of three components ATK, ATK-BRIDGE, AT-SPI2 and having the last puzzle which is EAIL, EFL widgets are properly recognizable by AT clients such as Orca.

[Krzysztof Czech]

Created attachment 239873 [details]
proposed patch

[WebKit Commit Bot]

Attachment 239873 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:45:  _elm_win_list is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

(In reply to comment #2)
> Attachment 239873 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:45:  _elm_win_list is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Total errors found: 1 in 7 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Is something that comes from EAIL and should have underscores.

[Ryuan Choi]

Comment on attachment 239873 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239873&action=review

> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:43
> +* It does not make any trouble to Eail, because WebKit does not deal with any eail widget.

I don't know what it means.

Does elementary depend on external instance(_elm_win_list) implicitly for ACCESSIBILITY?

> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:46
> +Eina_List* _elm_win_list = nullptr;
> +

I think that code should be below of all includes.

> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:48
> +typedef int (*EailInitFunction)(void*);
> +typedef int (*EailShutdownFunction)(void*);

Should we need two typedef?

> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:52
> +    static NeverDestroyed<WebKit::Module> eail(ACCESSIBILITY_EAIL_DIRECTORY"/libeail-1.0.0.so");

And we need space between ACCESSIBILITY_EAIL_DIRECTORY and ...

> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:123
> +            if (EailInitFunction eailInitFunction = eail().functionPointer<EailInitFunction>("elm_modapi_init"))

Although I don't have strong objection, the name(elm_xxx) looks not good.

> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:124
> +                eailInitFunction(0);

nullptr?

> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:149
> +            eailShutdownFunction(0);

Ditto.

> Source/cmake/OptionsEfl.cmake:70
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY_EAIL_SUPPORT ON)

Should we need this macro?

In order to support ACCESSIBILITY in ewebkit, can we have another option?
If not, I think that we'd better just to use ENABLE_ACCESSIBILITY.

> Source/cmake/OptionsEfl.cmake:300
> +        set(ACCESSIBILITY_EAIL_DIRECTORY "${CMAKE_SOURCE_DIR}/WebKitBuild/Dependencies/Root/lib")
> +        add_definitions(-DENABLE_ACCESSIBILITY_EAIL_SUPPORT=1)
> +        add_definitions(-DACCESSIBILITY_EAIL_DIRECTORY=\"${ACCESSIBILITY_EAIL_DIRECTORY}\")

We should find right path wherever eail is installed.
WebKitBuild directory is only for deverlopers who develop ewebkit without installation.

> Tools/efl/jhbuild.modules:57
> +    <repository type="git" name="github"
> +      href="https://github.com/czechk"/>

Can I know any plan to upstream eail to enlightenment?

[Krzysztof Czech]

(In reply to comment #4)
> (From update of attachment 239873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239873&action=review
> 
> > Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:43
> > +* It does not make any trouble to Eail, because WebKit does not deal with any eail widget.
> 
> I don't know what it means.
Basically, setting this variable here to nullptr will not cause any problems in terms of loading eail later, so we are safe here.
> 
> Does elementary depend on external instance(_elm_win_list) implicitly for ACCESSIBILITY?
Well EAIL depends on external instance (_elm_win_list) that is defined in Elementary. So to not have undefined reference it needs to be defined here.
> 
> > Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:46
> > +Eina_List* _elm_win_list = nullptr;
> > +
> 
> I think that code should be below of all includes.
I guess here is more consistent place. There won't be any other place that'll deal with EAIL. 
> 
> > Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:48
> > +typedef int (*EailInitFunction)(void*);
> > +typedef int (*EailShutdownFunction)(void*);
> 
> Should we need two typedef?
> 
> > Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:52
> > +    static NeverDestroyed<WebKit::Module> eail(ACCESSIBILITY_EAIL_DIRECTORY"/libeail-1.0.0.so");
> 
> And we need space between ACCESSIBILITY_EAIL_DIRECTORY and ...
> 
> > Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:123
> > +            if (EailInitFunction eailInitFunction = eail().functionPointer<EailInitFunction>("elm_modapi_init"))
> 
> Although I don't have strong objection, the name(elm_xxx) looks not good.
That's how elementary modules work, they call elm_modapi_init/shutdown to load/shutdown modules while elm_init is called. I do not want to change it, because MiniBrowser may use it as well.
Basically loading EAIL should be transparent to the client application.
> 
> > Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:124
> > +                eailInitFunction(0);
> 
> nullptr?
Right
> 
> > Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:149
> > +            eailShutdownFunction(0);
> 
> Ditto.
Right
> 
> > Source/cmake/OptionsEfl.cmake:70
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY_EAIL_SUPPORT ON)
> 
> Should we need this macro?
EAIL could be easily turner off.
> 
> In order to support ACCESSIBILITY in ewebkit, can we have another option?
> If not, I think that we'd better just to use ENABLE_ACCESSIBILITY.
There is already a global macro HAVE_ACCESSIBILITY. I could change it to ENABLE_ACCESSIBILITY
> 
> > Source/cmake/OptionsEfl.cmake:300
> > +        set(ACCESSIBILITY_EAIL_DIRECTORY "${CMAKE_SOURCE_DIR}/WebKitBuild/Dependencies/Root/lib")
> > +        add_definitions(-DENABLE_ACCESSIBILITY_EAIL_SUPPORT=1)
> > +        add_definitions(-DACCESSIBILITY_EAIL_DIRECTORY=\"${ACCESSIBILITY_EAIL_DIRECTORY}\")
> 
> We should find right path wherever eail is installed.
> WebKitBuild directory is only for deverlopers who develop ewebkit without installation.
Sounds reasonable any ideas where to put it.
> 
> > Tools/efl/jhbuild.modules:57
> > +    <repository type="git" name="github"
> > +      href="https://github.com/czechk"/>
> 
> Can I know any plan to upstream eail to enlightenment?
I wish I could not that either. I will try to get some info about it.

[Ryuan Choi]

Comment on attachment 239873 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239873&action=review

>>> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:46
>>> +
>> 
>> I think that code should be below of all includes.
> 
> I guess here is more consistent place. There won't be any other place that'll deal with EAIL.

I think that we'd better to separate includes and codes,
although I can see that below ECORE_X related codes look also similar (I also feels not good).

>>> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:48
>>> +typedef int (*EailShutdownFunction)(void*);
>> 
>> Should we need two typedef?
> 
> That's how elementary modules work, they call elm_modapi_init/shutdown to load/shutdown modules while elm_init is called. I do not want to change it, because MiniBrowser may use it as well.
> Basically loading EAIL should be transparent to the client application.

I means that we don't need to make two typedefs for instances which have same signatures.

>>> Source/cmake/OptionsEfl.cmake:70
>>> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY_EAIL_SUPPORT ON)
>> 
>> Should we need this macro?
>> 
>> In order to support ACCESSIBILITY in ewebkit, can we have another option?
>> If not, I think that we'd better just to use ENABLE_ACCESSIBILITY.
> 
> EAIL could be easily turner off.

At least, this is only EFL specific. so we don't need to add it to WebKitFeatures.cmake.

And I want to kwnow whether only ACCESSIBILITY mode is meaningfull.
If not, we can turn off ACCESSIBILITY if eail is not available.
Or, we can check the existance of eail and enable it.

[Krzysztof Czech]

(In reply to comment #6)
> (From update of attachment 239873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239873&action=review
> 
> >>> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:46
> >>> +
> >> 
> >> I think that code should be below of all includes.
> > 
> > I guess here is more consistent place. There won't be any other place that'll deal with EAIL.
> 
> I think that we'd better to separate includes and codes,
> although I can see that below ECORE_X related codes look also similar (I also feels not good).
Sounds good to me. I guess moving ECORE_* related could go in a separate patch.
> 
> >>> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:48
> >>> +typedef int (*EailShutdownFunction)(void*);
> >> 
> >> Should we need two typedef?
> > 
> > That's how elementary modules work, they call elm_modapi_init/shutdown to load/shutdown modules while elm_init is called. I do not want to change it, because MiniBrowser may use it as well.
> > Basically loading EAIL should be transparent to the client application.
> 
> I means that we don't need to make two typedefs for instances which have same signatures.
Yes, I agree you are right, thanks.
> 
> >>> Source/cmake/OptionsEfl.cmake:70
> >>> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY_EAIL_SUPPORT ON)
> >> 
> >> Should we need this macro?
> >> 
> >> In order to support ACCESSIBILITY in ewebkit, can we have another option?
> >> If not, I think that we'd better just to use ENABLE_ACCESSIBILITY.
> > 
> > EAIL could be easily turner off.
> 
> At least, this is only EFL specific. so we don't need to add it to WebKitFeatures.cmake.
Ok.
> 
> And I want to kwnow whether only ACCESSIBILITY mode is meaningfull.
> If not, we can turn off ACCESSIBILITY if eail is not available.
> Or, we can check the existance of eail and enable it.
Yes, I guess you might have right. Well EAIL seems as a final puzzle to have Accessibility stack working properly in EFL. In the other hand core Accessibility could leave without EAIL and can be easily verified by running LayoutTests. I think we could try only with HAVE_ACCESSIBILITY.

[Krzysztof Czech]

> > Source/cmake/OptionsEfl.cmake:300
> > +        set(ACCESSIBILITY_EAIL_DIRECTORY "${CMAKE_SOURCE_DIR}/WebKitBuild/Dependencies/Root/lib")
> > +        add_definitions(-DENABLE_ACCESSIBILITY_EAIL_SUPPORT=1)
> > +        add_definitions(-DACCESSIBILITY_EAIL_DIRECTORY=\"${ACCESSIBILITY_EAIL_DIRECTORY}\")
> 
> We should find right path wherever eail is installed.
> WebKitBuild directory is only for deverlopers who develop ewebkit without
> installation.
>
Do you think we could set this path through environment variable and let the user to build it yourself ?. It is publicly available.

[Ryuan Choi]

(In reply to comment #8)
> > > Source/cmake/OptionsEfl.cmake:300
> > > +        set(ACCESSIBILITY_EAIL_DIRECTORY "${CMAKE_SOURCE_DIR}/WebKitBuild/Dependencies/Root/lib")
> > > +        add_definitions(-DENABLE_ACCESSIBILITY_EAIL_SUPPORT=1)
> > > +        add_definitions(-DACCESSIBILITY_EAIL_DIRECTORY=\"${ACCESSIBILITY_EAIL_DIRECTORY}\")
> > 
> > We should find right path wherever eail is installed.
> > WebKitBuild directory is only for deverlopers who develop ewebkit without
> > installation.
> >
> Do you think we could set this path through environment variable and let the
> user to build it yourself ?. It is publicly available.

Does EAIL provide any information for it?
Like eail.pc? or something which pkg-config or CMakeConfig can read.

[Krzysztof Czech]

(In reply to comment #9)
> (In reply to comment #8)
> > > > Source/cmake/OptionsEfl.cmake:300
> > > > +        set(ACCESSIBILITY_EAIL_DIRECTORY "${CMAKE_SOURCE_DIR}/WebKitBuild/Dependencies/Root/lib")
> > > > +        add_definitions(-DENABLE_ACCESSIBILITY_EAIL_SUPPORT=1)
> > > > +        add_definitions(-DACCESSIBILITY_EAIL_DIRECTORY=\"${ACCESSIBILITY_EAIL_DIRECTORY}\")
> > > 
> > > We should find right path wherever eail is installed.
> > > WebKitBuild directory is only for deverlopers who develop ewebkit without
> > > installation.
> > >
> > Do you think we could set this path through environment variable and let the
> > user to build it yourself ?. It is publicly available.
> 
> Does EAIL provide any information for it?
> Like eail.pc? or something which pkg-config or CMakeConfig can read.
 Yes, it does provide a .pc file

[Krzysztof Czech]

Created attachment 241027 [details]
proposed patch

[WebKit Commit Bot]

Attachment 241027 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:44:  _elm_win_list is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

(In reply to comment #12)
> Attachment 241027 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:44: 
> _elm_win_list is incorrectly named. Don't use underscores in your identifier
> names.  [readability/naming/underscores] [4]
> Total errors found: 1 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.
It's something that comes from EAIL and should have underscores.

[Krzysztof Czech]

gyuyoung, ryuan any comments, review ?

[Gyuyoung Kim]

Comment on attachment 241027 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241027&action=review

> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:41
> +* This should be defined and filled in elementary but to avoid undefined reference it's defined here.

Wrong indentation and use.

> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:55
> +    static NeverDestroyed<WebKit::Module> eail(eailLibraryPath());

Don't you need to check if eailLibraryPath() is null ?

> Tools/ChangeLog:8
> +        Eail, atk-bridge, at-spi2 are not mandatory modules for webkit-efl. They are rather optional.

I wonder how is this file used for us. jhbuild.module will call this file ?

[Krzysztof Czech]

(In reply to comment #15)
> Comment on attachment 241027 [details]
> proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241027&action=review
> 
> > Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:41
> > +* This should be defined and filled in elementary but to avoid undefined reference it's defined here.
> 
> Wrong indentation and use.
Thanks
> 
> > Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:55
> > +    static NeverDestroyed<WebKit::Module> eail(eailLibraryPath());
> 
> Don't you need to check if eailLibraryPath() is null ?
I do not need it. Result of eailLibraryPath is forwarded directly to Module's constructor that expects a String. It could be null value, then Module's load method will take care whether Eina_Module is null or not. Later I'm checking whether load() is ok or not.
> 
> > Tools/ChangeLog:8
> > +        Eail, atk-bridge, at-spi2 are not mandatory modules for webkit-efl. They are rather optional.
> 
> I wonder how is this file used for us. jhbuild.module will call this file ?
I forgot to add a part that modifies jhbuild.modules that includes *optional file.
Basically jhbuild will not use it in regular updates. It should be called on demand by the user to test Accessibility.

[Krzysztof Czech]

Created attachment 241775 [details]
patch

[WebKit Commit Bot]

Attachment 241775 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:44:  _elm_win_list is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

Thanks Gyuyoung for the review. I corrected my patch.

[Gyuyoung Kim]

Comment on attachment 241775 [details]
patch

LGTM

[WebKit Commit Bot]

Comment on attachment 241775 [details]
patch

Clearing flags on attachment: 241775

Committed r176514: <http://trac.webkit.org/changeset/176514>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #21)
> Comment on attachment 241775 [details]
> patch
> 
> Clearing flags on attachment: 241775
> 
> Committed r176514: <http://trac.webkit.org/changeset/176514>

It made all performance tests fail:
lib/eina/eina_module.c:273 eina_module_new() safety check failed: len > 0 is false

Is EAIL optional or not? If it is needed to have a working WebKit,
we should add it non-optional (mandatory) jhbuild module. If it
is really optional, there is a bug near r176514.

[Csaba Osztrogonác]

new bug report to fix it: bug139037

[Krzysztof Czech]

(In reply to comment #23)
> (In reply to comment #21)
> > Comment on attachment 241775 [details]
> > patch
> > 
> > Clearing flags on attachment: 241775
> > 
> > Committed r176514: <http://trac.webkit.org/changeset/176514>
> 
> It made all performance tests fail:
> lib/eina/eina_module.c:273 eina_module_new() safety check failed: len > 0 is
> false
> 
> Is EAIL optional or not? If it is needed to have a working WebKit,
> we should add it non-optional (mandatory) jhbuild module. If it
> is really optional, there is a bug near r176514.

Thanks I thought eina_module_new does nothing in case of 0 length path.
EAIL is optional that's way I added it to jhbuild-optional.modules