Convert GeckoApp/BrowserApp events to bundle events

RESOLVED FIXED in Firefox 53

Status

defect
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

unspecified
mozilla53
All
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(6 attachments)

Make GeckoApp/BrowserApp events use BundleEventListener/GeckoBundle
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)
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)
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)
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)
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
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1332731
Depends on: 1349612
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

4 months 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.