Bug 55053 - Implement non-activating clicks to allow dragging out of a background window
Summary: Implement non-activating clicks to allow dragging out of a background window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-02-23 09:06 PST by mitz
Modified: 2011-04-12 16:52 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.82 KB, patch)
2011-04-11 17:22 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch2 (10.01 KB, patch)
2011-04-12 14:00 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch3 (9.88 KB, patch)
2011-04-12 16:26 PDT, Enrica Casucci
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2011-02-23 09:06:31 PST
WKView should implement -acceptsFirstMouse: and -shouldDelayWindowOrderingForEvent: in a manner similar to WebHTMLView in order to allow non-activating clicks.
Comment 1 mitz 2011-02-23 09:07:00 PST
<rdar://problem/9042197>
Comment 2 Enrica Casucci 2011-04-11 17:22:33 PDT
Created attachment 89130 [details]
Patch
Comment 3 WebKit Review Bot 2011-04-11 17:23:49 PDT
Attachment 89130 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/WebPage.h:326:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/WebPage/WebPage.h:327:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Alexey Proskuryakov 2011-04-11 17:53:01 PDT
Comment on attachment 89130 [details]
Patch

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

> Source/WebKit2/UIProcess/API/mac/WKView.mm:998
> +    if ([hitView isKindOfClass:[self class]]) {

Can this ever fail in WebKit2?
Comment 5 mitz 2011-04-11 18:00:53 PDT
I think we should avoid sync messaging the web process in most cases. For example, if there is no (non-caret) selection in the page, there is no need to involve the web process at all. Doesn’t the page proxy have this (or similar) knowledge already?
Comment 6 Enrica Casucci 2011-04-12 11:41:45 PDT
(In reply to comment #4)
> (From update of attachment 89130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89130&action=review
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:998
> > +    if ([hitView isKindOfClass:[self class]]) {
> 
> Can this ever fail in WebKit2?

Probably not. I think it is redundant in WebKit2.
Comment 7 Enrica Casucci 2011-04-12 11:42:37 PDT
(In reply to comment #5)
> I think we should avoid sync messaging the web process in most cases. For example, if there is no (non-caret) selection in the page, there is no need to involve the web process at all. Doesn’t the page proxy have this (or similar) knowledge already?

I believe you're right. The page proxy has that knowledge and I will add that check.
Comment 8 Enrica Casucci 2011-04-12 14:00:51 PDT
Created attachment 89267 [details]
Patch2

New patch that addresses Dan's comment.
Comment 9 Alexey Proskuryakov 2011-04-12 14:16:16 PDT
Comment on attachment 89267 [details]
Patch2

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

> Source/WebKit2/UIProcess/API/mac/WKView.mm:989
> +    if ([hitView isKindOfClass:[self class]]) {

Would it make sense to change this into an assertion?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1011
> +    if ([hitView isKindOfClass:[self class]]) {

Ditto.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:307
> +    process()->sendSync(Messages::WebPage::AcceptsFirstMouse(eventNumber, event), Messages::WebPage::AcceptsFirstMouse::Reply(result), m_pageID);

We probably need the usual 20 second timeout, or maybe even less than that - hardly anyone would wait 20 seconds to start dragging.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:697
> +        result = hitResult.scrollbar() != 0;

We don't compare to 0, and if we must cast to boolean for some reason, it's usually "!!".
Comment 10 Enrica Casucci 2011-04-12 14:28:12 PDT
(In reply to comment #9)
> (From update of attachment 89267 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89267&action=review
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:989
> > +    if ([hitView isKindOfClass:[self class]]) {
> 
> Would it make sense to change this into an assertion?
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:1011
> > +    if ([hitView isKindOfClass:[self class]]) {
> 
> Ditto.
>
I'm not sure.
 
> > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:307
> > +    process()->sendSync(Messages::WebPage::AcceptsFirstMouse(eventNumber, event), Messages::WebPage::AcceptsFirstMouse::Reply(result), m_pageID);
> 
> We probably need the usual 20 second timeout, or maybe even less than that - hardly anyone would wait 20 seconds to start dragging.

I think it makes sense.

> 
> > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:697
> > +        result = hitResult.scrollbar() != 0;
> 
> We don't compare to 0, and if we must cast to boolean for some reason, it's usually "!!".
Comment 11 Enrica Casucci 2011-04-12 16:26:59 PDT
Created attachment 89303 [details]
Patch3

confirmed with Sam the hit testing behavior and made the relevant changed.
Comment 12 Alexey Proskuryakov 2011-04-12 16:30:31 PDT
Comment on attachment 89303 [details]
Patch3

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

> Source/WebKit2/UIProcess/API/mac/WKView.mm:988
> +        return false;

This function returns a BOOL, so this should be NO, not false.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1001
> +        return false;

Ditto.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1009
> +        return false;

Ditto.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:680
> +    HitTestResult hitResult = frame->eventHandler()->hitTestResultAtPoint(event.position(), YES);

This is a C++ function call, so it should use true, not YES.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:692
> +    HitTestResult hitResult = frame->eventHandler()->hitTestResultAtPoint(event.position(), YES);

Ditto.
Comment 13 Sam Weinig 2011-04-12 16:33:48 PDT
Comment on attachment 89303 [details]
Patch3

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

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:297
> +bool WebPageProxy::shouldDelayWindowOrderingForEvent(const WebKit::WebMouseEvent &event)

& on the wrong side.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:300
> +    const double MessageTimeout = 20;

The variable should not start with a capital.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:305
> +bool WebPageProxy::acceptsFirstMouse(int eventNumber, const WebKit::WebMouseEvent &event)

Here too.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:308
> +    const double MessageTimeout = 20;

The variable should not start with a capital.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:673
> +void WebPage::shouldDelayWindowOrderingEvent(const WebKit::WebMouseEvent &event, bool& result)

& on the wrong side.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:685
> +void WebPage::acceptsFirstMouse(int eventNumber, const WebKit::WebMouseEvent &event, bool& result)

Here too.
Comment 14 Enrica Casucci 2011-04-12 16:47:19 PDT
I've addressed all Sam's and Alexey's comments.
As discussed privately with Sam, I'm changing the timeout on the synchronous call from 20 to 3 seconds.
Comment 15 Enrica Casucci 2011-04-12 16:52:23 PDT
http://trac.webkit.org/changeset/83665