We should add support to display the share button in the callout menu. rdar://problem/19772892
Created attachment 251378 [details] Patch
Attachment 251378 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1310: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1314: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Fixed the style.
Comment on attachment 251378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251378&action=review > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1299 > + _page->getSelectionOrContentsAsString([self](const String& string, CallbackBase::Error error) { Since this uses lambda syntax, not Objective-C block syntax, we need to explicitly retain/release self, otherwise we could access the view after it has been destroyed. May also need some checks if the thing is still up and frontmost and everything when the callback comes back. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1307 > + if (_textSelectionAssistant) { Should write: id assistant = _textSelectionAssistant ? : _webSelectionAssistant; if ([assistant respondsToSelector:@selector(showShareSheetFor:fromRect:)]) [assistant showShareSheetFor:string fromRect:presentationRect]; Nicer than repeating all the code twice, and no need for an explicit null check either if you write it this way. Might need some typecasts to id on that first line. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1310 > + if ([_textSelectionAssistant respondsToSelector:@selector(showShareSheetFor:fromRect:)]) { > + [(id)_textSelectionAssistant showShareSheetFor:string fromRect:presentationRect]; > + } WebKit style says no braces for single line if statement. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1314 > + if ([_webSelectionAssistant respondsToSelector:@selector(showShareSheetFor:fromRect:)]) { > + [(id)_webSelectionAssistant showShareSheetFor:string fromRect:presentationRect]; > + } WebKit style says no braces for single line if statement. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1454 > + if (!textLength || textLength > 200) Where does this constant 200 come from? This should be a named constant, probably at the top of the file, along with a comment explaining where the number came from.
Comment on attachment 251378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251378&action=review >> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1307 >> + if (_textSelectionAssistant) { > > Should write: > > id assistant = _textSelectionAssistant ? : _webSelectionAssistant; > if ([assistant respondsToSelector:@selector(showShareSheetFor:fromRect:)]) > [assistant showShareSheetFor:string fromRect:presentationRect]; > > Nicer than repeating all the code twice, and no need for an explicit null check either if you write it this way. Might need some typecasts to id on that first line. Not sure I can do this since _textSelectionAssistant and _webSelectionAssistant don't share a common interface or protocol. >> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1310 >> + } > > WebKit style says no braces for single line if statement. Already done >> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1454 >> + if (!textLength || textLength > 200) > > Where does this constant 200 come from? This should be a named constant, probably at the top of the file, along with a comment explaining where the number came from. There is a fixme few lines above that references the radar tracking the request to expose this constant. I'll add a comment referencing the above FIXME
Comment on attachment 251378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251378&action=review >> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1299 >> + _page->getSelectionOrContentsAsString([self](const String& string, CallbackBase::Error error) { > > Since this uses lambda syntax, not Objective-C block syntax, we need to explicitly retain/release self, otherwise we could access the view after it has been destroyed. May also need some checks if the thing is still up and frontmost and everything when the callback comes back. There are several other places in this file where this is used without retain/release. I'll fix all of them.
WebKit: Committed revision 183208.