Closed
Bug 1257319
Opened 8 years ago
Closed 8 years ago
Convert broadcast event to native method
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jchen, Assigned: jchen)
References
Details
Attachments
(7 files)
4.27 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
9.80 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
112.26 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
7.34 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
rbarker
:
review+
|
Details | Diff | Splinter Review |
6.91 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
8.47 KB,
patch
|
Details | Diff | Splinter Review |
Convert the BROADCAST GeckoEvent to a corresponding native call.
Assignee | ||
Comment 1•8 years ago
|
||
Add notifyObservers and the synchronous syncNotifyObservers methods to GeckoAppShell to replace the BROADCAST GeckoEvent.
Attachment #8731773 -
Flags: review?(snorp)
Assignee | ||
Comment 2•8 years ago
|
||
Update auto-generated bindings. The jni-stubs.inc change is due to the fact that we still have old-style native methods in GeckoAppShell.
Attachment #8731774 -
Flags: review+
Assignee | ||
Comment 3•8 years ago
|
||
Mass conversion of existing places where we use broadcast events to use new GeckoAppShell calls.
Attachment #8731775 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 4•8 years ago
|
||
HomeConfig.java saved a list of events to be sent later in a batch. This patch makes it save a pair of strings instead, and the strings are later used to make calls to GeckoAppShell.notifyObservers. The patch also makes two small optimizations. It makes the queue an ArrayList instead of a LinkedList to save memory. It also makes copying the queue a swap instead of a true copy.
Attachment #8731777 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 5•8 years ago
|
||
We should use observer service directly instead of broadcast event.
Attachment #8731778 -
Flags: review?(rbarker)
Assignee | ||
Comment 6•8 years ago
|
||
Remove the now-obsolete BROADCAST GeckoEvent.
Attachment #8731779 -
Flags: review+
Updated•8 years ago
|
Attachment #8731778 -
Flags: review?(rbarker) → review+
Comment on attachment 8731773 [details] [diff] [review] Add notifyObservers methods to GeckoAppShell (v1) Review of attachment 8731773 [details] [diff] [review]: ----------------------------------------------------------------- We should be able to remove Java_org_mozilla_gecko_GeckoAppShell_notifyGeckoObservers from AndroidJNI, right?
Attachment #8731773 -
Flags: review?(snorp) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8731775 [details] [diff] [review] Convert existing broadcast events to calls (v1) Review of attachment 8731775 [details] [diff] [review]: ----------------------------------------------------------------- Holy moly that's a lot of call sites, but this API is a nice improvement. I didn't look at every change terribly closely, but I did look through the whole patch and it all seems reasonable to me. ::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsGridLayout.java @@ +189,5 @@ > public void hide() { > lastSelectedTabId = Tabs.getInstance().getSelectedTab().getId(); > setVisibility(View.GONE); > Tabs.unregisterOnTabsChangedListener(this); > + GeckoAppShell.notifyObservers("Tab:Screenshot:Cancel", ""); I understand you're just changing what currently exists (as you should be!), but is there a reason for passing an empty string sometimes as opposed to null? Should we create a follow-up patch to make these null instead?
Attachment #8731775 -
Flags: review?(margaret.leibovic) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8731777 [details] [diff] [review] Convert broadcast event usage in HomeConfig.java (v1) Review of attachment 8731777 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfig.java @@ +1144,3 @@ > private final Thread mOriginalThread; > > + private List<Pair<String, String>> mNotificationQueue; Nit: I'd add a comment explaining what string values are expected to be in here.
Attachment #8731777 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #7) > Comment on attachment 8731773 [details] [diff] [review] > Add notifyObservers methods to GeckoAppShell (v1) > > Review of attachment 8731773 [details] [diff] [review]: > ----------------------------------------------------------------- > > We should be able to remove > Java_org_mozilla_gecko_GeckoAppShell_notifyGeckoObservers from AndroidJNI, > right? Yep, that's in attachment 8731779 [details] [diff] [review]
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to :Margaret Leibovic from comment #8) > Comment on attachment 8731775 [details] [diff] [review] > Convert existing broadcast events to calls (v1) > > Review of attachment 8731775 [details] [diff] [review]: > ----------------------------------------------------------------- > > Holy moly that's a lot of call sites, but this API is a nice improvement. sed kept me sane :) > ::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsGridLayout.java > @@ +189,5 @@ > > public void hide() { > > lastSelectedTabId = Tabs.getInstance().getSelectedTab().getId(); > > setVisibility(View.GONE); > > Tabs.unregisterOnTabsChangedListener(this); > > + GeckoAppShell.notifyObservers("Tab:Screenshot:Cancel", ""); > > I understand you're just changing what currently exists (as you should be!), > but is there a reason for passing an empty string sometimes as opposed to > null? Should we create a follow-up patch to make these null instead? I guess it depends on the JS handler expecting a null or an empty string. We should make it consistent though. I filed bug 1257943 as a follow-up. I wasn't sure if it would be a good first bug or not... maybe good second bug?
Assignee | ||
Comment 12•8 years ago
|
||
Convert b2gdroid broadcast events to using the new GeckoAppShell call.
Attachment #8732962 -
Flags: review?(fabrice)
Comment 13•8 years ago
|
||
Comment on attachment 8732962 [details] [diff] [review] Convert b2gdroid broadcast events (v1) Review of attachment 8732962 [details] [diff] [review]: ----------------------------------------------------------------- We rm'd mobile/b2gdroid so we don't need that anymore.
Attachment #8732962 -
Flags: review?(fabrice)
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0b229e5968 https://hg.mozilla.org/integration/mozilla-inbound/rev/bba3d09859d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/3e80b5052d0b https://hg.mozilla.org/integration/mozilla-inbound/rev/ba16cc23d7c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/9d6577fd74f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/48d6faa3bf0f
Comment 15•8 years ago
|
||
sorry had to back this out since this cause merge conflicts when merging mozilla-inbound to mozilla-central like warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java! (edit, then use 'hg resolve --mark') warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/home/HomeConfig.java! (edit, then use 'hg resolve --mark') 507 files updated, 3 files merged, 23 files removed, 2 files unresolved
Flags: needinfo?(nchen)
Comment 16•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c539b5bbdb36 https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ef1d7db142 https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d63892ad2c https://hg.mozilla.org/integration/mozilla-inbound/rev/71cdfc0d1448 https://hg.mozilla.org/integration/mozilla-inbound/rev/88dbd01499c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/7970a7b7e8e9
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f188815f4dbd https://hg.mozilla.org/integration/mozilla-inbound/rev/73ddc0c2a8a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa90692bddbb https://hg.mozilla.org/integration/mozilla-inbound/rev/87981beb2e89 https://hg.mozilla.org/integration/mozilla-inbound/rev/2a961ff556b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/29a11b0e6216
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f188815f4dbd https://hg.mozilla.org/mozilla-central/rev/73ddc0c2a8a4 https://hg.mozilla.org/mozilla-central/rev/fa90692bddbb https://hg.mozilla.org/mozilla-central/rev/87981beb2e89 https://hg.mozilla.org/mozilla-central/rev/2a961ff556b4 https://hg.mozilla.org/mozilla-central/rev/29a11b0e6216
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 19•8 years ago
|
||
(In reply to Jim Chen [:jchen] [:darchons] from comment #11) > (In reply to :Margaret Leibovic from comment #8) > > Comment on attachment 8731775 [details] [diff] [review] > > Convert existing broadcast events to calls (v1) > > > > Review of attachment 8731775 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Holy moly that's a lot of call sites, but this API is a nice improvement. > > sed kept me sane :) Just FYI, you can search and rewrite with Java-specific analysis (type restrictions, etc) using IntelliJ's Find Structurally... and Replace Structurally... It's pretty nice for non-trivial rewrites.
Assignee | ||
Comment 20•8 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #19) > (In reply to Jim Chen [:jchen] [:darchons] from comment #11) > > (In reply to :Margaret Leibovic from comment #8) > > > Comment on attachment 8731775 [details] [diff] [review] > > > Convert existing broadcast events to calls (v1) > > > > > > Review of attachment 8731775 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > Holy moly that's a lot of call sites, but this API is a nice improvement. > > > > sed kept me sane :) > > Just FYI, you can search and rewrite with Java-specific analysis (type > restrictions, etc) using IntelliJ's Find Structurally... and Replace > Structurally... It's pretty nice for non-trivial rewrites. Ah good to know. I'll try that next time!
Flags: needinfo?(nchen)
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/151059cf9645ddc1439062506107ee4cb8eed506 Bug 1257319 - (Bustage) Follow-up: Migrate new code to use notifyObservers() API. r=me CLOSED TREE
Comment 22•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #21) > https://hg.mozilla.org/integration/fx-team/rev/ > 151059cf9645ddc1439062506107ee4cb8eed506 > Bug 1257319 - (Bustage) Follow-up: Migrate new code to use notifyObservers() > API. r=me CLOSED TREE I landed new code using createBroadcastEvent on fx-team while this landed on inbound -> This broke after merge. This is the follow-up patch to fix fx-team.
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/151059cf9645
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•