RESOLVED FIXED 199625
[WPE][GTK] Build failure with ENABLE_ACCESSIBILITY=OFF
https://bugs.webkit.org/show_bug.cgi?id=199625
Summary [WPE][GTK] Build failure with ENABLE_ACCESSIBILITY=OFF
Pablo Saavedra
Reported 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.
Attachments
patch (3.18 KB, patch)
2019-07-09 10:24 PDT, Pablo Saavedra
no flags
patch (3.19 KB, patch)
2019-07-09 13:18 PDT, Pablo Saavedra
no flags
patch (3.20 KB, patch)
2019-07-10 01:42 PDT, Pablo Saavedra
no flags
patch (3.19 KB, patch)
2019-07-10 10:49 PDT, Pablo Saavedra
no flags
Patch to add AccessibilityControllerNone.cpp and AccessibilityUIElementNone.cpp (20.14 KB, patch)
2019-07-11 02:37 PDT, Fujii Hironori
no flags
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY) (71.03 KB, patch)
2019-07-11 10:22 PDT, Pablo Saavedra
no flags
Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY) (72.16 KB, patch)
2019-07-11 12:21 PDT, Pablo Saavedra
no flags
patch 2nd iteration (26.63 KB, patch)
2019-09-05 14:14 PDT, Pablo Saavedra
no flags
patch 2nd iteration (27.13 KB, patch)
2019-10-15 13:18 PDT, Pablo Saavedra
no flags
WIP Patch (24.72 KB, patch)
2022-01-18 13:46 PST, Don Olmstead
no flags
Pablo Saavedra
Comment 1 2019-07-09 10:24:02 PDT
Konstantin Tokarev
Comment 2 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
Konstantin Tokarev
Comment 3 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
Pablo Saavedra
Comment 4 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
Pablo Saavedra
Comment 5 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
Michael Catanzaro
Comment 6 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.
Pablo Saavedra
Comment 7 2019-07-09 13:18:39 PDT
Pablo Saavedra
Comment 8 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.
Michael Catanzaro
Comment 9 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.)
Konstantin Tokarev
Comment 10 2019-07-09 13:34:14 PDT
If we are using -DENABLE_ACCESSIBILITY=OFF build option, there must be corresponding WEBKIT_OPTION
Michael Catanzaro
Comment 11 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).
Fujii Hironori
Comment 12 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
Pablo Saavedra
Comment 13 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:
Pablo Saavedra
Comment 14 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.
Pablo Saavedra
Comment 15 2019-07-10 01:42:16 PDT
Michael Catanzaro
Comment 16 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).
Michael Catanzaro
Comment 17 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
Pablo Saavedra
Comment 18 2019-07-10 10:49:54 PDT
Konstantin Tokarev
Comment 19 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
Michael Catanzaro
Comment 20 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.
Pablo Saavedra
Comment 21 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
Michael Catanzaro
Comment 22 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.)
Michael Catanzaro
Comment 23 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.
Fujii Hironori
Comment 24 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.
Fujii Hironori
Comment 25 2019-07-11 02:37:39 PDT
Created attachment 373912 [details] Patch to add AccessibilityControllerNone.cpp and AccessibilityUIElementNone.cpp
Michael Catanzaro
Comment 26 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...?
Pablo Saavedra
Comment 27 2019-07-11 10:22:36 PDT
Created attachment 373927 [details] Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)
Pablo Saavedra
Comment 28 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?
Konstantin Tokarev
Comment 29 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
Michael Catanzaro
Comment 30 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.
Pablo Saavedra
Comment 31 2019-07-11 12:21:30 PDT
Created attachment 373936 [details] Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)
Pablo Saavedra
Comment 32 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.
WebKit Commit Bot
Comment 33 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>
WebKit Commit Bot
Comment 34 2019-07-11 14:51:34 PDT
All reviewed patches have been landed. Closing bug.
Don Olmstead
Comment 35 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.
Michael Catanzaro
Comment 36 2019-08-09 14:16:08 PDT
Reopening.
Pablo Saavedra
Comment 37 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.
Pablo Saavedra
Comment 38 2019-09-05 14:14:04 PDT
Created attachment 378120 [details] patch 2nd iteration
Pablo Saavedra
Comment 39 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.
Don Olmstead
Comment 40 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
Michael Catanzaro
Comment 41 2019-09-06 05:39:19 PDT
DumpRenderTree is WK1-only so yes, it's not built by GTK or WPE.
Don Olmstead
Comment 42 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+
Pablo Saavedra
Comment 43 2019-10-15 13:18:22 PDT
Created attachment 381015 [details] patch 2nd iteration
Carlos Alberto Lopez Perez
Comment 44 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.
Adrian Perez
Comment 45 2019-10-21 14:15:37 PDT
(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
Comment 46 2022-01-18 13:46:07 PST
Created attachment 449417 [details] WIP Patch
Don Olmstead
Comment 47 2022-01-18 14:55:22 PST
Closing in favor of https://bugs.webkit.org/show_bug.cgi?id=235335 for removing uses of HAVE_ACCESSIBILITY in Tools
Note You need to log in before you can comment on or make changes to this bug.