Bug 199625 - [WPE][GTK] Build failure with ENABLE_ACCESSIBILITY=OFF
Summary: [WPE][GTK] Build failure with ENABLE_ACCESSIBILITY=OFF
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pablo Saavedra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-09 10:21 PDT by Pablo Saavedra
Modified: 2019-10-16 14:06 PDT (History)
18 users (show)

See Also:


Attachments
patch (3.18 KB, patch)
2019-07-09 10:24 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
patch (3.19 KB, patch)
2019-07-09 13:18 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
patch (3.20 KB, patch)
2019-07-10 01:42 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
patch (3.19 KB, patch)
2019-07-10 10:49 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
Patch to add AccessibilityControllerNone.cpp and AccessibilityUIElementNone.cpp (20.14 KB, patch)
2019-07-11 02:37 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY) (71.03 KB, patch)
2019-07-11 10:22 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY) (72.16 KB, patch)
2019-07-11 12:21 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
patch 2nd iteration (26.63 KB, patch)
2019-09-05 14:14 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
patch 2nd iteration (27.13 KB, patch)
2019-10-15 13:18 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pablo Saavedra 2019-07-09 10:21:07 PDT
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.
Comment 1 Pablo Saavedra 2019-07-09 10:24:02 PDT
Created attachment 373730 [details]
patch
Comment 2 Konstantin Tokarev 2019-07-09 12:09:08 PDT
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
Comment 3 Konstantin Tokarev 2019-07-09 12:11:13 PDT
Now looking at https://bugs.webkit.org/show_bug.cgi?id=191831 I think that patch should be reverted
Comment 4 Pablo Saavedra 2019-07-09 12:52:00 PDT
(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
Comment 5 Pablo Saavedra 2019-07-09 12:54:14 PDT
(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
Comment 6 Michael Catanzaro 2019-07-09 13:07:39 PDT
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.
Comment 7 Pablo Saavedra 2019-07-09 13:18:39 PDT
Created attachment 373749 [details]
patch
Comment 8 Pablo Saavedra 2019-07-09 13:19:47 PDT
(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.
Comment 9 Michael Catanzaro 2019-07-09 13:26:18 PDT
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.)
Comment 10 Konstantin Tokarev 2019-07-09 13:34:14 PDT
If we are using -DENABLE_ACCESSIBILITY=OFF build option, there must be corresponding WEBKIT_OPTION
Comment 11 Michael Catanzaro 2019-07-09 13:56:14 PDT
ENABLE(ACCESSIBILITY) is a WPE-specific option. It can't be used in these cross-platform files. Just use the existing HAVE(ACCESSIBILITY).
Comment 12 Fujii Hironori 2019-07-09 18:15:48 PDT
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
Comment 13 Pablo Saavedra 2019-07-10 01:18:07 PDT
(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:
Comment 14 Pablo Saavedra 2019-07-10 01:21:03 PDT
(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.
Comment 15 Pablo Saavedra 2019-07-10 01:42:16 PDT
Created attachment 373828 [details]
patch
Comment 16 Michael Catanzaro 2019-07-10 08:01:06 PDT
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).
Comment 17 Michael Catanzaro 2019-07-10 08:01:44 PDT
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
Comment 18 Pablo Saavedra 2019-07-10 10:49:54 PDT
Created attachment 373848 [details]
patch
Comment 19 Konstantin Tokarev 2019-07-10 11:01:40 PDT
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
Comment 20 Michael Catanzaro 2019-07-10 11:53:04 PDT
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.
Comment 21 Pablo Saavedra 2019-07-10 14:05:58 PDT
(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
Comment 22 Michael Catanzaro 2019-07-10 14:51:46 PDT
(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.)
Comment 23 Michael Catanzaro 2019-07-10 14:53:31 PDT
(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.
Comment 24 Fujii Hironori 2019-07-11 02:36:39 PDT
(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.
Comment 25 Fujii Hironori 2019-07-11 02:37:39 PDT
Created attachment 373912 [details]
Patch to add AccessibilityControllerNone.cpp and AccessibilityUIElementNone.cpp
Comment 26 Michael Catanzaro 2019-07-11 08:48:09 PDT
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...?
Comment 27 Pablo Saavedra 2019-07-11 10:22:36 PDT
Created attachment 373927 [details]
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)
Comment 28 Pablo Saavedra 2019-07-11 10:36:01 PDT
(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?
Comment 29 Konstantin Tokarev 2019-07-11 10:45:37 PDT
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
Comment 30 Michael Catanzaro 2019-07-11 11:38:49 PDT
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.
Comment 31 Pablo Saavedra 2019-07-11 12:21:30 PDT
Created attachment 373936 [details]
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)
Comment 32 Pablo Saavedra 2019-07-11 12:25:58 PDT
(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.
Comment 33 WebKit Commit Bot 2019-07-11 14:51:32 PDT
Comment on attachment 373936 [details]
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)

Clearing flags on attachment: 373936

Committed r247367: <https://trac.webkit.org/changeset/247367>
Comment 34 WebKit Commit Bot 2019-07-11 14:51:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Don Olmstead 2019-08-09 11:37:27 PDT
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.
Comment 36 Michael Catanzaro 2019-08-09 14:16:08 PDT
Reopening.
Comment 37 Pablo Saavedra 2019-09-05 13:27:04 PDT
(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.
Comment 38 Pablo Saavedra 2019-09-05 14:14:04 PDT
Created attachment 378120 [details]
patch 2nd iteration
Comment 39 Pablo Saavedra 2019-09-05 14:23:56 PDT
(In reply to Pablo Saavedra from comment #38)
> Created attachment 378120 [details]
> patch 2nd iteration

Still needs some love.
Comment 40 Don Olmstead 2019-09-05 14:34:26 PDT
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
Comment 41 Michael Catanzaro 2019-09-06 05:39:19 PDT
DumpRenderTree is WK1-only so yes, it's not built by GTK or WPE.
Comment 42 Don Olmstead 2019-09-06 10:37:09 PDT
(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+
Comment 43 Pablo Saavedra 2019-10-15 13:18:22 PDT
Created attachment 381015 [details]
patch 2nd iteration
Comment 44 Carlos Alberto Lopez Perez 2019-10-16 14:06:02 PDT
Comment on attachment 381015 [details]
patch 2nd iteration

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