Convert GeckoApp/BrowserApp events to bundle events

RESOLVED FIXED in Firefox 53

Status

RESOLVED FIXED
2 years ago
21 days ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

unspecified
mozilla53
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(6 attachments)

Make GeckoApp/BrowserApp events use BundleEventListener/GeckoBundle
Created attachment 8816520 [details] [diff] [review]
1. Use GekcoBundle events in GeckoApp (v1)

Switch GeckoApp to using GeckoBundle events everywhere. UI or Gecko
events are used if the event requires the UI or Gecko thread,
respectively, and background events are used for all other events.

There are changes to some other Java classes, such as SnackbarBuilder
and GeckoAccessibility, due to the switch to GeckoBundle.

For "Snackbar:Show", we need the global EventDispatcher because the
event can be sent to both GeckoApp and GeckoPreferences. Howveer, we
only want one listener registered at the same time, so we register and
unregister in GeckoApp's and GeckoPreferences's onPause and onResume
methods.
Attachment #8816520 - Flags: review?(s.kaspari)
Created attachment 8816522 [details] [diff] [review]
2. Use appropriate JS EventDispatcher to send GeckoApp events (v1)

Change JS code that sends events to GeckoApp to use either the global
EventDispatcher or the per-window EventDispatcher.

"Session:StatePurged" is not used so it's removed. "Gecko:Ready" in
geckoview.js is not necessary because it's only used for GeckoApp, so
it's removed from geckoview.js.
Attachment #8816522 - Flags: review?(s.kaspari)
Created attachment 8816524 [details] [diff] [review]
3. Use GeckoBundle events in BrowserApp (v1)

Switch BrowserApp to using GeckoBundle events, in a similar vein as
GeckoApp. UI or Gecko events are used if the event handlers required UI
or Gecko thread, respectively, and background events are used for all
other events.

Some other Java classes also have to be modified as a result of
switching to GeckoBundle.
Attachment #8816524 - Flags: review?(snorp)
Attachment #8816524 - Flags: review?(s.kaspari)
Created attachment 8816525 [details] [diff] [review]
4. Use global EventDispatcher to send BrowserApp events (v1)

Change JS code that sends events to BrowserApp to use the global
EventDispatcher instead of "Messaging".
Attachment #8816525 - Flags: review?(snorp)
Attachment #8816525 - Flags: review?(s.kaspari)
Created attachment 8816526 [details] [diff] [review]
5. Update usages of events in tests (v1)

Update cases where we use or wait for events in tests.
Attachment #8816526 - Flags: review?(gbrown)
Attachment #8816526 - Flags: review?(gbrown) → review+
Attachment #8816524 - Flags: review?(snorp) → review+
Attachment #8816525 - Flags: review?(snorp) → review+
Attachment #8816520 - Flags: review?(s.kaspari) → review+
Attachment #8816522 - Flags: review?(s.kaspari) → review+
Comment on attachment 8816524 [details] [diff] [review]
3. Use GeckoBundle events in BrowserApp (v1)

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

r+ with a fix for the thing mentioned below.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +2007,2 @@
>                  final String location = message.getString("location");
> +                final boolean hasImage = message.getBoolean("hasImage");

This is a wrong refactoring: There's no field "hasImage". Instead the value is determined by whether the "image_url" field is set or not.
Attachment #8816524 - Flags: review?(s.kaspari) → review+
Attachment #8816525 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> Comment on attachment 8816524 [details] [diff] [review]
> 3. Use GeckoBundle events in BrowserApp (v1)
> 
> Review of attachment 8816524 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a wrong refactoring: There's no field "hasImage". Instead the value
> is determined by whether the "image_url" field is set or not.

I added "hasImage" in part 4 :)

Comment 8

2 years ago
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cce3b69bfc7
Use GekcoBundle events in GeckoApp/BrowserApp; r=snorp r=sebastian r=gbrown

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1cce3b69bfc7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1332731
Depends on: 1349612
Created attachment 8854674 [details] [diff] [review]
Update menu items using the correct ID (v1)

Regression caused by an incorrect ID used to update menu items.
Attachment #8854674 - Flags: review?(s.kaspari)
Attachment #8854674 - Flags: review?(s.kaspari) → review+

Comment 11

2 years ago
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84abf2ef2fab
Update menu items using the correct ID; r=sebastian
Depends on: 1351964

Updated

29 days ago
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.