Closed Bug 976605 Opened 6 years ago Closed 6 years ago

move active element from BrowserElementPanning.js to C++

Categories

(Core :: Panning and Zooming, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bkelly, Assigned: botond)

References

(Blocks 1 open bug)

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.
See Also: → 972081
Nice - let's record what kind of improvement we get (perceptual counts as well :)
blocking-b2g: 1.4? → 1.4+
(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=]
Priority: -- → P1
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)
Whiteboard: [c= p=4 s= u=] → [c= p=4 s= u=1.4]
Blocks: 966946
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.
Flags: needinfo?(21)
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)
(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)
(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)
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
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.
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+ → ---
Make sure we ask for uplift when we fix it though.
Priority: P1 → P2
Whiteboard: [c= p=4 s= u=1.4] → [c=handeye p= s= u=]
(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.
(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.
(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.
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
(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)
See Also: → 967884
No longer blocks: 985551
Attachment #8396032 - Attachment is obsolete: true
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).
Attachment #8403619 - Attachment is obsolete: true
Attachment #8404243 - Flags: review?(bugmail.mozilla)
Attachment #8403621 - Attachment is obsolete: true
Attachment #8404252 - Flags: review?(bugmail.mozilla)
Attachment #8403622 - Attachment is obsolete: true
Attachment #8404253 - Flags: review?(bugmail.mozilla)
Attachment #8403623 - Attachment is obsolete: true
Attachment #8404254 - Flags: review?(bugmail.mozilla)
Attachment #8403626 - Attachment is obsolete: true
Attachment #8404256 - Flags: review?(bugmail.mozilla)
Attachment #8404257 - Flags: review?(ehsan)
Attachment #8403627 - Attachment is obsolete: true
Attachment #8404258 - Flags: review?(bugmail.mozilla)
Attachment #8403629 - Attachment is obsolete: true
Attachment #8404259 - Flags: review?(bugmail.mozilla)
Attachment #8404260 - Flags: review?(bugmail.mozilla)
Here is the completed patch set, posted for review. I tested in on http://jsbin.com/vorib and it seems to be working well.
Attachment #8404243 - Flags: review?(bugmail.mozilla) → review+
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+
Attachment #8404251 - Flags: review?(bugmail.mozilla) → review+
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 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+
Attachment #8404253 - Flags: review?(bugmail.mozilla) → review+
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+
Attachment #8404255 - Flags: review?(bugmail.mozilla) → review+
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
(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 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 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+
Attachment #8404260 - Flags: review?(bugmail.mozilla)
Attachment #8404260 - Flags: review?(21)
Attachment #8404260 - Flags: review+
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 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+
(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.
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.
(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.
(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.
(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.
Addressed review comments. Carrying r+.
Attachment #8404243 - Attachment is obsolete: true
Attachment #8405559 - Flags: review+
Addressed review comments. Carrying r+.
Attachment #8404247 - Attachment is obsolete: true
Attachment #8405560 - Flags: review+
Addressed review comments. Carrying r+.
Attachment #8404252 - Attachment is obsolete: true
Attachment #8405561 - Flags: review+
Addressed review comments. Carrying r+.
Attachment #8404254 - Attachment is obsolete: true
Attachment #8405562 - Flags: review+
Addressed review comments.
Attachment #8404256 - Attachment is obsolete: true
Attachment #8405563 - Flags: review?(bugmail.mozilla)
Addressed review comments. Carrying r+.
Attachment #8404257 - Attachment is obsolete: true
Attachment #8405564 - Flags: review+
Addressed review comments. Carrying r+.
Attachment #8404258 - Attachment is obsolete: true
Attachment #8405565 - Flags: review+
Addressed review comments. Carrying r+.
Attachment #8404259 - Attachment is obsolete: true
Attachment #8405566 - Flags: review+
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 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+
(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.
Depends on: 995510
(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
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+
No longer depends on: 995510
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
(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.
Fixed typo that was mentioned in comment 70. Carrying r+.
Attachment #8404253 - Attachment is obsolete: true
Attachment #8406327 - Flags: review+
Attachment #8406331 - Flags: review?(bugmail.mozilla) → review+
(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
Depends on: 997287
Depends on: 997878
No longer depends on: 997878
You need to log in before you can comment on or make changes to this bug.