Bug 213124 – In isolated tree mode 2, AXIsolatedObject::setChildrenIDs should be called only on secondary thread.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andres Gonzalez]

In isolated tree mode 2, AXIsolatedObject::setChildrenIDs should be called only on secondary thread.

[Andres Gonzalez]

Created attachment 401727 [details]
Patch

[chris fleizach]

Comment on attachment 401727 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        - The unsigned int range for object IDs was being overrun for large number of objects like in the case of the sample page in <rdar://problem/59331146>. INcreased the size of AXID to size_t.

INcreased -> Increased

> Source/WebCore/accessibility/AccessibilityObjectInterface.h:75
> +typedef size_t AXID;

should we make this a uint64_t

[Andres Gonzalez]

Created attachment 401750 [details]
Patch

[Andres Gonzalez]

Created attachment 401751 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #2)
> Comment on attachment 401727 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401727&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        - The unsigned int range for object IDs was being overrun for large number of objects like in the case of the sample page in <rdar://problem/59331146>. INcreased the size of AXID to size_t.
> 
> INcreased -> Increased

Fixed.
> 
> > Source/WebCore/accessibility/AccessibilityObjectInterface.h:75
> > +typedef size_t AXID;
> 
> should we make this a uint64_t

size_t is guarantied the largest unsigned integer range the system can handle. But we can make it a uint64_t if we prefer to specify the size explicitly.

[EWS]

Committed r262966: <https://trac.webkit.org/changeset/262966>

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

[Radar WebKit Bug Importer]

<rdar://problem/64307457>

[Darin Adler]

Comment on attachment 401751 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:68
> +    void setChildrenIDs(Vector<AXID>&& childrenIDs)
> +    {
> +        // FIXME: The following ASSERT should be met but it is commented out at the
> +        // moment because of <rdar://problem/63985646> After calling _AXUIElementUseSecondaryAXThread(true),
> +        // still receives client request on main thread.
> +        // ASSERT(axObjectCache()->canUseSecondaryAXThread() ? !isMainThread() : isMainThread());
> +        m_childrenIDs = childrenIDs;
> +    }

Style thought for the future:

Generally seems like a poor pattern to move code from .cpp files to headers without a strong motivation; long term across many source files it likely makes our code slower to compile so we better be sure there is a benefit. Also, larger class definitions with function bodies can make it harder to see the design of the class.

I see, thought, that *this* is the kind of simple setter that can be super efficient so it’s tempting to make it inlined. Even if doing that, I think it’s better style to do that outside the class definition so it’s easier to refactor (move to and from the .cpp file). And I think that global optimization may be effective at inlining even when we don’t move things to headers.