1197976 - Reimplement Android nsAppShell event loop

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Current event loop is based on AndroidGeckoEvent, corresponding to the Java GeckoEvent. Since we're getting rid of AndroidGeckoEvent, we should reimplement the event loop.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Use runnable events for nsAppShell event loop (v1)

nsAppShell is currently based on AndroidGeckoEvent objects, which mirror
GeckoEvent on the Java side. With GeckoEvent going away, we will be
gradually removing AndroidGeckoEvent as well. This patch makes the
nsAppShell event loop based on runnable objects, which derive from
nsAppShell::Event. Using runnable objects is much more flexible and allows
us, for example, to post a lambda to the event loop to be run later.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Provide compatibility with AndroidGeckoEvent (v1)

With the new nsAppShell event loop based on runnable events, we need to
implement AndroidGeckoEvent handling as a runnable event. This patch adds
nsAppShell::LegacyGeckoEvent and adopts its implementation from existing
code that handles AndroidGeckoEvent.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8652546 [details] [diff] [review]
Use runnable events for nsAppShell event loop (v1)

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

Looks good with nits

::: widget/android/nsAppShell.h
@@ +115,5 @@
> +    nsresult AddObserver(const nsAString &aObserverKey, nsIObserver *aObserver);
> +
> +    void ScheduleNativeEventCallback() override
> +    {
> +        PostEvent([=] {

We only need 'this', right? So [this] may be clearer/safer

@@ +123,5 @@
> +
> +    class Queue
> +    {
> +        mozilla::Mutex mLock;
> +        mozilla::CondVar mPostedCond;

I think you want a Monitor instead, it combines a mutex and condvar with a nicer API

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8652547 [details] [diff] [review]
Provide compatibility with AndroidGeckoEvent (v1)

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

::: widget/android/nsAppShell.cpp
@@ +852,2 @@
>                  // drop the previous viewport event now that we have a new one
> +                // UniquePtr will delete and remove the queued event.

Can't you just delete mQueuedViewportEvent? The UniquePtr usage here just to free the event is a little confusing...

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/806b558ea520
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ebdd8dc3f2

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/806b558ea520
https://hg.mozilla.org/mozilla-central/rev/d8ebdd8dc3f2