<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: Add a boundary value to AXTextStateChangeType"
href="https://bugs.webkit.org/show_bug.cgi?id=153085">bug 153085</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 #269895 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: Add a boundary value to AXTextStateChangeType"
href="https://bugs.webkit.org/show_bug.cgi?id=153085#c47">Comment # 47</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: Add a boundary value to AXTextStateChangeType"
href="https://bugs.webkit.org/show_bug.cgi?id=153085">bug 153085</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=269895&action=diff" name="attach_269895" title="Patch">attachment 269895</a> <a href="attachment.cgi?id=269895&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=269895&action=review">https://bugs.webkit.org/attachment.cgi?id=269895&action=review</a>
Looks good. Some style issues.
When we use pointers for out arguments like this, normally we set the value of out arguments in all cases, rather than relying on them being initialized on entry. That means there are more places here we would need to set *reachedBoundary to false.
<span class="quote">> Source/WebCore/editing/FrameSelection.cpp:763
> +VisiblePosition FrameSelection::modifyMovingRight(TextGranularity granularity, bool * reachedBoundary)</span >
WebKit coding style would use bool* rather than bool * with a space.
I’m surprised you chose the out argument over the "VisiblePosition plus a bool" struct option. Another possibility would be to use Optional<VisiblePosition> and have “no position at all” indicate a boundary. That’s distinct from a null VisiblePosition. But probably too subtle and confusing.
<span class="quote">> Source/WebCore/editing/FrameSelection.cpp:796
> + case LineBoundary: {
> + pos = rightBoundaryOfLine(startForPlatform(), directionOfEnclosingBlock(), reachedBoundary);
> break;
> + }</span >
No braces in a case like this in WebKit coding style.
<span class="quote">> Source/WebCore/editing/FrameSelection.cpp:804
> +VisiblePosition FrameSelection::modifyMovingForward(TextGranularity granularity, bool * reachedBoundary)</span >
bool*
<span class="quote">> Source/WebCore/editing/FrameSelection.cpp:1014
> + case LineBoundary: {
> + pos = leftBoundaryOfLine(startForPlatform(), directionOfEnclosingBlock(), reachedBoundary);
> break;
> + }</span >
No braces in a case like this in WebKit coding style.
<span class="quote">> Source/WebCore/editing/FrameSelection.h:295
> + VisiblePosition modifyMovingRight(TextGranularity, bool * = nullptr);
> + VisiblePosition modifyMovingForward(TextGranularity, bool * = nullptr);</span >
Formatting should be bool* and when the argument meaning is not clear from the type alone, we require an argument name, so should be:
bool* reachedBoundary = nullptr
I’ll mention this only once, but the issue recurs many other times below.
<span class="quote">> Source/WebCore/editing/VisiblePosition.cpp:67
> {</span >
bool*
<span class="quote">> Source/WebCore/editing/VisiblePosition.cpp:78
> +VisiblePosition VisiblePosition::previous(EditingBoundaryCrossingRule rule, bool * reachedBoundary) const</span >
bool*
<span class="quote">> Source/WebCore/editing/VisiblePosition.cpp:259
> +VisiblePosition VisiblePosition::left(bool stayInEditableContent, bool * reachedBoundary) const</span >
bool*
I’ll stop mentioning this, since it happens many more times.</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>