1158029 - [Presentation WebAPI] support service discovery on Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Gary Chen [:xeonchen]]

Enable Presentation API on Fennec based on the work of Bug 1115480.

Comment 1

[Gary Chen [:xeonchen]]

Hi Mark,

We're planning to enable the Presentation API features on Fennec, and some platform-dependent work here is to implement the mDNS client on Fennec.

Can we use use |NsdManager| in Necko or is there any better solution?

Comment 2

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

This is an example of communicating between JS XPCOM and Java

https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/MediaPlayerApp.jsm

Comment 3

[Gary Chen [:xeonchen]]

Attachment: [WIP] mDNS via NsdManager

I'm working on this by sending message to Java-based code.

Comment 4

[Gary Chen [:xeonchen]]

Attachment: Part 1: JavaScript module of NsdManager

Based on Bug 1115480, implement service discovery feature on Fennec.
This module is the bridge to the NsdManager.

Comment 5

[Gary Chen [:xeonchen]]

Attachment: Part 2: mDNS XPCOM module using NsdManage

Implement the mDNS discovery XPCOM module on Android

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8606095 [details] [diff] [review]
Part 1: JavaScript module of NsdManager

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

A quick drive-by.

The Java parts of this should live in a subdirectory for its own package.

I'd love to see this behind a build flag, but the BrowserApp parts make that more difficult. For that reason I'd be keen to see it not hook into BrowserApp; I'm not sure what the motivation was for choosing that approach.

