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.
Created attachment 373730 [details] patch
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
Now looking at https://bugs.webkit.org/show_bug.cgi?id=191831 I think that patch should be reverted
(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
(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 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.
Created attachment 373749 [details] patch
(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 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.)
If we are using -DENABLE_ACCESSIBILITY=OFF build option, there must be corresponding WEBKIT_OPTION
ENABLE(ACCESSIBILITY) is a WPE-specific option. It can't be used in these cross-platform files. Just use the existing HAVE(ACCESSIBILITY).
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
(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:
(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.
Created attachment 373828 [details] patch
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).
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
Created attachment 373848 [details] patch
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 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.
(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
(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.)
(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.
(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.
Created attachment 373912 [details] Patch to add AccessibilityControllerNone.cpp and AccessibilityUIElementNone.cpp
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...?
Created attachment 373927 [details] Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)
(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 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 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.
Created attachment 373936 [details] Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY)
(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 on attachment 373936 [details] Replacing HAVE(ACCESSIBILITY) with ENABLE(ACCESSIBILITY) Clearing flags on attachment: 373936 Committed r247367: <https://trac.webkit.org/changeset/247367>
All reviewed patches have been landed. Closing bug.
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.
Reopening.
(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.
Created attachment 378120 [details] patch 2nd iteration
(In reply to Pablo Saavedra from comment #38) > Created attachment 378120 [details] > patch 2nd iteration Still needs some love.
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
DumpRenderTree is WK1-only so yes, it's not built by GTK or WPE.
(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+
Created attachment 381015 [details] patch 2nd iteration
Comment on attachment 381015 [details] patch 2nd iteration There are several EWS red with this patch, that has to be fixed.
(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.