Bug 236024 – [WPE] Fix for non-unified builds with ACCESSIBILITY=OFF

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Pablo Saavedra]

in the code you can see many blocks safeguarded by:

  #if ENABLE(ACCESSIBILITY) && USE(ATSPI)


But, at the same time, there is in many other places another variation of those checks without the ENABLE(ACCESSIBILITY) only checking the USE_ATSPI or USE_ATK values.


This generates inconsistencies because missing definitions when ACCESSIBILITY is OFF. For example here  where the definition of the "class AccessibilityObjectAtspi" is safeguarded in Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.h  with "#if ENABLE(ACCESSIBILITY) && USE(ATSPI)" but later in the   the Source/WebCore/accessibility/AccessibilityObjectInterface.h:

  #if PLATFORM(COCOA)
  OBJC_CLASS WebAccessibilityObjectWrapper;
  typedef WebAccessibilityObjectWrapper AccessibilityObjectWrapper;
  typedef struct _NSRange NSRange;
  typedef const struct __AXTextMarker* AXTextMarkerRef;
  typedef const struct __AXTextMarkerRange* AXTextMarkerRangeRef;
  #elif USE(ATSPI)
  typedef WebCore::AccessibilityObjectAtspi AccessibilityObjectWrapper; <<<<<<<<  Missing definition of WebCore::AccessibilityObjectAtspi
  #elif USE(ATK)
  typedef struct _WebKitAccessible WebKitAccessible;
  typedef struct _WebKitAccessible AccessibilityObjectWrapper;
  #else
  class AccessibilityObjectWrapper;
  #endif

The patch uploaded in this sets TRUE USE_ATK or USE_ATSPI as mutually exclusive  when ENABLE_ACCESSIBILITY=ON and forces both to FALSE if case of ENABLE_ACCESSIBILITY=OFF:


-if (ENABLE_ACCESSIBILITY AND NOT USE_ATSPI)
-    SET_AND_EXPOSE_TO_BUILD(USE_ATK TRUE)
+if (ENABLE_ACCESSIBILITY)
+    if (USE_ATSPI)
+        SET_AND_EXPOSE_TO_BUILD(USE_ATK FALSE)
+    else ()
+        SET_AND_EXPOSE_TO_BUILD(USE_ATK TRUE)
+    endif ()
+else ()
+    SET_AND_EXPOSE_TO_BUILD(USE_ATK FALSE)
+    SET_AND_EXPOSE_TO_BUILD(USE_ATSPI FALSE)


This simplifies the checks in the code since from now on a simple "#if USE(ATSPI)"  is equivalent to a "#if ENABLE(ACCESSIBILITY) && USE(ATSPI)"

[Pablo Saavedra]

Created attachment 450660 [details]
patch

[Pablo Saavedra]

Created attachment 450661 [details]
patch

[Darin Adler]

This solution is fine.

But I have a slight preference for solving as many of these problems as possible in C++ headers rather than in CMake. For example, we could have this code in one of the platform headers:

    #if !ENABLED(ACCESSIBILITY)
    #undef USE_ATK
    #undef USE_ATSPI
    #endif

Then we would not have to be careful about those settings in the build/make environment because the header would fix it. My view is that generally speaking our main implementation language is C++, and we should only do the things in the build system that cannot be cleanly done with C++ code.

[Pablo Saavedra]

(In reply to Darin Adler from comment #3)
> This solution is fine.
> 
> But I have a slight preference for solving as many of these problems as
> possible in C++ headers rather than in CMake. For example, we could have
> this code in one of the platform headers:
> 
>     #if !ENABLED(ACCESSIBILITY)
>     #undef USE_ATK
>     #undef USE_ATSPI
>     #endif
> 
> Then we would not have to be careful about those settings in the build/make
> environment because the header would fix it. My view is that generally
> speaking our main implementation language is C++, and we should only do the
> things in the build system that cannot be cleanly done with C++ code.


Sounds reasonable and Normally I might tend to agree with you and found your suggestion much more convenient. However those USE_ATK/USE_ATSPI are also used by other .cmake files like here:


  ./WebCore/PlatformWPE.cmake

  if (USE_ATSPI)
      set(WebCore_AtspiInterfaceFiles
      ...

In summary, if we don't set those definitions early in the Source/cmake/OptionsWPE.cmake then we could generate again inconsistencies because the differences between the values of those definitions in the cmake and then in the .h .cpp sources.

[Carlos Garcia Campos]

Comment on attachment 450661 [details]
patch

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

> Source/WebCore/ChangeLog:3
> +        [WPE] Fix for non-unified builds with ACCESSIBILITY=OFF

Why is this wpe specific?

[EWS]

Committed r289029 (246737@main): <https://commits.webkit.org/246737@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450661 [details].

[Radar WebKit Bug Importer]

<rdar://problem/88420464>