::: mobile/android/base/BrowserApp.java
@@ +433,5 @@
>  
> +    private MulticastDNSManager mMulticastDNSManager;
> +
> +    private void createMulticastDNSManager() {
> +        Log.d("GeckoMDNSManager", "createMulticastDNSManager");

Remove this line, or use LOGTAG.

@@ +443,5 @@
> +        }
> +    }
> +
> +    private void destroyMulticastDNSManager() {
> +        Log.d("GeckoMDNSManager", "destroyMulticastDNSManager");

Same.

@@ +784,5 @@
>  
>          mFindInPageBar = (FindInPageBar) findViewById(R.id.find_in_page);
>          mMediaCastingBar = (MediaCastingBar) findViewById(R.id.media_casting);
>  
> +        createMulticastDNSManager();

This shouldn't occur in onCreate. It should likely be in the DelayedStartup handler, around line 1830.

::: mobile/android/base/MulticastDNSManager.java
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko;

Different package.

@@ +22,5 @@
> +
> +import java.net.InetAddress;
> +
> +/**
> + * This class is the bridge between XPCOM mDNS module and NsdManager.

Pointer to the IDL?

@@ +27,5 @@
> + */
> +public class MulticastDNSManager implements NativeEventListener {
> +    private static final String LOGTAG = "GeckoMDNSManager";
> +
> +    private NsdManager mNsdManager = null;

final, remove the = null.

Also drop the Hungarian notation throughout.

@@ +40,5 @@
> +    }
> +
> +    @TargetApi(16)
> +    private MulticastDNSManager(Context context) {
> +        mNsdManager = (NsdManager)context.getSystemService(Context.NSD_SERVICE);

Space between cast:

  (NsdManager) context.…

@@ +71,5 @@
> +    }
> +
> +    @Override
> +    public void handleMessage(String event, NativeJSObject message, EventCallback callback) {
> +        Log.d(LOGTAG, "handleMessage: " + event);

Log.v

@@ +73,5 @@
> +    @Override
> +    public void handleMessage(String event, NativeJSObject message, EventCallback callback) {
> +        Log.d(LOGTAG, "handleMessage: " + event);
> +
> +        if ("NsdManager:DiscoverServices".equals(event)) {

Use switch. We support the Java 7 source level.

@@ +123,5 @@
> +        if (port != 0) {
> +            try {
> +                obj.put("port", port);
> +            } catch (JSONException e) {
> +                Log.e(LOGTAG, e.getMessage());

Why would this fail? Indeed, why would any of these fail? And if they do, would you prefer to muffle or crash? Crashing at least gets you a crash report.

@@ +137,5 @@
> +            }
> +        }
> +
> +        String serviceType = serviceInfo.getServiceType();
> +        Log.d(LOGTAG, "Found Service: " + serviceType);

This doesn't seem like the right log message for a JSON serializer.

@@ +154,5 @@
> +@TargetApi(16)
> +class DiscoveryListener implements NsdManager.DiscoveryListener {
> +    private static final String LOGTAG = "GeckoMDNSManager";
> +
> +    private NsdManager mNsdManager = null;

final.

@@ +156,5 @@
> +    private static final String LOGTAG = "GeckoMDNSManager";
> +
> +    private NsdManager mNsdManager = null;
> +    private EventCallback mStartCallback = null;
> +    private EventCallback mStopCallback = null;

Please document and enforce the threading properties of these members.

@@ +241,5 @@
> +@TargetApi(16)
> +class RegistrationListener implements NsdManager.RegistrationListener {
> +    private static final String LOGTAG = "GeckoMDNSManager";
> +
> +    private NsdManager mNsdManager = null;

final.

@@ +282,5 @@
> +    @Override
> +    public void onRegistrationFailed(NsdServiceInfo serviceInfo, int errorCode) {
> +        Log.e(LOGTAG, "onRegistrationFailed: " +
> +                serviceInfo.getServiceName() +
> +                "(" + errorCode + ")");

One line.

@@ +304,5 @@
> +    @Override
> +    public void onUnregistrationFailed(NsdServiceInfo serviceInfo, int errorCode) {
> +        Log.e(LOGTAG, "onUnregistrationFailed: " +
> +                serviceInfo.getServiceName() +
> +                "(" + errorCode + ")");

Same.

@@ +317,5 @@
> +@TargetApi(16)
> +class ResolveListener implements NsdManager.ResolveListener {
> +    private static final String LOGTAG = "GeckoMDNSManager";
> +
> +    private NsdManager mNsdManager = null;

final

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8606097 [details] [diff] [review]
Part 2: mDNS XPCOM module using NsdManage

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

Thanks for taking the time to not ship files that aren't necessary! Much appreciated!

Comment 8

[Gary Chen [:xeonchen]]

I'd like to say thank you for the comments!

(In reply to Richard Newman [:rnewman] from comment #6)
> Comment on attachment 8606095 [details] [diff] [review]
> Part 1: JavaScript module of NsdManager
> 
> Review of attachment 8606095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A quick drive-by.
> 
> The Java parts of this should live in a subdirectory for its own package.
> 
> I'd love to see this behind a build flag, but the BrowserApp parts make that
> more difficult. For that reason I'd be keen to see it not hook into
> BrowserApp; I'm not sure what the motivation was for choosing that approach.
> 

I was considering between BrowserApp and GeckoApp, because I have to make sure
the event listener is registered as soon as possible.
Is there better solution for this requirement?

Comment 9

[Richard Newman [:rnewman]]

(In reply to Gary Chen [:xeonchen] from comment #8)
> I'd like to say thank you for the comments!

You're very welcome!

> I was considering between BrowserApp and GeckoApp, because I have to make
> sure the event listener is registered as soon as possible.
> Is there better solution for this requirement?

GeckoApp and BrowserApp are much the same thing, but GeckoApp is shared by webapps. BrowserApp is for the Firefox browser front-end only.

What's the strict lifecycle requirement for this? Does it always have to be ready before Gecko?

Can the listener be lazily initialized in response to some start message from Gecko, perhaps sent on first use and no earlier?

In general we don't want a large set of components to be initialized early in startup, because doing so affects startup performance, and we don't want a large set of components to be eagerly initialized, because doing so affects memory footprint. The best kinds of components are initialized just before they're needed.

Comment 10

[Gary Chen [:xeonchen]]

Attachment: Part 1: JavaScript module of NsdManager

Revised

Comment 11

[Gary Chen [:xeonchen]]

Attachment: Part 2: mDNS XPCOM module using NsdManage

Comment 12

[Richard Newman [:rnewman]]

Gary: can you explain what the lifecycle of MDNSManager should be? I don't have enough information to know what you expect to work and when.

If you're attaching it to BrowserApp, then you'll hit similar issues to Bug 818994: messages will only be guaranteed to be handled when the browser activity is in the foreground.

If the user opens Settings, or switches to a different app momentarily, Gecko might still be running but the frontend will not receive messages.

And the inverse: do you expect the Android-side listeners to stick around and be queuing up events for Gecko when the Gecko thread isn't running?

Comment 13

[Gary Chen [:xeonchen]]

There are 3 parts of mDNS: resolution, discovery, and registration.

Discovery is usually called by user, and resolution comes after discovery. I think these parts works
only when the activity is in the foreground.
Thought it might lost some services if the user jumps out and back to BrowserApp during discovery,
but for discovery, that's not a big deal because you can always re-discover again.

The registration part involves a service listener, their status should be synchronized.
At least the service should be connectible if it is registered.

In bug 1080474 and bug 1115480, when Gecko starts, it creates all device providers, and mDNS provider
will register its service when it's being created.

I thought Gecko is always running, if not, events for Gecko is ok to be queued, but the service should
be unregistered because the service is unavailable.
I'm a little bit curious when will Gecko stop running? It's destroyed or the thread is just paused?
If it's in pause state, we should have a corresponding "pause" state for service registration.

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 8607427 [details] [diff] [review]
Part 2: mDNS XPCOM module using NsdManage

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

thanks for adding fennec!

Comment 15

[Richard Newman [:rnewman]]

Sorry for the delay in responding.

> I'm a little bit curious when will Gecko stop running? It's destroyed or the
> thread is just paused?

My main concern is about the Android UI activities going away, not so much the Gecko thread. The DiscoveryListener and RegistrationListener, and their containing manager, will be brand new each time BrowserApp is recreated. This will happen repeatedly as the browser UI is created and destroyed by Android.

The JS-side ServiceManager consequently won't match the Java-side objects.

Furthermore, messages you send to Java won't necessarily be received, and if they are, responses won't necessarily be sent to JS.

The best thing to do to test this is to turn on "Don't Keep Activities" in Android developer settings. Then you can launch Fennec, start your mDNS testing, hit the Home button, then come back into Fennec and see if things work correctly.


Currently when the Gecko thread exits we call system.exit, so it should never be the case that the Gecko thread starts and stops more than once within a single process lifespan. However, that might change to support GeckoView consumers, so it's worth bearing in mind as a hypothetical.

Comment 16

[u522318]

Attachment: Part 1: JavaScript module of NsdManager

Comment 17

[Gary Chen [:xeonchen]]

Attachment: Part 1: JavaScript module of NsdManager

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 8633915 [details] [diff] [review]
Part 1: JavaScript module of NsdManager

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

Assuming I got earlier reviews right, this looks good to me :D

Comment 19

[Gary Chen [:xeonchen]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f51844f14d63

Comment 20

[Gary Chen [:xeonchen]]

Attachment: Part 2: mDNS XPCOM module using NsdManage

fix rebase conflict, carry r+

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7c4170003ac8
https://hg.mozilla.org/integration/fx-team/rev/8e092ec5fbbd

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7c4170003ac8
https://hg.mozilla.org/mozilla-central/rev/8e092ec5fbbd