[Webkit-unassigned] [Bug 141862] AX: Expose caret browsing preference to accessibility API

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #247196|review?                     |review-
              Flags|                            |

--- Comment #17 from Darin Adler <darin at apple.com> ---
Comment on attachment 247196
  --> https://bugs.webkit.org/attachment.cgi?id=247196
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2609
> +bool AccessibilityObject::caretBrowsingEnabled() const
> +{
> +    if (FrameView *frameView = documentFrameView())
> +        return frameView->frame().mainFrame().settings().caretBrowsingEnabled();
> +    return false;
> +}

This should not involve the FrameView, nor need it call mainFrame. Instead it should just be:

    Frame* frame = this->frame();
    return frame && frame->settings().caretBrowsingEnabled();

> Source/WebCore/accessibility/AccessibilityObject.cpp:2614
> +    if (FrameView *frameView = documentFrameView())
> +        return frameView->frame().mainFrame().settings().setCaretBrowsingEnabled(on);

Same thing here:

    Frame* frame = this->frame();
    if (!frame)
        return;
    frame->settings().setCaretBrowsingEnabled(on);

Not normal WebKit style to use “return” with an expression of type void outside a template context.

> Source/WebCore/accessibility/AccessibilityObject.h:979
> +    bool caretBrowsingEnabled() const;
> +    void setCaretBrowsingEnabled(bool);

It’s kind of sloppy to put these operations on all accessibility objects when the intention is to only expose them for web area objects.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3359
> +// Used to set attributes synchronously on accessibility elements within tests.
> +// For use with DumpRenderTree only.
> +- (void)_accessibilitySetTestValue:(id)value forAttribute:(NSString*)attributeName
> +{
> +    [self _accessibilitySetValue:value forAttribute:attributeName];
>  }

How is this forwarding method helpful? Can’t the client code call _accessibilitySetValue:forAttribute: directly instead?

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:1414
> +{
> +    
> +}

Blank lines in empty function bodies like this are not WebKit coding style.

> Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm:453
> +{
> +    
> +}

Blank lines in empty function bodies like this are not WebKit coding style.

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:96
> +void setBoolAttributeValue(JSStringRef attribute, bool value) { }

This needs to go down below in the !COCOA section.

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:216
> +PassRefPtr<AccessibilityTextMarkerRange> AccessibilityUIElement::selectedTextMarkerRange(void) { return nullptr; }

I don’t see a version of this for PLATFORM(IOS).

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:226
> +void AccessibilityUIElement::setBoolAttributeValue(JSStringRef attribute, bool value)

We normally omit any unused arguments’ names.

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:229
> +{
> +    
> +}

Blank lines in empty function bodies like this are not WebKit coding style.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:82
> +// Set attribute values synchronously for tests

WebKit comment style is to use a period at the end of a sentence style comment like this. Also not sure this comment is helpful, since it’s repeated where the method is defined.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150225/b7b9b4a9/attachment-0002.html>