Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1892653003:
Extract touch handling logic from EventHandler (Closed)
|
Created:
4 years, 8 months ago by Navid Zolghadr Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews, dtapuska+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtract touch handling logic from EventHandler
Moving towards modularizing EventHandler class,
this change extract some touch handling logic
from EventHandler class to the touch designated
class.
BUG=603567
Committed: https://crrev.com/f0eb5cc52b1f14db73d58d1248213fe40131857e
Cr-Commit-Position: refs/heads/master@{#391035}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Rebased #
Total comments: 14
Patch Set 3 : Removing functional change #Patch Set 4 : #
Total comments: 23
Patch Set 5 : Rebased #
Total comments: 15
Patch Set 6 : Applying comments #Patch Set 7 : Removing the invalid comment #Patch Set 8 : Add TODO #Messages
Total messages: 33 (8 generated)
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
This is the refactoring change to touch logic. ptal. It depends on Mustaq's change. So I need to probably rebase it on master when Mustaq's change goes in and then run the bots.
landing this change will make us wanting to land some uma metrics for passive event listeners a little difficult. We may want to hold onto it a bit since we'd like to get those changes landed and merged into 51. https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:360: Where is the runtime feature check? https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.h:66: TouchEventManager& getTouchEventManager(); exposing this API seems odd. There is only one API used on it.. Can we just inline that API call?
Do you know when the UMA metric change will land? Wasn't 51 branch cut last Friday? I have to wait for Mustaq change to go in anyway and also applying all the comments on this review. So I guess there will be enough time until I get this in. https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:360: On 2016/04/15 01:30:50, dtapuska wrote: > Where is the runtime feature check? Just like before the checks for the feature being enabled or not is not in the early stages. Right now there is a check in flow that will prevent sending the pointer event in sendTouchPointerEvent->dispatchPointerEvent. During this refactoring at some point it will become impossible to not create the pointer event objects as we are moving into creating other events from the pointer events. In this very change I wanted to get rid of TouchInfo object and instead pass an array of PointerEvents. The region feature which seems to be in touch and not in pointer events prevented me. I will be investigating that and see why we don't have it in pointer events. But regardless what do you suggest here? Do you want me to put the checks earlier in the flow? If so how do you think we should proceed the refactoring? https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.h:66: TouchEventManager& getTouchEventManager(); On 2016/04/15 01:30:50, dtapuska wrote: > exposing this API seems odd. There is only one API used on it.. Can we just > inline that API call? Sure. I'll do that.
Thanks Navid, this CL will let us chop EventHandler down by 300+ lines! There is one major issue here: we shouldn't be stopping TE firing on TouchScrollStarted. This should be easy to fix in this CL, but it led me to an alternate design possibility with greater isolation of event-types. Let's give it a thought... https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/PointerEventFactory.h (right): https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/PointerEventFactory.h:40: const FloatPoint& pagePoint); The name "pagePoint" seems logical to me, but please double check with bokan's explanation to be sure: https://sites.google.com/a/chromium.org/dev/developers/design-documents/blink... https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:340: return WebInputEventResult::HandledSystem; This needs attention: the TouchScrollStarted event should affect only PE firing, not TE firing. Note that TEs will be throttled at the same time further upstream (at the browser/input-router level). This give me a second thought about the whole design: this is a case where TEs will affect PEs, and we will see a lot of such cases when we will start moving mouse events to a separate unit (MouseEventHandler?). Our final goal is to make per-type event-handling as much isolated as possible, with well-defined interaction between the "sub-units" of EventHandler. Does TEs-though-PE-manager sound bad from that perspective? Or PE still deserves a special case because of its capture releasing "power"? https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.h:49: WebInputEventResult handleTouchEvent( Since handlerTouchEvent() is the main service provided by this class, is "TouchEventHandler" a more appropriate name? https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.h:62: void setAllPropertiesofTouchInfos(HeapVector<TouchInfo>&); s/of/Of/
https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:340: return WebInputEventResult::HandledSystem; On 2016/04/20 15:40:01, mustaq wrote: > This needs attention: the TouchScrollStarted event should affect only PE firing, > not TE firing. Note that TEs will be throttled at the same time further upstream > (at the browser/input-router level). > > This give me a second thought about the whole design: this is a case where TEs > will affect PEs, and we will see a lot of such cases when we will start moving > mouse events to a separate unit (MouseEventHandler?). Our final goal is to make > per-type event-handling as much isolated as possible, with well-defined > interaction between the "sub-units" of EventHandler. Does TEs-though-PE-manager > sound bad from that perspective? > > Or PE still deserves a special case because of its capture releasing "power"? As we talked offline this behavior is okay and the same as before. This return statement only block sending TE with type TouchScrollStarted and not the upcoming touch events.
ptal. https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/PointerEventFactory.h (right): https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/PointerEventFactory.h:40: const FloatPoint& pagePoint); On 2016/04/20 15:40:01, mustaq wrote: > The name "pagePoint" seems logical to me, but please double check with bokan's > explanation to be sure: > https://sites.google.com/a/chromium.org/dev/developers/design-documents/blink... It was the name of the variable we were passing. But I'll ask Bokan to confirm. https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1892653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.h:49: WebInputEventResult handleTouchEvent( On 2016/04/20 15:40:01, mustaq wrote: > Since handlerTouchEvent() is the main service provided by this class, is > "TouchEventHandler" a more appropriate I just wanted to follow the PointerEventManager class naming as both of the classes keep track of the states and stuff like that. Let me do the renaming of all in one change maybe later if you like to have it all *Handler.
Description was changed from ========== Extract touch handling logic from EventHandler Moving towards modularizing EventHandler class, this change extract some touch handling logic from EventHandler class to the touch designated class. BUG=603567 ========== to ========== Extract touch handling logic from EventHandler Moving towards modularizing EventHandler class, this change extract some touch handling logic from EventHandler class to the touch designated class. BUG=603567 ==========
dtapuska@chromium.org changed reviewers: + tdresser@chromium.org
So I think the main issue I have with this change is that it makes a plumbing level changed combined with new functionality. I do think the new functionality might have some performance regressions. So if we can try to make sure the plumbing change is done independently that would probably be better. https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.h (right): https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.h:78: int getPointerEventId(const WebPointerProperties&); Can this be const? https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp (right): https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:104: EventTypeNames::pointerdown, PointerEventFactoryTest::PlatformTouchPointBuilder(pointerType, rawId, state), PlatformEvent::NoModifiers, FloatSize(0, 0), FloatPoint(0, 0)); Just FloatSize(), FloatPoint() is good enough https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:385: // Iterate through the touch points, sending PointerEvents to the targets as required. So do we expect any performance regressions here? The old code opt'd out early when sending pointer events. Now it will send them always. https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:412: // Do not send pointer events for stationary touches period. https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:67: bool isAnyTouchActive(); can likely be const no? https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:103: WebInputEventResult sendTouchPointerEvent( bit awkward formatting. No period at the end of the comment.
Dave, is this good enough for the functional vs refactoring separation that you had in mind? https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.h (right): https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.h:78: int getPointerEventId(const WebPointerProperties&); On 2016/04/25 20:15:00, dtapuska wrote: > Can this be const? Done. I marked the rest of the functions as const if possible too. https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp (right): https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:104: EventTypeNames::pointerdown, PointerEventFactoryTest::PlatformTouchPointBuilder(pointerType, rawId, state), PlatformEvent::NoModifiers, FloatSize(0, 0), FloatPoint(0, 0)); On 2016/04/25 20:15:00, dtapuska wrote: > Just FloatSize(), FloatPoint() is good enough Sure. https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:385: // Iterate through the touch points, sending PointerEvents to the targets as required. On 2016/04/25 20:15:00, dtapuska wrote: > So do we expect any performance regressions here? The old code opt'd out early > when sending pointer events. Now it will send them always. I did the early exit in the new patch with kind of removing the logic of pointer event coming before touch events. https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:412: // Do not send pointer events for stationary touches On 2016/04/25 20:15:00, dtapuska wrote: > period. :D. I should be more careful about these dots. I seem to be missing a lot of them. https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:67: bool isAnyTouchActive(); On 2016/04/25 20:15:00, dtapuska wrote: > can likely be const no? Done. Also marked any other function that was the same. https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:103: WebInputEventResult sendTouchPointerEvent( On 2016/04/25 20:15:00, dtapuska wrote: > bit awkward formatting. No period at the end of the comment. Not sure how it would be better. Is this any better?
LGTM with nit. https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:103: WebInputEventResult sendTouchPointerEvent( On 2016/04/26 21:57:51, Navid Zolghadr wrote: > On 2016/04/25 20:15:00, dtapuska wrote: > > bit awkward formatting. No period at the end of the comment. > > Not sure how it would be better. Is this any better? Doesn't fit a single line? https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:280: Double empty-lines. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:377: bool TouchEventManager::generateTouchInfos( Make the hittesting explicit in the name here so that in the next CL, the extra PE hittest before it becomes obvious in the caller. s/generateTouchInfos/generateTouchInfosAfterHittest/
thanks for making this change. Just a couple of minor things. Will double check it again when once lan's change has landed and you have rebased. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.h (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.h:77: // properties if exists otherwise s_invalidId periods at the end of comments :-) https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:370: newTouchSequence = false; break; ? https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:98: void dispatchTouchPointerEvents( Is it possible for HeapVector to be const? I'm not entirely sure because you load structs from it. At very worst case this will need a comment indicating it isn't an output parameter; but const is likely better. You might need to change the iterator to be a const auto& https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:172: If there isn't a comment I don't think it needs a new line. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:100: } ending comment terminating the namespace " // namespace" https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:112: namespace { Why double annonymous namespaces; can we collapse to use just one? https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:191: // touches and targetTouches should only contain information about Refer to variables enclosed in | https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:200: // Note that any touches that are in the TouchStationary state (e.g. if ditto https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:86: bool m_touchPressed; Put the two bools together. Should allow this object to be be decreased by sizeof(void*)
ptal. https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1892653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:103: WebInputEventResult sendTouchPointerEvent( On 2016/04/27 14:36:11, mustaq wrote: > On 2016/04/26 21:57:51, Navid Zolghadr wrote: > > On 2016/04/25 20:15:00, dtapuska wrote: > > > bit awkward formatting. No period at the end of the comment. > > > > Not sure how it would be better. Is this any better? > > Doesn't fit a single line? Yes. It does. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.h (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.h:77: // properties if exists otherwise s_invalidId On 2016/04/27 17:07:04, dtapuska wrote: > periods at the end of comments :-) Oh my god! This file has quite a few missing dots! :) https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:370: newTouchSequence = false; On 2016/04/27 17:07:04, dtapuska wrote: > break; ? Done. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:98: void dispatchTouchPointerEvents( On 2016/04/27 17:07:04, dtapuska wrote: > Is it possible for HeapVector to be const? > > I'm not entirely sure because you load structs from it. > > At very worst case this will need a comment indicating it isn't an output > parameter; but const is likely better. You might need to change the iterator to > be a const auto& It is kind of an output parameter in the sense that some of the fields are being set (i.e. consumed bit). https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:172: On 2016/04/27 17:07:04, dtapuska wrote: > If there isn't a comment I don't think it needs a new line. Done. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:100: } On 2016/04/27 17:07:04, dtapuska wrote: > ending comment terminating the namespace " // namespace" Done. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:112: namespace { On 2016/04/27 17:07:05, dtapuska wrote: > Why double annonymous namespaces; can we collapse to use just one? It was the case from EventHandler. So I thought there must be a reason for it. But sure. I'll merge them and see if it causes any issues or anything. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:191: // touches and targetTouches should only contain information about On 2016/04/27 17:07:04, dtapuska wrote: > Refer to variables enclosed in | There was quite a few occurrence of this in this existing code. I went over the file again and fixed all that I was able to see. Let me know if you find another one. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:200: // Note that any touches that are in the TouchStationary state (e.g. if On 2016/04/27 17:07:05, dtapuska wrote: > ditto Done. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:280: On 2016/04/27 14:36:11, mustaq wrote: > Double empty-lines. Done. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:377: bool TouchEventManager::generateTouchInfos( On 2016/04/27 14:36:11, mustaq wrote: > Make the hittesting explicit in the name here so that in the next CL, the extra > PE hittest before it becomes obvious in the caller. > s/generateTouchInfos/generateTouchInfosAfterHittest/ Done. https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:86: bool m_touchPressed; On 2016/04/27 17:07:05, dtapuska wrote: > Put the two bools together. Should allow this object to be be decreased by > sizeof(void*) Done.
lgtm % nit https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1892653003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:98: void dispatchTouchPointerEvents( On 2016/04/28 15:13:11, Navid Zolghadr wrote: > On 2016/04/27 17:07:04, dtapuska wrote: > > Is it possible for HeapVector to be const? > > > > I'm not entirely sure because you load structs from it. > > > > At very worst case this will need a comment indicating it isn't an output > > parameter; but const is likely better. You might need to change the iterator > to > > be a const auto& > > It is kind of an output parameter in the sense that some of the fields are being > set (i.e. consumed bit). Can we add a comment to the file describing what it does then? ie; it dispatches the touch events and will adjust the heap vector under X conditions... just because it isn't obvious.
One missed comment on comments, LGMT otherwise... https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:23: // This class takes care of dispatching all touch events and their properties. Nit: s/and.*$/and maintaining related states\./. (Is my regex even valid ? ;) ).
LGTM with nits. https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right): https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.cpp:361: const WebPointerProperties& properties) const Is |properties| guaranteed to be unique? https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.h (right): https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.h:40: const FloatPoint& pagePoint); client != page: was this a semantic change? https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:50: // Returns true if it succesfully generates touchInfos Missing . https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:73: bool allTouchReleased); allTouchesReleased? https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:77: // cleared in |PointerEventManager::clear()|. PointerEventManager -> TouchEventManager. https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:93: bool m_waitingForFirstTouchMove; Remove extra whitespace.
https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right): https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.cpp:361: const WebPointerProperties& properties) const On 2016/04/29 15:55:04, tdresser wrote: > Is |properties| guaranteed to be unique? There are two properties in the WebPointerProperties struct (i.e. pointerType and id) which we rely on to generate pointer events. Whether it is guaranteed or not maybe Mustaq knows better. But if it's not the other parts of the code are quite broken. So I assume the answer would be yes and why we went down this path. https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.h (right): https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.h:40: const FloatPoint& pagePoint); On 2016/04/29 15:55:04, tdresser wrote: > client != page: was this a semantic change? I don't know much about this one. I just picked the name of the parameter we were passing before and put it as the name of the parameter. Although I understand that we use it in setClientX/Y apis. I'll double check this with Bokan when I add him to this review to see which one should be changed. https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:23: // This class takes care of dispatching all touch events and their properties. On 2016/04/29 15:09:10, mustaq wrote: > Nit: s/and.*$/and maintaining related states\./. (Is my regex even valid ? ;) ). Done. https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:50: // Returns true if it succesfully generates touchInfos On 2016/04/29 15:55:04, tdresser wrote: > Missing . Done. https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:73: bool allTouchReleased); On 2016/04/29 15:55:04, tdresser wrote: > allTouchesReleased? Done. https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:77: // cleared in |PointerEventManager::clear()|. On 2016/04/29 15:55:04, tdresser wrote: > PointerEventManager -> TouchEventManager. Done. https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:93: bool m_waitingForFirstTouchMove; On 2016/04/29 15:55:04, tdresser wrote: > Remove extra whitespace. Done.
nzolghadr@chromium.org changed reviewers: + bokan@chromium.org
bokan@ can you also particularly look at a variable name that I was not sure about. It is in PointerEventFactory::create function. So I was not sure whether the variable should be called pagePoint or clientPoint or maybe just adjustedPagePoint. We use it in setClientX/Y apis in the implementation of that create function. So naturally I assume it would be called clientPoint. But when it is assigned (in TouchEventManager::setAllPropertiesOfTouchInfos) it is stored in adjustedPagePoint and it is derived from a variable called pagePoint.
https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right): https://codereview.chromium.org/1892653003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.cpp:361: const WebPointerProperties& properties) const On 2016/04/29 16:31:19, Navid Zolghadr wrote: > On 2016/04/29 15:55:04, tdresser wrote: > > Is |properties| guaranteed to be unique? > > There are two properties in the WebPointerProperties struct (i.e. pointerType > and id) which we rely on to generate pointer events. Whether it is guaranteed or > not maybe Mustaq knows better. But if it's not the other parts of the code are > quite broken. So I assume the answer would be yes and why we went down this > path. Navid is right: it is reasonable to assume that low-levels ids are unique per pointer type. This is true for mouse & touch for sure. We haven't explored multiple styli (styluses) IIRC, and definitely not multiple mouse pointers: https://wiki.archlinux.org/index.php/Multi-pointer_X Good question, Tim. Need to file a low-priority bug.
On 2016/04/29 16:35:11, Navid Zolghadr wrote:
> bokan@ can you also particularly look at a variable name that I was not sure
> about. It is in PointerEventFactory::create function. So I was not sure
whether
> the variable should be called pagePoint or clientPoint or maybe just
> adjustedPagePoint. We use it in setClientX/Y apis in the implementation of
that
> create function. So naturally I assume it would be called clientPoint. But
when
> it is assigned (in TouchEventManager::setAllPropertiesOfTouchInfos) it is
stored
> in adjustedPagePoint and it is derived from a variable called pagePoint.
After some digging, PlatformTouchPoint::m_pos is in root frame coordinates
(i.e. root's "client coordinates"). You can see where that comes from in
WebInputEventConversion's PlatformTouchPointBuilder constructor.
Two fixes we should make here:
- Please change the name (possibly in a followup, might be lots of use sites)
from
m_pos to m_positionInRootFrame.
- Could you remove this inaccurate comment @ WebInputEventConversion.cpp:457:
// This assumes convertFromRootFrame does only translations, not scales.
I looked in convertFromRootFrame and it does do scale appropriately.
This means touchInfo.adjustedPagePoint is correct and the point there is in page
coordinates, scaled by CSS zoom (i.e. in CSS pixels). However, the m_clientX/Y
members
of pointerEventInit expect to be in client coordinates so I think the current
code is
wrong. The error is at EventHandler::dispatchPointerEvents, we pass in the
adjustedPagePoint even though sendTouchPointerEvent expects client coordinates.
You have to convert from page to client coordinates. You do this by using the
current
FrameView's contentsToFrame to go from the frame's page coordinates to client.
IMO, this is yet another instance where crbug.com/417560 would have helped us.
One of
these quarters we should invest in that...
On 2016/04/29 17:21:54, bokan wrote: > On 2016/04/29 16:35:11, Navid Zolghadr wrote: > > bokan@ can you also particularly look at a variable name that I was not sure > > about. It is in PointerEventFactory::create function. So I was not sure > whether > > the variable should be called pagePoint or clientPoint or maybe just > > adjustedPagePoint. We use it in setClientX/Y apis in the implementation of > that > > create function. So naturally I assume it would be called clientPoint. But > when > > it is assigned (in TouchEventManager::setAllPropertiesOfTouchInfos) it is > stored > > in adjustedPagePoint and it is derived from a variable called pagePoint. > > After some digging, PlatformTouchPoint::m_pos is in root frame coordinates > (i.e. root's "client coordinates"). You can see where that comes from in > WebInputEventConversion's PlatformTouchPointBuilder constructor. > > Two fixes we should make here: > > - Please change the name (possibly in a followup, might be lots of use sites) > from > m_pos to m_positionInRootFrame. > > - Could you remove this inaccurate comment @ WebInputEventConversion.cpp:457: > // This assumes convertFromRootFrame does only translations, not scales. > I looked in convertFromRootFrame and it does do scale appropriately. > > This means touchInfo.adjustedPagePoint is correct and the point there is in page > coordinates, scaled by CSS zoom (i.e. in CSS pixels). However, the m_clientX/Y > members > of pointerEventInit expect to be in client coordinates so I think the current > code is > wrong. The error is at EventHandler::dispatchPointerEvents, we pass in the > adjustedPagePoint even though sendTouchPointerEvent expects client coordinates. > > You have to convert from page to client coordinates. You do this by using the > current > FrameView's contentsToFrame to go from the frame's page coordinates to client. > > IMO, this is yet another instance where crbug.com/417560 would have helped us. > One of > these quarters we should invest in that... I removed the comment in WebInputEventConversion.cpp:457. However, I'm a bit hesitant to change the logic of our coordinates in this refactoring change despite the fact that the current state is incorrect. Partly because I already removed a huge chunk of functional change from this code and don't want to add another functional change back in. But also I think probably it's better to keep this refactoring change only a refactoring change without changing any other behavior as Dave suggested in the earlier patches. This change moves a lot of code around and it was already blocked on some other changes in the same area and reverting it would likely cause a lot of pain. WDYT?
On 2016/04/29 17:49:51, Navid Zolghadr wrote: > On 2016/04/29 17:21:54, bokan wrote: > > On 2016/04/29 16:35:11, Navid Zolghadr wrote: > > > bokan@ can you also particularly look at a variable name that I was not sure > > > about. It is in PointerEventFactory::create function. So I was not sure > > whether > > > the variable should be called pagePoint or clientPoint or maybe just > > > adjustedPagePoint. We use it in setClientX/Y apis in the implementation of > > that > > > create function. So naturally I assume it would be called clientPoint. But > > when > > > it is assigned (in TouchEventManager::setAllPropertiesOfTouchInfos) it is > > stored > > > in adjustedPagePoint and it is derived from a variable called pagePoint. > > > > After some digging, PlatformTouchPoint::m_pos is in root frame coordinates > > (i.e. root's "client coordinates"). You can see where that comes from in > > WebInputEventConversion's PlatformTouchPointBuilder constructor. > > > > Two fixes we should make here: > > > > - Please change the name (possibly in a followup, might be lots of use > sites) > > from > > m_pos to m_positionInRootFrame. > > > > - Could you remove this inaccurate comment @ > WebInputEventConversion.cpp:457: > > // This assumes convertFromRootFrame does only translations, not scales. > > I looked in convertFromRootFrame and it does do scale appropriately. > > > > This means touchInfo.adjustedPagePoint is correct and the point there is in > page > > coordinates, scaled by CSS zoom (i.e. in CSS pixels). However, the m_clientX/Y > > members > > of pointerEventInit expect to be in client coordinates so I think the current > > code is > > wrong. The error is at EventHandler::dispatchPointerEvents, we pass in the > > adjustedPagePoint even though sendTouchPointerEvent expects client > coordinates. > > > > You have to convert from page to client coordinates. You do this by using the > > current > > FrameView's contentsToFrame to go from the frame's page coordinates to client. > > > > IMO, this is yet another instance where crbug.com/417560 would have helped us. > > One of > > these quarters we should invest in that... > > I removed the comment in WebInputEventConversion.cpp:457. However, I'm a bit > hesitant to change the logic of our coordinates in this refactoring change > despite the fact that the current state is incorrect. > Partly because I already removed a huge chunk of functional change from this > code and don't want to add another functional change back in. But also I think > probably it's better to keep this refactoring change only a refactoring change > without changing any other behavior as Dave suggested in the earlier patches. > This change moves a lot of code around and it was already blocked on some other > changes in the same area and reverting it would likely cause a lot of pain. > > WDYT? ptal.
Thanks for pinging, I hadn't seen your reply - sorry about the delay. lgtm to keeping this strictly code movement but please add a TODO with a bug # to fix.
On 2016/05/02 16:59:10, bokan wrote: > Thanks for pinging, I hadn't seen your reply - sorry about the delay. > > lgtm to keeping this strictly code movement but please add a TODO with a bug # > to fix. Sure.
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, dtapuska@chromium.org, tdresser@chromium.org, bokan@chromium.org Link to the patchset: https://codereview.chromium.org/1892653003/#ps140001 (title: "Add TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892653003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892653003/140001
Message was sent while issue was closed.
Description was changed from ========== Extract touch handling logic from EventHandler Moving towards modularizing EventHandler class, this change extract some touch handling logic from EventHandler class to the touch designated class. BUG=603567 ========== to ========== Extract touch handling logic from EventHandler Moving towards modularizing EventHandler class, this change extract some touch handling logic from EventHandler class to the touch designated class. BUG=603567 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Extract touch handling logic from EventHandler Moving towards modularizing EventHandler class, this change extract some touch handling logic from EventHandler class to the touch designated class. BUG=603567 ========== to ========== Extract touch handling logic from EventHandler Moving towards modularizing EventHandler class, this change extract some touch handling logic from EventHandler class to the touch designated class. BUG=603567 Committed: https://crrev.com/f0eb5cc52b1f14db73d58d1248213fe40131857e Cr-Commit-Position: refs/heads/master@{#391035} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f0eb5cc52b1f14db73d58d1248213fe40131857e Cr-Commit-Position: refs/heads/master@{#391035} |
