If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Complete the root-process widget support for APZ

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: botond, Assigned: botond)

Tracking

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [input-thread-uplift-part9])

Attachments

(13 attachments, 22 obsolete attachments)

2.30 KB, patch
kats
: review+
Details | Diff | Splinter Review
4.52 KB, patch
botond
: review+
Details | Diff | Splinter Review
23.11 KB, patch
kats
: review+
Details | Diff | Splinter Review
10.53 KB, patch
botond
: review+
Details | Diff | Splinter Review
9.05 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.58 KB, patch
kats
: review+
Details | Diff | Splinter Review
2.02 KB, patch
kats
: review+
Details | Diff | Splinter Review
9.30 KB, patch
kats
: review+
Details | Diff | Splinter Review
8.03 KB, patch
botond
: review+
Details | Diff | Splinter Review
40.60 KB, patch
botond
: review+
Details | Diff | Splinter Review
6.87 KB, patch
botond
: review+
Details | Diff | Splinter Review
6.17 KB, patch
botond
: review+
Details | Diff | Splinter Review
6.08 KB, patch
botond
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 1005815
(Assignee)

Updated

3 years ago
Assignee: nobody → botond
(Assignee)

Comment 1

3 years ago
Created attachment 8556185 [details] [diff] [review]
Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame
(Assignee)

Comment 2

3 years ago
Created attachment 8556186 [details] [diff] [review]
Part 2 - Fold APZCCallbackHelper::UpdateCallbackTransform into UpdateFrameCommon
(Assignee)

Updated

3 years ago
Blocks: 1114409
Blocks: 1129081
Blocks: 1126090
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8556185 - Flags: review?(bugmail.mozilla)
(Assignee)

Updated

3 years ago
Attachment #8556186 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 4

3 years ago
Created attachment 8560089 [details] [diff] [review]
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.
Attachment #8560089 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 5

3 years ago
Created attachment 8560091 [details] [diff] [review]
Part 4 - Apply the APZ callback transform to touch events targeted to the root process
Attachment #8560091 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 6

3 years ago
Created attachment 8560093 [details] [diff] [review]
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.
Attachment #8560093 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 7

3 years ago
Created attachment 8560095 [details] [diff] [review]
Part 6 - Implement proper sending of target-apzc notification and creation of displayport in the root process
Attachment #8560095 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 8

3 years ago
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.
Summary: Complete the ChromeProcessController implementation → Complete the root-process widget support for APZ
(Assignee)

Comment 9

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

Whoops, had a silly mistake in there.
Attachment #8560089 - Attachment is obsolete: true
Attachment #8560089 - Flags: review?(bugmail.mozilla)
Attachment #8560128 - Flags: review?(bugmail.mozilla)
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
Attachment #8556185 - Flags: review?(bugmail.mozilla) → review-
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.
Attachment #8556186 - Flags: review?(bugmail.mozilla)
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.
Attachment #8560091 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #8560093 - Flags: review?(bugmail.mozilla) → feedback+
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.
Attachment #8560128 - Flags: review?(bugmail.mozilla) → review+
Attachment #8560095 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 15

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

Comment 16

3 years ago
(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.
(Assignee)

Comment 17

3 years ago
Created attachment 8560643 [details] [diff] [review]
Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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).
Attachment #8556185 - Attachment is obsolete: true
Attachment #8560643 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 18

