Summary: | Adopt UIWKDocumentContext | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Tim Horton <thorton> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | bdakin, dbates, ews-watchlist, megan_gardner, rniwa, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Tim Horton
2019-03-20 15:55:13 PDT
Created attachment 365423 [details]
Patch
Created attachment 365445 [details]
Patch
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. Created attachment 365459 [details]
Patch
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 Ahhhhh I can't tell which comments are serious and which are sarcastic (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 (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 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? (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 π (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. (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 Created attachment 365469 [details]
Patch
(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. (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 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 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
Created attachment 365514 [details]
Patch
Created attachment 365521 [details]
Patch
Created attachment 365524 [details]
Patch
Created attachment 365612 [details]
Patch
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 Created attachment 365642 [details]
Patch
(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 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. Created attachment 365654 [details]
Patch
Created attachment 365666 [details]
Patch
|