Bug 196040 - Adopt UIWKDocumentContext
Summary: Adopt UIWKDocumentContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-20 15:55 PDT by Tim Horton
Modified: 2019-03-21 19:27 PDT (History)
7 users (show)

See Also:


Attachments
Patch (59.33 KB, patch)
2019-03-20 15:59 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (59.35 KB, patch)
2019-03-20 17:23 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (59.22 KB, patch)
2019-03-20 18:37 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (59.17 KB, patch)
2019-03-20 20:23 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.79 MB, application/zip)
2019-03-20 22:23 PDT, Build Bot
no flags Details
Patch (62.44 KB, patch)
2019-03-21 01:01 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (63.63 KB, patch)
2019-03-21 02:09 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (63.59 KB, patch)
2019-03-21 02:39 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (69.13 KB, patch)
2019-03-21 13:57 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (67.53 KB, patch)
2019-03-21 16:29 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (67.93 KB, patch)
2019-03-21 17:36 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (68.14 KB, patch)
2019-03-21 19:11 PDT, Tim Horton
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2019-03-20 15:55:13 PDT
Adopt UIWKDocumentContext
Comment 1 Tim Horton 2019-03-20 15:59:31 PDT
Created attachment 365423 [details]
Patch
Comment 2 Tim Horton 2019-03-20 15:59:33 PDT
<rdar://problem/48642440>
Comment 3 Tim Horton 2019-03-20 17:23:56 PDT
Created attachment 365445 [details]
Patch
Comment 4 Ryosuke Niwa 2019-03-20 17:27:26 PDT
Comment on attachment 365423 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3300
> +    Frame& frame = corePage()->focusController().focusedOrMainFrame();

Use Ref/RefPtr.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3302
> +    FrameSelection& frameSelection = frame.selection();
> +    const VisibleSelection& selection = frameSelection.selection();

Make copies here instead. It's not safe to have a reference.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3309
> +    auto root = frameSelection.rootEditableElementOrDocumentElement();
> +    auto range = selection.toNormalizedRange();

Each one of these function calls can trigger a layout & execute arbitrary scripts.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3333
> +    RefPtr<Range> range;

What's up with this range?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3335
> +    bool backwards = (direction == DirectionBackward);

No need for parenthesis.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3357
> +    return frame.visiblePositionForPoint(pointInDocument).deepEquivalent();

Why are we creating a VisiblePosition out of deepEquivalent?
Why can't we just return frame.visiblePositionForPoint(pointInDocument) instead?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3363
> +    Frame& frame = m_page->focusController().focusedOrMainFrame();
> +    const VisibleSelection& selection = frame.selection().selection();

Again, make copies. It's not safe.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3365
> +    frame.document()->updateLayoutIgnorePendingStylesheets();

This could execute arbitrary scripts and delete a frame.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3403
> +        RefPtr<Range> selectionRange;
> +        if (!selection.isNone())
> +            selectionRange = selection.toNormalizedRange();

No need for if. toNormalizedRange() returns nullptr when it's null.
Comment 5 Tim Horton 2019-03-20 18:37:06 PDT
Created attachment 365459 [details]
Patch
Comment 6 Daniel Bates 2019-03-20 19:07:14 PDT
Comment on attachment 365459 [details]
Patch

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

Did not review for correctness. Didn't even finish reviewing. 

@other reviewers: don't let me comments stop you from reviewing

> Source/WebKit/Shared/DocumentEditingContext.h:59
> +    WTF::Optional<WebKit::TextInputContext> textInputContext;

WTF::? πŸ˜€

> Source/WebKit/Shared/DocumentEditingContext.h:63
> +    UIWKDocumentContext *toPlatformContext(WTF::OptionSet<WebKit::DocumentEditingContextRequest::Options>);

WTF::? πŸ˜€

> Source/WebKit/Shared/DocumentEditingContext.mm:37
> +static NSRange toNSRange(DocumentEditingContext::Range range)

Ok as-is. Inline?

> Source/WebKit/Shared/DocumentEditingContext.mm:44
> +    RetainPtr<UIWKDocumentContext> platformContext = adoptNS([[NSClassFromString(@"UIWKDocumentContext") alloc] init]);

Ok as-is. Auto? You're so dynamic Tim πŸ˜€

> Source/WebKit/Shared/DocumentEditingContext.mm:81
> +

Ok as-is. This function has too much whitespace for me πŸ˜›

> Source/WebKit/Shared/DocumentEditingContext.mm:82
> +    if (!decoder.decode(range.location))

Btw I ❀️ that you are decoding into the struct. Beautiful.

