Closed
Bug 1321418
Opened 8 years ago
Closed 8 years ago
Convert GeckoApp/BrowserApp events to bundle events
Categories
(GeckoView :: General, defect)
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: jchen, Assigned: jchen)
References
Details
Attachments
(6 files)
58.28 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
21.01 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
57.21 KB,
patch
|
sebastian
:
review+
snorp
:
review+
|
Details | Diff | Splinter Review |
21.24 KB,
patch
|
sebastian
:
review+
snorp
:
review+
|
Details | Diff | Splinter Review |
28.64 KB,
patch
|
gbrown
:
review+
|
Details | Diff | Splinter Review |
1.39 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
Make GeckoApp/BrowserApp events use BundleEventListener/GeckoBundle
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
Update cases where we use or wait for events in tests.
Attachment #8816526 -
Flags: review?(gbrown)
![]() |
||
Updated•8 years ago
|
Attachment #8816526 -
Flags: review?(gbrown) → review+
Attachment #8816524 -
Flags: review?(snorp) → review+
Attachment #8816525 -
Flags: review?(snorp) → review+
Updated•8 years ago
|
Attachment #8816520 -
Flags: review?(s.kaspari) → review+
Updated•8 years ago
|
Attachment #8816522 -
Flags: review?(s.kaspari) → review+
Comment 6•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8816525 -
Flags: review?(s.kaspari) → review+
Assignee | ||
Comment 7•8 years ago
|
||
(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 :)
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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Assignee | ||
Comment 10•8 years ago
|
||
Regression caused by an incorrect ID used to update menu items.
Attachment #8854674 -
Flags: review?(s.kaspari)
Updated•8 years ago
|
Attachment #8854674 -
Flags: review?(s.kaspari) → review+
Comment 11•8 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
Comment 12•8 years ago
|
||
bugherder |
Updated•6 years ago
|
Product: Firefox for Android → GeckoView
Updated•6 years ago
|
Target Milestone: Firefox 53 → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•