[Webkit-unassigned] [Bug 153085] AX: Add a boundary value to AXTextStateChangeType

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jan 30 13:36:36 PST 2016


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
 Attachment #269895|review?                     |review+
              Flags|                            |

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

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

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.

> Source/WebCore/editing/FrameSelection.cpp:763
> +VisiblePosition FrameSelection::modifyMovingRight(TextGranularity granularity, bool * reachedBoundary)

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.

> Source/WebCore/editing/FrameSelection.cpp:796
> +    case LineBoundary: {
> +        pos = rightBoundaryOfLine(startForPlatform(), directionOfEnclosingBlock(), reachedBoundary);
>          break;
> +    }

No braces in a case like this in WebKit coding style.

> Source/WebCore/editing/FrameSelection.cpp:804
> +VisiblePosition FrameSelection::modifyMovingForward(TextGranularity granularity, bool * reachedBoundary)


> Source/WebCore/editing/FrameSelection.cpp:1014
> +    case LineBoundary: {
> +        pos = leftBoundaryOfLine(startForPlatform(), directionOfEnclosingBlock(), reachedBoundary);
>          break;
> +    }

No braces in a case like this in WebKit coding style.

> Source/WebCore/editing/FrameSelection.h:295
> +    VisiblePosition modifyMovingRight(TextGranularity, bool * = nullptr);
> +    VisiblePosition modifyMovingForward(TextGranularity, bool * = nullptr);

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.

> Source/WebCore/editing/VisiblePosition.cpp:67
>  {


> Source/WebCore/editing/VisiblePosition.cpp:78
> +VisiblePosition VisiblePosition::previous(EditingBoundaryCrossingRule rule, bool * reachedBoundary) const


> Source/WebCore/editing/VisiblePosition.cpp:259
> +VisiblePosition VisiblePosition::left(bool stayInEditableContent, bool * reachedBoundary) const


I’ll stop mentioning this, since it happens many more times.

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/20160130/7c144557/attachment-0001.html>

More information about the webkit-unassigned mailing list