Bug 219037 - Implementation of AXCoreObject::innerHTML and outerHTML.
Summary: Implementation of AXCoreObject::innerHTML and outerHTML.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-17 08:46 PST by Andres Gonzalez
Modified: 2020-11-17 14:15 PST (History)
10 users (show)

See Also:


Attachments
Patch (10.59 KB, patch)
2020-11-17 09:01 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (10.61 KB, patch)
2020-11-17 10:22 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2020-11-17 08:46:10 PST
Implementation of AXCoreObject::innerHTML and outerHTML.
Comment 1 Andres Gonzalez 2020-11-17 09:01:49 PST
Created attachment 414347 [details]
Patch
Comment 2 chris fleizach 2020-11-17 09:19:31 PST
Comment on attachment 414347 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:3584
> +String AccessibilityObject::innerHTML() const

can we do this without the debug flags? there's been lots of times now that I am trying to debug something in WK without debug symbols and just wishing I could convert from AX tree > HTML

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:2118
> +        const_cast<AXIsolatedObject*>(this)->setProperty(AXPropertyName::OuterHTML, value);

worried that we're setting this property on the main thread. I guess we'll be ok because the AX thread is also blocked. can you think of any other issues with this?
Comment 3 Darin Adler 2020-11-17 10:05:04 PST
Comment on attachment 414347 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:2103
> +        String value = axObject ? axObject->innerHTML().isolatedCopy()
> +            : String().isolatedCopy();

String().isolatedCopy() is the same as Striing().
Comment 4 Andres Gonzalez 2020-11-17 10:22:17 PST
Created attachment 414350 [details]
Patch
Comment 5 Andres Gonzalez 2020-11-17 10:30:27 PST
(In reply to chris fleizach from comment #2)
> Comment on attachment 414347 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414347&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:3584
> > +String AccessibilityObject::innerHTML() const
> 
> can we do this without the debug flags? there's been lots of times now that
> I am trying to debug something in WK without debug symbols and just wishing
> I could convert from AX tree > HTML

Done.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:2118
> > +        const_cast<AXIsolatedObject*>(this)->setProperty(AXPropertyName::OuterHTML, value);
> 
> worried that we're setting this property on the main thread. I guess we'll
> be ok because the AX thread is also blocked. can you think of any other
> issues with this?

It could be a problem if this method is called on the main thread and there are other isolated object methods being called on the AX thread. This shouldn't happen though. Let me see if I can add an assert to ensure this in debug.
Comment 6 Andres Gonzalez 2020-11-17 10:31:39 PST
(In reply to Darin Adler from comment #3)
> Comment on attachment 414347 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414347&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:2103
> > +        String value = axObject ? axObject->innerHTML().isolatedCopy()
> > +            : String().isolatedCopy();
> 
> String().isolatedCopy() is the same as Striing().

Fixed.
Comment 7 Andres Gonzalez 2020-11-17 10:54:07 PST
(In reply to chris fleizach from comment #2)
> Comment on attachment 414347 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414347&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:3584
> > +String AccessibilityObject::innerHTML() const
> 
> can we do this without the debug flags? there's been lots of times now that
> I am trying to debug something in WK without debug symbols and just wishing
> I could convert from AX tree > HTML
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:2118
> > +        const_cast<AXIsolatedObject*>(this)->setProperty(AXPropertyName::OuterHTML, value);
> 
> worried that we're setting this property on the main thread. I guess we'll
> be ok because the AX thread is also blocked. can you think of any other
> issues with this?

The problem with the assert is that if you break on the main thread during debugging with isolated mode on, then you cannot call these methods. You can only call it on the secondary thread. Would that be ok?
Comment 8 chris fleizach 2020-11-17 11:00:55 PST
(In reply to Andres Gonzalez from comment #7)
> (In reply to chris fleizach from comment #2)
> > Comment on attachment 414347 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=414347&action=review
> > 
> > > Source/WebCore/accessibility/AccessibilityObject.cpp:3584
> > > +String AccessibilityObject::innerHTML() const
> > 
> > can we do this without the debug flags? there's been lots of times now that
> > I am trying to debug something in WK without debug symbols and just wishing
> > I could convert from AX tree > HTML
> > 
> > > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:2118
> > > +        const_cast<AXIsolatedObject*>(this)->setProperty(AXPropertyName::OuterHTML, value);
> > 
> > worried that we're setting this property on the main thread. I guess we'll
> > be ok because the AX thread is also blocked. can you think of any other
> > issues with this?
> 
> The problem with the assert is that if you break on the main thread during
> debugging with isolated mode on, then you cannot call these methods. You can
> only call it on the secondary thread. Would that be ok?

I think we're probably OK because the secondary thread will be occupied while the main thread modifies the property map, but its not really air-tight.
Comment 9 EWS 2020-11-17 14:14:43 PST
Committed r269923: <https://trac.webkit.org/changeset/269923>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414350 [details].
Comment 10 Radar WebKit Bug Importer 2020-11-17 14:15:28 PST
<rdar://problem/71504019>