Bug 116691 – [GTK] It should process MouseMoved events when the parent window is not active

• Component: New Bugs
• Status: RESOLVED FIXED

[Claudio Saavedra]

[WK2][GTK] It should process MouseMoved events when the parent window is not active

[Claudio Saavedra]

Created attachment 202741 [details]
Patch

[Martin Robinson]

Comment on attachment 202741 [details]
Patch

Does web content expect this though? I think it's pretty important that we behave like other web browsers if there's consistency.

[Claudio Saavedra]

I can tell that at least Chrome and Firefox in Linux do behave like this and so does WK1 with the GTK+ port.

[Claudio Saavedra]

I know this patch is probably not ready for landing as the way it is implemented -- adding #idefs in shared code -- is not very clean, but I welcome suggestions on how to actually do it.

Same as to what's the rationale behind not passing mouse moved events in an inactive window. Is this the standard behavior for Mac applications?

[Michael Catanzaro]

Comment on attachment 202741 [details]
Patch

Does this fix some practical issue?

(In reply to comment #4)
> I know this patch is probably not ready for landing as the way it is
> implemented -- adding #idefs in shared code -- is not very clean, but I
> welcome suggestions on how to actually do it.

This seems like an appropriate use of #ifdefs to me. Sometimes it's just the cleanest way to handle things.

Since this patch is quite old, I'm going to remove the r? -- please reset it if you still want to land this.

[Claudio Saavedra]

(In reply to comment #5)
> Comment on attachment 202741 [details]
> Patch
> 
> Does this fix some practical issue?

If you hover a page in a window that is not active you'd expect the contents to be updated (in which ever way the web content developers designed it), not only when the page is active. Same with changing the cursor to a hand when hovering a link.

[Michael Catanzaro]

(In reply to comment #6)
> If you hover a page in a window that is not active you'd expect the contents
> to be updated (in which ever way the web content developers designed it),
> not only when the page is active. Same with changing the cursor to a hand
> when hovering a link.

OK. I agree, we should do this.

(In reply to comment #5)
> This seems like an appropriate use of #ifdefs to me. Sometimes it's just the
> cleanest way to handle things.

Thinking about this some more, we could do it more cleanly... you could put the #ifdef inside a function shouldProcessEventsWhenInactive(), or have a platformShouldProcessEventsWhenInactive().

[Jan]

So what is the current status of this?
Still planed to change the behavior of webkitgtk?

The suggested changes to the patch don't look like too much work (to my inexperienced eyes). Would it help if I sent in a patch?
If so, where would the function shouldProcessEventsWhenInactive or platformShouldProcessEventsWhenInactive go?

[Claudio Saavedra]

Created attachment 310258 [details]
Patch

[Claudio Saavedra]

Updating this to get it out of my way.

> (In reply to comment #4)
> 
> This seems like an appropriate use of #ifdefs to me. Sometimes it's just the
> cleanest way to handle things.

Looking at the rest of the #ifdefs in the file I think this is definitely a reasonable approach, so I just updated the patch and changelog a bit.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #7)
> Thinking about this some more, we could do it more cleanly... you could put
> the #ifdef inside a function shouldProcessEventsWhenInactive()

This is what I'd do, just because that makes the rationale here more self-documenting, and makes it easier to see that this is a choice that different platforms get to make.

I'm also not completely sure about using PLATFORM(GTK) here... it might make more sense to use !PLATFORM(COCOA), since I guess most other ports probably want this behavior too. Not sure. But I think it's OK.

[Michael Catanzaro]

Comment on attachment 310258 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2372
> +#elif

#else

[Claudio Saavedra]

Created attachment 310511 [details]
Patch

[Michael Catanzaro]

Comment on attachment 310511 [details]
Patch

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

OK, this looks fine to me. You'll need to ask a WK2 owner to provide the final review, though.

> Source/WebKit2/ChangeLog:11
> +        specially those targetting Linux, these events should be processed

especially, targeting

> Source/WebKit2/UIProcess/WebPageProxy.cpp:4886
> +#if PLATFORM(COCOA)

Or maybe it should be #if PLATFORM(MAC), since this specifically relates to mouse events.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:4891
> +    return true;

I bet this will trigger an unreachable code warning on Mac, so put this in an #else case.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2318
> +#if PLATFORM(COCOA)

Maybe #if PLATFORM(MAC)?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2319
> +            // We need to do a full, normal hit test during this mouse event if the page is active or if a mouse

I see you got a nice minor cleanup from moving this code, cool.

[Claudio Saavedra]

Created attachment 310630 [details]
Patch

[Claudio Saavedra]

(In reply to Michael Catanzaro from comment #14)
> OK, this looks fine to me. You'll need to ask a WK2 owner to provide the
> final review, though.

Adding a couple of owners as per Michael's suggestion based on suggest-reviewers output.

[Build Bot]

Comment on attachment 310630 [details]
Patch

Attachment 310630 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3775637

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-buffered.html

[Build Bot]

Created attachment 310646 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Claudio Saavedra]

(In reply to Build Bot from comment #17)

> New failing tests:
> imported/w3c/web-platform-tests/media-source/mediasource-buffered.html

This looks like an unrelated test that has been flaky? Do I need to trigger the mac-debug ews again somehow?

[Tim Horton]

No, you can just ignore it.

[Michael Catanzaro]

Comment on attachment 310630 [details]
Patch

Owners have had plenty of time to object by now, so r=me. But it would be nice to wait a couple more days from this reminder before pushing, to give owners even more time.

[Tim Horton]

Comment on attachment 310630 [details]
Patch

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

Either way, wk2r=me

> Source/WebKit2/UIProcess/WebPageProxy.cpp:4889
> +    return pageClient.isViewWindowActive();

Perhaps it would be better to keep the platform-specific code in e.g. PageClient, and add a PageClient::shouldSetCursor?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2318
> +#if PLATFORM(MAC)

Maybe make these PLATFORM(COCOA)? I’m not sure if they’re used on iOS (I think the synthesized click stuff is deeper than this, but I’m not sure). Probably has no impact, but it’s less change.

[Claudio Saavedra]

Created attachment 314611 [details]
Patch

[Claudio Saavedra]

Comment on attachment 314611 [details]
Patch

This should address Tim's comments. Please review again to make sure I got it right? Thanks!

[Michael Catanzaro]

I like this better, but the cost is an extra virtual function call.

Tim, do you want to review it again?

[Tim Horton]

Comment on attachment 314611 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +

Extra space here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:4856
> +    if (m_pageClient.shouldSetCursor())

Now that I *see* it, is there any reason for this not just to be folded into the implementation of setCursor? That addresses Michael's (very minor) concern and leaves even less madness here.

[Claudio Saavedra]

Created attachment 314716 [details]
Patch

[Claudio Saavedra]

Comment on attachment 314716 [details]
Patch

Since this already has r+ I'll wait for the EWS to complete before landing it.

[Claudio Saavedra]

Comment on attachment 314716 [details]
Patch

All bots green, landing as is. Please let me know if I missed something after Tim's last comments. Thanks!

[WebKit Commit Bot]

Comment on attachment 314716 [details]
Patch

Clearing flags on attachment: 314716

Committed r219195: <http://trac.webkit.org/changeset/219195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.