Closed
Bug 976605
Opened 10 years ago
Closed 10 years ago
move active element from BrowserElementPanning.js to C++
Categories
(Core :: Panning and Zooming, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: bkelly, Assigned: botond)
References
Details
(Keywords: perf, Whiteboard: [c=handeye p= s= u=])
Attachments
(13 files, 21 obsolete files)
1.57 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
5.10 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
5.23 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
9.02 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
10.13 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
6.22 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
6.11 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
7.94 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
4.52 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
6.08 KB,
patch
|
vingtetun
:
review+
|
Details | Diff | Splinter Review |
17.59 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
2.44 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
8.17 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
As discussed in bug 970125 we would like to disable touch event listeners in BrowserElementPanning.js as they are slow can can contribute to checkerboarding while swiping quickly. Kats suggests we should be able to move the active event handling to C++ in TabChild or APZCCallbackHelper to accomplish this. He also suggests looking at what was done in bug 972081 as it has already moved some logic from BrowserElementPanning.js to TabChild. Nom for 1.4? as it contributes to checkerboarding during fast scrolling.
Comment 1•10 years ago
|
||
Nice - let's record what kind of improvement we get (perceptual counts as well :)
blocking-b2g: 1.4? → 1.4+
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #1) > Nice - let's record what kind of improvement we get (perceptual counts as > well :) Its only probably about 10ms or so, but its right when we trigger a ton of other activity and want to start scrolling at maximum speed for a fling. Its a bit hard to quantify the impact beyond helping that condition. So its a bit speculative, but informed by previous profiling.
Whiteboard: [c= p= s= u=] → [c= p=4 s= u=]
Reporter | ||
Updated•10 years ago
|
Priority: -- → P1
Reporter | ||
Comment 3•10 years ago
|
||
Vivien, you previously asked if I wanted you to take a look at this, but I said I hoped to get to it soon. Clearly that hasn't happened as I've gotten wrapped up in bug 977975 and friends. Any chance you still have time to take this one?
Flags: needinfo?(21)
Reporter | ||
Updated•10 years ago
|
Whiteboard: [c= p=4 s= u=] → [c= p=4 s= u=1.4]
Comment 4•10 years ago
|
||
Ben (or whoever ends up working on this), please try to put the bulk of the code to manage the active element into someplace reusable, like APZCCallbackHelper. The functions can be invoked from TabChild; that way most of the code can be reused on Metro and eventually Fennec as well.
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(21)
Updated•10 years ago
|
Assignee: bkelly → botond
Assignee | ||
Comment 5•10 years ago
|
||
I have investigated the active element handling code in BEP.js, and there are a few things I do not understand about it. - First of all, why is it necessary to have active element handling code in BEP.js at all? My understanding is that nsEventStateManager sets and clears the :active flag when handling NS_MOUSE_BUTTON_DOWN/UP events. TabChild synthesizes such events for a tap [1], and indeed when I tap I see the :active state being set by both BEP.js and nsEventStateManager. Why is it necessary for BEP.js to do it as well? - If BEP.js is going to be setting the :active flag, why does it not also clear it? I see it clear it in the case where a touch turns into a pan [2], but why does it not also clear it on touch-end? - In fact, I see BEP.js _setting_ the :active flag on touch-end if the touch was a click rather than a pan [3]. Why is it doing that? - Finally, why does BEP.js's _findPannable() special-case the <html> element [4]? This results in it setting the :active flag right away rather than waiting for a timeout in case a pan starts [5]. Vivien, if you're familiar with this code, could you help clarify the above points? If not, could you pass the ni? on to someone who is? Thanks! [1] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?from=TabChild.cpp#1597 [2] http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#278 [3] http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#242 [4] http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#332 [5] http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#170
Flags: needinfo?(21)
Comment 6•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #5) > I have investigated the active element handling code in BEP.js, and there > are > a few things I do not understand about it. > > - First of all, why is it necessary to have active element handling code > in BEP.js at all? My understanding is that nsEventStateManager sets > and clears the :active flag when handling NS_MOUSE_BUTTON_DOWN/UP events. > TabChild synthesizes such events for a tap [1], and indeed when I tap > I see the :active state being set by both BEP.js and nsEventStateManager. > Why is it necessary for BEP.js to do it as well? The active flag needs to be set also when the finger is on the screen, for a certain amount of time in some cases. Mouse sequences are fired only when the user lift the finger. As a result if :active classes are not set in BEP.js then someone can let the finger on the screen for 10 seconds without having a feedback. Which is not a good UX. > > - If BEP.js is going to be setting the :active flag, why does it not also > clear it? I see it clear it in the case where a touch turns into a pan > [2], > but why does it not also clear it on touch-end? Because clearing on touchend means there will be a highlighting flash between the touch sequence and the mouse sequence. Also the :active class is set on the mousedown and cleared on the mouseup. That usually results into the highlighting time beeing way to short to be noticeable. > > - In fact, I see BEP.js _setting_ the :active flag on touch-end if the > touch was a click rather than a pan [3]. Why is it doing that? > The idea is to give a user feedback if the user action may do something, like clicking on a button. If the user action is not going to trigger anything then no highlight should be shown. > - Finally, why does BEP.js's _findPannable() special-case the <html> > element [4]? This results in it setting the :active flag right away > rather than waiting for a timeout in case a pan starts [5]. Same idea as before. The idea is to give a feedback as soon as possible. If there is no possibility to pan, then the feedback is given immediately. But if the user moves the finger too far to generate a click then the highlighting is cleared. > > Vivien, if you're familiar with this code, could you help clarify the above > points? If not, could you pass the ni? on to someone who is? Thanks! Hope it helps. Don't hesitate to ask me on IRC I something is unclear.
Flags: needinfo?(21)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6) > (In reply to Botond Ballo [:botond] from comment #5) > > I have investigated the active element handling code in BEP.js, and there > > are > > a few things I do not understand about it. > > > > - First of all, why is it necessary to have active element handling code > > in BEP.js at all? My understanding is that nsEventStateManager sets > > and clears the :active flag when handling NS_MOUSE_BUTTON_DOWN/UP events. > > TabChild synthesizes such events for a tap [1], and indeed when I tap > > I see the :active state being set by both BEP.js and nsEventStateManager. > > Why is it necessary for BEP.js to do it as well? > > The active flag needs to be set also when the finger is on the screen, for a > certain amount of time in some cases. > Mouse sequences are fired only when the user lift the finger. > > As a result if :active classes are not set in BEP.js then someone can let > the finger on the screen for 10 seconds without having a feedback. Which is > not a good UX. Makes sense. I should have thought of that. > > - If BEP.js is going to be setting the :active flag, why does it not also > > clear it? I see it clear it in the case where a touch turns into a pan > > [2], > > but why does it not also clear it on touch-end? > > Because clearing on touchend means there will be a highlighting flash > between the touch sequence and the mouse sequence. > > Also the :active class is set on the mousedown and cleared on the mouseup. > That usually results into the highlighting time beeing way to short to be > noticeable. This also makes sense. > > - In fact, I see BEP.js _setting_ the :active flag on touch-end if the > > touch was a click rather than a pan [3]. Why is it doing that? > > > > The idea is to give a user feedback if the user action may do something, > like clicking on a button. If the user action is not going to trigger > anything then no highlight should be shown. I think I understand now: on touch-start, if the element is pannable, we start the timer which will set :active if we haven't started panning by the time it fires. But if the user is doing a tap, the touch-end can occur before the timer expires, in which case we set the :active in the touch-end and cancel the timer. The :active will then be cleared in the mouse-down. > > - Finally, why does BEP.js's _findPannable() special-case the <html> > > element [4]? This results in it setting the :active flag right away > > rather than waiting for a timeout in case a pan starts [5]. > > Same idea as before. The idea is to give a feedback as soon as possible. If > there is no possibility to pan, then the feedback is given immediately. But > if the user moves the finger too far to generate a click then the > highlighting is cleared. This part I still don't understand. An <html> element can be panned if there is room in the scroll range. Should we not take the timer approach in that case, like for other (non-<html>) scrollable elements? > > Vivien, if you're familiar with this code, could you help clarify the above > > points? If not, could you pass the ni? on to someone who is? Thanks! > > Hope it helps. Don't hesitate to ask me on IRC I something is unclear. Haven't seen you on IRC. Maybe ping me when you're on? In any case, thanks for the info!
Flags: needinfo?(21)
Assignee | ||
Comment 8•10 years ago
|
||
OK, here's my plan for fixing this, along with some design rationale. First, I should mention something that might be obvious in hindsight but that took me a while to realize: we can't move the setting of the :active flag out of a touch event listener and into something like TabChild::RecvRealTouchEvent(), because in TabChild::RecvRealTouchEvent(), the event's target element on the child side - which is what we need to set the :active on - has not been set yet. It is set during after gecko hit testing when the event is dispatched on the child side. Therefore, we still need to have a touch event listener, but we can have it in C++ code instead of JS code. The performance gain that we hope to get, then, is from moving this code from JS to C++. So, my plan: - Introduce a mechanism by which APZ can provide widget code with auxiliary information about a touch event, such as: (1) on a touch-start, whether this could possibly be the start of a pan (2) on a touch-move, whether this is the start of a pan (3) on a touch-end, whether the touch was a click or a pan Specifically, I will introduce a class AuxiliaryTouchEventInfo which will contain the above flags, and have APZCTreeManager::ReceiveInputEvent populate an out parameter of this type. The rationale for having these determinations made by APZ rather than doing them in TabChild is to avoid duplication of code and to make sure that TabChild is consistent with APZ. For example, to determine (1) in BEP.js, we have a function _findPannable [1] which reasons about whether a content element can be panned or not. This reasoning, if ported over to C++ code verbatim, may not agree exactly with which elements are pannable according to APZ, which is what we're really interested in. We could replicate APZ's logic in TabChild, but that would entail code duplication. I believe the best approach is to get the information directly from APZ. - In TabChild, listen to touch-start events, and keep track of the event target. In RecvRealTouchEvent(), read the auxiliary touch event info and keep track of it too. In RecvRealTouchEvent() or the listener, put these two pieces of information together and set or reset the :active flag as appropriate. The logic here will be more or less a straight port form BEP.js. That is: - on touch-start - if APZ tells us this could be the start of a pan, start a timer - otherwise, set the :active flag immediately - on touch-move - if APZ tells us we are starting a pan - reset the :active flag - cancel the timer if running - on touch-end - if the touch was a click, set the :active flag (the timer from touch-start may not have fired yet) - cancel the timer if running - To keep the above reusable from other GeckoContentController implementations, we can make APZCCallbackHelper an instance class, and have it store the state mentioned above. TabChild would keep an instance of it, and later on other GeckoCC implementations could as well. - Since our logic for managing the :active flag in C++ code relies on information from APZ, we'd still need to keep the old code for managing it around in BEP.js. However, BEP.js would not register the JS touch listeners if APZ is enabled, so its code would only be active for the parent process and non-APZ-enabled children. Once we start using APZ everywhere, we can get rid of those listeners and everything they do altogether. I believe this is the cleaner approach, rather than trying to port handling of :active for non-APZ-enabled children to C++ code. Feedback is welcome! [1] http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#331
Assignee | ||
Comment 9•10 years ago
|
||
Vlad asked me to comment on the 1.4+ status of this bug: My understanding is that the rationale for blocking on this is that code in BrowserElementPanning.js is showing up in profiles as a significant contributor to checkerboarding (see https://bugzilla.mozilla.org/show_bug.cgi?id=975831#c28 and https://bugzilla.mozilla.org/show_bug.cgi?id=970125#c5). Moving that code from JS to C++, and having it do less by using some information available from APZ rather than recomputing it the way it does now, could reduce its impact significantly. Doing this move is relatively straightforward; see the plan I outlined in the previous comment.
Comment 10•10 years ago
|
||
Fair enough, the argument makes sense, but it's really the checkerboarding that blocks and this is potentially just one way to deal with those issues. Perhaps the most important, but we still wouldn't block if we got the symptoms to where we need them to be, but the code still lives in the js file.
blocking-b2g: 1.4+ → ---
Comment 11•10 years ago
|
||
Make sure we ask for uplift when we fix it though.
Updated•10 years ago
|
Priority: P1 → P2
Whiteboard: [c= p=4 s= u=1.4] → [c=handeye p= s= u=]
Comment 12•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #8) > First, I should mention something that might be obvious in hindsight but > that took me a while to realize: we can't move the setting of the :active > flag out of a touch event listener and into something like > TabChild::RecvRealTouchEvent(), because in TabChild::RecvRealTouchEvent(), > the event's target element on the child side - which is what we need to set > the :active on - has not been set yet. It is set during after gecko hit > testing when the event is dispatched on the child side. Note that the gecko hit testing happens inside the call to DispatchWidgetEvent in RecvRealTouchEvent. So as long as you put the code after the call to DispatchWidgetEvent I think you should be fine to put it directly in RecvRealTouchEvent (or some function called from it). It might actually make the most sense to put the active element handling inside the EventStateManager code, so that it is handled consistently with the active element handling for mouse events. > So, my plan: > > - Introduce a mechanism by which APZ can provide widget code with auxiliary > information about a touch event, such as: > (1) on a touch-start, whether this could possibly be the start of a pan > (2) on a touch-move, whether this is the start of a pan > (3) on a touch-end, whether the touch was a click or a pan > Specifically, I will introduce a class AuxiliaryTouchEventInfo which will > contain the above flags, and have APZCTreeManager::ReceiveInputEvent > populate an out parameter of this type. > > The rationale for having these determinations made by APZ rather than > doing them in TabChild is to avoid duplication of code and to make sure > that TabChild is consistent with APZ. For example, to determine (1) in > BEP.js, we have a function _findPannable [1] which reasons about whether > a content element can be panned or not. This reasoning, if ported over > to C++ code verbatim, may not agree exactly with which elements are > pannable according to APZ, which is what we're really interested in. > We could replicate APZ's logic in TabChild, but that would entail code > duplication. I believe the best approach is to get the information > directly from APZ. This seems a little heavyweight to me, but I don't have any concrete improvements to suggest. I would like to point out also the existence of the NotifyTransformBegin and NotifyTransformEnd functions on GeckoContentController which were added a while ago to show/hide scrollbars while actively panning. We might be able to use those for some of this as well. > - To keep the above reusable from other GeckoContentController > implementations, we can make APZCCallbackHelper an instance class, > and have it store the state mentioned above. TabChild would keep an > instance of it, and later on other GeckoCC implementations could as well. I would rather create a separate class to hold this code so that we can keep APZCCallbackHelper stateless. It can live alongside APZCCallbackHelper in widget/xpwidgets if necessary.
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12) > > - Introduce a mechanism by which APZ can provide widget code with auxiliary > > information about a touch event, such as: > > (1) on a touch-start, whether this could possibly be the start of a pan > > (2) on a touch-move, whether this is the start of a pan > > (3) on a touch-end, whether the touch was a click or a pan > > Specifically, I will introduce a class AuxiliaryTouchEventInfo which will > > contain the above flags, and have APZCTreeManager::ReceiveInputEvent > > populate an out parameter of this type. > > > > The rationale for having these determinations made by APZ rather than > > doing them in TabChild is to avoid duplication of code and to make sure > > that TabChild is consistent with APZ. For example, to determine (1) in > > BEP.js, we have a function _findPannable [1] which reasons about whether > > a content element can be panned or not. This reasoning, if ported over > > to C++ code verbatim, may not agree exactly with which elements are > > pannable according to APZ, which is what we're really interested in. > > We could replicate APZ's logic in TabChild, but that would entail code > > duplication. I believe the best approach is to get the information > > directly from APZ. > > This seems a little heavyweight to me, but I don't have any concrete > improvements to suggest. I would like to point out also the existence of the > NotifyTransformBegin and NotifyTransformEnd functions on > GeckoContentController which were added a while ago to show/hide scrollbars > while actively panning. We might be able to use those for some of this as > well. What is heavyweight about it? It does not introduce any new IPC calls. I have considered using NotifyTransformBegin/NotifyTransformEnd, but they do not e.g. get called on a touch-start, when we change from a NOTHING state to a TOUCHING state, both of which are non-transforming.
Comment 14•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #13) > What is heavyweight about it? It does not introduce any new IPC calls. Ok, thinking about it a bit more this seems fine.
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8396032 -
Attachment description: Part 1 - Introduce a mechanism for APZ to provide GeckoCC implementations with auxiliary information about touch events → [WIP] Part 1 - Introduce a mechanism for APZ to provide GeckoCC implementations with auxiliary information about touch events
Comment 16•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #7) > > > - Finally, why does BEP.js's _findPannable() special-case the <html> > > > element [4]? This results in it setting the :active flag right away > > > rather than waiting for a timeout in case a pan starts [5]. > > > > Same idea as before. The idea is to give a feedback as soon as possible. If > > there is no possibility to pan, then the feedback is given immediately. But > > if the user moves the finger too far to generate a click then the > > highlighting is cleared. > > This part I still don't understand. An <html> element can be panned if there > is room in the scroll range. Should we not take the timer approach in that > case, like for other (non-<html>) scrollable elements? > hmm. You could be right and that could be buggy. Most (if not all) Gaia apps used a scrollable div instead of scrolling directly the main window, so that could be something that has not been seen before. > > > Vivien, if you're familiar with this code, could you help clarify the above > > > points? If not, could you pass the ni? on to someone who is? Thanks! > > > > Hope it helps. Don't hesitate to ask me on IRC I something is unclear. > > Haven't seen you on IRC. Maybe ping me when you're on? > I'm lagging a bit those days so I turned off a bit my IRC in order to clean up a bit my todos and my inbox. Sorry for the inconvenience.
Flags: needinfo?(21)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8396032 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
Assignee | ||
Comment 26•10 years ago
|
||
This is my work in progress on this bug. Remaining work: - Complete the ActiveElementManager implementation (Part 8 patch). - Disable the active element handling code in BEP.js (this will be Part 10 and should be the last patch). - Test patches (so far they are untested).
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8403619 -
Attachment is obsolete: true
Attachment #8404243 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8403620 -
Attachment is obsolete: true
Attachment #8404247 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8404251 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8403621 -
Attachment is obsolete: true
Attachment #8404252 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #8403622 -
Attachment is obsolete: true
Attachment #8404253 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8403623 -
Attachment is obsolete: true
Attachment #8404254 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8403625 -
Attachment is obsolete: true
Attachment #8404255 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 34•10 years ago
|
||
Attachment #8403626 -
Attachment is obsolete: true
Attachment #8404256 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8404257 -
Flags: review?(ehsan)
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8403627 -
Attachment is obsolete: true
Attachment #8404258 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8403629 -
Attachment is obsolete: true
Attachment #8404259 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8404260 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 39•10 years ago
|
||
Here is the completed patch set, posted for review. I tested in on http://jsbin.com/vorib and it seems to be working well.
Updated•10 years ago
|
Attachment #8404243 -
Flags: review?(bugmail.mozilla) → review+
Comment 40•10 years ago
|
||
Comment on attachment 8404257 [details] [diff] [review] Part 9 - Expose inIDOMUtils via mozilla::services Review of attachment 8404257 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/inspector/inDOMView.cpp @@ +1234,5 @@ > uint16_t nodeType = 0; > > // Try and get DOM Utils in case we don't have one yet. > if (!mShowWhitespaceNodes && !mDOMUtils) { > + mDOMUtils = do_CreateInstance(IN_DOMUTILS_CONTRACTID); Please replace this with services::GetInDOMUtils.
Attachment #8404257 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8404251 -
Flags: review?(bugmail.mozilla) → review+
Comment 41•10 years ago
|
||
Comment on attachment 8404247 [details] [diff] [review] Part 2 - Turn GeckoCC::NotifyTransform[Begin|End] into an extensible APZ state change notification mechanism Review of attachment 8404247 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/PBrowser.ipdl @@ +411,5 @@ > HandleLongTapUp(CSSPoint point, ScrollableLayerGuid aGuid); > > /** > + * Notifies the child about various APZ state changes. > + * See GeckoContentController::NotifyAPZStateChange() for details. nit: trailing whitespace ::: gfx/layers/ipc/GeckoContentController.h @@ +152,5 @@ > + typedef mozilla::layers::GeckoContentController::APZStateChange APZStateChange; > + > + template <> > + struct ParamTraits<APZStateChange> > + : public TypedEnumSerializer<APZStateChange, move this to GfxMessageUtils.h for consistency ::: widget/windows/winrt/APZController.cpp @@ +309,4 @@ > { > + switch (aChange) > + { > + case APZStateChange::TransformBegin: Please follow existing switch/case style in winrt code.
Attachment #8404247 -
Flags: review?(bugmail.mozilla) → review+
Comment 42•10 years ago
|
||
Comment on attachment 8404252 [details] [diff] [review] Part 4 - Have APZ notify GeckoCC on start of touch block Review of attachment 8404252 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/AsyncPanZoomController.cpp @@ +1996,5 @@ > + } > + > + if (aNewState == TOUCHING) { > + APZCTreeManager* treeManagerLocal = mTreeManager; > + if (treeManagerLocal) { I wonder if this belongs in the NOTHING case of OnTouchStart. That's the only place that invokes SetState(TOUCHING) at the moment. I think if we fix bug 962243 we might want to re-enter state TOUCHING after lifting one finger from a pinch, and in that case we're not actually starting a new touch block. ::: gfx/layers/ipc/AsyncPanZoomController.h @@ +360,5 @@ > > + /** > + * Returns whether this APZC has room to be panned (in any direction). > + */ > + bool HasRoomToPan() const; I imagine that eventually this function will want to take into account touch-action, so maybe calling it something more generic like "IsPannable" might be better. Don't feel too strongly about this, and I'm think HasRoomToPan is a good name for the methods in Axis. ::: gfx/layers/ipc/Axis.cpp @@ +229,5 @@ > CSSRect pageRect = mAsyncPanZoomController->GetFrameMetrics().mScrollableRect; > return GetRectOffset(pageRect); > } > > +float Axis::GetPageLength() const { Squash this to part 1
Attachment #8404252 -
Flags: review?(bugmail.mozilla) → review+
Updated•10 years ago
|
Attachment #8404253 -
Flags: review?(bugmail.mozilla) → review+
Comment 43•10 years ago
|
||
Comment on attachment 8404254 [details] [diff] [review] Part 6 - Group some fields of AsyncPanZoomController into a TouchBlockState structure Review of attachment 8404254 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/GeckoContentController.h @@ +135,5 @@ > + /** > + * APZ finished processing a touch block. > + * |aArg| is 1 if touch was a click, 0 otherwise. > + */ > + EndTouchBlock, Move this to part 8
Attachment #8404254 -
Flags: review?(bugmail.mozilla) → review+
Updated•10 years ago
|
Attachment #8404255 -
Flags: review?(bugmail.mozilla) → review+
Comment 44•10 years ago
|
||
Comment on attachment 8404256 [details] [diff] [review] Part 8 - Have APZ notify GeckoCC on end of touch block Review of attachment 8404256 [details] [diff] [review]: ----------------------------------------------------------------- So my definition of a "touch block" is [1], meaning your OnTouchBlockEnd function doesn't get called exactly on the end of a touch block (the case it skips is when you go from one fingers to two fingers). In fact it's impossible to always tell what the last touch event in a touch block is, before the next block starts. [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/TouchEventHandler.java?rev=28537b55795f#26
Assignee | ||
Comment 45•10 years ago
|
||
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #40) > Comment on attachment 8404257 [details] [diff] [review] > Part 9 - Expose inIDOMUtils via mozilla::services > > Review of attachment 8404257 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/inspector/inDOMView.cpp > @@ +1234,5 @@ > > uint16_t nodeType = 0; > > > > // Try and get DOM Utils in case we don't have one yet. > > if (!mShowWhitespaceNodes && !mDOMUtils) { > > + mDOMUtils = do_CreateInstance(IN_DOMUTILS_CONTRACTID); > > Please replace this with services::GetInDOMUtils. I think that was only a replacement for do_GetService, not do_CreateInstance.
Comment 46•10 years ago
|
||
Comment on attachment 8404258 [details] [diff] [review] Part 10 - Introduce ActiveElementManager Review of attachment 8404258 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/xpwidgets/ActiveElementManager.cpp @@ +50,5 @@ > + // If HandleTouchStart() has already been called (mCanBePanSet is true), > + // handle the touch-start. Otherwise it will be handled in HandleTouchStart() > + // when it is called. > + if (mTarget && mCanBePanSet) { > + DoHandleTouchStart(); For this sort of idiom I prefer pushing the mTarget && mCanBePanSet check into the DoHandleTouchStart function, rather than duplicating it here and in HandleTouchStart. I would also prefer renaming DoHandleTouchStart to something like TriggerElementActivation, where it reflects what the function is attempting to do. @@ +98,5 @@ > +{ > + // If the touch was a click, make mTarget :active right away. > + CancelTask(); > + if (aWasClick) { > + SetActive(mTarget); Are we supposed to reset the active element after a click? Or is that taken care of somewhere else? (I haven't looked at the equivalent code in BEP.js/TabChild yet so it might be answered there). Worth adding a comment to indicate how it gets deactivated. @@ +137,5 @@ > + // This gets called from mSetActiveTask's Run() method. The message loop > + // deletes the task right after running it, so we need to null out > + // mSetActiveTask to make sure we're not left with a dangling pointer. > + mSetActiveTask = nullptr; > + SetActive(mTarget); Would prefer passing the target element as an argument to this function rather than relying on mTarget still being non-null and valid. I feel that would be more robust against future changes.
Attachment #8404258 -
Flags: review?(bugmail.mozilla) → review+
Comment 47•10 years ago
|
||
Comment on attachment 8404259 [details] [diff] [review] Part 11 - Hook up TabChild to the ActiveElementManager Review of attachment 8404259 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.cpp @@ +1945,5 @@ > return true; > } > > + if (aEvent.message == NS_TOUCH_START && localEvent.touches.Length() > 0) { > + mActiveElementManager->SetTargetElement(localEvent.touches[0]->Target()); Can Target() ever return null? I don't know if SetTargetElement will handle that gracefully. ::: dom/ipc/TabChild.h @@ +545,5 @@ > bool mWaitingTouchListeners; > void FireSingleTapEvent(LayoutDevicePoint aPoint); > > bool mIgnoreKeyPressEvent; > + nsRefPtr<ActiveElementManager> mActiveElementManager; Is there a reason you didn't make this stack allocated?
Attachment #8404259 -
Flags: review?(bugmail.mozilla) → review+
Updated•10 years ago
|
Attachment #8404260 -
Flags: review?(bugmail.mozilla)
Attachment #8404260 -
Flags: review?(21)
Attachment #8404260 -
Flags: review+
Comment 48•10 years ago
|
||
Comment on attachment 8404256 [details] [diff] [review] Part 8 - Have APZ notify GeckoCC on end of touch block Review of attachment 8404256 [details] [diff] [review]: ----------------------------------------------------------------- Minusing this one pending clarification of touch blocks (comment 44). I noticed that the code in the ActiveElementHandler deals with the case where we go from one touch point to two, so the code overall will probably have the correct behaviour. However I think then we should rename some things so that we don't call things touch blocks when they're really not. Thoughts?
Attachment #8404256 -
Flags: review?(bugmail.mozilla) → review-
Comment 49•10 years ago
|
||
Comment on attachment 8404260 [details] [diff] [review] Part 12 - Disable active element handling in BEP.js if APZ is enabled Review of attachment 8404260 [details] [diff] [review]: ----------------------------------------------------------------- Can you open a followup to split this file into 2 separated parts ? A big part of the code of this file is only relevant if panning is handled by it and I'm pretty sure that Tarako will be happy if there is less JS loaded into every process (maybe the followup can block bug 970125). That would also makes it easier to identify which parts of the code will be removed once APZ is enabled into the parent process, and what left to be converted from JS to cpp. ::: dom/browser-element/BrowserElementPanning.js @@ +66,1 @@ > } nit: I do wonder if all this code should not be moved to a separate method (something like _setupListenersForPanning). That would make it easier to see instantly that this method setup the event listeners only if APZ is enabled, but also set a few other listeners for both cases.
Attachment #8404260 -
Flags: review?(21) → review+
Comment 50•10 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #49) > Comment on attachment 8404260 [details] [diff] [review] > Part 12 - Disable active element handling in BEP.js if APZ is enabled > > Review of attachment 8404260 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you open a followup to split this file into 2 separated parts ? A big > part of the code of this file is only relevant if panning is handled by it > and I'm pretty sure that Tarako will be happy if there is less JS loaded > into every process (maybe the followup can block bug 970125). IIRC APZC is disabled on Tarako (in gaia apps) so this split would not help much there. It would only help in browser tabs. might still be worth doing this but we'd have to check.
Comment 51•10 years ago
|
||
this patch will not be in 1.3t for tarako so you should not worry about it. Once we move back to trunk we'll re-enable apz + tiling on tarako.
Assignee | ||
Comment 52•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #50) > (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, > needinfo? please) from comment #49) > > Comment on attachment 8404260 [details] [diff] [review] > > Part 12 - Disable active element handling in BEP.js if APZ is enabled > > > > Review of attachment 8404260 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Can you open a followup to split this file into 2 separated parts ? A big > > part of the code of this file is only relevant if panning is handled by it > > and I'm pretty sure that Tarako will be happy if there is less JS loaded > > into every process (maybe the followup can block bug 970125). > > IIRC APZC is disabled on Tarako (in gaia apps) so this split would not help > much there. It would only help in browser tabs. might still be worth doing > this but we'd have to check. I still like the idea of splitting BrowserElementPanning.js into 2 separate parts for code clarity reasons.
Comment 53•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #52) > I still like the idea of splitting BrowserElementPanning.js into 2 separate > parts for code clarity reasons. I'm not opposed to it. Although I think long-term we're going to put it all into C++ anyway. At least all the bits that are used when APZ is enabled.
Assignee | ||
Comment 54•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #41) > ::: gfx/layers/ipc/GeckoContentController.h > @@ +152,5 @@ > > + typedef mozilla::layers::GeckoContentController::APZStateChange APZStateChange; > > + > > + template <> > > + struct ParamTraits<APZStateChange> > > + : public TypedEnumSerializer<APZStateChange, > > move this to GfxMessageUtils.h for consistency Discussed this on IRC. This move would require having GfxMessageUtils.h include GeckoContentController.h, which is undesirable, so it will stay. (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #42) > Comment on attachment 8404252 [details] [diff] [review] > ::: gfx/layers/ipc/AsyncPanZoomController.cpp > @@ +1996,5 @@ > > + } > > + > > + if (aNewState == TOUCHING) { > > + APZCTreeManager* treeManagerLocal = mTreeManager; > > + if (treeManagerLocal) { > > I wonder if this belongs in the NOTHING case of OnTouchStart. That's the > only place that invokes SetState(TOUCHING) at the moment. I think if we fix > bug 962243 we might want to re-enter state TOUCHING after lifting one finger > from a pinch, and in that case we're not actually starting a new touch block. I agree, having it in the NOTHING case of OnTouchStart is better. > ::: gfx/layers/ipc/AsyncPanZoomController.h > @@ +360,5 @@ > > > > + /** > > + * Returns whether this APZC has room to be panned (in any direction). > > + */ > > + bool HasRoomToPan() const; > > I imagine that eventually this function will want to take into account > touch-action, so maybe calling it something more generic like "IsPannable" > might be better. Don't feel too strongly about this, and I'm think > HasRoomToPan is a good name for the methods in Axis. Renamed to IsPannable. (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44) > So my definition of a "touch block" is [1], meaning your OnTouchBlockEnd > function doesn't get called exactly on the end of a touch block (the case it > skips is when you go from one fingers to two fingers). In fact it's > impossible to always tell what the last touch event in a touch block is, > before the next block starts. > > [1] > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/ > TouchEventHandler.java?rev=28537b55795f#26 As discussed on IRC, renamed StartTouchBlock and EndTouchBlock to StartTouch and EndTouch and updated related terminology appropriate. (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #46) > @@ +98,5 @@ > > +{ > > + // If the touch was a click, make mTarget :active right away. > > + CancelTask(); > > + if (aWasClick) { > > + SetActive(mTarget); > > Are we supposed to reset the active element after a click? Or is that taken > care of somewhere else? (I haven't looked at the equivalent code in > BEP.js/TabChild yet so it might be answered there). Worth adding a comment > to indicate how it gets deactivated. My understanding based on comment 6 is that nsEventStateManager will reset the active element when processing the NS_MOUSE_BUTTON_UP event generated from TabChild::FireSingleTapEvent(). It will also set it when processing the NS_MOUSE_BUTTON_DOWN event that's generated immediately before, but we set it here so that the :active state is visible for a little longer. I added a comment to this effect.(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #47) > ::: dom/ipc/TabChild.cpp > @@ +1945,5 @@ > > return true; > > } > > > > + if (aEvent.message == NS_TOUCH_START && localEvent.touches.Length() > 0) { > > + mActiveElementManager->SetTargetElement(localEvent.touches[0]->Target()); > > Can Target() ever return null? I don't know if SetTargetElement will handle > that gracefully. We do check for a null target element in ActiveElementManager. I added a comment to SetTargetElement() saying that it's safe to call with nullptr. > ::: dom/ipc/TabChild.h > @@ +545,5 @@ > > bool mWaitingTouchListeners; > > void FireSingleTapEvent(LayoutDevicePoint aPoint); > > > > bool mIgnoreKeyPressEvent; > > + nsRefPtr<ActiveElementManager> mActiveElementManager; > > Is there a reason you didn't make this stack allocated? Using a method of ActiveElementManager with NewRunnableMethod requires ActiveElementManager to be reference-counted. A reference-counted class cannot be stack-allocated because Release() calls 'delete'. All other comments were addressed as well. I will post updated patches shortly.
Assignee | ||
Comment 55•10 years ago
|
||
Addressed review comments. Carrying r+.
Attachment #8404243 -
Attachment is obsolete: true
Attachment #8405559 -
Flags: review+
Assignee | ||
Comment 56•10 years ago
|
||
Addressed review comments. Carrying r+.
Attachment #8404247 -
Attachment is obsolete: true
Attachment #8405560 -
Flags: review+
Assignee | ||
Comment 57•10 years ago
|
||
Addressed review comments. Carrying r+.
Attachment #8404252 -
Attachment is obsolete: true
Attachment #8405561 -
Flags: review+
Assignee | ||
Comment 58•10 years ago
|
||
Addressed review comments. Carrying r+.
Attachment #8404254 -
Attachment is obsolete: true
Attachment #8405562 -
Flags: review+
Assignee | ||
Comment 59•10 years ago
|
||
Addressed review comments.
Attachment #8404256 -
Attachment is obsolete: true
Attachment #8405563 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 60•10 years ago
|
||
Addressed review comments. Carrying r+.
Attachment #8404257 -
Attachment is obsolete: true
Attachment #8405564 -
Flags: review+
Assignee | ||
Comment 61•10 years ago
|
||
Addressed review comments. Carrying r+.
Attachment #8404258 -
Attachment is obsolete: true
Attachment #8405565 -
Flags: review+
Assignee | ||
Comment 62•10 years ago
|
||
Addressed review comments. Carrying r+.
Attachment #8404259 -
Attachment is obsolete: true
Attachment #8405566 -
Flags: review+
Assignee | ||
Comment 63•10 years ago
|
||
Addressed review comments. I also realized that now that we guard the registration of the touch event handlers with 'docShell.asyncPanZoomEnabled === false', we don't need to guard the synchronous-scrolling code inside those handlers with the same, so I removed those guards. Re-requesting review to make sure I didn't mess anything up.
Attachment #8404260 -
Attachment is obsolete: true
Attachment #8405568 -
Flags: review?(21)
Comment 64•10 years ago
|
||
Comment on attachment 8405563 [details] [diff] [review] Part 8 - Have APZ notify GeckoCC on end of touch Review of attachment 8405563 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks!
Attachment #8405563 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 65•10 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #49) > Can you open a followup to split this file into 2 separated parts ? A big > part of the code of this file is only relevant if panning is handled by it > and I'm pretty sure that Tarako will be happy if there is less JS loaded > into every process (maybe the followup can block bug 970125). > > That would also makes it easier to identify which parts of the code will be > removed once APZ is enabled into the parent process, and what left to be > converted from JS to cpp. Filed bug 995394.
Assignee | ||
Comment 66•10 years ago
|
||
try |
Try push: https://tbpl.mozilla.org/?tree=Try&rev=5fd4a3198aca
Assignee | ||
Comment 67•10 years ago
|
||
try |
(In reply to Botond Ballo [:botond] from comment #66) > Try push: https://tbpl.mozilla.org/?tree=Try&rev=5fd4a3198aca There turned out to be a few problems: - I again used MOZ_BEGIN_ENUM_CLASS at class scope instead of MOZ_BEGIN_NESTED_ENUM_CLASS. - To be able to serialize APZStateChange with TypedEnumSerializer, I had to add a sentinel value to it. That caused some compilers to complain about switch statements that handle all the values except the sentinel and don't have a 'default' case. - GeckoContentController.h including IPCMessageUtils.h to bring in TypedEnumSerializer turned out to be a problem, because GeckoCC.h is included from some Android files, and Android isn't configured to include IPC stuff. I had to move the ParamTraits specialization into GfxMessageUtils.h after all. - There was a bug in TypedEnumSerializer where it would cause a Werror if the enum's underlying type was unsigned and the lowest value was 0. I filed this as bug 995510 and fixed it. ReTry with bug 995510 patch included: https://tbpl.mozilla.org/?tree=Try&rev=17ab37ba120d
Assignee | ||
Comment 68•10 years ago
|
||
Updated Part 2 patch that fixes the other three problems from the previous comment. Carrying r+.
Attachment #8405560 -
Attachment is obsolete: true
Attachment #8405692 -
Flags: review+
Assignee | ||
Comment 69•10 years ago
|
||
try |
The bug 995510 patch didn't fix the compiler error it was meant to fix. On second look, bug 995510 doesn't seem to be easily fixable, so instead I worked around it by making the underlying type of APZStateChange signed. Trying again: https://tbpl.mozilla.org/?tree=Try&rev=d54c017c5fcc
Attachment #8405568 -
Flags: review?(21) → review+
Assignee | ||
Comment 70•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #69) > The bug 995510 patch didn't fix the compiler error it was meant to fix. On > second look, bug 995510 doesn't seem to be easily fixable, so instead I > worked around it by making the underlying type of APZStateChange signed. > > Trying again: https://tbpl.mozilla.org/?tree=Try&rev=d54c017c5fcc The gtests failures had two causes: - A typo in the Part 5 patch (I accidentally replaced a use of 'nsEventStatus_eConsumeNoDefault' with 'nsEventStatus_eConsumeDoDefault'). - The APZ gtests were calling SetAllowedTouchBehaviour() for a touch block _before_ sending the touch-start for the touch block, when it really should called _after_. I will post a Part 13 patch that fixes this. The other test failures look unrelated.
Assignee | ||
Comment 71•10 years ago
|
||
Fixed typo that was mentioned in comment 70. Carrying r+.
Attachment #8404253 -
Attachment is obsolete: true
Attachment #8406327 -
Flags: review+
Assignee | ||
Comment 72•10 years ago
|
||
Attachment #8406331 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 73•10 years ago
|
||
green try |
ReTrying: https://tbpl.mozilla.org/?tree=Try&rev=fde25f6e0f84
Updated•10 years ago
|
Attachment #8406331 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 74•10 years ago
|
||
landing |
(In reply to Botond Ballo [:botond] from comment #73) > ReTrying: https://tbpl.mozilla.org/?tree=Try&rev=fde25f6e0f84 Clean! Landed: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=037f46cdfd80
Comment 75•10 years ago
|
||
backout |
Backed out for build failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=037f46cdfd80 remote: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=d481f89fee96
Assignee | ||
Comment 76•10 years ago
|
||
landing |
(In reply to Ed Morley [:edmorley UTC+0] from comment #75) > Backed out for build failures: > https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=037f46cdfd80 > > remote: > https://hg.mozilla.org/integration/mozilla-inbound/ > pushloghtml?changeset=d481f89fee96 TypedEnumSerializer was renamed to ContiguousTypedEnumSerializer in between my Try push and my landing. Fixed and relanded: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=e134b3aa718f
Comment 77•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4680398c9651 https://hg.mozilla.org/mozilla-central/rev/5a11ff4851a3 https://hg.mozilla.org/mozilla-central/rev/1eb0c794e09c https://hg.mozilla.org/mozilla-central/rev/50ee3a60f9eb https://hg.mozilla.org/mozilla-central/rev/a1a415eab04c https://hg.mozilla.org/mozilla-central/rev/863b6b28a775 https://hg.mozilla.org/mozilla-central/rev/14804946cd13 https://hg.mozilla.org/mozilla-central/rev/3802596aa078 https://hg.mozilla.org/mozilla-central/rev/35dd4b9b5caa https://hg.mozilla.org/mozilla-central/rev/23b5df89a997 https://hg.mozilla.org/mozilla-central/rev/6ca74f1d61b1 https://hg.mozilla.org/mozilla-central/rev/31631e0d4ada https://hg.mozilla.org/mozilla-central/rev/e134b3aa718f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•