Bug 206009

Created attachment 387235 [details]
Patch

Comment on attachment 387235 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:193
> +        return [NSValue valueWithPoint:(NSPoint)protectedSelf->m_page->accessibilityPosition()];

we should also check 
'if (!protectedSelf->m_page) here

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:203
> +            if (protectedSelf->m_page->corePage()->pageOverlayController().copyAccessibilityAttributeBoolValueForPoint(attribute, point, boolValue))

if (!protectedSelf->m_page)

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:208
> +            if (protectedSelf->m_page->corePage()->pageOverlayController().copyAccessibilityAttributeStringValueForPoint(attribute, point, stringValue))

if (!protectedSelf->m_page)

Created attachment 387242 [details]
Patch

(In reply to chris fleizach from comment #2)
> Comment on attachment 387235 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387235&action=review

Also added the check in line 118.
> 
> > Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:193
> > +        return [NSValue valueWithPoint:(NSPoint)protectedSelf->m_page->accessibilityPosition()];
> 
> we should also check 
> 'if (!protectedSelf->m_page) here

Done.
> 
> > Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:203
> > +            if (protectedSelf->m_page->corePage()->pageOverlayController().copyAccessibilityAttributeBoolValueForPoint(attribute, point, boolValue))
> 
> if (!protectedSelf->m_page)

Done.
> 
> > Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:208
> > +            if (protectedSelf->m_page->corePage()->pageOverlayController().copyAccessibilityAttributeStringValueForPoint(attribute, point, stringValue))
> 
> if (!protectedSelf->m_page)

Done.

Comment on attachment 387242 [details]
Patch

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

Is there any testable behaviour here?

> Source/WebKit/ChangeLog:3
> +        WKAccessibilityWebPageObjectt should use Accessibility::retrieveValueFromMainThread.

Typo Objectt

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:56
> +#define PROTECTED_SELF protectedSelf = RetainPtr<WKAccessibilityWebPageObject>(self)

We don't normally do something like this. We just duplicate the code. I don't really mind though.

Created attachment 387244 [details]
Patch

(In reply to Dean Jackson from comment #5)
> Comment on attachment 387242 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387242&action=review
> 
> Is there any testable behaviour here?

Thanks Dean. We don't have a framework to test the WKAccessibilityWebPageObject functionality. We are working on a way to test the accessibility multi-threaded mode though, but are not there yet.

> 
> > Source/WebKit/ChangeLog:3
> > +        WKAccessibilityWebPageObjectt should use Accessibility::retrieveValueFromMainThread.
> 
> Typo Objectt

Fixed.
> 
> > Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:56
> > +#define PROTECTED_SELF protectedSelf = RetainPtr<WKAccessibilityWebPageObject>(self)
> 
> We don't normally do something like this. We just duplicate the code. I
> don't really mind though.

Thought that it is more readable this way.

Comment on attachment 387244 [details]
Patch

Clearing flags on attachment: 387244

Committed r254294: <https://trac.webkit.org/changeset/254294>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/58455650>