Bug 145283 – AX: debugging attributes for text markers

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Doug Russell]

AXTextMarkers are opaque data types which can make debugging navigation and selection difficult. New debug build only accessibility attributes to return debug descriptions or to print debug info would be very helpful.

[Radar WebKit Bug Importer]

<rdar://problem/21064014>

[Doug Russell]

Created attachment 253542 [details]
patch

[chris fleizach]

Comment on attachment 253542 [details]
patch

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

thanks

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3796
> +    if ([attribute isEqualToString:@"AXTextMarkerDebugDescription"]) {

can you put these into a separate method completely and can you put the logic in an ObjC method so that these can be called from within the debugger itself

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3797
> +        VisiblePosition visiblePosition = [self visiblePositionForTextMarker:(textMarker)];

i don't think the parens are necessary here
(textMarker)

> Source/WebCore/dom/Text.cpp:231
> +        result.appendLiteral("count=");

this looks like it should be length instead of count

[chris fleizach]

also change log needed

[Doug Russell]

Created attachment 253692 [details]
patch

[chris fleizach]

Comment on attachment 253692 [details]
patch

Doesn't seem like any review comments were addressed

[Doug Russell]

(In reply to comment #6)
> Comment on attachment 253692 [details]
> patch
> 
> Doesn't seem like any review comments were addressed

Sorry, uploaded the old patch. Attaching the right one now.

[Doug Russell]

Created attachment 253714 [details]
patch

[Darin Adler]

Comment on attachment 253714 [details]
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.

[Doug Russell]

Created attachment 254126 [details]
patch

updates from review

[WebKit Commit Bot]

Comment on attachment 254126 [details]
patch

Rejecting attachment 254126 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 254126, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/5969369683197952

[WebKit Commit Bot]

Comment on attachment 254126 [details]
patch

Clearing flags on attachment: 254126

Committed r185137: <http://trac.webkit.org/changeset/185137>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #12)
> Comment on attachment 254126 [details]
> patch
> 
> Clearing flags on attachment: 254126
> 
> Committed r185137: <http://trac.webkit.org/changeset/185137>

It broke the debug GTK build:
../../Source/WebCore/dom/Text.cpp:238:60: error: ‘strlcpy’ was not declared in this scope
     strlcpy(buffer, result.toString().utf8().data(), length);

$man strncpy
"strlcpy() is not present in glibc and is not standardized by POSIX,
but is available on Linux via the libbsd library."

I just reported this issue, but don't have any time
to try to fix it myself. cc-ing GTK maintainters.

[Hyungwook Lee]

(In reply to comment #14)
> (In reply to comment #12)
> > Comment on attachment 254126 [details]
> > patch
> > 
> > Clearing flags on attachment: 254126
> > 
> > Committed r185137: <http://trac.webkit.org/changeset/185137>
> 
> It broke the debug GTK build:
> ../../Source/WebCore/dom/Text.cpp:238:60: error: ‘strlcpy’ was not declared
> in this scope
>      strlcpy(buffer, result.toString().utf8().data(), length);
> 
> $man strncpy
> "strlcpy() is not present in glibc and is not standardized by POSIX,
> but is available on Linux via the libbsd library."
> 
> I just reported this issue, but don't have any time
> to try to fix it myself. cc-ing GTK maintainters.

EFL port has same build break too. I upload a fix to Bug 145596.