1127066 - Complete the root-process widget support for APZ

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Bug 1005815 implements enough of ChromeProcessController to make it viable to start using it in the B2G parent process.

However, it needs more work to reach feature parity with RemoteContentController/TabChild. This bug is to track that work.

Comment 1

[Botond Ballo [:botond]]

Attachment: Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame

Comment 2

[Botond Ballo [:botond]]

Attachment: Part 2 - Fold APZCCallbackHelper::UpdateCallbackTransform into UpdateFrameCommon

Comment 3

[Botond Ballo [:botond]]

I have an initial batch of patches ready. There is more to be done, but we might as well start getting what's done reviewed and landed.

Comment 4

[Botond Ballo [:botond]]

Attachment: Part 3 - Extract a helper to apply the APZ callback transform to a touch event

From here on the patches apply on top of those in bug 930939.

If we decide to land this before bug 930939, I can rebase them.

Comment 5

[Botond Ballo [:botond]]

Attachment: Part 4 - Apply the APZ callback transform to touch events targeted to the root process

Comment 6

[Botond Ballo [:botond]]

Attachment: Part 5 - Extract SendSetTargetAPZCNotification and its helpers into APZCCallbackHelper

The use of callbacks can be made considerably nicer in the future with better C++ standard support:

  - Something like std::function would allow us to avoid the tailor-made
    callback base class SetTargetAPZCCallback.

  - Lambdas would allow us to avoid having to write out-of-line classes
    that implement the callbacks.

Comment 7

[Botond Ballo [:botond]]

Attachment: Part 6 - Implement proper sending of target-apzc notification and creation of displayport in the root process

Comment 8

[Botond Ballo [:botond]]

Somewhat amusingly for a bug titled "Complete the ChromeProcessController implementation", the patches so far manage not to touch ChromeProcessController at all :)

While later patches will do so, I'm going to rename the bug to reflect its scope more accurately.

Comment 9

[Botond Ballo [:botond]]

Attachment: Part 3 - Extract a helper to apply the APZ callback transform to a touch event

Whoops, had a silly mistake in there.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8556185 [details] [diff] [review]
Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame

Review of attachment 8556185 [details] [diff] [review]:
-----------------------------------------------------------------

