<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: debugging attributes for text markers"
href="https://bugs.webkit.org/show_bug.cgi?id=145283">bug 145283</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #253714 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: debugging attributes for text markers"
href="https://bugs.webkit.org/show_bug.cgi?id=145283#c9">Comment # 9</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: debugging attributes for text markers"
href="https://bugs.webkit.org/show_bug.cgi?id=145283">bug 145283</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=253714&action=diff" name="attach_253714" title="patch">attachment 253714</a> <a href="attachment.cgi?id=253714&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=253714&action=review">https://bugs.webkit.org/attachment.cgi?id=253714&action=review</a>
<span class="quote">> 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);</span >
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.
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3506
> + const int FormatBufferSize = 2048;
> + char format[FormatBufferSize];
> + formatForDebugger(visiblePositionRange, format, FormatBufferSize);
> + return (NSString *)String(format, FormatBufferSize);</span >
I would write this differently:
char description[2048];
formatForDebugger(visiblePositionRange, description, sizeof(description));
return [NSString stringWithUTF8String:description];
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3515
> + node->showNode("");</span >
This should just be:
node->showNode();
No reason to pass "" explicitly.
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3541
> + strncpy(buffer, result.toString().utf8().data(), length - 1);</span >
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.
<span class="quote">> Source/WebCore/dom/Text.cpp:234
> + result.appendLiteral("; ");
> + result.appendLiteral("value=\"");</span >
Seems like those could be a single call to appendLiteral.
<span class="quote">> Source/WebCore/dom/Text.cpp:236
> + result.appendLiteral("\"");</span >
This could be append('"') instead, slightly more efficient.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>