Bug 210885 - [iOS] Implement UIKit SPI _isHorizontalWritingMode
Summary: [iOS] Implement UIKit SPI _isHorizontalWritingMode
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 13
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2020-04-22 15:44 PDT by Daniel Bates
Modified: 2020-04-28 16:03 PDT (History)
4 users (show)

See Also:


Attachments
Patch and tests (30.05 KB, patch)
2020-04-22 16:26 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and tests (30.25 KB, patch)
2020-04-22 16:35 PDT, Daniel Bates
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2020-04-22 15:44:06 PDT
Implement UIKit SPI _isHorizontalWritingMode added in <rdar://problem/57865890>.
Comment 1 Radar WebKit Bug Importer 2020-04-22 15:44:46 PDT
<rdar://problem/62215704>
Comment 2 Daniel Bates 2020-04-22 16:26:13 PDT
Created attachment 397284 [details]
Patch and tests
Comment 3 Daniel Bates 2020-04-22 16:35:34 PDT
Created attachment 397286 [details]
Patch and tests
Comment 4 Darin Adler 2020-04-24 11:24:16 PDT
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?
Comment 5 Daniel Bates 2020-04-27 08:50:47 PDT
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.
Comment 6 Daniel Bates 2020-04-28 16:03:22 PDT
After thinking about this some more, I decided to implement the -markedTextRange API instead fo adding this SPI. See bug #211148.