Bug 145283

Summary: AX: debugging attributes for text markers
Product: WebKit Reporter: Doug Russell <d_russell>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, cgarcia, clopez, commit-queue, dmazzoni, esprehn+autocc, hyungwook.lee, jcraig, jdiggs, kangil.han, mario, mrobinson, ossy, samuel_white, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=145608
Bug Depends on:    
Bug Blocks: 145596    
Attachments:
Description Flags
patch
cfleizach: review-
patch
cfleizach: review-
patch
darin: review+
patch none

Description Doug Russell 2015-05-21 14:21:13 PDT
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.
Comment 1 Radar WebKit Bug Importer 2015-05-21 14:22:48 PDT
<rdar://problem/21064014>
Comment 2 Doug Russell 2015-05-21 14:33:41 PDT
Created attachment 253542 [details]
patch
Comment 3 chris fleizach 2015-05-21 17:48:19 PDT
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
Comment 4 chris fleizach 2015-05-21 17:48:29 PDT
also change log needed
Comment 5 Doug Russell 2015-05-25 13:28:23 PDT
Created attachment 253692 [details]
patch
Comment 6 chris fleizach 2015-05-25 14:56:58 PDT
Comment on attachment 253692 [details]
patch

Doesn't seem like any review comments were addressed
Comment 7 Doug Russell 2015-05-26 11:55:25 PDT
(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.
Comment 8 Doug Russell 2015-05-26 11:56:12 PDT
Created attachment 253714 [details]
patch
Comment 9 Darin Adler 2015-05-27 12:14:36 PDT
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.
Comment 10 Doug Russell 2015-06-02 17:39:12 PDT
Created attachment 254126 [details]
patch

updates from review
Comment 11 WebKit Commit Bot 2015-06-02 17:42:16 PDT
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
Comment 12 WebKit Commit Bot 2015-06-02 18:30:08 PDT
Comment on attachment 254126 [details]
patch

Clearing flags on attachment: 254126

Committed r185137: <http://trac.webkit.org/changeset/185137>
Comment 13 WebKit Commit Bot 2015-06-02 18:30:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Csaba Osztrogonác 2015-06-02 22:03:57 PDT
(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.
Comment 15 Hyungwook Lee 2015-06-02 23:11:22 PDT
(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.