Bug 127119 – [EFL][GTK] Get EFL and GTK compiling with ACCESSIBILITY disabled

• Component: Accessibility
• Status: RESOLVED FIXED

[Thiago de Barros Lacerda]

If ACCESSBILITY is disabled EFL and GTK ports do not build.

[Radar WebKit Bug Importer]

<rdar://problem/15836356>

[Thiago de Barros Lacerda]

Created attachment 221390 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 221390 [details]
Patch

Attachment 221390 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/4762591591137280

[Sergio Villar Senin]

Comment on attachment 221390 [details]
Patch

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

The idea of allowing accessibility to be disabled looks appealing to me. I have some comments though.

> Source/WebCore/ChangeLog:7
> +

Some description of the change missing.

> Source/WebKit/gtk/ChangeLog:7
> +

Ditto.

> Source/WebKit2/ChangeLog:7
> +

Ditto.

> Source/WebCore/accessibility/AXObjectCache.h:267
> +inline AXObjectCache::AXObjectCache(Document& document) : m_document(document), m_notificationPostTimer(this, (Timer<AXObjectCache>::TimerFiredFunction) nullptr) { }

Do we really need the cast here? Also how is that related to this bug?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:949
> +#if PLATFORM(GTK) && USE(TEXTURE_MAPPER_GL)

Again not related to this change right?

> Tools/ChangeLog:7
> +

Description.

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:36
> +#include "JSRetainPtr.h"

Why?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:96
> +JSRetainPtr<JSStringRef> AccessibilityController::platformName() { return JSRetainPtr<JSStringRef>(Adopt, JSStringCreateWithUTF8CString("")); }

Is this really required?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:132
> +bool AccessibilityUIElement::isIndeterminate() const { return false; }

Ditto.

[Mario Sanchez Prada]

Comment on attachment 221390 [details]
Patch

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

I basically agree with all of the comments from Sergio and have a couple of them more.

r- for now because of those comments, but the patch seems to be going in the right direction. Thanks!

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1420
> +#if HAVE(ACESSBILITY)

s/ACESSBILITY/ACCESSIBILITY

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3112
> +#if HAVE(ACESSBILITY)

s/ACESSBILITY/ACCESSIBILITY

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:251
> +#elif PLATFORM(GTK) && HAVE(ACCESSIBILITY)

I think this should be #elif HAVE(ACCESSIBILITY) && (PLATFORM(GTK) || PLATFORM(EFL))

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:56
> +#if HAVE(ACCESSIBILITY)

Nit. Move this if before the comment, which is a11y related anyway

> Tools/DumpRenderTree/gtk/AccessibilityControllerGtk.cpp:29
> +#if HAVE(ACCESSBILITY)

s/ACESSBILITY/ACCESSIBILITY

> Tools/DumpRenderTree/gtk/AccessibilityControllerGtk.cpp:79
> +#endif // HAVE(ACCESSBILITY)

s/ACESSBILITY/ACCESSIBILITY

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:84
> +#if HAVE(ACCESSBILITY)

s/ACESSBILITY/ACCESSIBILITY

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:534
> +#if HAVE(ACCESSBILITY)

s/ACESSBILITY/ACCESSIBILITY

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:928
> +#if HAVE(ACCESSBILITY)

s/ACESSBILITY/ACCESSIBILITY

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1533
> +#if HAVE(ACCESSBILITY)

s/ACESSBILITY/ACCESSIBILITY

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1549
> +#if HAVE(ACCESSBILITY)

s/ACESSBILITY/ACCESSIBILITY

>> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:96
>> +JSRetainPtr<JSStringRef> AccessibilityController::platformName() { return JSRetainPtr<JSStringRef>(Adopt, JSStringCreateWithUTF8CString("")); }
> 
> Is this really required?

I think the problem is that in WKTR we do not guard things like this one as we do in DRT, so you need to make sure that you provide dummy implementations for those cases that do not use Accessibility.

However, I think it would be better, if possible, to just skip the AccessibilityController and AccessibilityUIElement completely, if we are going to build without a11y enabled.

[Mario Sanchez Prada]

Fixed typo in bug title

[Thiago de Barros Lacerda]

Comment on attachment 221390 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.h:267
>> +inline AXObjectCache::AXObjectCache(Document& document) : m_document(document), m_notificationPostTimer(this, (Timer<AXObjectCache>::TimerFiredFunction) nullptr) { }
> 
> Do we really need the cast here? Also how is that related to this bug?

Yes, otherwise it won't build. The compiler will not know which Timer's constructor to call.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:251
>> +#elif PLATFORM(GTK) && HAVE(ACCESSIBILITY)
> 
> I think this should be #elif HAVE(ACCESSIBILITY) && (PLATFORM(GTK) || PLATFORM(EFL))

I just kept the #if as is, just adding the ACCESSIBILITY condition, the PLATFORM(EFL) was not there before, only GTK. I think that we should keep with only GTK.

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:949
>> +#if PLATFORM(GTK) && USE(TEXTURE_MAPPER_GL)
> 
> Again not related to this change right?

If we disable ACCESSIBILITY the m_nativeWindowHandle won't be a member of WebPage and there are other parts in the code (not guarded by ACCESSIBILITY) that access m_nativeWindowHandle, so we need to separate them in two different #if

>>> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:96
>>> +JSRetainPtr<JSStringRef> AccessibilityController::platformName() { return JSRetainPtr<JSStringRef>(Adopt, JSStringCreateWithUTF8CString("")); }
>> 
>> Is this really required?
> 
> I think the problem is that in WKTR we do not guard things like this one as we do in DRT, so you need to make sure that you provide dummy implementations for those cases that do not use Accessibility.
> 
> However, I think it would be better, if possible, to just skip the AccessibilityController and AccessibilityUIElement completely, if we are going to build without a11y enabled.

