Bug 196040 – Adopt UIWKDocumentContext

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Adopt UIWKDocumentContext

[Tim Horton]

Created attachment 365423 [details]
Patch

[Tim Horton]

<rdar://problem/48642440>

[Tim Horton]

Created attachment 365445 [details]
Patch

[Ryosuke Niwa]

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.

[Tim Horton]

Created attachment 365459 [details]
Patch

[Daniel Bates]

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

[Tim Horton]

Ahhhhh I can't tell which comments are serious and which are sarcastic

[Tim Horton]

(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

[Daniel Bates]

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

[Daniel Bates]

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?

[Daniel Bates]

(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 😀

[Tim Horton]

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

[Daniel Bates]

(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

[Tim Horton]

Created attachment 365469 [details]
Patch

[Daniel Bates]

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

[Tim Horton]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tim Horton]

Created attachment 365514 [details]
Patch

[Tim Horton]

Created attachment 365521 [details]
Patch

[Tim Horton]

Created attachment 365524 [details]
Patch

[Tim Horton]

Created attachment 365612 [details]
Patch

[Ryosuke Niwa]

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

[Tim Horton]

Created attachment 365642 [details]
Patch

[Tim Horton]

(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!

[Ryosuke Niwa]

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.

[Tim Horton]

Created attachment 365654 [details]
Patch

[Tim Horton]

Created attachment 365666 [details]
Patch

[Tim Horton]

https://trac.webkit.org/changeset/243354/webkit