Bug 103036 – [EFL] Possibility to turn off accessibility feature for WebKit-EFL.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Krzysztof Czech]

Possibility to turn off accessibility feature for WebKit-EFL.

[Laszlo Gombos]

Created attachment 177339 [details]
proposed patch

I had this to change in my environment as I needed it to proceed - I decided to share it.

[WebKit Review Bot]

Comment on attachment 177339 [details]
proposed patch

Rejecting attachment 177339 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ripts/update-webkit line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
   9961a7b..ef0ca77  HEAD       -> origin/HEAD
error: Ref refs/remotes/origin/master is at ef0ca77a554ed2a07b001ee4935266d6301b2bf8 but expected 9961a7b754078ea893b086fbc787c57745c8a4db
 ! 9961a7b..ef0ca77  master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15101901

[Krzysztof Czech]

We will not turn off accessibility (ATK) only by removing ATK from building process. There is still ATK dependant code in WebCore. Global HAVE_ACCESSIBILITY macro is always defined for EFL and some specific ATK code is covered by this macro. Turning off Accessibility completely requires some additional changes in WebCore. I started doing this part but there are some things that should be considered.

[Laszlo Gombos]

(In reply to comment #3)
> We will not turn off accessibility (ATK) only by removing ATK from building process. There is still ATK dependant code in WebCore. Global HAVE_ACCESSIBILITY macro is always defined for EFL and some specific ATK code is covered by this macro. Turning off Accessibility completely requires some additional changes in WebCore. I started doing this part but there are some things that should be considered.

Thanks Krzysztof, I just noticed as well that my patch is not complete. As the patch is not landed yet, I will make it obsolete and please feel free to pick it up from where I left it and make it complete.

For my need the most important things is to have a build option to disable the ATK dependent parts (when ATK is not available). It is not a strictly needed to disable all the accessibility bits, but certainly that would be best solution.

[Krzysztof Czech]

Created attachment 178547 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebCore/PlatformEfl.cmake:349:  One space between command "endif" and its parentheses, should be "endif ("  [whitespace/parentheses] [5]
Source/WebCore/accessibility/AccessibilityObject.h:785:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


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

[Early Warning System Bot]

Comment on attachment 178547 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

Attachment 178547 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15230635

[Early Warning System Bot]

Comment on attachment 178547 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

Attachment 178547 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15231505

[Build Bot]

Comment on attachment 178547 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

Attachment 178547 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15228646

[Krzysztof Czech]

Created attachment 178552 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebCore/PlatformEfl.cmake:349:  One space between command "endif" and its parentheses, should be "endif ("  [whitespace/parentheses] [5]
Source/WebCore/accessibility/AccessibilityObject.h:785:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


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

[Krzysztof Czech]

Created attachment 178553 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

[Krzysztof Czech]

I added some additional bits so that we can completely turn off the Accessibility feature for EFL. Modifications in WebCore/PlatformEfl.cmake may not give us desired effect. If we turn off a11y by only disabling it in WebCore/PLatformEfl.cmake, compilation errors appear due to the fact, there are ATK routines left in WebKit/accessibility/. I added additional macro ENABLE_ACCESSIBILITY_EFL so that we can play with those ATK routines.

[Krzysztof Czech]

Created attachment 178562 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

[chris fleizach]

Comment on attachment 178553 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

there's already a  HAVE(ACCESSIBILITY) macro. can you use that?

[Gyuyoung Kim]

Comment on attachment 178562 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

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

> Source/cmake/OptionsEfl.cmake:217
> +    add_definitions(-DENABLE_ACCESSIBILITY_EFL=1)

I think ACCESSIBILITY_EFL is not needed. PLATFORM(EFL) && ENABLE(ACCESSIBILITY) is enough. BTW, why should we use them together ? Can't we use PLATFORM(EFL) as GTK port ?

[Laszlo Gombos]

Comment on attachment 178562 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

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

> Source/WebCore/accessibility/AXObjectCache.h:273
> +#if (PLATFORM(EFL) && !ENABLE(ACCESSIBILITY_EFL))

Would it be possible to simply use "#if !HAVE(ACCESSIBILITY)" here and combine this list with the list above" ?

> Source/cmake/OptionsEfl.cmake:51
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY ON)
>  WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_3D_RENDERING ON)
>  WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ANIMATION_API ON)

It would be good to keep this list sorted.

