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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=153085

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
Patch

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)

bool*

> 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
>  {

bool*

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

bool*

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

bool*

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>