[webkit-reviews] review granted: [Bug 210885] [iOS] Implement UIKit SPI _isHorizontalWritingMode : [Attachment 397286] Patch and tests

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 210885: [iOS] Implement UIKit SPI _isHorizontalWritingMode
https://bugs.webkit.org/show_bug.cgi?id=210885

Attachment 397286: Patch and tests

https://bugs.webkit.org/attachment.cgi?id=397286&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 397286
  --> https://bugs.webkit.org/attachment.cgi?id=397286
Patch and tests

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

> Source/WebCore/editing/FrameSelection.cpp:2
> + * Copyright (C) 2004, 2008-2010, 2014-2015, 2020 Apple Inc. All rights
reserved.

Just 2004-2020 is fine.

> Source/WebCore/editing/FrameSelection.cpp:1650
> +    if (m_selection.isNone())
> +	   return true;

This check is not necessary.

> Source/WebCore/editing/FrameSelection.h:187
> +    WEBCORE_EXPORT bool isHorizontal() const;

I think this name is too short. I would not say that "a selection is
horizontal". I think a good name could be "startsInHorizontalWritingMode".

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:439
> + at property (nonatomic, readonly, getter=_isHorizontalWritingMode) BOOL
_horizontalWritingMode;

Not thrilled with the naming here. Saying that "content view is horizontal
writing mode" doesn’t make sense. Maybe "is in horizontal writing mode" would
be better, but even that doesn’t seem quite right.

But I guess this is UIKit SPI, not something we are naming?