1363885 - Remove GeckoAppShell.AppStateListener

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

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

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 1

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

Attachment: [PATCH 01/10] Bug 1363885 - 1. Remove GeckoAppShell.AppStateListener;

Remove GeckoAppShell.AppStateListener and its associated methods in
GeckoInterface (orientation listener and app state listener methods).
---
 .../base/java/org/mozilla/gecko/GeckoApp.java      | 51 ----------------------
 .../java/org/mozilla/gecko/BaseGeckoInterface.java | 14 ------
 .../main/java/org/mozilla/gecko/GeckoAppShell.java | 29 ------------
 3 files changed, 94 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/138850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138850/

Comment 2

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

Attachment: Bug 1363885 - 2. Sync ViERenderer with upstream;

ViERenderer is not used anywhere and it adds dependencies on
enableOrientationListener and disableOrientationListener and
GeckoInterface.
---
 .../src/org/webrtc/videoengine/ViERenderer.java    | 64 ----------------------
 mobile/android/base/moz.build                      |  1 -
 2 files changed, 65 deletions(-)
 delete mode 100644 media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViERenderer.java

Review commit: https://reviewboard.mozilla.org/r/138852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138852/

Comment 3

[Dylan Roeh (:droeh) (he/him)]

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.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 5

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

Comment on attachment 8867331 [details]
[PATCH 01/10] Bug 1363885 - 1. Remove GeckoAppShell.AppStateListener;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138850/diff/1-2/

Comment 6

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

Comment on attachment 8867332 [details]
Bug 1363885 - 2. Sync ViERenderer with upstream;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138852/diff/1-2/

Comment 7

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

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

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

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

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

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.

Comment 10

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

Attachment: 2. Remove ViERenderer dependency on orientation listener (v1)

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de4a54a0212c
https://hg.mozilla.org/mozilla-central/rev/6b91b33444eb