The order of operations is important, and this refactoring changes that. For example, the scroll-position-clamping-scroll-port-size needs to be set before the scroll position, otherwise the scroll position may get clamped incorrectly. I'd rather see the two individual operations (setting the scroll position, and setting the displayport margins) extracted into separate functions for reuse.

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +120,5 @@
> +  // Precondition checks
> +  MOZ_ASSERT(aUtils);
> +  MOZ_ASSERT(aMetrics.GetUseDisplayPortMargins());
> +  if (aMetrics.GetScrollId() == FrameMetrics::NULL_SCROLL_ID) {
> +      return;

Stick to two or four-space indent

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8556186 [details] [diff] [review]
Part 2 - Fold APZCCallbackHelper::UpdateCallbackTransform into UpdateFrameCommon

Review of attachment 8556186 [details] [diff] [review]:
-----------------------------------------------------------------

This will probably change somewhat due to part 1, so I'll review it after that.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8560091 [details] [diff] [review]
Part 4 - Apply the APZ callback transform to touch events targeted to the root process

Review of attachment 8560091 [details] [diff] [review]:
-----------------------------------------------------------------

Watch out for the changes dvander is making in bug 1126090 also.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8560093 [details] [diff] [review]
Part 5 - Extract SendSetTargetAPZCNotification and its helpers into APZCCallbackHelper

Review of attachment 8560093 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine but I'm concerned about the reference cycle. f+ for now.

::: dom/ipc/TabChild.cpp
@@ +873,5 @@
>    , mTouchEndCancelled(false)
>    , mEndTouchIsClick(false)
>    , mIgnoreKeyPressEvent(false)
>    , mActiveElementManager(new ActiveElementManager())
> +  , mSetTargetAPZCCallback(new TabChildSetTargetAPZCCallback(this))

Doesn't this set up a TabChild <-> TabChildSetTargetAPZCCallback reference cycle that you need to tell the cycle collector about?

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +577,5 @@
> +      }
> +      // TODO: Do other types of events need to be handled?
> +
> +      if (!targets.IsEmpty()) {
> +        layers::SendSetTargetAPZCNotification(shell, aInputBlockId, targets,

I'd rather you renamed the helper and drop the scoping

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8560128 [details] [diff] [review]
Part 3 - Extract a helper to apply the APZ callback transform to a touch event

Review of attachment 8560128 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/APZCCallbackHelper.h
@@ +97,5 @@
>  
> +    /* Convenience function for applying a callback transform to all touch
> +     * points of a touch event.
> +     * Note: despite the 'const' on |aEvent|, this function conceptually
> +     * modifies it. */

This seems bad. We should make this non-const, and change the callsite in TabChild to make the "localEvent" copy earlier and call this function on that copy. Also then SendSetTargetAPZCNotification would take the callback-transformed localEvent. I'm ok with deferring that to a follow-up though since it's a pre-existing issue.

Comment 15

[Botond Ballo [:botond]]

This is a Try push for the original patches that shows a number of problems: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7149d26830eb

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #15)
> This is a Try push for the original patches that shows a number of problems:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7149d26830eb

Specifically:

  - As a reference-counted base class, SetTargetAPZCCallback's destructor
    needs to be private.

  - SetTargetAPZCCallback is defined as a struct, but it's forward-declared
    as a class in nsBaseWidget.h.

  - The refactoring in Part 1 introduced a call to nsLayoutUtils::FindContentFor() 
    with NULL_SCROLL_ID, but FindContentFor() asserts on that.

This will all be fixed in the updated patches.

Comment 17

[Botond Ballo [:botond]]

Attachment: Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> The order of operations is important, and this refactoring changes that. For
> example, the scroll-position-clamping-scroll-port-size needs to be set
> before the scroll position, otherwise the scroll position may get clamped
> incorrectly.

This patch fixes this problem by moving the call to UpdateFrameCommon after the call to SetScrollPositionClampingScrollPortSize.

> I'd rather see the two individual operations (setting the
> scroll position, and setting the displayport margins) extracted into
> separate functions for reuse.

I'll post an alternative that does this.

> ::: gfx/layers/apz/util/APZCCallbackHelper.cpp
> @@ +120,5 @@
> > +  // Precondition checks
> > +  MOZ_ASSERT(aUtils);
> > +  MOZ_ASSERT(aMetrics.GetUseDisplayPortMargins());
> > +  if (aMetrics.GetScrollId() == FrameMetrics::NULL_SCROLL_ID) {
> > +      return;
> 
> Stick to two or four-space indent

Fixed (sticking to two).

Comment 18

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #17)
> Created attachment 8560643 [details] [diff] [review]
> Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and
> UpdateSubFrame

This update also fixes the problem of calling FindContentFor() on NULL_SCROLL_ID.

Comment 19

[Botond Ballo [:botond]]

Attachment: Part 2 - Fold APZCCallbackHelper::UpdateCallbackTransform into UpdateFrameCommon

Comment 20

[Botond Ballo [:botond]]

Attachment: [Alterantive] Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame

Alternative version of Part 1 which splits UpdateFrameCommon into ScrollFrame and SetDisplayPortMargins.

I'll let you pick which you like better; I slightly prefer UpdateFrameCommon, but I don't feel strongly about it.

Comment 21

[Botond Ballo [:botond]]

Attachment: [Alternative] Part 2 - Fold APZCCallbackHelper::UpdateCallbackTransform into ScrollFrame

Comment 22

[Botond Ballo [:botond]]

Attachment: Part 3 - Extract a helper to apply the APZ callback transform to a touch event

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> > +    /* Convenience function for applying a callback transform to all touch
> > +     * points of a touch event.
> > +     * Note: despite the 'const' on |aEvent|, this function conceptually
> > +     * modifies it. */
> 
> This seems bad. We should make this non-const, and change the callsite in
> TabChild to make the "localEvent" copy earlier and call this function on
> that copy. Also then SendSetTargetAPZCNotification would take the
> callback-transformed localEvent. I'm ok with deferring that to a follow-up
> though since it's a pre-existing issue.

I just went ahead and did this. Carrying r+.

Comment 23

[Botond Ballo [:botond]]

Attachment: Part 5 - Extract SendSetTargetAPZCNotification and its helpers into APZCCallbackHelper

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> ::: dom/ipc/TabChild.cpp
> @@ +873,5 @@
> >    , mTouchEndCancelled(false)
> >    , mEndTouchIsClick(false)
> >    , mIgnoreKeyPressEvent(false)
> >    , mActiveElementManager(new ActiveElementManager())
> > +  , mSetTargetAPZCCallback(new TabChildSetTargetAPZCCallback(this))
> 
> Doesn't this set up a TabChild <-> TabChildSetTargetAPZCCallback reference
> cycle that you need to tell the cycle collector about?

Good point!

I opted to use a weak pointer instead of the cycle collector, following how DelayedFireSingleTapEvent did it before bug 1005815 (and how it still does now, but with the widget instead of the tab child).

> ::: gfx/layers/apz/util/APZCCallbackHelper.cpp
> @@ +577,5 @@
> > +      }
> > +      // TODO: Do other types of events need to be handled?
> > +
> > +      if (!targets.IsEmpty()) {
> > +        layers::SendSetTargetAPZCNotification(shell, aInputBlockId, targets,
> 
> I'd rather you renamed the helper and drop the scoping

Done.

Comment 24

[Botond Ballo [:botond]]

Attachment: Part 6 - Implement proper sending of target-apzc notification and creation of displayport in the root process

Fixed the struct-forward-declared-as-class issue. Carrying r+.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8560646 [details] [diff] [review]
[Alterantive] Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame

Review of attachment 8560646 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +115,5 @@
> + * between the requested and actual scroll positions.
> + */
> +static void
> +ScrollFrame(nsIContent* aContent,
> +            FrameMetrics& aMetrics)

aContent isn't used in this function, you can remove the parameter.

@@ +192,4 @@
>  
> +  nsIContent* content = nsLayoutUtils::FindContentFor(aMetrics.GetScrollId());
> +  ScrollFrame(content, aMetrics);
> +  SetDisplayPortMargins(aUtils, content, aMetrics);

Just to be safe can we move this SetDisplayPortMargins() below the SetResolutionAndScaleTo()? I'm not 100% sure that there isn't an order-of-operations dependency here and it would be safer to keep the ordering the same as it was before. Note also that you can move the FindContentFor down with the SetDisplayPortMargins() since the content isn't used in ScrollFrame().

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8560643 [details] [diff] [review]
Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame

Review of attachment 8560643 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah I definitely prefer the alternate version.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8560647 [details] [diff] [review]
[Alternative] Part 2 - Fold APZCCallbackHelper::UpdateCallbackTransform into ScrollFrame

Review of attachment 8560647 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +154,5 @@
>  SetDisplayPortMargins(nsIDOMWindowUtils* aUtils,
>                        nsIContent* aContent,
>                        FrameMetrics& aMetrics)
>  {
> +  // Set the displayport.

This comment can be removed entirely, the new function name is self-documenting.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8560650 [details] [diff] [review]
Part 5 - Extract SendSetTargetAPZCNotification and its helpers into APZCCallbackHelper

Review of attachment 8560650 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming (as before) that the code is mostly just moved over. I looked at the entry/exit points to review changes there but otherwise didn't review the stuff that was moved wholesale.

::: dom/ipc/TabChild.cpp
@@ +837,5 @@
>  
> +class TabChildSetTargetAPZCCallback : public SetTargetAPZCCallback {
> +public:
> +  TabChildSetTargetAPZCCallback(TabChild* aTabChild)
> +    : mTabChild(do_GetWeakReference(static_cast<nsITabChild*>(aTabChild)))

Is the static_cast necessary? I'm assuming yes or it wouldn't be there but if it's removable without the compiler complaining it would be nice to do that.

Comment 29

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> Assuming (as before) that the code is mostly just moved over. I looked at
> the entry/exit points to review changes there but otherwise didn't review
> the stuff that was moved wholesale.

I generally just moved the functions over and only made adjustments to get things wired up properly.

I did combine a call site to SendSetTargetAPZCNotification() in TabChild (for touch events) and a place where the helpers were called directly (for wheel events) together into the factored-out SendSetTargetAPZCNotification(), which now takes a WidgetGUIEvent rather than a WidgetTouchEvent.

> 
> ::: dom/ipc/TabChild.cpp
> @@ +837,5 @@
> >  
> > +class TabChildSetTargetAPZCCallback : public SetTargetAPZCCallback {
> > +public:
> > +  TabChildSetTargetAPZCCallback(TabChild* aTabChild)
> > +    : mTabChild(do_GetWeakReference(static_cast<nsITabChild*>(aTabChild)))
> 
> Is the static_cast necessary? I'm assuming yes or it wouldn't be there but
> if it's removable without the compiler complaining it would be nice to do
> that.

It's necessary. The compiler gives the following error without it:

./../../dom/ipc/TabChild.cpp: In constructor 'TabChildSetTargetAPZCCallback::TabChildSetTargetAPZCCallback(mozilla::dom::TabChild*)':
../../../dom/ipc/TabChild.cpp:841:46: error: 'nsISupports' is an ambiguous base of 'mozilla::dom::TabChild'

Comment 30

[Botond Ballo [:botond]]

Attachment: Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> ::: gfx/layers/apz/util/APZCCallbackHelper.cpp
> @@ +115,5 @@
> > + * between the requested and actual scroll positions.
> > + */
> > +static void
> > +ScrollFrame(nsIContent* aContent,
> > +            FrameMetrics& aMetrics)
> 
> aContent isn't used in this function, you can remove the parameter.

aContent will be used in Part 2 to set the callback-transform so I left it in. Confirmed on IRC, carrying r+.

> > +  nsIContent* content = nsLayoutUtils::FindContentFor(aMetrics.GetScrollId());
> > +  ScrollFrame(content, aMetrics);
> > +  SetDisplayPortMargins(aUtils, content, aMetrics);
> 
> Just to be safe can we move this SetDisplayPortMargins() below the
> SetResolutionAndScaleTo()? I'm not 100% sure that there isn't an
> order-of-operations dependency here and it would be safer to keep the
> ordering the same as it was before. 

Done.

> Note also that you can move the
> FindContentFor down with the SetDisplayPortMargins() since the content isn't
> used in ScrollFrame().

(This is moot given that we're keeping the aContent parameter.)

Comment 31

[Botond Ballo [:botond]]

Attachment: Part 2 - Fold APZCCallbackHelper::UpdateCallbackTransform into ScrollFrame

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #27)
> ::: gfx/layers/apz/util/APZCCallbackHelper.cpp
> @@ +154,5 @@
> >  SetDisplayPortMargins(nsIDOMWindowUtils* aUtils,
> >                        nsIContent* aContent,
> >                        FrameMetrics& aMetrics)
> >  {
> > +  // Set the displayport.
> 
> This comment can be removed entirely, the new function name is
> self-documenting.

Done.

I also enclosed the setting of the callback-transform in ScrollFrame() in an 'if (aContent)' block, as I realized that it was previously protected by such a check in the code that has now moved to SetDisplayPortMargins().

Comment 32

[Botond Ballo [:botond]]

Try push with revised patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=745f893dd384

Comment 33

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #32)
> Try push with revised patches:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=745f893dd384

ReTrying with constructors made explicit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca144f0ffade

Comment 34

[Botond Ballo [:botond]]

Attachment: Part 7 - Extract an APZEventState class from TabChild

Posting some more patches. They still need a bit more work and testing before being reviewed.

Comment 35

[Botond Ballo [:botond]]

Attachment: Part 8 - Use APZEventState in nsBaseWidget

This patch currently introduces a reference cycle between nsBaseWidget and APZEventState which I need to figure out how to break.

Comment 36

[Botond Ballo [:botond]]

Attachment: Part 9 - Use (nsBaseWidget's) APZEventState in ChromeProcessController

Comment 37

[Botond Ballo [:botond]]

Attachment: Part 7 - Extract an APZEventState class from TabChild

I fixed the reference cycle and some other issues that I discovered during testing. Posting updated patches for review.

Comment 38

[Botond Ballo [:botond]]

Attachment: Part 8 - Use a weak reference to the widget in APZEventState to avoid reference cycles

There are no reference cycles just yet, but there will be once we start using APZEventState in nsBaseWidget. This patch anticipates that.

Comment 39

[Botond Ballo [:botond]]

Attachment: Part 9 - Complain loudly if APZEventState is used with a widget that doesn't support weak references

Not all widget types support weak references. (For example, gonk's nsWindow doesn't, until my next patch that is.) As we expand the use of ChromeProcessController to more widgets, we want to make sure that a particular widget not supporting weak references is caught early with an assertion, rather than causing silent failures.

Comment 40

[Botond Ballo [:botond]]

Attachment: Part 10 - Add weak reference support to the gonk nsWindow type

This makes APZEventState usable on gonk.

Comment 41

[Botond Ballo [:botond]]

Attachment: Part 11 - Use APZEventState in nsBaseWidget

Comment 42

[Botond Ballo [:botond]]

Attachment: Part 12 - Use (nsBaseWidget's) APZEventState in ChromeProcessController

Comment 43

[Botond Ballo [:botond]]

Try push with all patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5888180791d5

Comment 44

[Michael Wu [:mwu]]

Comment on attachment 8561538 [details] [diff] [review]
Part 10 - Add weak reference support to the gonk nsWindow type

Review of attachment 8561538 [details] [diff] [review]:
-----------------------------------------------------------------

I'll let kats make the call on this one since he has more context on this.

Comment 45

[Botond Ballo [:botond]]

Comment on attachment 8561531 [details] [diff] [review]
Part 7 - Extract an APZEventState class from TabChild

Try push turned up a few compiler errors, will fix and re-post.

Comment 46

[Botond Ballo [:botond]]

Attachment: Part 7 - Extract an APZEventState class from TabChild

Updated to make destructor private. I also moved it out of line and consequently was able to avoid including ActiveElementManager.h).

Comment 47

[Botond Ballo [:botond]]

Attachment: Part 8 - Use a weak reference to the widget in APZEventState to avoid reference cycles

Updated to fix include-what-you-use errors.

Comment 48

[Botond Ballo [:botond]]

Attachment: Part 12 - Use (nsBaseWidget's) APZEventState in ChromeProcessController

Updated to fix include-what-you-use errors.

Comment 49

[Botond Ballo [:botond]]

Attachment: Part 13 - Implement ChromeProcessController::HandleLongTapUp()

This leaves just two GeckoCC methods unimplemented in ChromeProcessController:

  1) HandleDoubleTap

     I'm not inclined to implement this right now, because APZ only calls it
     for scroll frames for which double-tap zoom is allowed, which is currently
     just the RCD-RSF, which does not live in the parent process.

     Even if for some reason we do want this implemented, I'd rather wait until
     the code in BrowserElementPanning.js for processing the "Gesture:DoubleTap"
     message is ported to C++ and then call that, rather than port the existing
     TabChild code using message managers and such to ChromePC.

     I'm open to being convinced otherwise on either count.

  2) SendAsyncScrollDOMEvent

     I don't understand this one very well. Kats, do you know if this is
     something we need/want in the parent process?

Comment 50

[Botond Ballo [:botond]]

Attachment: Part 12 - Use (nsBaseWidget's) APZEventState in ChromeProcessController

Gah, I had another compiler error in here: I mis-fowrarddeclared APZEventState in the global namespace in ChromeProcessController.h. GCC tolerates that, but not MSVC.

New Try push, this time including Part 13 as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3f00236fd7a

Comment 51

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8561580 [details] [diff] [review]
Part 7 - Extract an APZEventState class from TabChild

Review of attachment 8561580 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +2061,5 @@
>  TabChild::RecvHandleSingleTap(const CSSPoint& aPoint, const ScrollableLayerGuid& aGuid)
>  {
>    TABC_LOG("Handling single tap at %s on %s with %p %p %d\n",
>      Stringify(aPoint).c_str(), Stringify(aGuid).c_str(), mGlobal.get(),
>      mTabChildGlobal.get(), mTouchEndCancelled);

I would like to move this log into ProcessSingleTap as well. That way we get it for ChromeProcessController as well, and it's more unified (i.e. only need to turn on one file's logging). You can omit the mGlobal and mTabChildGlobal stuff from the log, as I don't think I've ever run into a case where they are null. I'm fine with doing this in a follow-up patch or bug as it's minor.

@@ +2073,5 @@
>  bool
>  TabChild::RecvHandleLongTap(const CSSPoint& aPoint, const ScrollableLayerGuid& aGuid, const uint64_t& aInputBlockId)
>  {
>    TABC_LOG("Handling long tap at %s with %p %p\n",
>      Stringify(aPoint).c_str(), mGlobal.get(), mTabChildGlobal.get());

Ditto on logging

::: gfx/layers/apz/util/APZCCallbackHelper.h
@@ +125,5 @@
>                                                         const LayoutDevicePoint& aRefPoint,
>                                                         nsIWidget* aWidget);
>  
> +    /* Dispatch a mouse event with the given parameters.
> +     * Return whether or not the page has called preventDefault on the event. */

s/the page has/any listeners have/

::: gfx/layers/apz/util/APZEventState.h
@@ +13,5 @@
> +#include "mozilla/EventForwards.h"
> +#include "mozilla/layers/GeckoContentController.h"  // for APZStateChange
> +#include "nsCOMPtr.h"
> +#include "nsRefPtr.h"
> +#include "nsISupportsImpl.h"  // for NS_INLINE_DECL_REFCOUNTING

Alphabetize includes

Comment 52

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8561581 [details] [diff] [review]
Part 8 - Use a weak reference to the widget in APZEventState to avoid reference cycles

Review of attachment 8561581 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/APZEventState.h
@@ +79,5 @@
>    uint64_t mPendingTouchPreventedBlockId;
>    bool mEndTouchIsClick;
>    bool mTouchEndCancelled;
> +
> +  already_AddRefed<nsIWidget> GetWidget() const;

Move this up to be with the other private functions

Comment 53

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8561538 [details] [diff] [review]
Part 10 - Add weak reference support to the gonk nsWindow type

Review of attachment 8561538 [details] [diff] [review]:
-----------------------------------------------------------------

Would it be better to put this on nsBaseWidget? Or do all the concrete subclasses have to implement it individually?

Comment 54

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8560652 [details] [diff] [review]
Part 6 - Implement proper sending of target-apzc notification and creation of displayport in the root process

Review of attachment 8560652 [details] [diff] [review]:
-----------------------------------------------------------------

Some more comments for part 6, sorry

::: widget/nsBaseWidget.cpp
@@ +941,5 @@
>    nsRefPtr<GeckoContentController> controller = new ChromeProcessController(this);
>    return controller.forget();
>  }
>  
> +class ParentProcessSetTargetAPZCCallback : public SetTargetAPZCCallback {

Probably better to rename this s/Parent/Chrome/ as well

@@ +949,5 @@
> +  {}
> +
> +  void Run(uint64_t aInputBlockId, const nsTArray<ScrollableLayerGuid>& aTargets) const MOZ_OVERRIDE {
> +    // need a local var to disambiguate between the SetTargetAPZC overloads.
> +    void (APZCTreeManager::*setTargetApzcFunc)(uint64_t, const nsTArray<ScrollableLayerGuid>&)

add a MOZ_ASSERT for main-thread here

Comment 55

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8561540 [details] [diff] [review]
Part 11 - Use APZEventState in nsBaseWidget

Review of attachment 8561540 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsBaseWidget.cpp
@@ +960,5 @@
>  private:
>    nsRefPtr<APZCTreeManager> mTreeManager;
>  };
>  
> +class ParentProcessContentReceivedInputBlockCallback : public ContentReceivedInputBlockCallback {

For consistency with ChromeProcessController it might be better to call this ChromeProcessContentReceivedInputBlockCallback

@@ +967,5 @@
> +    : mTreeManager(aTreeManager)
> +  {}
> +
> +  void Run(const ScrollableLayerGuid& aGuid, uint64_t aInputBlockId, bool aPreventDefault) const MOZ_OVERRIDE {
> +    APZThreadUtils::RunOnControllerThread(NewRunnableMethod(

add a MOZ_ASSERT for main thread here

Comment 56

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8561730 [details] [diff] [review]
Part 13 - Implement ChromeProcessController::HandleLongTapUp()

Review of attachment 8561730 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +2086,5 @@
>  bool
>  TabChild::RecvHandleLongTapUp(const CSSPoint& aPoint, const ScrollableLayerGuid& aGuid)
>  {
> +  TABC_LOG("Handling long tap up at %s with %p %p\n",
> +    Stringify(aPoint).c_str(), mGlobal.get(), mTabChildGlobal.get());

Move the logging into APZEventState::ProcessLongTapUp

Comment 57

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #49)
> Created attachment 8561730 [details] [diff] [review]
> Part 13 - Implement ChromeProcessController::HandleLongTapUp()
> 
> This leaves just two GeckoCC methods unimplemented in
> ChromeProcessController:
> 
>   1) HandleDoubleTap
> 
>      I'm not inclined to implement this right now, because APZ only calls it
>      for scroll frames for which double-tap zoom is allowed, which is
> currently
>      just the RCD-RSF, which does not live in the parent process.

Sounds good to me.

>   2) SendAsyncScrollDOMEvent
> 
>      I don't understand this one very well. Kats, do you know if this is
>      something we need/want in the parent process?

As far as I can tell this triggers a message to the gaia system process (in the root b2g process) informing it when the scroll position of the root scrollable in a child process changes. So we don't need to do anything with this in ChromePC, and in fact I'd like to get rid of this callback entirely if possible. If nothing else we should be able infer it from repaint requests. That can be done later though, it's not so important.

Comment 58

[Botond Ballo [:botond]]

Attachment: Part 6 - Implement proper sending of target-apzc notification and creation of displayport in the root process

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #54)
> ::: widget/nsBaseWidget.cpp
> @@ +941,5 @@
> >    nsRefPtr<GeckoContentController> controller = new ChromeProcessController(this);
 >    return controller.forget();
> >  }
> >  
> > +class ParentProcessSetTargetAPZCCallback : public SetTargetAPZCCallback {
> 
> Probably better to rename this s/Parent/Chrome/ as well

Done.

> @@ +949,5 @@
> > +  {}
> > +
> > +  void Run(uint64_t aInputBlockId, const nsTArray<ScrollableLayerGuid>& aTargets) const MOZ_OVERRIDE {
> > +    // need a local var to disambiguate between the SetTargetAPZC overloads.
> > +    void (APZCTreeManager::*setTargetApzcFunc)(uint64_t, const nsTArray<ScrollableLayerGuid>&)
> 
> add a MOZ_ASSERT for main-thread here

Done.

Carrying r+.

Comment 59

[Botond Ballo [:botond]]

Attachment: Part 7 - Extract an APZEventState class from TabChild

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #51)
> ::: dom/ipc/TabChild.cpp
> @@ +2061,5 @@
> >  TabChild::RecvHandleSingleTap(const CSSPoint& aPoint, const ScrollableLayerGuid& aGuid)
> >  {
> >    TABC_LOG("Handling single tap at %s on %s with %p %p %d\n",
> >      Stringify(aPoint).c_str(), Stringify(aGuid).c_str(), mGlobal.get(),
> >      mTabChildGlobal.get(), mTouchEndCancelled);
> 
> I would like to move this log into ProcessSingleTap as well. That way we get
> it for ChromeProcessController as well, and it's more unified (i.e. only
> need to turn on one file's logging). You can omit the mGlobal and
> mTabChildGlobal stuff from the log, as I don't think I've ever run into a
> case where they are null. I'm fine with doing this in a follow-up patch or
> bug as it's minor.

I went ahead and did it.

> @@ +2073,5 @@
> >  bool
> >  TabChild::RecvHandleLongTap(const CSSPoint& aPoint, const ScrollableLayerGuid& aGuid, const uint64_t& aInputBlockId)
> >  {
> >    TABC_LOG("Handling long tap at %s with %p %p\n",
> >      Stringify(aPoint).c_str(), mGlobal.get(), mTabChildGlobal.get());
> 
> Ditto on logging

Done.

> ::: gfx/layers/apz/util/APZCCallbackHelper.h
> @@ +125,5 @@
> >                                                         const LayoutDevicePoint& aRefPoint,
> >                                                         nsIWidget* aWidget);
> >  
> > +    /* Dispatch a mouse event with the given parameters.
> > +     * Return whether or not the page has called preventDefault on the event. */
> 
> s/the page has/any listeners have/

Fixed.

> ::: gfx/layers/apz/util/APZEventState.h
> @@ +13,5 @@
> > +#include "mozilla/EventForwards.h"
> > +#include "mozilla/layers/GeckoContentController.h"  // for APZStateChange
> > +#include "nsCOMPtr.h"
> > +#include "nsRefPtr.h"
> > +#include "nsISupportsImpl.h"  // for NS_INLINE_DECL_REFCOUNTING
> 
> Alphabetize includes

Done.

Carrying r+.

Comment 60

[Botond Ballo [:botond]]

Attachment: Part 8 - Use a weak reference to the widget in APZEventState to avoid reference cycles

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #52)
> ::: gfx/layers/apz/util/APZEventState.h
> @@ +79,5 @@
> >    uint64_t mPendingTouchPreventedBlockId;
> >    bool mEndTouchIsClick;
> >    bool mTouchEndCancelled;
> > +
> > +  already_AddRefed<nsIWidget> GetWidget() const;
> 
> Move this up to be with the other private functions

Done.

Carrying r+.

Comment 61

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #53)
> Would it be better to put this on nsBaseWidget? Or do all the concrete
> subclasses have to implement it individually?

I think it would make sense to inherit nsBaseWidget, rather than each of its concrete clases, from nsSupportsWeakReference. I'll do this in a follow-up.

Comment 62

[Botond Ballo [:botond]]

Attachment: Part 11 - Use APZEventState in nsBaseWidget

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #55)
> ::: widget/nsBaseWidget.cpp
> @@ +960,5 @@
> >  private:
> >    nsRefPtr<APZCTreeManager> mTreeManager;
> >  };
> >  
> > +class ParentProcessContentReceivedInputBlockCallback : public ContentReceivedInputBlockCallback {
> 
> For consistency with ChromeProcessController it might be better to call this
> ChromeProcessContentReceivedInputBlockCallback

Done.


> @@ +967,5 @@
> > +    : mTreeManager(aTreeManager)
> > +  {}
> > +
> > +  void Run(const ScrollableLayerGuid& aGuid, uint64_t aInputBlockId, bool aPreventDefault) const MOZ_OVERRIDE {
> > +    APZThreadUtils::RunOnControllerThread(NewRunnableMethod(
> 
> add a MOZ_ASSERT for main thread here

Done.

Carrying r+.

Comment 63

[Botond Ballo [:botond]]

Attachment: Part 13 - Implement ChromeProcessController::HandleLongTapUp()

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #56)
> ::: dom/ipc/TabChild.cpp
> @@ +2086,5 @@
> >  bool
> >  TabChild::RecvHandleLongTapUp(const CSSPoint& aPoint, const ScrollableLayerGuid& aGuid)
> >  {
> > +  TABC_LOG("Handling long tap up at %s with %p %p\n",
> > +    Stringify(aPoint).c_str(), mGlobal.get(), mTabChildGlobal.get());
> 
> Move the logging into APZEventState::ProcessLongTapUp

Done.

Carrying r+.

Comment 64

[Botond Ballo [:botond]]

One more Try run, with all patches rebased to latest m-c, and all comments addressed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1202594bd9d

Comment 66

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #64)
> One more Try run, with all patches rebased to latest m-c, and all comments
> addressed:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1202594bd9d

Looking clean!

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=b9150539d787

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea7de6b88d85
https://hg.mozilla.org/mozilla-central/rev/7acb85b25fbb
https://hg.mozilla.org/mozilla-central/rev/5669834a9454
https://hg.mozilla.org/mozilla-central/rev/cc71f20bede1
https://hg.mozilla.org/mozilla-central/rev/4bd0d2b7b41d
https://hg.mozilla.org/mozilla-central/rev/c55657471570
https://hg.mozilla.org/mozilla-central/rev/135a42383535
https://hg.mozilla.org/mozilla-central/rev/9431d4193075
https://hg.mozilla.org/mozilla-central/rev/712021ac1b69
https://hg.mozilla.org/mozilla-central/rev/4f4de86d9c3f
https://hg.mozilla.org/mozilla-central/rev/0c3e7de46a88
https://hg.mozilla.org/mozilla-central/rev/40a4e0b2dd08
https://hg.mozilla.org/mozilla-central/rev/b9150539d787

Comment 68

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8560716 [details] [diff] [review]
Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): required for parent-process APZ (bug 950934) and input-thread (bug 930939) which are 2.2+ features. I'm tracking the full set of bugs that will need uplifting together at http://mzl.la/1zvKgmk; requesting approval on bugs individually but will wait until everything has approval before I uplift any of it.
User impact if declined: can't have parent-process APZ or input-thread
Testing completed: on master. some of these bugs have been baking for a while; others are more recent.
Risk to taking this patch (and alternatives if risky): there's likely some edge cases that we haven't run into yet and will be uncovered with further testing. Bug 1128887 is the only known issue that we don't yet have a fix for but I consider that relatively minor and something we can fix after uplifting everything else.
String or UUID changes made by this patch: none

Comment 70

[bhavana bajaj [:bajaj]]

kats in-future, a roll-up patch in these kinda cases is a lot helpful to me when doing  approvals as I have never had instances where i'd a- part's of attachments. Thanks !

Comment 71

[Kartikaya Gupta (email:[e-mail])]

(In reply to bhavana bajaj [:bajaj] from comment #70)
> kats in-future, a roll-up patch in these kinda cases is a lot helpful to me
> when doing  approvals as I have never had instances where i'd a- part's of
> attachments. Thanks !

Instead of creating a roll-up patch, wouldn't it be better to just move the flag from being a patch-level flag to a bug-level flag? It seems like a waste to create a roll-up because when I uplift I'll want to uplift the parts individually in case it needs to be bisected.

Comment 72

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #61)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #53)
> > Would it be better to put this on nsBaseWidget? Or do all the concrete
> > subclasses have to implement it individually?
> 
> I think it would make sense to inherit nsBaseWidget, rather than each of its
> concrete clases, from nsSupportsWeakReference. I'll do this in a follow-up.

Filed bug 1133150 for this.

Comment 73

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/pushloghtml?changeset=604fe707d582