Closed Bug 1363885 Opened 3 years ago Closed 3 years ago

Remove GeckoAppShell.AppStateListener

Categories

(GeckoView :: General, enhancement)

Unspecified
Android
enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(2 files, 1 obsolete file)

GeckoAppShell.AppStateListener is not used anywhere, and getting rid of it lets us remove the orientation listener and app state listener methods in GeckoInterface.
Comment on attachment 8867331 [details]
[PATCH 01/10] Bug 1363885 - 1. Remove GeckoAppShell.AppStateListener;

https://reviewboard.mozilla.org/r/138850/#review142260

Looks good to me.
Attachment #8867331 - Flags: review?(droeh) → review+
Comment on attachment 8867332 [details]
Bug 1363885 - 2. Sync ViERenderer with upstream;

https://reviewboard.mozilla.org/r/138852/#review142714

r- for a few reasons:

First, this is imported upstream code.  That doesn't mean we can't modify it, but to carry a modification that upstream won't accept needs good justification (in some cases we might be able to get it into upstream with ifdefs/etc).

Second, this imported code may well have changed in the much newer import underway currently in Bug 1341285

Third, while we're not currently inputting the rotation/orientation information into webrtc encoding, we plan to (in some orientations it avoids needing to rotate the data, and also moves the requirement to rotate to the receiver not the sender.  That would require tracking the current rotation.
Attachment #8867332 - Flags: review?(rjesup) → review-
(In reply to Randell Jesup [:jesup] from comment #4)
> Comment on attachment 8867332 [details]
> Bug 1363885 - 2. Remove ViERenderer;
> 
> https://reviewboard.mozilla.org/r/138852/#review142714
> 
> r- for a few reasons:
> 
> First, this is imported upstream code.  That doesn't mean we can't modify
> it, but to carry a modification that upstream won't accept needs good
> justification (in some cases we might be able to get it into upstream with
> ifdefs/etc).

Ok we should probably keep ViERenderer.java then. However, our in-tree ViERenderer.java looks completely different from the upstream version. This new patch syncs our in-tree copy to the upstream version. Practically this has no effect because ViERenderer is not used in the code.

> Second, this imported code may well have changed in the much newer import
> underway currently in Bug 1341285

What's the timeline for Bug 1341285? If it's longer than a week or two, I'd like to fix this bug first.

> Third, while we're not currently inputting the rotation/orientation
> information into webrtc encoding, we plan to (in some orientations it avoids
> needing to rotate the data, and also moves the requirement to rotate to the
> receiver not the sender.  That would require tracking the current rotation.

The orientation listener API used in ViERenderer are obsolete and should be removed. If we want to add orientation tracking in the future, we should just design and implement a new API.
(In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> (In reply to Randell Jesup [:jesup] from comment #4)
> > Comment on attachment 8867332 [details]
> > Bug 1363885 - 2. Remove ViERenderer;
> > 
> > https://reviewboard.mozilla.org/r/138852/#review142714
> > 
> > r- for a few reasons:
> > 
> > First, this is imported upstream code.  That doesn't mean we can't modify
> > it, but to carry a modification that upstream won't accept needs good
> > justification (in some cases we might be able to get it into upstream with
> > ifdefs/etc).
> 
> Ok we should probably keep ViERenderer.java then. However, our in-tree
> ViERenderer.java looks completely different from the upstream version. This
> new patch syncs our in-tree copy to the upstream version. Practically this
> has no effect because ViERenderer is not used in the code.

Did you look at the current source (which I linked to in IRC), or to the branch 49 source (which is what current m-c is based on)?  As best I can tell, ViERenderer.java no longer exists in 49 or 57 or current tip of upstream.  Likely it was made moot when we imported 49, which removed most of the old ViE (video) interfaces in favor of a new API refactor.  This was all done by pkerr.

> > Second, this imported code may well have changed in the much newer import
> > underway currently in Bug 1341285
> 
> What's the timeline for Bug 1341285? If it's longer than a week or two, I'd
> like to fix this bug first.

It's almost green on try - one test (simulcast) is permafailing, and there are two other known test failures.  We're about to start reviews.  I'm hoping it will land within a week.

> > Third, while we're not currently inputting the rotation/orientation
> > information into webrtc encoding, we plan to (in some orientations it avoids
> > needing to rotate the data, and also moves the requirement to rotate to the
> > receiver not the sender.  That would require tracking the current rotation.
> 
> The orientation listener API used in ViERenderer are obsolete and should be
> removed. If we want to add orientation tracking in the future, we should
> just design and implement a new API.

Likely there's other code actually being used that can read the orientation (though we haven't hooked it through).
Comment on attachment 8867332 [details]
Bug 1363885 - 2. Sync ViERenderer with upstream;

(In reply to Randell Jesup [:jesup] from comment #8)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> > (In reply to Randell Jesup [:jesup] from comment #4)
> > > Comment on attachment 8867332 [details]
> > > Bug 1363885 - 2. Remove ViERenderer;
> > >
> > What's the timeline for Bug 1341285? If it's longer than a week or two, I'd
> > like to fix this bug first.
> 
> It's almost green on try - one test (simulcast) is permafailing, and there
> are two other known test failures.  We're about to start reviews.  I'm
> hoping it will land within a week.


Ok in that case I think we can just wait on Bug 1341285.
Attachment #8867332 - Attachment is obsolete: true
Attachment #8867332 - Flags: review?(rjesup)
Depends on: 1341285
ViERenderer is not used anywhere but has a couple calls to the obsolete
GeckoAppShell orientation listener. The entire ViERenderer.java file is
getting removed in the upcoming WebRTC update, so we should just go
ahead and remove those lines.
Attachment #8871320 - Flags: review?(snorp)
Attachment #8871320 - Flags: review?(snorp) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de4a54a0212c
1. Remove GeckoAppShell.AppStateListener; r=droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b91b33444eb
2. Remove ViERenderer dependency on orientation listener; r=snorp
https://hg.mozilla.org/mozilla-central/rev/de4a54a0212c
https://hg.mozilla.org/mozilla-central/rev/6b91b33444eb
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.