| Summary: | [iOS] Implement UIKit SPI _isHorizontalWritingMode | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
| Component: | WebKit Misc. | Assignee: | Daniel Bates <dbates> | ||||||
| Status: | RESOLVED WONTFIX | ||||||||
| Severity: | Normal | CC: | darin, ews-watchlist, mifenton, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar, PlatformOnly | ||||||
| Version: | WebKit Local Build | ||||||||
| Hardware: | iPhone / iPad | ||||||||
| OS: | iOS 13 | ||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=210875 https://bugs.webkit.org/show_bug.cgi?id=210877 https://bugs.webkit.org/show_bug.cgi?id=211148 |
||||||||
| Attachments: |
|
||||||||
|
Description
Daniel Bates
2020-04-22 15:44:06 PDT
Created attachment 397284 [details]
Patch and tests
Created attachment 397286 [details]
Patch and tests
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 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. After thinking about this some more, I decided to implement the -markedTextRange API instead fo adding this SPI. See bug #211148. |