Bug 252924

<rdar://problem/105895024>

Created attachment 465157 [details]
Patch

Comment on attachment 465157 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:3337
> +    auto validate = characterOffsetFromVisiblePosition(visiblePosition);

I know you didn't name this so feel free to not address this, but I don't understand why this variable is called validate. Do you? I wonder if we could give this a more fitting name for how it's used,

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:136
> +    // The AttributedString is cached with spelling info. If the caller does not request spelling info, we have to remove it before returning.

It feels like this comment belongs below this if-statement, since below the if-statement is where we actually remove the spelling info.

Created attachment 465160 [details]
Patch

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 465157 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465157&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:3337
> > +    auto validate = characterOffsetFromVisiblePosition(visiblePosition);
> 
> I know you didn't name this so feel free to not address this, but I don't
> understand why this variable is called validate. Do you? I wonder if we
> could give this a more fitting name for how it's used,
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:136
> > +    // The AttributedString is cached with spelling info. If the caller does not request spelling info, we have to remove it before returning.
> 
> It feels like this comment belongs below this if-statement, since below the
> if-statement is where we actually remove the spelling info.

Fixed both. Thanks.

Comment on attachment 465160 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:-1584
> -        return passwordFieldValue().length();

why do we want to remove this password field override?

> Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:28
> +#import <Foundation/NSRange.h>

do we really need this import? I haver to imagine that this is brought in automatically

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:112
> +        if (RefPtr axObject = associatedAXObject())

why do we need a RefPtr here? most of other places use

if (auto* axObject = associatedAXObject())

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:124
> +    if (!nsRange)

if no range, should we assume the full length of the string?

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:136
> +    if (spellCheck == SpellCheck::Yes)

feel like this is a bit of a risky return early, can we structure like

if (spellCheck == NO) {
   remove
   remove
}

return result

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:141
> +    (*nsRange).location = 0;

can we make a new NSRange rather than modifying this existing one?
NSRange fullRange = NSMakeRange(0, result.length);

then we also won't need this assert

Created attachment 465206 [details]
Patch

(In reply to chris fleizach from comment #6)
> Comment on attachment 465160 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465160&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:-1584
> > -        return passwordFieldValue().length();
> 
> why do we want to remove this password field override?

It is not needed because:

String AccessibilityRenderObject::text() const
{
    if (isPasswordField())
        return passwordFieldValue();

> 
> > Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:28
> > +#import <Foundation/NSRange.h>
> 
> do we really need this import? I haver to imagine that this is brought in
> automatically

I think the "good citizen" rule is that if you use x, include x.h. This is primarily to avoid breakage in non-unified builds. I think that all builds for Mac and iOS are currently unified, but it doesn't hurt to try to follow the rule in case that changes.
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:112
> > +        if (RefPtr axObject = associatedAXObject())
> 
> why do we need a RefPtr here? most of other places use
> 
> if (auto* axObject = associatedAXObject())

There is an ongoing push to adopt some rules to improve security and one of them is that there shouldn't be raw pointers or references as local variables, except in some specific cases. The reason behind it is that it is not evident that the code that uses that pointer or reference may cause the pointed object to die, directly or most likely indirectly, then causing an use-after-free. So at least new code should be in the habit of using smart pointers, where we may have used a raw pointer before.
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:124
> > +    if (!nsRange)
> 
> if no range, should we assume the full length of the string?

No range means that the TextMarkerRange includes more than one object, and thus we cannot retrieve the corresponding AttributedString at this time. This will be addressed when we support TextMarkerRanges expanding multiple objects on the AX thread. For now if that is the case, we are falling back to dispatching to the main thread.
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:136
> > +    if (spellCheck == SpellCheck::Yes)
> 
> feel like this is a bit of a risky return early, can we structure like
> 
> if (spellCheck == NO) {
>    remove
>    remove
> }
> 
> return result
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:141
> > +    (*nsRange).location = 0;
> 
> can we make a new NSRange rather than modifying this existing one?
> NSRange fullRange = NSMakeRange(0, result.length);
> 
> then we also won't need this assert

Done, agree that is a lot more legible this way. Thanks.

Comment on attachment 465206 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:32
> +#import <Foundation/NSRange.h>

This is still unnecessary. The entire Foundation umbrella header is already pulled when compiling.

Created attachment 465217 [details]
Patch

(In reply to chris fleizach from comment #9)
> Comment on attachment 465206 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465206&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:32
> > +#import <Foundation/NSRange.h>
> 
> This is still unnecessary. The entire Foundation umbrella header is already
> pulled when compiling.

Fixed. Thanks.

Committed 260937@main (6423c272af85): <https://commits.webkit.org/260937@main>

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