1199752 - Removing all tab mirroring UI code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

When we started exploring Tab Mirroring, we added some UI code to the app to make it easy to test. Since we are removing Tab Mirroring implementation code, we should just remove the front-end code too.

Comment 1

[Andrew Brown]

Hello Mark - I'd be happy to work on this bug (it'll be my first).  I'll probably need your help finding where in the code base I can find the relevant code.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Hi Andrew!

The tab mirroring code happend over several different landings, so I expect we'll need to peel it away in layers. Looking at the code that originally landed will help.

Here is the last bit of code (I think) that landed:
http://hg.mozilla.org/mozilla-central/rev/8d7b53cc415e

Here is the original code that landed (again, I think):
http://hg.mozilla.org/mozilla-central/rev/b350df009721

Based on these, I think we should start by:
1. Removing any tab mirroring code from CastingApps.js
2. Remove TabMirror.jsm completely
3. Remove the casting.mirrorTab* sttings from browser.properties
4. Remove the mirror function from MediaPlayerApp.jsm

We should look into ChromeCast.java and MediaPlayerManager.java to see what should and shouldn't be removed. Some of those changes might be formatting or bug fixes.

I'm pretty sure SimpleServiceDiscovery.jsm has already been cleaned up and there is no relevant code in that file for mirroring anymore.

That should be enough to get us going. Jump on Mozilla IRC #mobile to ask questions you might have as well.

Comment 3

[Andrew Brown]

Attachment: Removed tab mirroring code from CastingApps.js

Comment 4

[Andrew Brown]

Attachment: Removed mirror function from MediaPlayerApp.jsm

Comment 5

[Andrew Brown]

Attachment: Removed casting.mirrorTab settings from browser.properties

Comment 6

[Andrew Brown]

Attachment: Removed tab mirroring from ChromeCast.java

Not 100% sure I got everything on this one - the call to new GoogleApiClient at the end for instance

Comment 7

[Andrew Brown]

Attachment: Removed mirroring from MediaPlayerManager.java

Comment 8

[Andrew Brown]

Hey Mark - how do I create a patch that completely removes a file (TabMirror.jsm)?  Just upload a blank file as the patch?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Andrew Brown from comment #8)
> Hey Mark - how do I create a patch that completely removes a file
> (TabMirror.jsm)?  Just upload a blank file as the patch?

Andrew - Just use | hg remove <path/to/file> | and then you should see the completely deleted file in your | hg diff |

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8657849 [details] [diff] [review]
Removed tab mirroring code from CastingApps.js

>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js

