1257319 - Convert broadcast event to native method

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Convert the BROADCAST GeckoEvent to a corresponding native call.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add notifyObservers methods to GeckoAppShell (v1)

Add notifyObservers and the synchronous syncNotifyObservers methods to
GeckoAppShell to replace the BROADCAST GeckoEvent.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Update auto-generated bindings; r=me (v1)

Update auto-generated bindings. The jni-stubs.inc change is due to the
fact that we still have old-style native methods in GeckoAppShell.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Convert existing broadcast events to calls (v1)

Mass conversion of existing places where we use broadcast events to use
new GeckoAppShell calls.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Convert broadcast event usage in HomeConfig.java (v1)

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.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Don't use broadcast event in AndroidContentController (v1)

We should use observer service directly instead of broadcast event.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Remove BROADCAST GeckoEvent; r=me (v1)

Remove the now-obsolete BROADCAST GeckoEvent.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

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?

Comment 8

[:Margaret Leibovic]

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?

Comment 9

[:Margaret Leibovic]

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.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to James Willcox (:snorp) ([e-mail]) 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]

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

(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?

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Convert b2gdroid broadcast events (v1)

Convert b2gdroid broadcast events to using the new GeckoAppShell call.

Comment 13

[[:fabrice] Fabrice Desré]

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.

Comment 14

[Pulsebot]

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

[Carsten Book [:Tomcat]]

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

Comment 16

[Pulsebot]

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

[Pulsebot]

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

[Carsten Book [:Tomcat]]

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

Comment 19

[Nick Alexander :nalexander [he/him]]

(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.

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

(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!

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

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

[Sebastian Kaspari (:sebastian; :pocmo)]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/151059cf9645