Bug 199625 – [WPE][GTK] Build failure with ENABLE_ACCESSIBILITY=OFF

• Status: RESOLVED
• Resolution: FIXED
• Component: CMake

[Pablo Saavedra]

Linking stage build error using the non-default ENABLE_ACCESSIBILITY=OFF. For example:


    ./Tools/Scripts/build-webkit --release --wpe --cmakeargs="-DENABLE_ACCESSIBILITY=OFF" --no-experimental-features --no-video --no-web-rtc


Error:


...
/home/bot/toolchain_env/wandboard-mesa-wpe-manual/sysroots/x86_64-pokysdk-linux/usr/libexec/arm-poky-linux-gnueabi/gcc/arm-poky-linux-gnueabi/8.3.0/real-ld: Tools/WebKitTestRunner/CMakeFiles/TestRunnerInjectedBundle.dir/InjectedBundle/AccessibilityController.cpp.o: in function `WTR::AccessibilityController::elementAtPoint(int, int)':
/home/bot/webkit/WebKitBuild/Release/../../Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:94: undefined reference to `WTR::AccessibilityUIElement::elementAtPoint(int, int)'
/home/bot/toolchain_env/wandboard-mesa-wpe-manual/sysroots/x86_64-pokysdk-linux/usr/libexec/arm-poky-linux-gnueabi/gcc/arm-poky-linux-gnueabi/8.3.0/real-ld: Tools/WebKitTestRunner/CMakeFiles/TestRunnerInjectedBundle.dir/InjectedBundle/AccessibilityUIElement.cpp.o: in function `WTR::AccessibilityUIElement::create(void*)':
/home/bot/webkit/WebKitBuild/Release/../../Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:35: undefined reference to `WTR::AccessibilityUIElement::AccessibilityUIElement(void*)'
/home/bot/toolchain_env/wandboard-mesa-wpe-manual/sysroots/x86_64-pokysdk-linux/usr/libexec/arm-poky-linux-gnueabi/gcc/arm-poky-linux-gnueabi/8.3.0/real-ld: Tools/WebKitTestRunner/CMakeFiles/TestRunnerInjectedBundle.dir/InjectedBundle/AccessibilityUIElement.cpp.o: in function `WTR::AccessibilityUIElement::create(WTR::AccessibilityUIElement const&)':
/home/bot/webkit/WebKitBuild/Release/../../Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:40: undefined reference to `WTR::AccessibilityUIElement::AccessibilityUIElement(WTR::AccessibilityUIElement const&)'
/home/bot/toolchain_env/wandboard-mesa-wpe-manual/sysroots/x86_64-pokysdk-linux/usr/libexec/arm-poky-linux-gnueabi/gcc/arm-poky-linux-gnueabi/8.3.0/real-ld: Tools/WebKitTestRunner/CMakeFiles/TestRunnerInjectedBundle.dir/__/__/DerivedSources/InjectedBundle/JSAccessibilityController.cpp.o: in function `WTR::JSAccessibilityController::addNotificationListener(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned int, OpaqueJSValue const* const*, OpaqueJSValue const**)':
/home/bot/webkit/WebKitBuild/Release/DerivedSources/InjectedBundle/JSAccessibilityController.cpp:130: undefined reference to `WTR::AccessibilityController::addNotificationListener(OpaqueJSValue const*)'
...


This issue could be related to:

    [CMake] Remove ENABLE_ACCESSIBILITY CMake variable
    https://bugs.webkit.org/show_bug.cgi?id=191831
    
    Reviewed by Michael Catanzaro.

[Pablo Saavedra]

Created attachment 373730 [details]
patch

[Konstantin Tokarev]

Comment on attachment 373730 [details]
patch

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

> Tools/WebKitTestRunner/CMakeLists.txt:68
> +if (ACCESSIBILITY)

I think it should be if (ENABLE_ACCESSIBILITY)

