Remove GeckoAppShell.AppStateListener

RESOLVED FIXED in Firefox 55

Status

enhancement
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

unspecified
mozilla55
Unspecified
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
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 4

2 years ago
mozreview-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-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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+

Comment 11

2 years ago
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

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/de4a54a0212c
https://hg.mozilla.org/mozilla-central/rev/6b91b33444eb
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

6 months ago
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.