144077 2015-04-22 16:08:26 -0700 Support share button 2015-04-23 15:01:35 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified iOS 8.2 RESOLVED FIXED P2 Normal --- 1 enrica enrica commit-queue oldest_to_newest 1087625 0 enrica 2015-04-22 16:08:26 -0700 We should add support to display the share button in the callout menu. rdar://problem/19772892 1087628 1 251378 enrica 2015-04-22 16:11:03 -0700 Created attachment 251378 Patch 1087630 2 commit-queue 2015-04-22 16:12:51 -0700 Attachment 251378 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. 1087640 3 enrica 2015-04-22 16:28:18 -0700 Fixed the style. 1087666 4 251378 darin 2015-04-22 18:02:39 -0700 Comment on attachment 251378 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. 1087670 5 251378 enrica 2015-04-22 18:26:41 -0700 Comment on attachment 251378 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 1087675 6 251378 enrica 2015-04-22 18:40:42 -0700 Comment on attachment 251378 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. 1088021 7 enrica 2015-04-23 15:01:35 -0700 WebKit: Committed revision 183208. 251378 2015-04-22 16:11:03 -0700 2015-04-22 18:02:39 -0700 Patch WK2-share-sheet-webkit.txt text/plain 3215 enrica SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4MzE0MykKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE1LTA0LTIyICBFbnJpY2Eg Q2FzdWNjaSAgPGVucmljYUBhcHBsZS5jb20+CisKKyAgICAgICAgU3VwcG9ydCBzaGFyZSBidXR0 b24uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDQw NzcKKyAgICAgICAgcmRhcjovL3Byb2JsZW0vMTk3NzI4OTIKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIFVJUHJvY2Vzcy9pb3MvV0tDb250ZW50Vmll d0ludGVyYWN0aW9uLm1tOgorICAgICAgICAoLVtXS0NvbnRlbnRWaWV3IF9zaGFyZTpdKToKKyAg ICAgICAgKC1bV0tDb250ZW50VmlldyBjYW5QZXJmb3JtQWN0aW9uOndpdGhTZW5kZXI6XSk6CisK IDIwMTUtMDQtMjIgIEJyZW50IEZ1bGdoYW0gIDxiZnVsZ2hhbUBhcHBsZS5jb20+CiAKICAgICAg ICAgVW5yZXZpZXdlZCBidWlsZCBmaXggYWZ0ZXIgcjE4MzEzNi4KSW5kZXg6IFNvdXJjZS9XZWJL aXQyL1VJUHJvY2Vzcy9pb3MvV0tDb250ZW50Vmlld0ludGVyYWN0aW9uLm1tCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9pb3MvV0tDb250ZW50Vmlld0ludGVyYWN0aW9u Lm1tCShyZXZpc2lvbiAxODMxMTQpCisrKyBTb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvaW9zL1dL Q29udGVudFZpZXdJbnRlcmFjdGlvbi5tbQkod29ya2luZyBjb3B5KQpAQCAtMTgxLDYgKzE4MSw3 IEBAIEBpbnRlcmZhY2UgVUlUZXh0SW50ZXJhY3Rpb25Bc3Npc3RhbnQgKFMKIC0gKHZvaWQpc2No ZWR1bGVSZXBsYWNlbWVudHNGb3JUZXh0OihOU1N0cmluZyAqKXRleHQ7CiAtICh2b2lkKXNob3dU ZXh0U2VydmljZUZvcjooTlNTdHJpbmcgKilzZWxlY3RlZFRlcm0gZnJvbVJlY3Q6KENHUmVjdClw cmVzZW50YXRpb25SZWN0OwogLSAodm9pZClzY2hlZHVsZUNoaW5lc2VUcmFuc2xpdGVyYXRpb25G b3JUZXh0OihOU1N0cmluZyAqKXRleHQ7CistICh2b2lkKXNob3dTaGFyZVNoZWV0Rm9yOihOU1N0 cmluZyAqKXNlbGVjdGVkVGVybSBmcm9tUmVjdDooQ0dSZWN0KXByZXNlbnRhdGlvblJlY3Q7CiBA ZW5kCiAKIEBpbnRlcmZhY2UgVUlXS1NlbGVjdGlvbkFzc2lzdGFudCAoU3RhZ2luZ1RvUmVtb3Zl KQpAQCAtMTI5Myw2ICsxMjk0LDI4IEBAIC0gKE5TQXJyYXkgKilzdXBwb3J0ZWRQYXN0ZWJvYXJk VHlwZXNGb3IKICAgICByZXR1cm4gKF9wYWdlLT5lZGl0b3JTdGF0ZSgpLmlzQ29udGVudFJpY2hs eUVkaXRhYmxlKSA/IHJpY2hUeXBlcyA6IHBsYWluVGV4dFR5cGVzOwogfQogCistICh2b2lkKV9z aGFyZTooaWQpc2VuZGVyCit7CisgICAgX3BhZ2UtPmdldFNlbGVjdGlvbk9yQ29udGVudHNBc1N0 cmluZyhbc2VsZl0oY29uc3QgU3RyaW5nJiBzdHJpbmcsIENhbGxiYWNrQmFzZTo6RXJyb3IgZXJy b3IpIHsKKyAgICAgICAgaWYgKGVycm9yICE9IENhbGxiYWNrQmFzZTo6RXJyb3I6Ok5vbmUpCisg ICAgICAgICAgICByZXR1cm47CisgICAgICAgIGlmICghc3RyaW5nKQorICAgICAgICAgICAgcmV0 dXJuOworCisgICAgICAgIENHUmVjdCBwcmVzZW50YXRpb25SZWN0ID0gX3BhZ2UtPmVkaXRvclN0 YXRlKCkucG9zdExheW91dERhdGEoKS5zZWxlY3Rpb25SZWN0c1swXS5yZWN0KCk7CisKKyAgICAg ICAgaWYgKF90ZXh0U2VsZWN0aW9uQXNzaXN0YW50KSB7CisgICAgICAgICAgICBpZiAoW190ZXh0 U2VsZWN0aW9uQXNzaXN0YW50IHJlc3BvbmRzVG9TZWxlY3RvcjpAc2VsZWN0b3Ioc2hvd1NoYXJl U2hlZXRGb3I6ZnJvbVJlY3Q6KV0pIHsKKyAgICAgICAgICAgICAgICBbKGlkKV90ZXh0U2VsZWN0 aW9uQXNzaXN0YW50IHNob3dTaGFyZVNoZWV0Rm9yOnN0cmluZyBmcm9tUmVjdDpwcmVzZW50YXRp b25SZWN0XTsKKyAgICAgICAgICAgIH0KKyAgICAgICAgfSBlbHNlIHsKKyAgICAgICAgICAgIGlm IChbX3dlYlNlbGVjdGlvbkFzc2lzdGFudCByZXNwb25kc1RvU2VsZWN0b3I6QHNlbGVjdG9yKHNo b3dTaGFyZVNoZWV0Rm9yOmZyb21SZWN0OildKSB7CisgICAgICAgICAgICAgICAgWyhpZClfd2Vi U2VsZWN0aW9uQXNzaXN0YW50IHNob3dTaGFyZVNoZWV0Rm9yOnN0cmluZyBmcm9tUmVjdDpwcmVz ZW50YXRpb25SZWN0XTsKKyAgICAgICAgICAgIH0KKyAgICAgICAgfQorICAgIH0pOworfQorCiAt ICh2b2lkKV9hZGRTaG9ydGN1dDooaWQpc2VuZGVyCiB7CiAgICAgaWYgKF90ZXh0U2VsZWN0aW9u QXNzaXN0YW50ICYmIFtfdGV4dFNlbGVjdGlvbkFzc2lzdGFudCByZXNwb25kc1RvU2VsZWN0b3I6 QHNlbGVjdG9yKHNob3dUZXh0U2VydmljZUZvcjpmcm9tUmVjdDopXSkKQEAgLTE0MjMsNiArMTQ0 NiwxNyBAQCAgICAgICAgIC8vIGFjY2VwdGFibGUsIGJ1dCB0aGUgaW50ZXJmYWNlCiAgICAgICAg IHJldHVybiBZRVM7CiAgICAgfQogCisgICAgaWYgKGFjdGlvbiA9PSBAc2VsZWN0b3IoX3NoYXJl OikpIHsKKyAgICAgICAgaWYgKF9wYWdlLT5lZGl0b3JTdGF0ZSgpLmlzSW5QYXNzd29yZEZpZWxk IHx8ICEoaGFzV2ViU2VsZWN0aW9uIHx8IF9wYWdlLT5lZGl0b3JTdGF0ZSgpLnNlbGVjdGlvbklz UmFuZ2UpKQorICAgICAgICAgICAgcmV0dXJuIE5POworCisgICAgICAgIE5TVUludGVnZXIgdGV4 dExlbmd0aCA9IF9wYWdlLT5lZGl0b3JTdGF0ZSgpLnBvc3RMYXlvdXREYXRhKCkuc2VsZWN0ZWRU ZXh0TGVuZ3RoOworICAgICAgICBpZiAoIXRleHRMZW5ndGggfHwgdGV4dExlbmd0aCA+IDIwMCkK KyAgICAgICAgICAgIHJldHVybiBOTzsKKyAgICAgICAgCisgICAgICAgIHJldHVybiBZRVM7Cisg ICAgfQorCiAgICAgaWYgKGFjdGlvbiA9PSBAc2VsZWN0b3IoX2FkZFNob3J0Y3V0OikpIHsKICAg ICAgICAgaWYgKF9wYWdlLT5lZGl0b3JTdGF0ZSgpLmlzSW5QYXNzd29yZEZpZWxkIHx8ICEoaGFz V2ViU2VsZWN0aW9uIHx8IF9wYWdlLT5lZGl0b3JTdGF0ZSgpLnNlbGVjdGlvbklzUmFuZ2UpKQog ICAgICAgICAgICAgcmV0dXJuIE5POwo=