Bug 77477

Created attachment 124817 [details] patch

Created attachment 124818 [details] patch

Comment on attachment 124818 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=124818&action=review this looks reasonable to me. Maybe we could ask jamesr or fishd to take a look? > Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:136 > + ASSERT(!m_inScroll); I think you're safe doing this because in these types of gesture events can't have event handlers. > Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:158 > + m_inputHandlerClient->scrollEnd(); this would be where we wire up flick...

We investigated. this code can be committed independently of 73350 -- but needs 73350 to work correctly overflow on DIVs.

Created attachment 125000 [details] patch I made a couple of changes in the patch to make it work with https://bugs.webkit.org/show_bug.cgi?id=73350.

Created attachment 125012 [details] Added a check to make sure ScrollUpdate and ScrollEnd gestures arrive only after ScrollBegin

Created attachment 125028 [details] Added a test

Attachment 125028 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:47: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:47: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:48: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:53: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:54: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:57: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:58: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:61: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:62: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:63: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:68: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:76: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:77: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:79: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:88: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:89: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:92: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:138: mock_input_handler is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:140: mock_client is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:190: Use 0 instead of NULL. [readability/null] [5] Total errors found: 20 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 125032 [details] style

Comment on attachment 125032 [details] style View in context: https://bugs.webkit.org/attachment.cgi?id=125032&action=review Looks great - have many nits, but nothing serious. > Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:155 > + if (m_scrollStarted) { i think we should ASSERT() on m_scrollStarted, not branch. if we get a scrollupdate without a scrollbegin it means that the caller has broken the contract with us - we should fail fast and loudly, not try to limp along IMO > Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.h:74 > + bool m_expectScrollUpdateEnd; > + bool m_scrollStarted; since these are just for assertions can you wrap them (and their use) in #ifndef NDEBUG? I like to keep state that's just kept for debugging separate from state that's used for actual logic > Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:77 > + MockWebCompositorInputHandlerClient() : m_handled(false), m_sendToWidget(false) { } expand the initialization out please - we generally don't like to have more than one statement per line > Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:80 > + void Reset() in WebKit, method names are lowecase > Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:143 > + OwnPtr<WebCompositorInputHandlerImpl> comp = WebCompositorInputHandlerImpl::create(&mockInputHandler); we don't typically like abbreviations in WebKit, even in tests. it's also not super clear that 'comp' means 'WebCompositorInputHandlerImpl' can you name this something like "inputHandler" ? > Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:149 > + gesture.x = gesture.y = gesture.globalX = gesture.globalY = gesture.deltaX = gesture.deltaY = 0; WebGestureEvent's constructor zeros all these fields out, no need to do so yourself

Comment on attachment 125032 [details] style View in context: https://bugs.webkit.org/attachment.cgi?id=125032&action=review >> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:155 >> + if (m_scrollStarted) { > > i think we should ASSERT() on m_scrollStarted, not branch. if we get a scrollupdate without a scrollbegin it means that the caller has broken the contract with us - we should fail fast and loudly, not try to limp along IMO Indeed. I am using m_expectedScrollUpdateEnd for precisely this purpose (e.g see lines 139, 154, 162). m_scrollStarted is used to make sure we don't call scrollBy or scrollEnd if the call to scrollBegin returned something other than ScrollStarted. >> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.h:74 >> + bool m_scrollStarted; > > since these are just for assertions can you wrap them (and their use) in #ifndef NDEBUG? I like to keep state that's just kept for debugging separate from state that's used for actual logic m_expectedScrollUpdated is only used for assertions. So I have put it in #ifndef. Since m_scrollStarted is used to decide whether or not to call scrollBy/scrollEnd, I have left it unchanged. >> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:77 >> + MockWebCompositorInputHandlerClient() : m_handled(false), m_sendToWidget(false) { } > > expand the initialization out please - we generally don't like to have more than one statement per line Done. >> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:80 >> + void Reset() > > in WebKit, method names are lowecase Done. >> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:143 >> + OwnPtr<WebCompositorInputHandlerImpl> comp = WebCompositorInputHandlerImpl::create(&mockInputHandler); > > we don't typically like abbreviations in WebKit, even in tests. it's also not super clear that 'comp' means 'WebCompositorInputHandlerImpl' can you name this something like "inputHandler" ? Done. >> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:149 >> + gesture.x = gesture.y = gesture.globalX = gesture.globalY = gesture.deltaX = gesture.deltaY = 0; > > WebGestureEvent's constructor zeros all these fields out, no need to do so yourself Cool! Thanks. Removed.

Created attachment 125248 [details] patch Addressed all the comments (made code changes as suggested, or left comment explaining)

