Bug 143618 – Clients of WKWebView should be able to override drag functions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Enrica Casucci]

We need to make sure that the clients of WKWebView can override drag methods.

[Enrica Casucci]

Created attachment 250532 [details]
patch

[WebKit Commit Bot]

Attachment 250532 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1693:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1694:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1695:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1696:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1697:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1698:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 250532 [details]
patch

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

I think the factoring here isn’t great. Shouldn’t be duplicating that call to dragImage. I have multiple ideas below on how to avoid that.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1685
> +- (void)_setDragImage:(NSImage *)image at:(NSPoint)clientPoint linkDrag:(BOOL)linkDrag

Can we find a way to share more code with WKView _setDragImage? The only difference seems to be which object we call dragImage: on (and a slight difference in how we fetch the mouse down event). Seems like we could refactor to share code instead of repeating this whole method body. That fits the design of how we handle all the other methods below better. Lets just expose an internal method for use here instead of exposing mouseDownEvent. It could be:

    - (void)dragImageForView:(NSView *)view image:(NSImage *)image at:(NSPoint)clientPoint linkDrag:(BOOL)linkDrag;

Then we would just call it here:

    [_wkView dragImageForView:self image:image at:clientPoint linkDrag:linkDrag];

And then inside WKView:

    [self dragImageForView:self image:image at:clientPoint linkDrag:linkDrag];

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1688
> +    // The call below could release this WKView.
> +    RetainPtr<WKWebView> protector(self);

That comment doesn’t make things clear enough. Since we make only one method call, it’s not obvious why doing a retain/release is helpful. We don’t try to use self after returning from the method. Is this to work around bugs of some sort in the dragImage method? Maybe AppKit doesn’t protect itself against a possible release, and so we are protecting it? I guess this was copied from WKView’s _setDragImage method, and all these comments apply there too.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1695
> +              event:(linkDrag) ? [NSApp currentEvent] :[_wkView mouseDownEvent]

Missing space after the ":" here, again copied from the WKView _setDragImage method.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1710
> +    

Extra blank line here.

> Source/WebKit2/UIProcess/mac/PageClientImpl.mm:413
> +    if (m_webView)
> +        [m_webView _setDragImage:dragNSImage.get() at:clientPosition linkDrag:isLinkDrag];
> +    else
> +        [m_wkView _setDragImage:dragNSImage.get() at:clientPosition linkDrag:isLinkDrag];

If _setDragImage is truly only used here, then maybe it should have a different name (not sure why a function that calls dragImage is called setDragImage) and maybe it could just *be* the one I suggested above, which takes a view argument and we can pass m_webView.

I’m not sure I we have the right division of labor between this function and the _setDragImage method. Maybe that method doesn’t have to exist at all and we can put all the code here, as long as we can get at the mouse down event. That might be the best way to go.

[Enrica Casucci]

(In reply to comment #3)
> Comment on attachment 250532 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250532&action=review
> 
> I think the factoring here isn’t great. Shouldn’t be duplicating that call
> to dragImage. I have multiple ideas below on how to avoid that.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1685
> > +- (void)_setDragImage:(NSImage *)image at:(NSPoint)clientPoint linkDrag:(BOOL)linkDrag
> 
> Can we find a way to share more code with WKView _setDragImage? The only
> difference seems to be which object we call dragImage: on (and a slight
> difference in how we fetch the mouse down event). Seems like we could
> refactor to share code instead of repeating this whole method body. That
> fits the design of how we handle all the other methods below better. Lets
> just expose an internal method for use here instead of exposing
> mouseDownEvent. It could be:
> 
>     - (void)dragImageForView:(NSView *)view image:(NSImage *)image
> at:(NSPoint)clientPoint linkDrag:(BOOL)linkDrag;
> 
> Then we would just call it here:
> 
>     [_wkView dragImageForView:self image:image at:clientPoint
> linkDrag:linkDrag];
> 
> And then inside WKView:
> 
>     [self dragImageForView:self image:image at:clientPoint
> linkDrag:linkDrag];
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1688
> > +    // The call below could release this WKView.
> > +    RetainPtr<WKWebView> protector(self);
> 
> That comment doesn’t make things clear enough. Since we make only one method
> call, it’s not obvious why doing a retain/release is helpful. We don’t try
> to use self after returning from the method. Is this to work around bugs of
> some sort in the dragImage method? Maybe AppKit doesn’t protect itself
> against a possible release, and so we are protecting it? I guess this was
> copied from WKView’s _setDragImage method, and all these comments apply
> there too.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1695
> > +              event:(linkDrag) ? [NSApp currentEvent] :[_wkView mouseDownEvent]
> 
> Missing space after the ":" here, again copied from the WKView _setDragImage
> method.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1710
> > +    
> 
> Extra blank line here.
> 
> > Source/WebKit2/UIProcess/mac/PageClientImpl.mm:413
> > +    if (m_webView)
> > +        [m_webView _setDragImage:dragNSImage.get() at:clientPosition linkDrag:isLinkDrag];
> > +    else
> > +        [m_wkView _setDragImage:dragNSImage.get() at:clientPosition linkDrag:isLinkDrag];
> 
> If _setDragImage is truly only used here, then maybe it should have a
> different name (not sure why a function that calls dragImage is called
> setDragImage) and maybe it could just *be* the one I suggested above, which
> takes a view argument and we can pass m_webView.
> 
> I’m not sure I we have the right division of labor between this function and
> the _setDragImage method. Maybe that method doesn’t have to exist at all and
> we can put all the code here, as long as we can get at the mouse down event.
> That might be the best way to go.

I'll rework it based on your suggestions.

[Enrica Casucci]

I reworked the patch according to Darin's suggestions.
Committed revision 182765.

[Alexey Proskuryakov]

This broke 32-bit build:

/Volumes/Data/slave/yosemite-32bit-release/build/Source/WebKit2/UIProcess/mac/PageClientImpl.mm:410:46: error: incompatible operand types ('WKWebView *' and 'WKView *') [-Werror]

[Alexey Proskuryakov]

Attempted a build fix in r182775. I'm not sure if I understand why such conditional behavior is correct though.

[Alexey Proskuryakov]

More 32-bit build fixing in r182779.