Bug 127119 - [EFL][GTK] Get EFL and GTK compiling with ACCESSIBILITY disabled
Summary: [EFL][GTK] Get EFL and GTK compiling with ACCESSIBILITY disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago de Barros Lacerda
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-16 10:18 PST by Thiago de Barros Lacerda
Modified: 2014-01-22 15:08 PST (History)
21 users (show)

See Also:


Attachments
Patch (13.95 KB, patch)
2014-01-16 10:23 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Requested changes (14.68 KB, patch)
2014-01-20 20:00 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago de Barros Lacerda 2014-01-16 10:18:18 PST
If ACCESSBILITY is disabled EFL and GTK ports do not build.
Comment 1 Radar WebKit Bug Importer 2014-01-16 10:18:38 PST
<rdar://problem/15836356>
Comment 2 Thiago de Barros Lacerda 2014-01-16 10:23:57 PST
Created attachment 221390 [details]
Patch
Comment 3 kov's GTK+ EWS bot 2014-01-16 16:48:22 PST
Comment on attachment 221390 [details]
Patch

Attachment 221390 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/4762591591137280
Comment 4 Sergio Villar Senin 2014-01-17 00:49:31 PST
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.
Comment 5 Mario Sanchez Prada 2014-01-17 02:27:54 PST
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.
Comment 6 Mario Sanchez Prada 2014-01-17 02:28:34 PST
Fixed typo in bug title
Comment 7 Thiago de Barros Lacerda 2014-01-17 11:16:23 PST
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?
Comment 8 Thiago de Barros Lacerda 2014-01-20 20:00:30 PST
Created attachment 221715 [details]
Requested changes
Comment 9 Martin Robinson 2014-01-20 20:46:42 PST
Comment on attachment 221715 [details]
Requested changes

Feel free to do this for EFL, but we aren't interested in this for GTK+.
Comment 10 Sergio Villar Senin 2014-01-21 02:22:34 PST
(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.
Comment 11 Mario Sanchez Prada 2014-01-21 06:26:34 PST
[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.
Comment 12 Martin Robinson 2014-01-21 08:34:18 PST
(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.
Comment 13 Thiago de Barros Lacerda 2014-01-22 07:54:18 PST
(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.
Comment 14 Martin Robinson 2014-01-22 08:18:46 PST
(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.
Comment 15 Mario Sanchez Prada 2014-01-22 09:15:05 PST
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.
Comment 16 Martin Robinson 2014-01-22 09:20:18 PST
(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.
Comment 17 Mario Sanchez Prada 2014-01-22 09:23:40 PST
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.
Comment 18 WebKit Commit Bot 2014-01-22 11:17:31 PST
Comment on attachment 221715 [details]
Requested changes

Clearing flags on attachment: 221715

Committed r162538: <http://trac.webkit.org/changeset/162538>
Comment 19 WebKit Commit Bot 2014-01-22 11:17:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Ryuan Choi 2014-01-22 15:08:21 PST
(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.