[WK2][GTK] It should process MouseMoved events when the parent window is not active
Created attachment 202741 [details] Patch
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.
I can tell that at least Chrome and Firefox in Linux do behave like this and so does WK1 with the GTK+ port.
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?
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.
(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.
(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().
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?
Created attachment 310258 [details] Patch
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.
(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.
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
Created attachment 310511 [details] Patch
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.
Created attachment 310630 [details] Patch
(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.
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
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
(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?
No, you can just ignore it.
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.
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.
Created attachment 314611 [details] Patch
Comment on attachment 314611 [details] Patch This should address Tim's comments. Please review again to make sure I got it right? Thanks!
I like this better, but the cost is an extra virtual function call. Tim, do you want to review it again?
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.
Created attachment 314716 [details] Patch
Comment on attachment 314716 [details] Patch Since this already has r+ I'll wait for the EWS to complete before landing it.
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!
Comment on attachment 314716 [details] Patch Clearing flags on attachment: 314716 Committed r219195: <http://trac.webkit.org/changeset/219195>
All reviewed patches have been landed. Closing bug.