1202312 - Use mozilla::Function in APZ code

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

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.

Comment 1

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1202312 - Use mozilla::Function for the SetAllowedTouchBehavior callback. r=kats

Bug 1202312 - Use mozilla::Function for the SetAllowedTouchBehavior callback. r=kats

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1202312 - Use mozilla::Function for the ContentReceivedInputBlock callback. r=kats

Bug 1202312 - Use mozilla::Function for the ContentReceivedInputBlock callback. r=kats

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1202312 - Remove an old forward declaration and typedef. r=kats

Bug 1202312 - Remove an old forward declaration and typedef. r=kats

Comment 4

[Botond Ballo [:botond]]

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 5

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

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

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 7

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

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

Comment 8

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

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

Comment 9

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

(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!

Comment 10

[Botond Ballo [:botond]]

Addressed review comments locally. Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bbd21c01c77

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25e572cce005
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbfb900230fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/8df7bb41efbc

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/25e572cce005
https://hg.mozilla.org/mozilla-central/rev/fbfb900230fe
https://hg.mozilla.org/mozilla-central/rev/8df7bb41efbc