Bug 210885 – [iOS] Implement UIKit SPI _isHorizontalWritingMode

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Daniel Bates]

Implement UIKit SPI _isHorizontalWritingMode added in <rdar://problem/57865890>.

[Radar WebKit Bug Importer]

<rdar://problem/62215704>

[Daniel Bates]

Created attachment 397284 [details]
Patch and tests

[Daniel Bates]

Created attachment 397286 [details]
Patch and tests

[Darin Adler]

Comment on attachment 397286 [details]
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
> +@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?

[Daniel Bates]

Comment on attachment 397286 [details]
Patch and tests

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

Thanks for the 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.

Cool

>> Source/WebCore/editing/FrameSelection.cpp:1650
>> +        return true;
> 
> This check is not necessary.

OK. I added as a fast path (without measuring) but it's cool to remove.

>> 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".

I like your suggestion! I'm renaming...

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:439
>> +@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?

I'm adding the SPI. So, we can name it! I was having a hard time naming this thing throughout the patch....I'll go with isInHorizontalWritingMode for now.

[Daniel Bates]

After thinking about this some more, I decided to implement the -markedTextRange API instead fo adding this SPI. See bug #211148.