(In reply to comment #11) > (From update of attachment 125032 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125032&action=review > > >> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:155 > >> + if (m_scrollStarted) { > > > > i think we should ASSERT() on m_scrollStarted, not branch. if we get a scrollupdate without a scrollbegin it means that the caller has broken the contract with us - we should fail fast and loudly, not try to limp along IMO > > Indeed. I am using m_expectedScrollUpdateEnd for precisely this purpose (e.g see lines 139, 154, 162). m_scrollStarted is used to make sure we don't call scrollBy or scrollEnd if the call to scrollBegin returned something other than ScrollStarted. > Right. I'm saying you should NOT do this, you should just ASSERT().

Comment on attachment 125032 [details] style View in context: https://bugs.webkit.org/attachment.cgi?id=125032&action=review >>>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:155 >>>> + if (m_scrollStarted) { >>> >>> i think we should ASSERT() on m_scrollStarted, not branch. if we get a scrollupdate without a scrollbegin it means that the caller has broken the contract with us - we should fail fast and loudly, not try to limp along IMO >> >> Indeed. I am using m_expectedScrollUpdateEnd for precisely this purpose (e.g see lines 139, 154, 162). m_scrollStarted is used to make sure we don't call scrollBy or scrollEnd if the call to scrollBegin returned something other than ScrollStarted. > > Right. I'm saying you should NOT do this, you should just ASSERT(). Just to make sure I understand clearly :-) ... consider the following case: - A GestureScrollBegin event comes in. It triggers a scrollBegin. However, for whatever reason, scrollBegin returns ScrollIgnored, therefore setting m_scrollStarted to false. The caller of the function (i.e. the source of the event) isn't aware that scrolling didn't start successfully. - The event-source sends a GestureScrollUpdate event. Since scrolling didn't start successfully, i.e. m_scrollStarted is false, this will trip the ASSERT. But should this really cause a crash? The event-source did send a GestureScrollBegin before sending a GestureScrollUpdate, thereby meeting the expectation of WebCompositorInputHandlerImpl.

Gah, I realize my mistake now! Sorry to mislead for the last several comments. To start, the caller should never send a GestureScrollUpdate (or any type of input event) before the previous input event is ACK'd. I believe that we propagate the disposition of the scroll in the reply to the GestureScrollBegin, correct? So I guess the three cases are: (1) we've decided to accept the scroll on the compositor thread, in which case we accept the input on the thread and expect take a sequence of GestureScrollUpdates followed by a GestureScrollEnd. (2) we've decided not to accept the scroll on the compositor thread, but the main thread might want to accept it. in this case, the input handler calls didNotHandleInputEvent(true) and then has to forward all subsequence GestureScrollUpdates up to and including the GestureScrollEnd to the main thread. I think this is what your patch (correctly) does. (3) we've decided not to scroll on the compositor thread and we know we also don't want to scroll on the main thread. there are two possibilities here: either we tell the browser side to not send any more gesture events at all for this GestureScroll, or the browser still sends them and we ignore them. I'm not sure which behavior this patch is trying to adopt. So the question is then what happens for (3) - do we expect to get the GestureScrollUpdates / GestureScrollEnd and ignore them, or is the caller supposed to not generate those? I think it'd be more efficient if the browser was smart enough not to send these to the renderer at all but that might be tricky.

Sorry for not explaining this better in my previous attempt. (In reply to comment #15) > Gah, I realize my mistake now! Sorry to mislead for the last several comments. > > To start, the caller should never send a GestureScrollUpdate (or any type of input event) before the previous input event is ACK'd. I believe that we propagate the disposition of the scroll in the reply to the GestureScrollBegin, correct? So I guess the three cases are: > > (1) we've decided to accept the scroll on the compositor thread, in which case we accept the input on the thread and expect take a sequence of GestureScrollUpdates followed by a GestureScrollEnd. > > (2) we've decided not to accept the scroll on the compositor thread, but the main thread might want to accept it. in this case, the input handler calls didNotHandleInputEvent(true) and then has to forward all subsequence GestureScrollUpdates up to and including the GestureScrollEnd to the main thread. I think this is what your patch (correctly) does. > > (3) we've decided not to scroll on the compositor thread and we know we also don't want to scroll on the main thread. there are two possibilities here: either we tell the browser side to not send any more gesture events at all for this GestureScroll, or the browser still sends them and we ignore them. I'm not sure which behavior this patch is trying to adopt. > > So the question is then what happens for (3) - do we expect to get the GestureScrollUpdates / GestureScrollEnd and ignore them, or is the caller supposed to not generate those? I think it'd be more efficient if the browser was smart enough not to send these to the renderer at all but that might be tricky. Interesting. I hadn't considered (3). With this change for case (3), gesture events will continue to come in, and the events will simply be ignored here. I think this is a safe way of dealing with the events. Right now, when the MT compositor is not in use, the browser always send all the gesture events, and they are processed/ignored as appropriate by Webkit. So this behaviour will match that. Having said that, I like your idea of not sending gesture update/end if gesture-begin didn't initiate a scroll. From what I can see, this will need to happen in chrome (CompositorThread/InputEventFilter). I will look into implementing that now. If I can get this to work before https://bugs.webkit.org/show_bug.cgi?id=73350 lands, then I will update this patch here. Thanks for your suggestions!

OK, glad we're on the same page then. I've had a cold this week and it's been messing with my head. I think we can treat (3) as a future improvement. The main benefit is that without special handling for that case what will happen is all GestureScrollUpdates will be sent to the renderer, then sent to the main thread, and the ACK will be held until the render process main thread responds. This means we can't process any further gestures (or input events at all) until the main thread becomes responsive which might cause jank. We should try to avoid the main thread whenever we know that we can get away with it. I agree this needs some work in the chromium side of the world.

Comment on attachment 125248 [details] patch R=me on this. Can you file separate bug(s) about the behavior on (3)? It'll require code changes outside of WebKit, but I think the WebKit code can also be made less tolerant once that happens.

(In reply to comment #18) > (From update of attachment 125248 [details]) > R=me on this. Can you file separate bug(s) about the behavior on (3)? It'll require code changes outside of WebKit, but I think the WebKit code can also be made less tolerant once that happens. Thanks! I have just filed http://crbug.com/112692 to keep track of this.

Comment on attachment 125248 [details] patch Requesting cq? so that subsequent work for pinch etc. can go ahead. Although 73350 hasn't landed yet, this shouldn't introduce regression.

Comment on attachment 125248 [details] patch Clearing flags on attachment: 125248 Committed r106987: <http://trac.webkit.org/changeset/106987>

All reviewed patches have been landed. Closing bug.