> Tools/WebKitTestRunner/CMakeLists.txt:85
> +if (ACCESSIBILITY)

Ditto

[Konstantin Tokarev]

Now looking at https://bugs.webkit.org/show_bug.cgi?id=191831 I think that patch should be reverted

[Pablo Saavedra]

(In reply to Konstantin Tokarev from comment #2)
> Comment on attachment 373730 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373730&action=review
> 
> > Tools/WebKitTestRunner/CMakeLists.txt:68
> > +if (ACCESSIBILITY)
> 
> I think it should be if (ENABLE_ACCESSIBILITY)
> 
> > Tools/WebKitTestRunner/CMakeLists.txt:85
> > +if (ACCESSIBILITY)
> 
> Ditto

Oops you are right

[Pablo Saavedra]

(In reply to Konstantin Tokarev from comment #3)
> Now looking at https://bugs.webkit.org/show_bug.cgi?id=191831 I think that
> patch should be reverted

+1

[Michael Catanzaro]

Comment on attachment 373730 [details]
patch

It doesn't need to be reverted. Just guard all the files with #if HAVE(ACCESSIBILITY). Let's try to build files unconditionally at the CMake level and use guards in the files instead whenever we can.

[Pablo Saavedra]

Created attachment 373749 [details]
patch

[Pablo Saavedra]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 373730 [details]
> patch
> 
> It doesn't need to be reverted. Just guard all the files with #if
> HAVE(ACCESSIBILITY). Let's try to build files unconditionally at the CMake
> level and use guards in the files instead whenever we can.

Let's do it.

[Michael Catanzaro]

Comment on attachment 373749 [details]
patch

No I mean in the files themselves. E.g. AccessibilityController.cpp already has an #if HAVE(ACCESSIBILITY) guard, but AccessibilityController.h is missing the guard. (Instead the header has some internal guards, which is really weird. I would try removing those.)

[Konstantin Tokarev]

If we are using -DENABLE_ACCESSIBILITY=OFF build option, there must be corresponding WEBKIT_OPTION

[Michael Catanzaro]

ENABLE(ACCESSIBILITY) is a WPE-specific option. It can't be used in these cross-platform files. Just use the existing HAVE(ACCESSIBILITY).

[Fujii Hironori]

I didn't know ENABLE_ACCESSIBILITY has been added again in r245565.
IMHO, all HAVE(ACCESSIBILITY) should be replaced with ENABLE(ACCESSIBILITY) in despite of the previous discussion.
https://lists.webkit.org/pipermail/webkit-dev/2018-November/030258.html

[Pablo Saavedra]

(In reply to Michael Catanzaro from comment #9)
> Comment on attachment 373749 [details]
> patch
> 
> No I mean in the files themselves. E.g. AccessibilityController.cpp already
> has an #if HAVE(ACCESSIBILITY) guard, but AccessibilityController.h is
> missing the guard. (Instead the header has some internal guards, which is
> really weird. I would try removing those.)



I tried 


```
diff --git a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp b/Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp
index bf8f555b876..7616c9d8dd1 100644
--- a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp
+++ b/Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp
@@ -24,6 +24,9 @@
  */
 
 #include "config.h"
+
+#if HAVE(ACCESSIBILITY)
+
 #include "AccessibilityController.h"
 
 #include "AccessibilityUIElement.h"
@@ -95,3 +98,4 @@ RefPtr<AccessibilityUIElement> AccessibilityController::elementAtPoint(int x, in
 }
 
 } // namespace WTR
+#endif // HAVE(ACCESSIBILITY)
diff --git a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityTextMarker.cpp b/Tools/WebKitTestRunner/InjectedBundle/AccessibilityTextMarker.cpp
index 65336041dc0..06e72e90352 100644
--- a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityTextMarker.cpp
+++ b/Tools/WebKitTestRunner/InjectedBundle/AccessibilityTextMarker.cpp
@@ -24,6 +24,9 @@
  */
 
 #include "config.h"
+
+#if HAVE(ACCESSIBILITY)
+
 #include "AccessibilityTextMarker.h"
 
 #include "AccessibilityUIElement.h"
@@ -71,4 +74,4 @@ JSClassRef AccessibilityTextMarker::wrapperClass()
 }
 
 } // namespace WTR
-
+#endif // HAVE(ACCESSIBILITY)
diff --git a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityTextMarkerRange.cpp b/Tools/WebKitTestRunner/InjectedBundle/AccessibilityTextMarkerRange.cpp
index 2b21e8a8801..39f77d36bf9 100644
--- a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityTextMarkerRange.cpp
+++ b/Tools/WebKitTestRunner/InjectedBundle/AccessibilityTextMarkerRange.cpp
@@ -24,6 +24,9 @@
  */
 
 #include "config.h"
+
+#if HAVE(ACCESSIBILITY)
+
 #include "AccessibilityTextMarker.h"
 
 #include "AccessibilityUIElement.h"
@@ -71,4 +74,4 @@ JSClassRef AccessibilityTextMarkerRange::wrapperClass()
 }
     
 } // namespace WTR
-
+#endif // HAVE(ACCESSIBILITY)
diff --git a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp b/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp
index 73c1610ce4f..e6ef7e4d0cb 100644
--- a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp
+++ b/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp
@@ -24,6 +24,9 @@
  */
 
 #include "config.h"
+
+#if HAVE(ACCESSIBILITY)
+
 #include "AccessibilityUIElement.h"
 
 #include "JSAccessibilityUIElement.h"
@@ -108,3 +111,4 @@ RefPtr<AccessibilityTextMarker> AccessibilityUIElement::previousSentenceStartTex
 #endif
 
 } // namespace WTR
+#endif // HAVE(ACCESSIBILITY)
```


But the issue is still present:


error: ...
/home/bot/webkit/WebKitBuild/Release/DerivedSources/InjectedBundle/JSAccessibilityController.cpp:97: undefined reference to `WTR::AccessibilityController::enableEnhancedAccessibility(bool)'
/home/bot/toolchain_env/wandboard-mesa-wpe-manual/sysroots/x86_64-pokysdk-linux/usr/libexec/arm-poky-linux-gnueabi/gcc/arm-poky-linux-gnueabi/8.3.0/real-ld: Tools/WebKitTestRunner/CMakeFiles/TestRunnerInjectedBundle.dir/__/__/DerivedSources/InjectedBundle/JSAccessibilityController.cpp.o: in function `WTR::JSAccessibilityController::addNotificationListener(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned int, OpaqueJSValue const* const*, OpaqueJSValue const**)':
/home/bot/webkit/WebKitBuild/Release/DerivedSources/InjectedBundle/JSAccessibilityController.cpp:130: undefined reference to `WTR::AccessibilityController::addNotificationListener(OpaqueJSValue const*)'
... 



Actually  I don't understand at all where/how these derived sources come form:

[Pablo Saavedra]

(In reply to Fujii Hironori from comment #12)
> I didn't know ENABLE_ACCESSIBILITY has been added again in r245565.
> IMHO, all HAVE(ACCESSIBILITY) should be replaced with ENABLE(ACCESSIBILITY)
> in despite of the previous discussion.
> https://lists.webkit.org/pipermail/webkit-dev/2018-November/030258.html

According with this thread. it seems HAVE(ACCESSIBILITY) is problematic every then and now.

I prone to propose a simple fix for the Tools/WebKitTestRunner/CMakeLists.txt using the ENABLE_ACCESSIBILITY as I proposed firstly.

[Pablo Saavedra]

Created attachment 373828 [details]
patch

[Michael Catanzaro]

Comment on attachment 373828 [details]
patch

Again, this is r- because ENABLE_ACCESSIBILITY is only defined for WPE. I'm really surprised this only broke the WinCairo EWS. The GTK EWS doesn't run tests, so maybe we wouldn't notice the accessibility layout tests failing until this lands.

Fujii, what do you think? I'm OK with replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY).

[Michael Catanzaro]

BTW the WinCairo failure is:

Creating library lib64\TestRunnerInjectedBundle.lib and object lib64\TestRunnerInjectedBundle.exp
InjectedBundle.cpp.obj : error LNK2019: unresolved external symbol "public: static class WTF::Ref<class WTR::AccessibilityController,struct WTF::DumbPtrTraits<class WTR::AccessibilityController> > __cdecl WTR::AccessibilityController::create(void)" (?create@AccessibilityController@WTR@@SA?AV?$Ref@VAccessibilityController@WTR@@U?$DumbPtrTraits@VAccessibilityController@WTR@@@WTF@@@WTF@@XZ) referenced in function "private: void __cdecl WTR::InjectedBundle::beginTesting(struct OpaqueWKDictionary const *,enum WTR::InjectedBundle::BegingTestingMode)" (?beginTesting@InjectedBundle@WTR@@AEAAXPEBUOpaqueWKDictionary@@W4BegingTestingMode@12@@Z)
InjectedBundlePage.cpp.obj : error LNK2019: unresolved external symbol "public: void __cdecl WTR::AccessibilityController::makeWindowObject(struct OpaqueJSContext const *,struct OpaqueJSValue *,struct OpaqueJSValue const * *)" (?makeWindowObject@AccessibilityController@WTR@@QEAAXPEBUOpaqueJSContext@@PEAUOpaqueJSValue@@PEAPEBU4@@Z) referenced in function "private: void __cdecl WTR::InjectedBundlePage::didClearWindowForFrame(struct OpaqueWKBundleFrame const *,struct OpaqueWKBundleScriptWorld const *)" (?didClearWindowForFrame@InjectedBundlePage@WTR@@AEAAXPEBUOpaqueWKBundleFrame@@PEBUOpaqueWKBundleScriptWorld@@@Z)
AccessibilityControllerWin.cpp.obj : error LNK2019: unresolved external symbol "public: static class WTF::Ref<class WTR::AccessibilityUIElement,struct WTF::DumbPtrTraits<class WTR::AccessibilityUIElement> > __cdecl WTR::AccessibilityUIElement::create(void *)" (?create@AccessibilityUIElement@WTR@@SA?AV?$Ref@VAccessibilityUIElement@WTR@@U?$DumbPtrTraits@VAccessibilityUIElement@WTR@@@WTF@@@WTF@@PEAX@Z) referenced in function "public: class WTF::Ref<class WTR::AccessibilityUIElement,struct WTF::DumbPtrTraits<class WTR::AccessibilityUIElement> > __cdecl WTR::AccessibilityController::focusedElement(void)" (?focusedElement@AccessibilityController@WTR@@QEAA?AV?$Ref@VAccessibilityUIElement@WTR@@U?$DumbPtrTraits@VAccessibilityUIElement@WTR@@@WTF@@@WTF@@XZ)
AccessibilityUIElementWin.cpp.obj : error LNK2001: unresolved exterFailed to run "['perl', 'Tools\\Scripts\\build-webkit', '--release', '--wincairo']" exit_code: 1

[Pablo Saavedra]

Created attachment 373848 [details]
patch

[Konstantin Tokarev]

Comment on attachment 373848 [details]
patch

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

> Tools/WebKitTestRunner/CMakeLists.txt:68
> +if (HAVE_ACCESSIBILITY)

Please use indentation inside cmake if blocks

[Michael Catanzaro]

Comment on attachment 373848 [details]
patch

OK, HAVE(ACCESSIBILITY) is valid on all ports in the code itself, after including Platform.h, which is good. But at the CMake level it only exists for PlayStation (where it's set to OFF) and WPE ports. So again, this will break everything except WPE. There is no way to do this at the CMake level without introducing a new variable that's defined on all platforms. We could e.g. add ENABLE_ACCESSIBILITY to WebKitFeatures.cmake and then use  if (ENABLE_ACCESSIBILITY) here. That would be OK.

Of course the ideal solution would be to debug the linker error and get this working using only guards in the files themselves, but I understand that's hard.

[Pablo Saavedra]

(In reply to Michael Catanzaro from comment #20)
> Comment on attachment 373848 [details]
> patch
> 
> OK, HAVE(ACCESSIBILITY) is valid on all ports in the code itself, after
> including Platform.h, which is good. But at the CMake level it only exists
> for PlayStation (where it's set to OFF) and WPE ports. So again, this will
> break everything except WPE. There is no way to do this at the CMake level
> without introducing a new variable that's defined on all platforms. We could
> e.g. add ENABLE_ACCESSIBILITY to WebKitFeatures.cmake and then use  if
> (ENABLE_ACCESSIBILITY) here. That would be OK.
> 

You mean, to promote the WPE option to WebKitFeatures:

  cmake/OptionsWPE.cmake:WEBKIT_OPTION_DEFINE(ENABLE_ACCESSIBILITY "Whether to enable support for accessibility" PUBLIC ON)
  cmake/OptionsWPE.cmake:if (ENABLE_ACCESSIBILITY)
  cmake/OptionsWPE.cmake:        message(FATAL_ERROR "atk is needed for ENABLE_ACCESSIBILITY")
  cmake/OptionsWPE.cmake:        message(FATAL_ERROR "at-spi2-atk is needed for ENABLE_ACCESSIBILITY")
  cmake/OptionsWPE.cmake:SET_AND_EXPOSE_TO_BUILD(HAVE_ACCESSIBILITY ${ENABLE_ACCESSIBILITY})
  cmake/OptionsWPE.cmake:SET_AND_EXPOSE_TO_BUILD(USE_ATK ${ENABLE_ACCESSIBILITY})


right?

> Of course the ideal solution would be to debug the linker error and get this
> working using only guards in the files themselves, but I understand that's
> hard.

I could do it, but the the barrier that I had to face is how are being generated the derived files like WebKitBuild/Release/DerivedSources/InjectedBundle/JSAccessibilityController.cpp because they are the files to guard with `HAVE(ACCESSIBILITY)`  (Comment 13). If you can enlightenment, I'd delighted

[Michael Catanzaro]

(In reply to Pablo Saavedra from comment #21)
> You mean, to promote the WPE option to WebKitFeatures:
> 
>   cmake/OptionsWPE.cmake:WEBKIT_OPTION_DEFINE(ENABLE_ACCESSIBILITY "Whether
> to enable support for accessibility" PUBLIC ON)
>   cmake/OptionsWPE.cmake:if (ENABLE_ACCESSIBILITY)
>   cmake/OptionsWPE.cmake:        message(FATAL_ERROR "atk is needed for
> ENABLE_ACCESSIBILITY")
>   cmake/OptionsWPE.cmake:        message(FATAL_ERROR "at-spi2-atk is needed
> for ENABLE_ACCESSIBILITY")
>   cmake/OptionsWPE.cmake:SET_AND_EXPOSE_TO_BUILD(HAVE_ACCESSIBILITY
> ${ENABLE_ACCESSIBILITY})
>   cmake/OptionsWPE.cmake:SET_AND_EXPOSE_TO_BUILD(USE_ATK
> ${ENABLE_ACCESSIBILITY})
> 
> 
> right?

Indeed.
 
> I could do it, but the the barrier that I had to face is how are being
> generated the derived files like
> WebKitBuild/Release/DerivedSources/InjectedBundle/JSAccessibilityController.
> cpp because they are the files to guard with `HAVE(ACCESSIBILITY)`  (Comment
> 13). If you can enlightenment, I'd delighted

They're in Tools/WebKitTestRunner/InjectedBundle/Bindings, and processed by CodeGeneratorJS.pm in Source/WebCore/bindings/scripts. I think we want to do something like:

[
    Conditional=ACCESSIBILITY,
] interface AccessibilityTextMarkerRange {
    boolean isEqual(AccessibilityTextMarkerRange otherMarkerRange);
};

Except the flaw in that plan is -- wait for it -- that will expand to #if ENABLE(ACCESSIBILITY), not #if HAVE(ACCESSIBILITY) like we want. The idl syntax doesn't allow HAVE(). And do we really want this to turn into an issue to allow HAVE() in .idl files? I don't think so.

(In reply to Fujii Hironori from comment #12)
> I didn't know ENABLE_ACCESSIBILITY has been added again in r245565.
> IMHO, all HAVE(ACCESSIBILITY) should be replaced with ENABLE(ACCESSIBILITY)
> in despite of the previous discussion.
> https://lists.webkit.org/pipermail/webkit-dev/2018-November/030258.html

So yes, I agree with Fujii. My vote is to add ENABLE(ACCESSIBILITY) to WebKitFeatures.cmake so that we can use it here, and then replace all use of HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY). There's no need for most ports to support disabling this option.

HAVE(ACCESSIBILITY) would be OK if it doesn't need to be a build option for WPE, but I guess the option is probably needed. (That's up to Carlos Garcia and/or Zan, both of whom are CCed.)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #22)
> There's no need for most ports to support disabling this option.

I mean: just adding it to WebKitFeatures.cmake does not require ports to expose it (all features defined there are PRIVATE by default) or support toggling it. Only WPE would expose it as a PUBLIC option.

[Fujii Hironori]

(In reply to Pablo Saavedra from comment #0)
> Linking stage build error using the non-default ENABLE_ACCESSIBILITY=OFF.
> For example:
> 
> 
>     ./Tools/Scripts/build-webkit --release --wpe
> --cmakeargs="-DENABLE_ACCESSIBILITY=OFF" --no-experimental-features
> --no-video --no-web-rtc

I can't reproduce the linkage error on my Linux box by invoking
your command because "-Wl,--no-undefined" is not specifed to
WebKitTestRunnerInjectedBundleBindings.
I need to explictly "include(WebKitCompilerFlags)" in Tools/WebKitTestRunner/CMakeLists.txt.

[Fujii Hironori]

Created attachment 373912 [details]
Patch to add AccessibilityControllerNone.cpp and AccessibilityUIElementNone.cpp

[Michael Catanzaro]

Comment on attachment 373912 [details]
Patch to add AccessibilityControllerNone.cpp and AccessibilityUIElementNone.cpp

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

> Tools/WebKitTestRunner/CMakeLists.txt:60
>      ${WEBKIT_TESTRUNNER_INJECTEDBUNDLE_DIR}/AccessibilityController.cpp
> +    ${WEBKIT_TESTRUNNER_INJECTEDBUNDLE_DIR}/AccessibilityControllerNone.cpp

Well we don't want to build them both at once...?

[Pablo Saavedra]

Created attachment 373927 [details]
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)

[Pablo Saavedra]

(In reply to Fujii Hironori from comment #25)
> Created attachment 373912 [details]
> Patch to add AccessibilityControllerNone.cpp and
> AccessibilityUIElementNone.cpp


Sorry, I a bit confusing at this point. Do you think we need this even if we control do the ENABLE(ACCESSIBILITY) stuff?

[Konstantin Tokarev]

Comment on attachment 373927 [details]
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)

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

LGTM

> Source/WTF/ChangeLog:6
> +        Added ENABLE(ACCESSIBILITY)

Sentences
Which are not terminated with periods
Look weird

[Michael Catanzaro]

Comment on attachment 373927 [details]
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)

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

Also, in OptionsPlayStation.cmake, remove:

# Temporarily turn off Accessibility support
SET_AND_EXPOSE_TO_BUILD(HAVE_ACCESSIBILITY OFF)

and add:

WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ASYNC_SCROLLING PRIVATE OFF)

>> Source/WTF/ChangeLog:6
>> +        Added ENABLE(ACCESSIBILITY)
> 
> Sentences
> Which are not terminated with periods
> Look weird

Indeed.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:403
> +    my $conditionalString = $$self{codeGenerator}->GenerateConditionalString($interface);
> +    unshift(@contents, "\n#if ${conditionalString}\n\n") if $conditionalString;

Ah wow, good fix. I didn't even realize there was a separate CodeGeneratorTestRunner.pm script!

> ChangeLog:8
> +        Added ENABLE(ACCESSIBILITY) and replaced HAVE(ACCESSIBILITY)
> +        with ENABLE(ACCESSIBILITY) in the code
> +        The TestRunner code generator honors the Conditional IDL format

So yeah, as Konstantin said, please write this out as a normal paragraph:

Added ENABLE(ACCESSIBILITY) and replaced HAVE(ACCESSIBILITY)
with ENABLE(ACCESSIBILITY) in the code.

Additionally, the TestRunner code generator now honors the
Conditional IDL format.

[Pablo Saavedra]

Created attachment 373936 [details]
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)

[Pablo Saavedra]

(In reply to Konstantin Tokarev from comment #29)
> Comment on attachment 373927 [details]
> Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373927&action=review
> 
> LGTM
> 
> > Source/WTF/ChangeLog:6
> > +        Added ENABLE(ACCESSIBILITY)
> 
> Sentences
> Which are not terminated with periods
> Look weird

Yeah, totally right.

[WebKit Commit Bot]

Comment on attachment 373936 [details]
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)

Clearing flags on attachment: 373936

Committed r247367: <https://trac.webkit.org/changeset/247367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Don Olmstead]

There are still cases of HAVE(ACCESSIBILITY) and HAVE_ACCESSIBILITY in the code.

There are quite a few instances of HAVE(ACCESSIBILITY) in the Tools directory. Platform.h has a HAVE_ACCESSIBILITY define, and some WPE code in the Tools directory checks HAVE_ACCESSIBILITY.

[Michael Catanzaro]

Reopening.

[Pablo Saavedra]

(In reply to Don Olmstead from comment #35)
> There are still cases of HAVE(ACCESSIBILITY) and HAVE_ACCESSIBILITY in the
> code.
> 
> There are quite a few instances of HAVE(ACCESSIBILITY) in the Tools
> directory. Platform.h has a HAVE_ACCESSIBILITY define, and some WPE code in
> the Tools directory checks HAVE_ACCESSIBILITY.

I see. I will take care about this. Thanks.

[Pablo Saavedra]

Created attachment 378120 [details]
patch 2nd iteration

[Pablo Saavedra]

(In reply to Pablo Saavedra from comment #38)
> Created attachment 378120 [details]
> patch 2nd iteration

Still needs some love.

[Don Olmstead]

Comment on attachment 378120 [details]
patch 2nd iteration

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

Just a couple questions on it and a suggestion to USE(ATK) where applicable.

> Tools/DumpRenderTree/AccessibilityController.h:38
> +#if ENABLE(ACCESSIBILITY) && PLATFORM(GTK)

I think technically this should be ENABLE(ACCESSIBILITY) && USE(ATK) but I also don't see DumpRenderTree being used for GTK so these should probably be deleted.

> Tools/DumpRenderTree/AccessibilityController.h:80
> +#if ENABLE(ACCESSIBILITY) && PLATFORM(GTK)

Ditto

> Tools/DumpRenderTree/AccessibilityController.h:101
> +#if ENABLE(ACCESSIBILITY) && PLATFORM(GTK)

Ditto

> Tools/DumpRenderTree/AccessibilityUIElement.h:48
> +#elif ENABLE(ACCESSIBILITY) && PLATFORM(GTK)

Ditto

> Tools/MiniBrowser/wpe/main.cpp:36
> +#if ENABLE(ACCESSIBILITY)

ATK is right there so it should probably be && USE(ATK)

> Tools/MiniBrowser/wpe/main.cpp:290
> +#if ENABLE(ACCESSIBILITY)

Same

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h:72
> +#if !ENABLE(ACCESSIBILITY) && (PLATFORM(GTK) || PLATFORM(WPE))

Should this just be PLATFORM(GTK) || PLATFORM(WPE)? Wondering if this should just go into an else with the above.

> Tools/wpe/backends/ViewBackend.cpp:32
> +#if ENABLE(ACCESSIBILITY)

&& USE(ATK)

> Tools/wpe/backends/ViewBackend.cpp:40
> +#if ENABLE(ACCESSIBILITY)

Ditto

> Tools/wpe/backends/ViewBackend.cpp:205
> +#if ENABLE(ACCESSIBILITY)

Same

> Tools/wpe/backends/ViewBackend.cpp:236
> +#if ENABLE(ACCESSIBILITY)

Same

> Tools/wpe/backends/ViewBackend.cpp:262
> +#if ENABLE(ACCESSIBILITY)

Same

> Tools/wpe/backends/ViewBackend.h:58
> +#if ENABLE(ACCESSIBILITY)

Same

> Tools/wpe/backends/WebKitAccessibleApplication.h:28
> +#if ENABLE(ACCESSIBILITY)

Same

[Michael Catanzaro]

DumpRenderTree is WK1-only so yes, it's not built by GTK or WPE.

[Don Olmstead]

(In reply to Michael Catanzaro from comment #41)
> DumpRenderTree is WK1-only so yes, it's not built by GTK or WPE.

Ok so remove any code that is unused by GTK and WPE, update to USE(ATK) and make it so things are building and I'll r+

[Pablo Saavedra]

Created attachment 381015 [details]
patch 2nd iteration

[Carlos Alberto Lopez Perez]

Comment on attachment 381015 [details]
patch 2nd iteration

There are several EWS red with this patch, that has to be fixed.

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #44)
> Comment on attachment 381015 [details]
> patch 2nd iteration
> 
> There are several EWS red with this patch, that has to be fixed.

In the case of the WPE build failure the definition of the ENABLE()
macro is missing, see:

  ../../Tools/wpe/backends/ViewBackend.h:58:5: warning: "ENABLE" is not defined, evaluates to 0 [-Wundef]
  #if ENABLE(ACCESSIBILITY) && USE(ATK)
      ^~~~~~
  ../../Tools/wpe/backends/ViewBackend.h:58:11: error: missing binary operator before token "("
  #if ENABLE(ACCESSIBILITY) && USE(ATK)
            ^

The build failure for the Cocoa (iOS + Mac) ports is trickier to find,
it's best to download the whole build log from buildbot and grep around:

/Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:454:108: error: use of undeclared identifier 'kAXSApplicationAccessibilityEnabledNotification'
    m_accessibilityEnabledObserver = [[NSNotificationCenter defaultCenter] addObserverForName:(__bridge id)kAXSApplicationAccessibilityEnabledNotification object:nil queue: [NSOperationQueue currentQueue] usingBlock:^(NSNotification *) {
                                                                                                           ^

IIUC the identifier that the compiler reports as missing is related
to accessibility, so you may want need to look around for it.

[Don Olmstead]

Created attachment 449417 [details]
WIP Patch

[Don Olmstead]

Closing in favor of https://bugs.webkit.org/show_bug.cgi?id=235335 for removing uses of HAVE_ACCESSIBILITY in Tools