[webkit-reviews] review denied: [Bug 127119] [EFL][GTK] Get EFL and GTK compiling with ACCESSBILITY disabled : [Attachment 221390] Patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has denied Thiago de Barros Lacerda
<thiago.lacerda at openbossa.org>'s request for review:
Bug 127119: [EFL][GTK] Get EFL and GTK compiling with ACCESSBILITY disabled
https://bugs.webkit.org/show_bug.cgi?id=127119

Attachment 221390: Patch
https://bugs.webkit.org/attachment.cgi?id=221390&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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.