>@@ -70,18 +70,16 @@ var mediaPlayerDevice = {
>       modelName: display.modelName,
>       mirror: display.mirror

We can remove the | mirror | property here. I think MediaPlayerApp.jsm looks for it.

>-    Services.obs.addObserver(this, "Casting:Mirror", false);
>     Services.obs.addObserver(this, "ssdp-service-found", false);
>     Services.obs.addObserver(this, "ssdp-service-lost", false);

Looks like we no longer need "ssdp-service-found" and "ssdp-service-lost" either.

>   isMirroringEnabled: function isMirroringEnabled() {
>     return Services.prefs.getBoolPref("browser.mirroring.enabled");
>   },

isMirroringEnabled can be removed too. There is no harm in removing the "browser.mirroring.enabled" perference from mobile.js:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#293 (lines 293 to 298)

>       case "ssdp-service-found":
>         {
>           this.serviceAdded(SimpleServiceDiscovery.findServiceForID(aData));
>           break;
>         }
>       case "ssdp-service-lost":
>         {
>           this.serviceLost(SimpleServiceDiscovery.findServiceForID(aData));

These can go too.

Great start, but let's keep removing code!

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8657851 [details] [diff] [review]
Removed mirror function from MediaPlayerApp.jsm

>diff --git a/mobile/android/modules/MediaPlayerApp.jsm b/mobile/android/modules/MediaPlayerApp.jsm

>   remoteMedia: function remoteMedia(callback, listener) {
>     if (callback) {
>       callback(new RemoteMedia(this.id, listener));
>     }
>   },
> 
>-  mirror: function mirror(callback) {
>-    send("MediaPlayer:Mirror", { id: this.id }, (result, err) => {
>-      if (callback) {
>-        callback(err == null);
>-      }
>-    });
>-  }

Looks good, but can you remove the "," and remaining blank line after the | remoteMedia | method. Put up a fixed patch and I'll r+ it too.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8657857 [details] [diff] [review]
Removed tab mirroring from ChromeCast.java


>diff --git a/mobile/android/base/ChromeCast.java b/mobile/android/base/ChromeCast.java

>-    @Override
>-    public void mirror(final EventCallback callback) {
>-        final CastDevice device = CastDevice.getFromBundle(route.getExtras());
>-        Cast.CastOptions.Builder apiOptionsBuilder = Cast.CastOptions.builder(device, new Cast.Listener() {
>-                @Override
>-                public void onApplicationStatusChanged() { }
>-
>-                @Override
>-                public void onVolumeChanged() { }
>-
>-                @Override
>-                public void onApplicationDisconnected(int errorCode) { }
>-            });
>-
>         apiClient = new GoogleApiClient.Builder(context)
>             .addApi(Cast.API, apiOptionsBuilder.build())
>             .addConnectionCallbacks(new GoogleApiClient.ConnectionCallbacks() {
>                     @Override
>                     public void onConnected(Bundle connectionHint) {
>                         // Sometimes apiClient is null here. See bug 1061032
>                         if (apiClient == null || !apiClient.isConnected()) {
>                             return;

Yeah. Looks like we need to remove more (or less) for this code to build. Let's get you set up to build these changes, so we know we didn't break the build. If you have a Chromecast, we should test "video flinging" to make sure it's not broken when we remove "tab mirroring". If you don't have a Chromecast, I (or someone else) can do the testing.

Comment 13

[Andrew Brown]

Attachment: Removed tab mirroring code from CastingApps.js - V1.01

Comment 14

[Andrew Brown]

Attachment: Removed tab mirror function and linespace from MediaPlayerApp.jsm - V1.01

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

Modified whitelist 'good-first-bug'->'good first bug'.
see https://wiki.mozilla.org/Good_first_bug

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Finkle, is this still relevant?

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Yes, but I can probably try to land some of the code that is "close enough" and see what's left over.

Comment 18

[Srivatsav Gunisetty]

Hey Mark, can I work on this bug since I see no one is working on this bug? Could you inform whether it is still relevant? If yes help me with this bug since I am new to bug fixing.

Comment 19

[Srivatsav Gunisetty]

Attachment: bug-1199752-fix.patch

I have removed all the mirror UI code as mentioned in the comment2. Please review my patch.

Comment 20

[Kevin Brosnan [Ex-Mozilla]]

Comment on attachment 8825045 [details] [diff] [review]
bug-1199752-fix.patch

Sebastian is more active.

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8825045 [details] [diff] [review]
bug-1199752-fix.patch

@schien: Can you take a quick look at this patch too? I have no clue about our casting code yet - and you have at least work on this area a bit. :)

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8825045 [details] [diff] [review]
bug-1199752-fix.patch

Review of attachment 8825045 [details] [diff] [review]:
-----------------------------------------------------------------

The modification in CastingApps.js and MediaPlayerApp.jsm looks good to me in general.

In addition you'll need to remove https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/CastingApps.js#57 as well.

::: mobile/android/chrome/content/CastingApps.js
@@ -92,5 @@
>      Services.obs.addObserver(this, "Casting:Pause", false);
>      Services.obs.addObserver(this, "Casting:Stop", false);
> -    Services.obs.addObserver(this, "Casting:Mirror", false);
> -    Services.obs.addObserver(this, "ssdp-service-found", false);
> -    Services.obs.addObserver(this, "ssdp-service-lost", false);

ssdp-service-found and ssdp-service-lost is a general callback from SimpleServiceDiscovery.jsm. We should keep them if this bug is only for removing tab mirroring.

@@ -111,5 @@
> -    NativeWindow.menu.update(this.mirrorStartMenuId, { visible: false });
> -    NativeWindow.menu.update(this.mirrorStopMenuId, { visible: true });
> -  },
> -
> -  serviceAdded: function(aService) {

keep serviceAdded as an empty function. Remote playback should use this notification to update casting icon if casting device is added after video element is loaded (this should be done in another bug).

@@ -148,5 @@
> -      NativeWindow.menu.update(this.mirrorStopMenuId, { visible: false });
> -    }
> -  },
> -
> -  serviceLost: function(aService) {

keep serviceLost as empty function for the same reason.

@@ -168,5 @@
>      return Services.prefs.getBoolPref("browser.casting.enabled");
>    },
>  
> -  isMirroringEnabled: function isMirroringEnabled() {
> -    return Services.prefs.getBoolPref("browser.mirroring.enabled");

should remove the preference from mobile/android/app/mobile.js as well.

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8825045 [details] [diff] [review]
bug-1199752-fix.patch

Thanks, schien! I'll remove the review flag for me until a new version of the patch is available.

Comment 24

[Srivatsav Gunisetty]

Attachment: bug-1199752-fix-v3.patch

I have made the corrections which you have suggested in comment22. Please review it.

Comment 25

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8825779 [details] [diff] [review]
bug-1199752-fix-v3.patch

Review of attachment 8825779 [details] [diff] [review]:
-----------------------------------------------------------------

overall looks good! but one more thing to fix.

::: mobile/android/app/mobile.js
@@ +299,5 @@
>  pref("browser.search.noCurrentEngine", true);
>  
>  // Control media casting & mirroring features
>  pref("browser.casting.enabled", true);
>  #ifdef RELEASE_OR_BETA

This line should be removed as well.

Comment 26

[Srivatsav Gunisetty]

Attachment: bug-1199752-fix-v4.patch

Made the suggested changes. Please review it.

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8826100 [details] [diff] [review]
bug-1199752-fix-v4.patch

Review of attachment 8826100 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/app/mobile.js
@@ -299,5 @@
> -pref("browser.search.noCurrentEngine", true);
> -
> -// Control media casting & mirroring features
> -pref("browser.casting.enabled", true);
> -#ifdef RELEASE_OR_BETA

What I meant in previous comment is including `#ifdef RELEASE_OR_BETA`. You should not touch line 298-302.

Comment 28

[Srivatsav Gunisetty]

Attachment: bug-1199752-fix-v5.patch

Sorry for the previous patch. I have removed the line which you said. Please review it.

Comment 29

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8826103 [details] [diff] [review]
bug-1199752-fix-v5.patch

Review of attachment 8826103 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/CastingApps.js
@@ +113,4 @@
>    },
>  
>    serviceLost: function(aService) {
> +

nit: remove this empty line as well.

::: mobile/android/modules/MediaPlayerApp.jsm
@@ -65,5 @@
> -        callback(err == null);
> -      }
> -    });
> -  }
> -}