> Source/WebKit/Shared/DocumentEditingContext.mm:168
> +    return context;

This is πŸ¦‹. ❀️ it: struct initialization.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6291
> +

Ok as-is. Too much whitespace in this function  for meπŸ˜›

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6328
> +    _page->adjustSelectionWithDelta((int64_t)deltaRange.location, (int64_t)deltaRange.length, [capturedCompletionHandler = makeBlockPtr(completionHandler)] {

Ok as-is. Static_cast?s

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6337
> +    _page->requestDocumentEditingContext(webRequest, [capturedCompletionHandler = makeBlockPtr(completionHandler), options](WebKit::DocumentEditingContext editingContext) {

Ok as-is. Missing a space after capture args
Comment 7 Tim Horton 2019-03-20 19:21:17 PDT
Ahhhhh I can't tell which comments are serious and which are sarcastic
Comment 8 Tim Horton 2019-03-20 19:55:48 PDT
(In reply to Daniel Bates from comment #6)
> Comment on attachment 365459 [details]
> > Source/WebKit/Shared/DocumentEditingContext.mm:44
> > +    RetainPtr<UIWKDocumentContext> platformContext = adoptNS([[NSClassFromString(@"UIWKDocumentContext") alloc] init]);
> 
> Ok as-is. Auto? You're so dynamic Tim πŸ˜€

Definitely don't think auto is a good idea in this case.

> > Source/WebKit/Shared/DocumentEditingContext.mm:82
> > +    if (!decoder.decode(range.location))
> 
> Btw I ❀️ that you are decoding into the struct. Beautiful.

Cannot tell if serious.

> > Source/WebKit/Shared/DocumentEditingContext.mm:168
> > +    return context;
> 
> This is πŸ¦‹. ❀️ it: struct initialization.

I can extract nothing from this! Is there a problem?

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6328
> > +    _page->adjustSelectionWithDelta((int64_t)deltaRange.location, (int64_t)deltaRange.length, [capturedCompletionHandler = makeBlockPtr(completionHandler)] {
> 
> Ok as-is. Static_cast?s

Sure
Comment 9 Daniel Bates 2019-03-20 20:14:42 PDT
(In reply to Tim Horton from comment #7)
> Ahhhhh I can't tell which comments are serious and which are sarcastic

😠  None are sarcastic.  Can't you take a complement πŸ˜€ Seriously, I loved some parts of your patch. Everything looked ok as-is, didn't read it all though.
Comment 10 Daniel Bates 2019-03-20 20:18:36 PDT
Comment on attachment 365459 [details]
Patch

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

>>> Source/WebKit/Shared/DocumentEditingContext.mm:44
>>> +    RetainPtr<UIWKDocumentContext> platformContext = adoptNS([[NSClassFromString(@"UIWKDocumentContext") alloc] init]);
>> 
>> Ok as-is. Auto? You're so dynamic Tim πŸ˜€
> 
> Definitely don't think auto is a good idea in this case.

Ok, I'm going to bite the ⚫️. Teach me. Why not? Compiler knows the type doesn't it? Had too to allocate. Adopt returns a retainptr. What am I missing?
Comment 11 Daniel Bates 2019-03-20 20:20:57 PDT
(In reply to Daniel Bates from comment #10)
> Comment on attachment 365459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365459&action=review
> 
> >>> Source/WebKit/Shared/DocumentEditingContext.mm:44
> >>> +    RetainPtr<UIWKDocumentContext> platformContext = adoptNS([[NSClassFromString(@"UIWKDocumentContext") alloc] init]);
> >> 
> >> Ok as-is. Auto? You're so dynamic Tim πŸ˜€
> > 
> > Definitely don't think auto is a good idea in this case.
> 
> Ok, I'm going to bite the ⚫️. Teach me. Why not? Compiler knows the type
> doesn't it? Had too to allocate. Adopt returns a retainptr. What am I
> missing?

No, don't tell me adopt returned a retainptr<id> or <instancetype>? Dang you objective-c runtime πŸ˜€
Comment 12 Tim Horton 2019-03-20 20:21:49 PDT
(In reply to Daniel Bates from comment #10)
> Comment on attachment 365459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365459&action=review
> 
> >>> Source/WebKit/Shared/DocumentEditingContext.mm:44
> >>> +    RetainPtr<UIWKDocumentContext> platformContext = adoptNS([[NSClassFromString(@"UIWKDocumentContext") alloc] init]);
> >> 
> >> Ok as-is. Auto? You're so dynamic Tim πŸ˜€
> > 
> > Definitely don't think auto is a good idea in this case.
> 
> Ok, I'm going to bite the ⚫️. Teach me. Why not? Compiler knows the type
> doesn't it? Had too to allocate. Adopt returns a retainptr. What am I
> missing?

By definition the compiler does *not* know the type in the NSClassFromString case, and allocation in ObjC is dynamic. NSClassFromString just returns Class, all type information is gone.
Comment 13 Daniel Bates 2019-03-20 20:22:37 PDT
(In reply to Tim Horton from comment #8)
> (In reply to Daniel Bates from comment #6)
> > Comment on attachment 365459 [details]
> > > Source/WebKit/Shared/DocumentEditingContext.mm:44
> > > +    RetainPtr<UIWKDocumentContext> platformContext = adoptNS([[NSClassFromString(@"UIWKDocumentContext") alloc] init]);
> > 
> > Ok as-is. Auto? You're so dynamic Tim πŸ˜€
> 
> Definitely don't think auto is a good idea in this case.
> 
> > > Source/WebKit/Shared/DocumentEditingContext.mm:82
> > > +    if (!decoder.decode(range.location))
> > 
> > Btw I ❀️ that you are decoding into the struct. Beautiful.
> 
> Cannot tell if serious.
> 

Serious.

> > > Source/WebKit/Shared/DocumentEditingContext.mm:168
> > > +    return context;
> > 
> > This is πŸ¦‹. ❀️ it: struct initialization.
> 
> I can extract nothing from this! Is there a problem?


No problem here. Keep doing this please. You're one of the few that do except for me.

> 
> > > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6328
> > > +    _page->adjustSelectionWithDelta((int64_t)deltaRange.location, (int64_t)deltaRange.length, [capturedCompletionHandler = makeBlockPtr(completionHandler)] {
> > 
> > Ok as-is. Static_cast?s
> 
> Sure
Comment 14 Tim Horton 2019-03-20 20:23:32 PDT
Created attachment 365469 [details]
Patch
Comment 15 Daniel Bates 2019-03-20 20:23:55 PDT
(In reply to Tim Horton from comment #12)
> (In reply to Daniel Bates from comment #10)
> > Comment on attachment 365459 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=365459&action=review
> > 
> > >>> Source/WebKit/Shared/DocumentEditingContext.mm:44
> > >>> +    RetainPtr<UIWKDocumentContext> platformContext = adoptNS([[NSClassFromString(@"UIWKDocumentContext") alloc] init]);
> > >> 
> > >> Ok as-is. Auto? You're so dynamic Tim πŸ˜€
> > > 
> > > Definitely don't think auto is a good idea in this case.
> > 
> > Ok, I'm going to bite the ⚫️. Teach me. Why not? Compiler knows the type
> > doesn't it? Had too to allocate. Adopt returns a retainptr. What am I
> > missing?
> 
> By definition the compiler does *not* know the type in the NSClassFromString
> case, and allocation in ObjC is dynamic. NSClassFromString just returns
> Class, all type information is gone.

Yep, that's what I surmised in comment 11.
Comment 16 Tim Horton 2019-03-20 20:25:46 PDT
(In reply to Daniel Bates from comment #15)
> (In reply to Tim Horton from comment #12)
> > (In reply to Daniel Bates from comment #10)
> > > Comment on attachment 365459 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=365459&action=review
> > > 
> > > >>> Source/WebKit/Shared/DocumentEditingContext.mm:44
> > > >>> +    RetainPtr<UIWKDocumentContext> platformContext = adoptNS([[NSClassFromString(@"UIWKDocumentContext") alloc] init]);
> > > >> 
> > > >> Ok as-is. Auto? You're so dynamic Tim πŸ˜€
> > > > 
> > > > Definitely don't think auto is a good idea in this case.
> > > 
> > > Ok, I'm going to bite the ⚫️. Teach me. Why not? Compiler knows the type
> > > doesn't it? Had too to allocate. Adopt returns a retainptr. What am I
> > > missing?
> > 
> > By definition the compiler does *not* know the type in the NSClassFromString
> > case, and allocation in ObjC is dynamic. NSClassFromString just returns
> > Class, all type information is gone.
> 
> Yep, that's what I surmised in comment 11.

Indeed, we collided as if in mid-air.
Comment 17 Build Bot 2019-03-20 22:23:57 PDT
Comment on attachment 365469 [details]
Patch

Attachment 365469 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11592252

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Comment 18 Build Bot 2019-03-20 22:23:59 PDT
Created attachment 365486 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 19 Tim Horton 2019-03-21 01:01:30 PDT
Created attachment 365514 [details]
Patch
Comment 20 Tim Horton 2019-03-21 02:09:53 PDT
Created attachment 365521 [details]
Patch
Comment 21 Tim Horton 2019-03-21 02:39:24 PDT
Created attachment 365524 [details]
Patch
Comment 22 Tim Horton 2019-03-21 13:57:20 PDT
Created attachment 365612 [details]
Patch
Comment 23 Ryosuke Niwa 2019-03-21 15:03:40 PDT
Comment on attachment 365612 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:436
> -		142B97CA13138943008BEF4B /* TextControlInnerElements.h in Headers */ = {isa = PBXBuildFile; fileRef = 142B97C813138943008BEF4B /* TextControlInnerElements.h */; };
> +		142B97CA13138943008BEF4B /* TextControlInnerElements.h in Headers */ = {isa = PBXBuildFile; fileRef = 142B97C813138943008BEF4B /* TextControlInnerElements.h */; settings = {ATTRIBUTES = (Private, ); }; };

We shouldn't be using this header in WebKit. It's a layering violation.

> Source/WebKit/Shared/DocumentEditingContext.mm:196
> +    Optional<Optional<WebKit::TextInputContext>> optionalTextInputContext;

Wow, it's crazy that we have to do this.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6288
> +static inline OptionSet<WebKit::DocumentEditingContextRequest::Options> toWebDocumentRequestOptions(UIWKDocumentRequestFlags flags)

Do we really need to qualify that we're in WebKit namespace here??
And ditto elsewhere in the file.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6323
> +    // These casts are legitimate; UIKit is putting signed values into NSRange.

It appears to me that we have to deal with negative values instead of casting them to be non-negative.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3401
> +        if (is<HTMLTextFormControlElement>(element))
> +            element = downcast<HTMLTextFormControlElement>(*element).innerTextElement().get();
> +        if (!element) {
> +            completionHandler({ });
> +            return;
> +        }
> +
> +        rangeOfInterestStart = VisiblePosition(firstPositionInOrBeforeNode(element.get()));
> +        rangeOfInterestEnd = VisiblePosition(lastPositionInOrAfterNode(element.get()));

We really don't want any code outside of HTMLInputElement to be reaching out to innerTextElement like this
because it's really an implementation detail that could change in the future.
Call visiblePositionForIndex on HTMLTextFormControlElement instead.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3418
> +    auto makeString = [&](VisiblePosition& start, VisiblePosition& end) -> NSAttributedString * {

Why not declare this right above where it's used?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3428
> +    // The subset of the selection that is inside the range of interest.
> +    VisiblePosition relevantSelectionStart;

Instead of adding a comment like this, let's just call them something like:
startOfRangeOfInterestInSelection
endOfRangeOfInterestInSelection
Comment 24 Tim Horton 2019-03-21 16:29:21 PDT
Created attachment 365642 [details]
Patch
Comment 25 Tim Horton 2019-03-21 16:29:32 PDT
(In reply to Ryosuke Niwa from comment #23)
> Comment on attachment 365612 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365612&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:436
> > -		142B97CA13138943008BEF4B /* TextControlInnerElements.h in Headers */ = {isa = PBXBuildFile; fileRef = 142B97C813138943008BEF4B /* TextControlInnerElements.h */; };
> > +		142B97CA13138943008BEF4B /* TextControlInnerElements.h in Headers */ = {isa = PBXBuildFile; fileRef = 142B97C813138943008BEF4B /* TextControlInnerElements.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> We shouldn't be using this header in WebKit. It's a layering violation.

OK.

> > Source/WebKit/Shared/DocumentEditingContext.mm:196
> > +    Optional<Optional<WebKit::TextInputContext>> optionalTextInputContext;
> 
> Wow, it's crazy that we have to do this.

Agreed.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6288
> > +static inline OptionSet<WebKit::DocumentEditingContextRequest::Options> toWebDocumentRequestOptions(UIWKDocumentRequestFlags flags)
> 
> Do we really need to qualify that we're in WebKit namespace here??
> And ditto elsewhere in the file.

We do.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6323
> > +    // These casts are legitimate; UIKit is putting signed values into NSRange.
> 
> It appears to me that we have to deal with negative values instead of
> casting them to be non-negative.

No, that's backwards. We're casting from unsigned to signed, to reveal the negatives that were smashed in. It's crazy but works.

> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3401
> > +        if (is<HTMLTextFormControlElement>(element))
> > +            element = downcast<HTMLTextFormControlElement>(*element).innerTextElement().get();
> > +        if (!element) {
> > +            completionHandler({ });
> > +            return;
> > +        }
> > +
> > +        rangeOfInterestStart = VisiblePosition(firstPositionInOrBeforeNode(element.get()));
> > +        rangeOfInterestEnd = VisiblePosition(lastPositionInOrAfterNode(element.get()));
> 
> We really don't want any code outside of HTMLInputElement to be reaching out
> to innerTextElement like this
> because it's really an implementation detail that could change in the future.
> Call visiblePositionForIndex on HTMLTextFormControlElement instead.

Fine, easy fix.

> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3418
> > +    auto makeString = [&](VisiblePosition& start, VisiblePosition& end) -> NSAttributedString * {
> 
> Why not declare this right above where it's used?

Sure

> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3428
> > +    // The subset of the selection that is inside the range of interest.
> > +    VisiblePosition relevantSelectionStart;
> 
> Instead of adding a comment like this, let's just call them something like:
> startOfRangeOfInterestInSelection
> endOfRangeOfInterestInSelection

OK!
Comment 26 Ryosuke Niwa 2019-03-21 17:05:54 PDT
Comment on attachment 365642 [details]
Patch

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

> Source/WebKit/Shared/DocumentEditingContext.h:55
> +    WebCore::TextGranularity surroundingGranularity;
> +    int64_t granularityCount;

Initialize these just in case?

> Source/WebKit/Shared/DocumentEditingContext.h:73
> +        uint64_t location;
> +        uint64_t length;

Intialize?

> Source/WebKit/Shared/DocumentEditingContext.h:78
> +    struct TextRect {

This is a confusing struct name.
Can we rename it to something like TextRangeWithRect or TextRectWithRange?
Or maybe TextRectRange??

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6308
> +    WebKit::DocumentEditingContextRequest webRequest;

I'd usually prefer writing this as webRequest = { ~ }

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6323
> +    // These casts are legitimate; UIKit is putting signed values into NSRange.

Oh, I see. I got confused by this comment. How about this?
// UIKit is putting casted signed integers into NSRange. Re-caste them back to reveal any negative values.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3311
> +void WebPage::adjustSelectionWithDelta(int64_t locationDelta, int64_t lengthDelta, CompletionHandler<void()>&& completionHandler)

Why not just updateSelectionWithDelta?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3321
> +    auto root = frame->selection().rootEditableElementOrDocumentElement();
> +    auto range = selection.toNormalizedRange();

Maybe we should also bail out when root or range is null?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3327
> +    CheckedInt64 newSelectionLocation(selectionLocation);

Nit: Use { ~ }?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3328
> +    CheckedInt64 newSelectionLength(selectionLength);

Ditto.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3343
> +static VisiblePosition visiblePositionAdjacentToVisiblePosition(Frame& frame, VisiblePosition& position, TextGranularity granularity, uint64_t granularityCount, SelectionDirection direction)

This function name is quite confusing. "Adjacent" doesn't really tell us what this function does.
How about moveByGranularityRespectingWordBoundary?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3354
> +        nextPosition = backwards ? startOfWord(nextPosition) : endOfWord(nextPosition);

How is that we're always calling startOfWord/endOfWord without regards to granularity?
It seems that we'd end up moving twice as much as we've been asked in the case of word boundary.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3360
> +

Why don't we do it here?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3380
> +    frame->document()->updateLayoutIgnorePendingStylesheets();

Note that this could totally blow away frame & selection.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3401
> +            rangeOfInterestStart = VisiblePosition(firstPositionInOrBeforeNode(element.get()));

We should be able to just do: rangeOfInterestStart = firstPositionInOrBeforeNode(element.get())

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3415
> +    if (rangeOfInterestStart.isNull() || rangeOfInterestEnd.isNull()) {

We probably want to check isOrphan() (detached from document) as well.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3435
> +        RefPtr<Node> rootNode = rangeOfInterest->commonAncestorContainer();
> +        RefPtr<ContainerNode> rootContainerNode = rootNode->isContainerNode() ? downcast<ContainerNode>(rootNode.get()) : rootNode->parentNode();

I think we should add a nullptr check for rootNode to be cautious.
Comment 27 Tim Horton 2019-03-21 17:36:40 PDT
Created attachment 365654 [details]
Patch
Comment 28 Tim Horton 2019-03-21 19:11:18 PDT
Created attachment 365666 [details]
Patch
Comment 29 Tim Horton 2019-03-21 19:27:08 PDT
https://trac.webkit.org/changeset/243354/webkit