Use mozilla::Function in APZ code

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: botond, Unassigned)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

4 years ago
Bug 1198451 introduces a type-erased callable wrapper type, mozilla::Function (similar to std::function). I posted an example patch there demonstrating its usage in APZ code; I'd like to land that patch, as well as patches that use it in some other places in APZ code, in this bug.
Reporter

Updated

4 years ago
Depends on: 1198451
Reporter

Comment 1

4 years ago
Bug 1202312 - Use mozilla::Function for the SetAllowedTouchBehavior callback. r=kats
Attachment #8657665 - Flags: review?(bugmail.mozilla)
Reporter

Comment 2

4 years ago
Bug 1202312 - Use mozilla::Function for the ContentReceivedInputBlock callback. r=kats
Attachment #8657666 - Flags: review?(bugmail.mozilla)
Reporter

Comment 3

4 years ago
Bug 1202312 - Remove an old forward declaration and typedef. r=kats
Attachment #8657667 - Flags: review?(bugmail.mozilla)
Reporter

Comment 4

4 years ago
Some notes:

  - In some places I use a pattern like this:

      Type localVar = someExpression;
      SomeFunction([localVar](...) { ... });
      // that was the only use of |localVar|

    That is, I declare a local variable solely for the purpose of capturing it
    by the lambda. This is necessary when we need the lambda object to store
    a member whose value (|someExpression|) is derived from things in scope
    when the lambda is created (such as the |this| object), but we don't want
    it to capture those things themselves.

    C++14's generalized lambda captures will make this nicer:

      SomeFunction([localVar = someExpression](...) { ... });

  - The APZEventState constructor takes the ContentReceivedInputBlock callback
    by rvalue reference because mozilla::Function is not copyable (because it
    has a UniquePtr data member).

  - We used to have a third callback type in APZ code, SetTargetAPZCCallback,
    but that was removed in bug 1154130. The third patch removes a forward
    declaration and a typedef that accidentally stuck around.
Comment on attachment 8657665 [details]
MozReview Request: Bug 1202312 - Use mozilla::Function for the SetAllowedTouchBehavior callback. r=kats

https://reviewboard.mozilla.org/r/18387/#review16451

Cool!

::: dom/ipc/TabChild.h:644
(Diff revision 1)
> -    nsRefPtr<SetAllowedTouchBehaviorCallback> mSetAllowedTouchBehaviorCallback;
> +    SetAllowedTouchBehaviorCallback mSetAllowedTouchBehaviorCallback;

What value does get this initialized to, assuming nothing is assigned to it?

::: widget/nsBaseWidget.h:8
(Diff revision 1)
> +#include "mozilla/layers/APZCCallbackHelper.h"

Move this down one line to alphabetize
Attachment #8657665 - Flags: review?(bugmail.mozilla) → review+
Reporter

Comment 6

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> Comment on attachment 8657665 [details]
> MozReview Request: Bug 1202312 - Use mozilla::Function for the
> SetAllowedTouchBehavior callback. r=kats

That was quick :)

> ::: dom/ipc/TabChild.h:644
> (Diff revision 1)
> > -    nsRefPtr<SetAllowedTouchBehaviorCallback> mSetAllowedTouchBehaviorCallback;
> > +    SetAllowedTouchBehaviorCallback mSetAllowedTouchBehaviorCallback;
> 
> What value does get this initialized to, assuming nothing is assigned to it?

Conceptually, it's "empty", and invoking an empty Function is undefined behaviour. (I added an assert to the call operator to increase the likelihood of catching such cases.)

In terms of implementation, in the empty state the pointer the Function stores to the object that stores the wrapped callable will be null, and invoking the Function in this state will lead to a segfault.
Comment on attachment 8657666 [details]
MozReview Request: Bug 1202312 - Use mozilla::Function for the ContentReceivedInputBlock callback. r=kats

https://reviewboard.mozilla.org/r/18389/#review16459

::: dom/ipc/TabChild.cpp:835
(Diff revision 1)
> -      new TabChildContentReceivedInputBlockCallback(this));
> +      ContentReceivedInputBlockCallback(

I'd prefer making this a local variable rather than stuffing the entire body into the argument list of |new APZEventState|.

::: widget/nsBaseWidget.cpp:887
(Diff revision 1)
> -      new ChromeProcessContentReceivedInputBlockCallback(mAPZC));
> +      ContentReceivedInputBlockCallback(

Ditto here, extract a local variable
Attachment #8657666 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8657667 [details]
MozReview Request: Bug 1202312 - Remove an old forward declaration and typedef. r=kats

https://reviewboard.mozilla.org/r/18391/#review16461
Attachment #8657667 - Flags: review?(bugmail.mozilla) → review+
(In reply to Botond Ballo [:botond] from comment #6)
> In terms of implementation, in the empty state the pointer the Function
> stores to the object that stores the wrapped callable will be null, and
> invoking the Function in this state will lead to a segfault.

Sounds good, thanks for the explanation!
Reporter

Comment 10

4 years ago
Addressed review comments locally. Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bbd21c01c77
You need to log in before you can comment on or make changes to this bug.