Closed Bug 1329268 Opened 3 years ago Closed 3 years ago

Convert Tabs events to bundle events

Categories

(GeckoView :: General, defect)

All
Android
defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(3 files)

No description provided.
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 :-)
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.
Attachment #8826015 - Flags: review?(s.kaspari)
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.
Attachment #8826016 - Flags: review?(s.kaspari)
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.
Attachment #8826017 - Flags: review?(s.kaspari)
I'm going to wait for this to land first and adapt my patch accordingly.
(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 :)
Not yet, I'm waiting for Sebastian, too.
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.
Attachment #8826015 - Flags: review?(s.kaspari) → review+
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?
Attachment #8826016 - Flags: review?(s.kaspari) → review+
Attachment #8826017 - Flags: review?(s.kaspari) → review+
(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");
Pushed by nchen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/10913967fc3a
https://hg.mozilla.org/mozilla-central/rev/732854ef17e3
https://hg.mozilla.org/mozilla-central/rev/e8d6755b7a39
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1337459
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 53 → mozilla53
You need to log in before you can comment on or make changes to this bug.