Closed Bug 1000676 Opened 6 years ago Closed 5 years ago

Add about:devices page to display available second screens

Categories

(Firefox for Android :: Screencasting, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: nalexander, Assigned: nalexander)

References

(Depends on 1 open bug)

Details

Attachments

(4 files)

At the moment, Fennec doesn't surface what additional screens have been discovered.  We should do that, and an about: page seems simplest.

This could:

* list screens currently reachable;
* show screens reachable in the past;
* show casting history;
* allow for toggling the casting pref;
* show details about the current network connection;
* allow to manually add a screen (by IP address).
Let's paint these sheds!  I called the code "aboutCastingDevices" since I find "aboutDevices" too generic.  I registered about:devices since that's what you (mfinkle) suggested.  And I defined MOZ_CASTING because I reckon this will grow to be lots of casting-related functionality, and because I hope this won't be Android specific.
wesj: feel free to steal reviews.
Flags: needinfo?(wjohnston)
Comment on attachment 8413256 [details] [diff] [review]
Part 1: Add aboutCastingDevices.* files. r=mfinkle

re: CastingDevices vs Devices

I'm glad you thought about the naming, but I kinda want this to be somewhat generic. Many types of devices can be "discovered" via SSDP, maybe even other Firefoxes running on laptops and such. I noticed that Windows and Linux both have "Devices", where Rokus, Chromecasts and Smart TVs show up.

If it's not a pain, I'd rather go with a "Device" naming.

>diff --git a/mobile/android/chrome/content/aboutCastingDevices.js b/mobile/android/chrome/content/aboutCastingDevices.js
>+// TODO: Export these from SimpleServiceDiscovery.
>+const EVENT_SERVICE_FOUND = "ssdp-service-found";
>+const EVENT_SERVICE_LOST  = "ssdp-service-lost";

personally I'm not a fan of aligning "=" but I can look away.

>+  uninit: function() {
>+	  dump("uninit");
>+	  Services.obs.removeObserver(this, EVENT_SERVICE_FOUND);
>+	  Services.obs.removeObserver(this, EVENT_SERVICE_LOST);
>+
>+    if (this._savedSearchInterval > 0) {
>+	    SimpleServiceDiscovery.search(this._savedSearchInterval);
>+    }
>+  },

Indents are off in this function

>+  _createItemForDevice: function(device) {

I can see us adding more details here (model manufacturer, etc) but that's a new bug

>+  updateDeviceList: function() {

>+    let list = document.getElementById("devices-list");
>+    while (list.firstChild) {
>+      list.removeChild(list.firstChild);
>+    }

There's a "faster" way to do this, but I'm fine with this since the list should be small.

fwiw:

let list = document.getElementById("devices-list");
list.innerHTML = "";

(each removeChild causes a layout and render. setting innerHTML is one shot)

>+  observe: function(subject, topic, data) {
>+    if (topic == EVENT_SERVICE_FOUND || EVENT_SERVICE_LOST) {

fix:
      if (topic == EVENT_SERVICE_FOUND || topic == EVENT_SERVICE_LOST) {

Great start. Fixup the few issues and consider the naming. Let's wait until after Merge Day to land though.
Attachment #8413256 - Flags: review?(mark.finkle) → review+
Comment on attachment 8413257 [details] [diff] [review]
Part 2: Add MOZ_CASTING and register about:devices. r=mfinkle

In a new bug we could use MOZ_CASTING to wrap some more code
Attachment #8413257 - Flags: review?(mark.finkle) → review+
Attachment #8413258 - Flags: review?(mark.finkle) → review+
Comment on attachment 8413259 [details] [diff] [review]
Post: Return existing interval from search(). r=mfinkle

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

>+    this._savedSearchInterval =
>+      SimpleServiceDiscovery.search(SEARCH_INTERVAL_IN_MILLISECONDS);

No need to wrap
Attachment #8413259 - Flags: review?(mark.finkle) → review+
Clearing needinfo. I'm not really a fan of us hiding more system info across tons of different about pages, but finkle already knows that.
Flags: needinfo?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #10)
> Clearing needinfo. I'm not really a fan of us hiding more system info across
> tons of different about pages, but finkle already knows that.

I don't think we have decided that about:devices would ship to release, but it will be very handy for debugging. That said, I'm not sure where else we'd put this kind of capabilities. This could easily grow into something like about:addons or about:downloads. We might convert it into a native UI, but I think we'll have enough features and information related to devices to need a specific UI for it.
mfinkle: so:

* MOZ_DEVICES build flag;
* about:devices registration;
* aboutDevices.* files.

good?
Flags: needinfo?(mark.finkle)
(In reply to Nick Alexander :nalexander from comment #12)
> mfinkle: so:
> 
> * MOZ_DEVICES build flag;
> * about:devices registration;
> * aboutDevices.* files.
> 
> good?

Good
Flags: needinfo?(mark.finkle)
Depends on: 1006046
Assignee: nobody → nalexander
Blocks: 1254729
You need to log in before you can comment on or make changes to this bug.