Closed Bug 1257319 Opened 4 years ago Closed 4 years ago

Convert broadcast event to native method

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files)

Convert the BROADCAST GeckoEvent to a corresponding native call.
Add notifyObservers and the synchronous syncNotifyObservers methods to
GeckoAppShell to replace the BROADCAST GeckoEvent.
Attachment #8731773 - Flags: review?(snorp)
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+
Mass conversion of existing places where we use broadcast events to use
new GeckoAppShell calls.
Attachment #8731775 - Flags: review?(margaret.leibovic)
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)
We should use observer service directly instead of broadcast event.
Attachment #8731778 - Flags: review?(rbarker)
Remove the now-obsolete BROADCAST GeckoEvent.
Attachment #8731779 - Flags: review+
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 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 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+
(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]
(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?
Convert b2gdroid broadcast events to using the new GeckoAppShell call.
Attachment #8732962 - Flags: review?(fabrice)
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)
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)
(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.
(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)
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
(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.
You need to log in before you can comment on or make changes to this bug.