>> Source/cmake/OptionsEfl.cmake:217
>> +    add_definitions(-DENABLE_ACCESSIBILITY_EFL=1)
> 
> I think ACCESSIBILITY_EFL is not needed. PLATFORM(EFL) && ENABLE(ACCESSIBILITY) is enough. BTW, why should we use them together ? Can't we use PLATFORM(EFL) as GTK port ?

I think this should be changed to add_definitions(-DHAVE_ACCESSIBILITY=1)

[Krzysztof Czech]

Created attachment 178768 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

[Krzysztof Czech]

(In reply to comment #17)
> (From update of attachment 178562 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178562&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.h:273
> > +#if (PLATFORM(EFL) && !ENABLE(ACCESSIBILITY_EFL))
> 
> Would it be possible to simply use "#if !HAVE(ACCESSIBILITY)" here and combine this list with the list above" ?
> 
> > Source/cmake/OptionsEfl.cmake:51
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY ON)
> >  WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_3D_RENDERING ON)
> >  WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ANIMATION_API ON)
> 
> It would be good to keep this list sorted.
> 
> >> Source/cmake/OptionsEfl.cmake:217
> >> +    add_definitions(-DENABLE_ACCESSIBILITY_EFL=1)
> > 
> > I think ACCESSIBILITY_EFL is not needed. PLATFORM(EFL) && ENABLE(ACCESSIBILITY) is enough. BTW, why should we use them together ? Can't we use PLATFORM(EFL) as GTK port ?
> 
> I think this should be changed to add_definitions(-DHAVE_ACCESSIBILITY=1)
You are right Laszlo, add_definitions(-DHAVE_ACCESSIBILITY=1) it's a good idea. Having this in OptionsEfl.cmake give us possibility to switch on/off WebKit-EFL accessibility. I moved its declaration for EFL platform to OptionsEfl.cmake.

[Krzysztof Czech]

(In reply to comment #16)
> (From update of attachment 178562 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178562&action=review
> 
> > Source/cmake/OptionsEfl.cmake:217
> > +    add_definitions(-DENABLE_ACCESSIBILITY_EFL=1)
> 
> I think ACCESSIBILITY_EFL is not needed. PLATFORM(EFL) && ENABLE(ACCESSIBILITY) is enough. BTW, why should we use them together ? Can't we use PLATFORM(EFL) as GTK port ?
Yes I agree, ACCESSIBILITY_EFL is not needed. Laszlo pointed out that HAVE_ACCESSIBILITY can be used instead.
> BTW, why should we use them together ?  Can't we use PLATFORM(EFL) as GTK port ?
In some of the places PLATFORM(EFL) is not enough since ATK library is external lib and in sources there are some ATK routines. That's way we got to have second guard.

[Krzysztof Czech]

(In reply to comment #15)
> (From update of attachment 178553 [details])
> there's already a  HAVE(ACCESSIBILITY) macro. can you use that?
Yes I can use this macro. I corrected my patch.

[Laszlo Gombos]

Comment on attachment 178768 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

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

Overall looks good to me, but it would be good to fix the remaining minor issues.

> Source/WTF/wtf/Platform.h:674
> -#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && !OS(ANDROID)) || PLATFORM(EFL)
> +#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && !OS(ANDROID))

I prefer to keep PLATFORM(EFL) listed here as that is still the default for EFL. We can still disable ACCESSIBILITY (see the comment below). If you remove this change than you do not need to change WTF.

> Source/WebCore/accessibility/AccessibilityObject.h:784
> -#elif !PLATFORM(CHROMIUM)
> +#elif !PLATFORM(CHROMIUM) 

Extra white space. This change is not needed.

> Source/cmake/OptionsEfl.cmake:218
> +if (ENABLE_ACCESSIBILITY)
> +    find_package(ATK REQUIRED)
> +    add_definitions(-DHAVE_ACCESSIBILITY=1)
> +endif ()

I think it is better to have the default in .h file and have the non-typical option (the ability to disable) in the make system. What do you think of the following:

if (ENABLE_ACCESSIBILITY)
    find_package(ATK REQUIRED)
else ()
    add_definitions(-DHAVE_ACCESSIBILITY=0)
endif ()

[Krzysztof Czech]

Created attachment 179052 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

[Krzysztof Czech]

Created attachment 179058 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

[Laszlo Gombos]

Comment on attachment 179058 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

lgtm.

[WebKit Review Bot]

Comment on attachment 179058 [details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.

Clearing flags on attachment: 179058

Committed r137475: <http://trac.webkit.org/changeset/137475>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.