[Webkit-unassigned] [Bug 121959] New: [ATK] Normalize checks in entry points for DRT and WKTR

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=121959

           Summary: [ATK] Normalize checks in entry points for DRT and
                    WKTR
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: Accessibility
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: mario at webkit.org
                CC: cfleizach at apple.com, jdiggs at igalia.com,
                    k.czech at samsung.com,
                    webkit-bug-importer at group.apple.com


By taking a look to ATK's DRT and WKTR, is easy to check that there is no consistency in the way we do the checks at the entry points for AccessibilityUIElement, since sometimes we check !m_element, other times we check !ATK_IS_OBJECT(m_element) - or an specific ATK interface -, sometimes we check both... and sometimes we just don't check, which leads to some funny messages in stderr such as this one:

  atk_object_ref_accessible_child: assertion `ATK_IS_OBJECT (accessible)' failed

At least, this is a problem with tests such as accessibility/loading-iframe-updates-axtree.html or accessibility/svg-bounds.html, which are spitting that kind of error because the stuff that is needed for the test to pass is not implemented (either in the ATK code or in the DRT/WKTR).

Another manifestation of this issue, again due to tests running whose needs are not [properly] implemented yet, is what you can see in the logs for WK2GTK layout tests:

  07:30:55.934 26675 worker/1 accessibility/canvas-fallback-content-2.html output stderr lines:
  07:30:55.934 26675   invalid cast from `WAIType191' to `AtkValue'
  07:30:55.934 26675   atk_value_get_current_value: assertion `ATK_IS_VALUE (obj)' failed
  07:30:55.934 26675   invalid cast from `WAIType199' to `AtkValue'
  07:30:55.934 26675   atk_value_get_current_value: assertion `ATK_IS_VALUE (obj)' failed
  07:30:55.934 26675   invalid cast from `WAIType191' to `AtkValue'
  [...]

So, I think we should agree on a normalized way to check the entry points in DRT/WKTR, and my proposal is to check that we are in front of an instance of AtkObject implementing, if needed, the interfaces that we need for implementing a given functionality, gracefully exiting otherwise. No more checking for non-null and maybe later asserting for the right interface, since that will be of little help if the object we are receiving is not of the expected type (maybe, because the hierarchy exposed for GTK does not match in that case the one exposed for the Mac). And anyway, we will still detect that there's an issue or a missing implementation somewhere since the expectations won't match.

For instance, consider the following partial diff between the current situation and the proposed one:

  [...]
   int AccessibilityUIElement::columnCount()
   {
  -    if (!m_element)
  +    if (!ATK_IS_TABLE(m_element))
           return 0;

  -    ASSERT(ATK_IS_TABLE(m_element));
  -
       return atk_table_get_n_columns(ATK_TABLE(m_element));
   }

   int AccessibilityUIElement::childrenCount()
   {
  -    if (!m_element)
  +    if (!ATK_IS_OBJECT(m_element))
           return 0;

  -    ASSERT(ATK_IS_OBJECT(m_element));
  -
       return atk_object_get_n_accessible_children(ATK_OBJECT(m_element));
   }

   AccessibilityUIElement AccessibilityUIElement::elementAtPoint(int x, int y)
   {
  -    if (!m_element)
  +    if (!ATK_IS_COMPONENT(m_element))
           return 0;

       return AccessibilityUIElement(atk_component_ref_accessible_at_point(ATK_COMPONENT(m_element), x, y, ATK_XY_WINDOW));
   }
  [...]


As the ATK_IS_OBJECT, ATK_IS_TABLE... macros already do the null check, we do not need to check that explicitly. Also, we do not need to check for ATK_IS_OBJECT if we are already checking for an ATK interface, since that will always be implemented by an ATK_OBJECT. Last, I would not use ASSERT() here because we do not certainly want to continue if we don't have the proper object. Let's just the results be wrong, so we can realize of something being wrong.

For all these reasons, and also to prevent further confusion when adding code to these files in DRT and WKTR, I propose to write a patch now to normalize the situation. A patch that I've been already working on and testing today and that I expect to upload soon.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.