We do need that in order to build. Skip those files completely will be much more work to do, maybe we can handle this in another patch. Is that ok?

[Thiago de Barros Lacerda]

Created attachment 221715 [details]
Requested changes

[Martin Robinson]

Comment on attachment 221715 [details]
Requested changes

Feel free to do this for EFL, but we aren't interested in this for GTK+.

[Sergio Villar Senin]

(In reply to comment #9)
> (From update of attachment 221715 [details])
> Feel free to do this for EFL, but we aren't interested in this for GTK+.

Aren't we? What about having this for devices were accessibility does not make much sense or is not implemented at all? We might be adding extra processing time and resource waste.

[Mario Sanchez Prada]

[Replying Thiago and Martin/Sergio at the same time]

(In reply to comment #7)
> [...] 
> > I think the problem is that in WKTR we do not guard things like this one as 
> > we do in DRT, so you need to make sure that you provide dummy 
> > implementations for those cases that do not use Accessibility.
> > 
> > However, I think it would be better, if possible, to just skip the 
> > AccessibilityController and AccessibilityUIElement completely, if we are 
> > going to build without a11y enabled.
> 
> We do need that in order to build. Skip those files completely will be much 
> more work to do, maybe we can handle this in another patch. Is that ok?

Yes, I think that's perfectly fine.


(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 221715 [details] [details])
> > Feel free to do this for EFL, but we aren't interested in this for GTK+.
> 
> Aren't we? What about having this for devices were accessibility does not 
> make much sense or is not implemented at all? We might be adding extra 
> processing time and resource waste.

I agree with Sergio on that it's interesting to be able to build/run WebKitGTK+ without accessibility enabled. And this patch seems to me to be a godd opportunity to make it real.

[Martin Robinson]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 221715 [details] [details])
> > Feel free to do this for EFL, but we aren't interested in this for GTK+.
> 
> Aren't we? What about having this for devices were accessibility does not make much sense or is not implemented at all? We might be adding extra processing time and resource waste.

I'm fine with it if such a device exists and it uses WebKitGTK+, otherwise I don't see the benefit of adding extra maintenance burden.

[Thiago de Barros Lacerda]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 221715 [details] [details] [details])
> > > Feel free to do this for EFL, but we aren't interested in this for GTK+.
> > 
> > Aren't we? What about having this for devices were accessibility does not make much sense or is not implemented at all? We might be adding extra processing time and resource waste.
> 
> I'm fine with it if such a device exists and it uses WebKitGTK+, otherwise I don't see the benefit of adding extra maintenance burden.

I think that the changes are very tiny, as you can see, in order to face it as "burden". Additionally, WebKit is about to make the project as modular as possible (by disabling and enabling features) and this patch makes it possible for accessibility.
The patch is ready to land, in my opinion, but it is up to you guys.

[Martin Robinson]

(In reply to comment #13)

> I think that the changes are very tiny, as you can see, in order to face it as "burden". Additionally, WebKit is about to make the project as modular as possible (by disabling and enabling features) and this patch makes it possible for accessibility.
> The patch is ready to land, in my opinion, but it is up to you guys.

We use to allow everything to be configurable in WebKitGTK+ and we received many bugs as people tried to enable and disable a combinatorial explosion of features. Nowadays we only make bits of WebKit configurable if there's a good reason to. For instance, one reason is if we would otherwise have to raise our dependencies. This allows WebKit to keep building on older systems.

I'm not opposed to making accessibility configurable, I'm just opposed to doing it without some justification.

[Mario Sanchez Prada]

I agree with Thiago on that this is a nice and useful patch, and my vote goes for getting it in, even though we keep to enable accessibility by default in the GTK+ port (my vote goes for that too). Besides, it's a tiny patch that just fixes a building issue with the infrastructure that is already in place to allow building with and without accessibility.

So, I think this patch is a good addition, unless we want to get rid of all the HAVE(ACCESSSIBILITY) guards and assume that WebKit will always build with accessibility support on mind (which is another option).

About specific devices that might benefit of this, it comes to my mind some devices (e.g. TVs) that are currently not using this feature and so would benefit from this patch just by having smaller binary files.

[Martin Robinson]

(In reply to comment #15)

> About specific devices that might benefit of this, it comes to my mind some devices (e.g. TVs) that are currently not using this feature and so would benefit from this patch just by having smaller binary files.

If this will be used, I'm not opposed. I don't think we should add a configure flag though.

[Mario Sanchez Prada]

Comment on attachment 221715 [details]
Requested changes

(In reply to comment #16)
> (In reply to comment #15)
> 
> > About specific devices that might benefit of this, it comes to my mind some devices (e.g. TVs) that are currently not using this feature and so would benefit from this patch just by having smaller binary files.
> 
> If this will be used, I'm not opposed. I don't think we should add a configure flag though.

I agree with this too (no need for configure flag).

Setting r+ then as per comments.

[WebKit Commit Bot]

Comment on attachment 221715 [details]
Requested changes

Clearing flags on attachment: 221715

Committed r162538: <http://trac.webkit.org/changeset/162538>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryuan Choi]

(In reply to comment #17)
> (From update of attachment 221715 [details])
> (In reply to comment #16)
> > (In reply to comment #15)
> > 
> > > About specific devices that might benefit of this, it comes to my mind some devices (e.g. TVs) that are currently not using this feature and so would benefit from this patch just by having smaller binary files.
> > 
> > If this will be used, I'm not opposed. I don't think we should add a configure flag though.
> 
> I agree with this too (no need for configure flag).
> 
> Setting r+ then as per comments.

Thanks.

FYI, some platform which uses EFL does not enable ACCESSIBILITY yet.