1329268 - Convert Tabs events to bundle events

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Comment 1

[Jan Henning [:JanH]]

You mean these events: https://dxr.mozilla.org/mozilla-central/rev/0d823cf54df53e0cea75a74adebace956bd333d8/mobile/android/base/java/org/mozilla/gecko/Tabs.java#114?

In bug 1301160 I might add another event to the list, so we should watch out which one of us lands first :-)

Comment 2

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

Attachment: 1. Refactor SiteIdentity to use GeckoBundle (v1)

Make SiteIdentity.update use GeckoBundle, and also simplify the enums
in SiteIdentity, by using Enum.valueOf to get enum values from names,
instead of custom code.

Comment 3

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

Attachment: 2. Convert Tabs events to bundle events (v1)

Convert events used in Tabs to GeckoBundle/BundleEventListener events.

All of the events are converted to UI thread events, because a lot of
the listeners modify states in Tab (e.g. through
`Tab.handleLocationChange()`), and those states are later accessed on
the UI thread. We used to modify the states on the Gecko thread, and
then access them on the UI thread, which introduces potential race
conditions. Therefore, I think it's best to convert these events
wholesale to UI thread events to avoid races.

Tabs.notifyListeners now calls the listeners directly if it's already on
the UI thread, instead of posting to the UI thread again. This is meant
as an optimization because the events are now coming in on the UI thread
already.

The "Tab:SelectAndForeground" and "Tab:Select" events are merged into
one "Tab:Select" event, and a "foreground" option is added.

The "Tab:StreamStart" and "Tab:StreamStop" events are merged into one
"Tab:RecordingChange" event, and a "recording" option is added to
indicate starting or stopping.

Comment 4

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

Attachment: 3. Convert Tabs observers to events (v1)

Convert the "Tab:*" observers in browser.js, which go through the observer
service, to events that go through GlobalEventDispatcher.

FindHelper.js uses "Tab:Selected" through a lazy loader, but I don't
think FindHelper should listen to "Tab:Selected" at all until it is
being used. THerefore, this patch adds "Tab:Selected" registration to
FindHelper.js itself.

Comment 5

[Jan Henning [:JanH]]

I'm going to wait for this to land first and adapt my patch accordingly.

Comment 6

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

(In reply to Jan Henning [:JanH] from comment #5)
> I'm going to wait for this to land first and adapt my patch accordingly.

Feel free to land yours first if it's reviewed already :)

Comment 7

[Jan Henning [:JanH]]

Not yet, I'm waiting for Sebastian, too.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8826015 [details] [diff] [review]
1. Refactor SiteIdentity to use GeckoBundle (v1)

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

::: mobile/android/base/java/org/mozilla/gecko/SiteIdentity.java
@@ +81,2 @@
>          try {
> +            return Enum.valueOf(enumType, value.toUpperCase(Locale.ENGLISH));

In other places (telemetry) we explicitly use Locale.US. And according to the Android docs Locale.US is guaranteed to be available on all devices.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8826016 [details] [diff] [review]
2. Convert Tabs events to bundle events (v1)

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

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java
@@ +123,5 @@
> +            "Tab:Close",
> +            "Tab:MediaPlaybackChange",
> +            "Tab:RecordingChange",
> +            "Tab:Select",
> +            null);

I don't really understand the null value here?

Comment 10

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

(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Comment on attachment 8826016 [details] [diff] [review]
> 2. Convert Tabs events to bundle events (v1)
> 
> Review of attachment 8826016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/java/org/mozilla/gecko/Tabs.java
> @@ +123,5 @@
> > +            "Tab:Close",
> > +            "Tab:MediaPlaybackChange",
> > +            "Tab:RecordingChange",
> > +            "Tab:Select",
> > +            null);
> 
> I don't really understand the null value here?

It's just a list terminator for styling, so when you add a new item at the end of the list, your diff looks like,

>              "Tab:Select",
> +            "FOO:BAR",
>              null);

Instead of,

>              "Tab:RecordingChange",
> -            "Tab:Select");
> +            "Tab:Select",
> +            "FOO:BAR");

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10913967fc3a
1. Refactor SiteIdentity to use GeckoBundle; r=sebastian
https://hg.mozilla.org/integration/mozilla-inbound/rev/732854ef17e3
2. Convert Tabs events to bundle events; r=sebastian
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d6755b7a39
3. Convert Tabs observers to events; r=sebastian

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10913967fc3a
https://hg.mozilla.org/mozilla-central/rev/732854ef17e3
https://hg.mozilla.org/mozilla-central/rev/e8d6755b7a39