Bug 127085 - Expose scrollView on WKView
Summary: Expose scrollView on WKView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-16 00:17 PST by Ian Henderson
Modified: 2014-05-10 16:55 PDT (History)
2 users (show)

See Also:


Attachments
patch (6.81 KB, patch)
2014-01-16 00:53 PST, Ian Henderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Henderson 2014-01-16 00:17:23 PST
We need to expose scrollView as a property on WKView.
Comment 1 Ian Henderson 2014-01-16 00:53:32 PST
Created attachment 221342 [details]
patch
Comment 2 mitz 2014-01-16 02:37:55 PST
Comment on attachment 221342 [details]
patch

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

> Source/WebKit2/UIProcess/API/ios/WKScrollView.mm:50
> +- (NSMethodSignature *)methodSignatureForSelector:(SEL)aSelector

We’d normally name the argument “selector”, regardless of what the NSObject header calls it.

> Source/WebKit2/UIProcess/API/ios/WKScrollView.mm:60
> +- (BOOL)respondsToSelector:(SEL)aSelector

Ditto.

> Source/WebKit2/UIProcess/API/ios/WKScrollView.mm:65
> +- (void)forwardInvocation:(NSInvocation *)anInvocation

We’d normally name the argument “invocation”.

> Source/WebKit2/UIProcess/API/ios/WKScrollView.mm:78
> +{
> +    BOOL messageHandled = NO;
> +    if ([_internalDelegate respondsToSelector:[anInvocation selector]]) {
> +        [anInvocation invokeWithTarget:_internalDelegate];
> +        messageHandled = YES;
> +    }
> +    if ([_externalDelegate respondsToSelector:[anInvocation selector]]) {
> +        [anInvocation invokeWithTarget:_externalDelegate];
> +        messageHandled = YES;
> +    }
> +    if (!messageHandled)
> +        [super forwardInvocation:anInvocation];
> +}

I wonder if we should also override -forwardingTargetForSelector: so that we can do faster (possibly by an order of magnitude) forwarding of selectors that only one of the delegates implements. Probably not unless we see that this is slow and the alternative is faster.
Comment 3 mitz 2014-01-16 02:40:18 PST
Comment on attachment 221342 [details]
patch

I didn’t mean to revert Antti’s cq+, I simply started writing my comments before he set the flag. I’m changing it back to cq+ but feel free to make changes.
Comment 4 WebKit Commit Bot 2014-01-16 02:49:06 PST
Comment on attachment 221342 [details]
patch

Clearing flags on attachment: 221342

Committed r162121: <http://trac.webkit.org/changeset/162121>
Comment 5 WebKit Commit Bot 2014-01-16 02:49:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 mitz 2014-01-16 07:11:57 PST
(In reply to comment #3)
> (From update of attachment 221342 [details])
> I didn’t mean to revert Antti’s cq+, I simply started writing my comments before he set the flag. I’m changing it back to cq+ but feel free to make changes.

Actually, I should have left it at cq? so that this only got landed after review by a WebKit2 owner.
Comment 7 mitz 2014-05-10 16:55:34 PDT
(In reply to comment #2)
> I wonder if we should also override -forwardingTargetForSelector: so that we can do faster (possibly by an order of magnitude) forwarding of selectors that only one of the delegates implements. Probably not unless we see that this is slow and the alternative is faster.

Tim Horton just discovered that indeed this is slow and the alternative is faster. See bug 132790.