3 years ago
(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.
(Assignee)

Comment 19

3 years ago
Created attachment 8560645 [details] [diff] [review]
Part 2 - Fold APZCCallbackHelper::UpdateCallbackTransform into UpdateFrameCommon
Attachment #8556186 - Attachment is obsolete: true
Attachment #8560645 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 20

3 years ago
Created attachment 8560646 [details] [diff] [review]
[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.
Attachment #8560646 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 21

3 years ago
Created attachment 8560647 [details] [diff] [review]
[Alternative] Part 2 - Fold APZCCallbackHelper::UpdateCallbackTransform into ScrollFrame
Attachment #8560647 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 22

3 years ago
Created attachment 8560649 [details] [diff] [review]
Part 3 - Extract a helper to apply the APZ callback transform to a touch event

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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+.
Attachment #8560128 - Attachment is obsolete: true
Attachment #8560649 - Flags: review+
(Assignee)

Comment 23

3 years ago
Created attachment 8560650 [details] [diff] [review]
Part 5 - Extract SendSetTargetAPZCNotification and its helpers into APZCCallbackHelper

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8560093 - Attachment is obsolete: true
Attachment #8560650 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 24

3 years ago
Created attachment 8560652 [details] [diff] [review]
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+.
Attachment #8560095 - Attachment is obsolete: true
Attachment #8560652 - Flags: review+
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().
Attachment #8560646 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8560643 - Flags: review?(bugmail.mozilla)
Attachment #8560645 - Flags: review?(bugmail.mozilla)
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.
Attachment #8560647 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8560650 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 29

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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'
(Assignee)

Updated

3 years ago
Attachment #8560643 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8560645 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
Created attachment 8560716 [details] [diff] [review]
Part 1 - Factor out code common to APZCCallbackHelper::UpdateRootFrame and UpdateSubFrame

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.)
Attachment #8560646 - Attachment is obsolete: true
Attachment #8560716 - Flags: review+
(Assignee)

Comment 31

3 years ago
Created attachment 8560720 [details] [diff] [review]
Part 2 - Fold APZCCallbackHelper::UpdateCallbackTransform into ScrollFrame

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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().
Attachment #8560647 - Attachment is obsolete: true
Attachment #8560720 - Flags: review+
(Assignee)

Comment 32

3 years ago
Try push with revised patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=745f893dd384
(Assignee)

Comment 33

3 years ago
(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
(Assignee)

Comment 34

3 years ago
Created attachment 8560790 [details] [diff] [review]
Part 7 - Extract an APZEventState class from TabChild

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

Comment 35

3 years ago
Created attachment 8560791 [details] [diff] [review]
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.
(Assignee)

Comment 36

3 years ago
Created attachment 8560792 [details] [diff] [review]
Part 9 - Use (nsBaseWidget's) APZEventState in ChromeProcessController
(Assignee)

Comment 37

3 years ago
Created attachment 8561531 [details] [diff] [review]
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.
Attachment #8560790 - Attachment is obsolete: true
Attachment #8561531 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 38

3 years ago
Created attachment 8561533 [details] [diff] [review]
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.
Attachment #8560791 - Attachment is obsolete: true
Attachment #8561533 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 39

3 years ago
Created attachment 8561537 [details] [diff] [review]
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.
Attachment #8560792 - Attachment is obsolete: true
Attachment #8561537 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 40

3 years ago
Created attachment 8561538 [details] [diff] [review]
Part 10 - Add weak reference support to the gonk nsWindow type

This makes APZEventState usable on gonk.
Attachment #8561538 - Flags: review?(mwu)
Attachment #8561538 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 41

3 years ago
Created attachment 8561540 [details] [diff] [review]
Part 11 - Use APZEventState in nsBaseWidget
(Assignee)

Updated

3 years ago
Attachment #8561540 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 42

3 years ago
Created attachment 8561541 [details] [diff] [review]
Part 12 - Use (nsBaseWidget's) APZEventState in ChromeProcessController
Attachment #8561541 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 43

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

Comment 44

3 years ago
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.
Attachment #8561538 - Flags: review?(mwu)
(Assignee)

Comment 45

3 years ago
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.
Attachment #8561531 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 46

3 years ago
Created attachment 8561580 [details] [diff] [review]
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).
Attachment #8561531 - Attachment is obsolete: true
Attachment #8561580 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 47

3 years ago
Created attachment 8561581 [details] [diff] [review]
Part 8 - Use a weak reference to the widget in APZEventState to avoid reference cycles

Updated to fix include-what-you-use errors.
Attachment #8561533 - Attachment is obsolete: true
Attachment #8561533 - Flags: review?(bugmail.mozilla)
Attachment #8561581 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 48

3 years ago
Created attachment 8561582 [details] [diff] [review]
Part 12 - Use (nsBaseWidget's) APZEventState in ChromeProcessController

Updated to fix include-what-you-use errors.
Attachment #8561541 - Attachment is obsolete: true
Attachment #8561541 - Flags: review?(bugmail.mozilla)
Attachment #8561582 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 49

3 years ago
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.

     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?
Flags: needinfo?(bugmail.mozilla)
Attachment #8561730 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 50

3 years ago
Created attachment 8561746 [details] [diff] [review]
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
Attachment #8561582 - Attachment is obsolete: true
Attachment #8561582 - Flags: review?(bugmail.mozilla)
Attachment #8561746 - Flags: review?(bugmail.mozilla)
(Assignee)

Updated

3 years ago
Blocks: 943537
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
Attachment #8561580 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #8561581 - Flags: review?(bugmail.mozilla) → review+
Attachment #8561537 - Flags: review?(bugmail.mozilla) → review+
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?
Attachment #8561538 - Flags: review?(bugmail.mozilla) → review+
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 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
Attachment #8561540 - Flags: review?(bugmail.mozilla) → review+
Attachment #8561746 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #8561730 - Flags: review?(bugmail.mozilla) → review+
(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.
Flags: needinfo?(bugmail.mozilla)
Whiteboard: [NO_UPLIFT][input-thread-uplift-part9]
(Assignee)

Comment 58

3 years ago
Created attachment 8562884 [details] [diff] [review]
Part 6 - Implement proper sending of target-apzc notification and creation of displayport in the root process

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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+.
Attachment #8560652 - Attachment is obsolete: true
Attachment #8562884 - Flags: review+
(Assignee)

Comment 59

3 years ago
Created attachment 8562886 [details] [diff] [review]
Part 7 - Extract an APZEventState class from TabChild

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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+.
Attachment #8561580 - Attachment is obsolete: true
Attachment #8562886 - Flags: review+
(Assignee)

Comment 60

3 years ago
Created attachment 8562887 [details] [diff] [review]
Part 8 - Use a weak reference to the widget in APZEventState to avoid reference cycles

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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+.
Attachment #8561581 - Attachment is obsolete: true
Attachment #8562887 - Flags: review+
(Assignee)

Comment 61

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Comment 62

3 years ago
Created attachment 8562889 [details] [diff] [review]
Part 11 - Use APZEventState in nsBaseWidget

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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+.
Attachment #8561540 - Attachment is obsolete: true
Attachment #8562889 - Flags: review+
(Assignee)

Comment 63

3 years ago
Created attachment 8562891 [details] [diff] [review]
Part 13 - Implement ChromeProcessController::HandleLongTapUp()

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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+.
Attachment #8561730 - Attachment is obsolete: true
Attachment #8562891 - Flags: review+
(Assignee)

Comment 64

3 years ago
greentry
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
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1131852
(Assignee)

Comment 66

3 years ago
landing
(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
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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
Attachment #8560716 - Flags: approval-mozilla-b2g37?
Attachment #8560720 - Flags: approval-mozilla-b2g37?
Attachment #8560649 - Flags: approval-mozilla-b2g37?
Attachment #8560091 - Flags: approval-mozilla-b2g37?
Attachment #8560650 - Flags: approval-mozilla-b2g37?
Attachment #8562884 - Flags: approval-mozilla-b2g37?
Attachment #8562886 - Flags: approval-mozilla-b2g37?
Attachment #8562887 - Flags: approval-mozilla-b2g37?
Attachment #8561537 - Flags: approval-mozilla-b2g37?
Attachment #8561538 - Flags: approval-mozilla-b2g37?
Attachment #8562889 - Flags: approval-mozilla-b2g37?
Attachment #8561746 - Flags: approval-mozilla-b2g37?
Attachment #8562891 - Flags: approval-mozilla-b2g37?
No longer blocks: 1129081
Duplicate of this bug: 1129081

Updated

3 years ago
Attachment #8560091 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
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 !

Updated

3 years ago
Attachment #8560649 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8560650 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8560716 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8560720 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8561537 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8561538 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8561746 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8562884 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8562886 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8562887 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8562889 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8562891 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
(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.
Depends on: 1132741
Depends on: 1133083
(Assignee)

Comment 72

3 years ago
(In reply to Botond Ballo [:botond] from comment #61)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/pushloghtml?changeset=604fe707d582
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Whiteboard: [NO_UPLIFT][input-thread-uplift-part9] → [input-thread-uplift-part9]
Blocks: 1133105
Blocks: 1132980
No longer blocks: 1132980
Depends on: 1132980
Blocks: 1132967
No longer blocks: 1133105
Depends on: 1133105
No longer blocks: 1132967
Depends on: 1132967
Blocks: 1133008
No longer blocks: 1133008
Depends on: 1133008
Depends on: 1136519
Depends on: 1158926
You need to log in before you can comment on or make changes to this bug.