should not remove the last '}'.

Comment 30

[Srivatsav Gunisetty]

Attachment: bug-1199752-fix-v6.patch

Oops, my fault. I have rectified the mistake.

Comment 31

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8826116 [details] [diff] [review]
bug-1199752-fix-v6.patch

Review of attachment 8826116 [details] [diff] [review]:
-----------------------------------------------------------------

Sebastian can do the final review now.

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8826116 [details] [diff] [review]
bug-1199752-fix-v6.patch

Review of attachment 8826116 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a good start.

Following the "MediaPlayer:Mirror" event there should be more (Java) code that can be removed. Do you want to remove that too?

Comment 33

[Srivatsav Gunisetty]

Yes, I would like to work on that too. What else should be removed?

Comment 34

[Sebastian Kaspari (:sebastian; :pocmo)]

You removed the code that sends the 'MediaPlayer:Mirror' event. If you search for that on DXR you will find some more Java code that handles receiving this event:
https://dxr.mozilla.org/mozilla-central/search?q=MediaPlayer%3AMirror

Comment 35

[Srivatsav Gunisetty]

Attachment: bug-1199752-fix-v7.patch

I have removed the other dependent code also please review it. Can I ping you directly in IRC when I have any doubt?

Comment 36

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8827546 [details] [diff] [review]
bug-1199752-fix-v7.patch

Review of attachment 8827546 [details] [diff] [review]:
-----------------------------------------------------------------

You'll need to remove the file from the build file too:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/moz.build#31

This makes me wonder: Did you actually build the app and try whether everything still works before submitting a patch?

::: mobile/android/base/java/org/mozilla/gecko/MediaPlayerManager.java
@@ -135,5 @@
>                  player.pause(callback);
>              } else if ("MediaPlayer:End".equals(event)) {
>                  player.end(callback);
> -            } else if ("MediaPlayer:Mirror".equals(event)) {
> -                player.mirror(callback);

I think we could go even deeper: The mirror() method was only called here. Sothere might be more code that can be removed in the GeckoMediaPlayer interface and ChromecastPlayer.java. However to not expand the scope on and on I'm going to file a new bug for this.

Comment 37

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8827546 [details] [diff] [review]
bug-1199752-fix-v7.patch

Review of attachment 8827546 [details] [diff] [review]:
-----------------------------------------------------------------

Applying the patch locally I noticed that it fails with some conflicts. Can you try to rebase your patch and upload a new version?

Comment 38

[Srivatsav Gunisetty]

Attachment: bug-1199752-fix-v8.patch

I have removed the line you have suggested. Sorry for not verifying the patch before submitting it for review. I have rebased the patch. Please review it.

Comment 39

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=591792987b07

Comment 40

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8828771 [details] [diff] [review]
bug-1199752-fix-v8.patch

Review of attachment 8828771 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Let's see what try says:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=16e154e282747494d183cdb8eaf0fc09078b2497

Comment 41

[Srivatsav Gunisetty]

Attachment: bug-1199752-fix-v8.patch

Comment 42

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing before it can land.

Comment 43

[Srivatsav Gunisetty]

Attachment: bug-1199752-fix.patch

Hey Sebastian, there have been some changes in the core since my last patch. So please review this patch again. I hope I did everything needed.

Comment 44

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8832781 [details] [diff] [review]
bug-1199752-fix.patch

Review of attachment 8832781 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. I'll verify that the new patch applies and land it. Thank you!

Comment 45

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/676201b830f06fd9058946529db870573d837dcb
Bug 1199752 - Removing all tab mirroring UI code. r=sebastian

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/676201b830f0
Removing all tab mirroring UI code. r=sebastian

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/676201b830f0