[Webkit-unassigned] [Bug 145283] AX: debugging attributes for text markers

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #253714|review?                     |review+
              Flags|                            |

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

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3495
> +    VisiblePosition visiblePosition = [self visiblePositionForTextMarker:textMarker];
> +    const int FormatBufferSize = 1024;
> +    char format[FormatBufferSize];
> +    visiblePosition.formatForDebugger(format, FormatBufferSize);
> +    return (NSString *)String(format, FormatBufferSize);

I would write this differently:

    char description[1024];
    [self visiblePositionForTextMarker:textMarker](description, sizeof(description));
    return [NSString stringWithUTF8String:description];

There’s no reason for the local variables or to involve WTF::String.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3506
> +    const int FormatBufferSize = 2048;
> +    char format[FormatBufferSize];
> +    formatForDebugger(visiblePositionRange, format, FormatBufferSize);
> +    return (NSString *)String(format, FormatBufferSize);

I would write this differently:

    char description[2048];
    formatForDebugger(visiblePositionRange, description, sizeof(description));
    return [NSString stringWithUTF8String:description];

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3515
> +    node->showNode("");

This should just be:

    node->showNode();

No reason to pass "" explicitly.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3541
> +    strncpy(buffer, result.toString().utf8().data(), length - 1);

Even though the other format for debugger functions incorrectly do this, you should not do it. The good way to do it in Cocoa-specific code is:

    strlcpy(buffer, result.toString().utf8().data(), length);

As you can see on the man page for strncpy, it doesn’t do the right thing unless you write an additional line of code to NUL terminate the string when it overflows the buffer size. This also should be fixed in the other places in WebKit that do this incorrectly, but please don’t introduce a new one.

> Source/WebCore/dom/Text.cpp:234
> +        result.appendLiteral("; ");
> +        result.appendLiteral("value=\"");

Seems like those could be a single call to appendLiteral.

> Source/WebCore/dom/Text.cpp:236
> +        result.appendLiteral("\"");

This could be append('"') instead, slightly more efficient.

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