Host devices catalog on a CDN

RESOLVED FIXED in Firefox 39

Status

()

Firefox
Developer Tools
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: janx, Assigned: janx)

Tracking

unspecified
Firefox 39
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(4 attachments, 12 obsolete attachments)

12.38 KB, application/json
Details
26.28 KB, patch
janx
: review+
Details | Diff | Splinter Review
9.65 KB, patch
janx
: review+
Details | Diff | Splinter Review
2.72 KB, patch
janx
: review-
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Instead of embedding the catalog of common devices in code, it would be better to host it on a CDN (e.g. alongside WebIDE's addon list) so that it can be kept up-to-date.
(Assignee)

Updated

3 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 1

3 years ago
Sole, can you please help with this? I'll upload the `devices.json` file here in a few days.
Flags: needinfo?(sole)
(Assignee)

Comment 2

3 years ago
Created attachment 8567918 [details]
devices.json

Here is the list I'd like to upload to the CDN.

I also hosted it at https://github.com/jankeromnes/devices.json, maybe at some point we'd want that under github.com/mozilla/ to accept pull requests.
(Assignee)

Comment 3

3 years ago
Created attachment 8567919 [details]
devices.json
Attachment #8567918 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Comment 5

3 years ago
Created attachment 8570087 [details] [diff] [review]
Make devices.js use a CDN.

In addition to hosting the list on a CDN, we also need `devices.js` to actually use it. This for that.
(Assignee)

Comment 6

3 years ago
Created attachment 8570090 [details]
devices.json
Attachment #8567923 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8570100 [details] [diff] [review]
Make devices.js use a CDN.
Attachment #8570087 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1090949
Summary: Host device catalog on a CDN → Host devices catalog on a CDN
(Assignee)

Comment 8

3 years ago
Comment on attachment 8570100 [details] [diff] [review]
Make devices.js use a CDN.

Modulo the pref pointing to my personal github repo (since the list is not uploaded to the CDN yet) what do you think?
Attachment #8570100 - Flags: review?(poirot.alex)
(Assignee)

Comment 9

3 years ago
Tim, I guess you are using browser/devtools/shared/devices.js in your Emulation devtool prototype :)

Just FYI, this patch changes the API a little, from:

    require('…').Devices.FirefoxOS` / `.Others`

to a single list:

    `require('…').GetDevices().then(function(devices){ /* devices.phones, etc */ })`
Flags: needinfo?(ntim007)

Comment 10

3 years ago
(In reply to Jan Keromnes [:janx] from comment #9)
> Tim, I guess you are using browser/devtools/shared/devices.js in your
> Emulation devtool prototype :)
> 
> Just FYI, this patch changes the API a little, from:
> 
>     require('…').Devices.FirefoxOS` / `.Others`
> 
> to a single list:
> 
>     `require('…').GetDevices().then(function(devices){ /* devices.phones,
> etc */ })`

I probably won't have time to change this, but feel free to :)
Flags: needinfo?(ntim007)
Comment on attachment 8570100 [details] [diff] [review]
Make devices.js use a CDN.

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

I would like to see usages of GetDevices and GetDevicesString, and final https link, before r+.

::: browser/devtools/shared/devices.js
@@ +12,4 @@
>  const Strings = Services.strings.createBundle("chrome://browser/locale/devtools/device.properties");
>  
> +/* `Devices` is a catalog of common web-enabled devices and their properties,
> + * intended for (mobile) device emulation.

You would have to slightly update this comment as there is no more `Devices` global.
Attachment #8570100 - Flags: review?(poirot.alex) → feedback+
Jan, you should ping the server ops to get access to the CDN. I can't be the bottleneck in updating this :-)
They should give you S3 credentials so you can upload the file directly. I will ask them with a needinfo?
Flags: needinfo?(sole) → needinfo?(server-ops-webops)
(Assignee)

Comment 13

3 years ago
Created attachment 8571930 [details]
devices.json

Thanks Sole!

Dear server ops, please grant me S3 access so I can upload `devices.json` to a URL like https://code.cdn.mozilla.net/devices/devices.json
Attachment #8570090 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
(stealing Sole's needinfo so I can track it)
Flags: needinfo?(server-ops-webops)
(Assignee)

Comment 15

3 years ago
Dear server ops, please grant me S3 access so I can upload the `devices.json` file to a URL like:

https://code.cdn.mozilla.net/devices/devices.json
Flags: needinfo?(server-ops-webops)
(Assignee)

Comment 16

3 years ago
Created attachment 8573879 [details] [diff] [review]
Make devices.js use a CDN.

Fixed nits, added tests. Still uses the GitHub link. For usage examples, see:

- browser/devtools/shared/test/browser_devices.js (this patch)
- browser/devtools/webide/content/simulator.js (bug 1090949)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=001f923e04e0
Attachment #8570100 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
Created attachment 8574009 [details]
devices.json

Since Chris Turra left and "server-ops-webops@mozilla-org.bugs" is not a real address,

Philippe, could you please grant me s3 credentials so that I can upload the `devices.json` file to a URL like:

https://code.cdn.mozilla.net/devices/devices.json ?

That CDN was set up in bug 981801.
Attachment #8571930 - Attachment is obsolete: true
Flags: needinfo?(server-ops-webops) → needinfo?(gozer)
(Assignee)

Comment 18

3 years ago
Comment on attachment 8573879 [details] [diff] [review]
Make devices.js use a CDN.

Alex, apart from the soon-to-be-fixed URL, what do you think of the patch now? Please see comment 16.
Attachment #8573879 - Flags: review?(poirot.alex)
Comment on attachment 8573879 [details] [diff] [review]
Make devices.js use a CDN.

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

Looks good but I'd like to see usages of devices.js before r+.
Attachment #8573879 - Flags: review?(poirot.alex) → feedback+
Flags: needinfo?(gozer) → needinfo?(rsoderberg)

Updated

3 years ago
Whiteboard: [kanban:https://webops.kanbanize.com/ctrl_board/2/712]
Jan,

So I've figured out how to give you access, I just need to make sure I can send over the details over to you securely. 

Once I've done that, I'll get back to you (or Richard will).
Jan,

Couple of questions :

1) What's a good way to get you something securely? (do you use gpg/1password etc?) 
2) How are you planning to upload things? Via the AWS web interface or the awscli or?
Flags: needinfo?(rsoderberg) → needinfo?(janx)
(Assignee)

Comment 22

3 years ago
Thanks Shyam!

1) Is there a way to simply authorize my SSH public key / similar? Or else I do use GPG.
2) I've never done that before, but I guess I'll use the AWS web interface if it's handy, or simply linux tools like scp / sftp / other (maybe the awscli if I have to).
Flags: needinfo?(janx) → needinfo?(smani)
If you have SSH access to people.mozilla.org, maybe Shyam is able to place the AWS credentials in your people.m.o user directory?
(In reply to J. Ryan Stinnett [:jryans] from comment #23)
> If you have SSH access to people.mozilla.org, maybe Shyam is able to place
> the AWS credentials in your people.m.o user directory?

This is prohibited by security policy, sorry. Access is via IAM credentials (rather than SSH key) and so GPG would be the correct choice here.
(In reply to Richard Soderberg [:atoll] from comment #24)
> (In reply to J. Ryan Stinnett [:jryans] from comment #23)
> > If you have SSH access to people.mozilla.org, maybe Shyam is able to place
> > the AWS credentials in your people.m.o user directory?
> 
> This is prohibited by security policy, sorry. Access is via IAM credentials
> (rather than SSH key) and so GPG would be the correct choice here.

Hmm, it's possible I misunderstand...  but I was not trying to suggest the SSH key is used with AWS directly.  Instead, you would create the AWS IAM credentials, and then place them in Jan's people.m.o directory, so he can retrieve the credentials in a secure way via SSH.  His SSH key is only used to retrieve the new AWS credentials you have generated.

I believe that's the approach Chris Turra used when setting up Sole's account for this same CDN bucket (see bug 981801 comment 17 and other comments).
(In reply to Jan Keromnes [:janx] from comment #22)
> Thanks Shyam!
> 2) I've never done that before, but I guess I'll use the AWS web interface
> if it's handy, or simply linux tools like scp / sftp / other (maybe the
> awscli if I have to).

If it's a single file, the web interface might be handy enough.

If you can email me your gpg key-id or public key, I'll be happy to finish this setup and email your credentials to you. 

I'm also going to spin off an actual bug to track that work (for our purposes)
Flags: needinfo?(smani)

Updated

3 years ago
Depends on: 1142830

Updated

3 years ago
Whiteboard: [kanban:https://webops.kanbanize.com/ctrl_board/2/712]
Jan,

Please update Bug 1142830 in the future regarding access. Thanks!
(Assignee)

Comment 28

3 years ago
Thanks Shyam!

Uploaded successfully to https://code.cdn.mozilla.net/devices/devices.json
(Assignee)

Comment 29

3 years ago
Created attachment 8579557 [details] [diff] [review]
Make devices.js use a CDN.

Alex, please have a look. The patch finally uses the CDN, has tests, and is required for bug 1090949.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69245826a7d6
Attachment #8573879 - Attachment is obsolete: true
Attachment #8579557 - Flags: review?(poirot.alex)
Comment on attachment 8579557 [details] [diff] [review]
Make devices.js use a CDN.

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

Sorry for the drive-by, just a few quick tweaks I wanted to suggest.

::: browser/devtools/shared/devices.js
@@ +4,5 @@
>  
>  "use strict";
>  
>  const { Ci, Cc } = require("chrome");
> +const { GetDevicesJSON } = require("devtools/webide/remote-resources");

It feels a bit awkward for |shared| modules to depend on a particular tool like this.  Maybe you should invert things by moving the general |getJSON| function to a new shared module, which you use here, and then WebIDE's module also depends on?

::: browser/devtools/webide/webide-prefs.js
@@ +17,5 @@
>  pref("devtools.webide.adbAddonURL", "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/adb-helper/#OS#/adbhelper-#OS#-latest.xpi");
>  pref("devtools.webide.adbAddonID", "adbhelper@mozilla.org");
>  pref("devtools.webide.adaptersAddonURL", "https://ftp.mozilla.org/pub/mozilla.org/labs/fxdt-adapters/#OS#/fxdt-adapters-#OS#-latest.xpi");
>  pref("devtools.webide.adaptersAddonID", "fxdevtools-adapters@mozilla.org");
> +pref("devtools.webide.devicesURL", "https://code.cdn.mozilla.net/devices/devices.json");

Since the |devices| module is shared, this pref should not be in this file, and the pref name should not contain "webide".
Attachment #8579557 - Flags: feedback+
Comment on attachment 8579557 [details] [diff] [review]
Make devices.js use a CDN.

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

What :jryans says.

::: browser/devtools/shared/devices.js
@@ +4,5 @@
>  
>  "use strict";
>  
>  const { Ci, Cc } = require("chrome");
> +const { GetDevicesJSON } = require("devtools/webide/remote-resources");

+1
Attachment #8579557 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 32

3 years ago
Created attachment 8580841 [details] [diff] [review]
Move getjson from webide/ to shared/.

As requested, moving the GetJSON function out of WebIDE and into shared territory.
Attachment #8580841 - Flags: review?(jryans)
(Assignee)

Comment 33

3 years ago
Created attachment 8580843 [details] [diff] [review]
Make devices.js use a CDN.

New patch using the moved GetJSON and declaring its pref elsewhere.

Carrying over Alex's r+.
Attachment #8579557 - Attachment is obsolete: true
Attachment #8580843 - Flags: review+
(Assignee)

Comment 34

3 years ago
Try for both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3807c13fe71f
Comment on attachment 8580841 [details] [diff] [review]
Move getjson from webide/ to shared/.

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

::: browser/devtools/shared/getjson.js
@@ +1,1 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public

I guess you really did move the file correctly?  It's quite odd that it shows the new name in the splinter review mode, but the diff view and actual patch don't show the file move...

Make sure the move really does happen!

@@ +8,5 @@
>  
>  const XMLHttpRequest = CC("@mozilla.org/xmlextras/xmlhttprequest;1");
>  
> +// Downloads and caches a JSON file from a URL given by the pref.
> +exports.GetJSON = function (prefName, bypassCache) {

I am not quite sure why Paul started these methods with an upper case letter, as it's kind of an unusual style choice I think...  I would prefer methods on an object to start with a lower case letter, like they do in nearly every other case.  If you really like this style though, I guess it's okay.

Also, methods that take boolean arguments are very confusing to read later at the call site[1].  I think it would be better to pass an object for the params here.  You can still leave out |bypassCache| for the default use case, but at least when it's used you are forced to type the words at the call site.  Or, use a different approach if you like, but I would really prefer to not end up with call sites that say |getJSON(..., true)|, since that's quite confusing to read.

I realize it was already this way before, but I think it should be a pretty simple improvement!

[1]: http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html

@@ +22,1 @@
>    let xhr = new XMLHttpRequest();

I bet this function is so much simpler with the new |fetch| API! ;) Just thinking out loud, don't need to change anything...

::: browser/devtools/webide/content/webide.js
@@ +34,5 @@
>  const MAX_ZOOM = 1.4;
>  const MIN_ZOOM = 0.6;
>  
> +// Download remote resources early
> +GetJSON("devtools.webide.addonsURL", true);

Hmm, did you mean to add this?  It doesn't seem like were actually loading the add-ons here before...

::: browser/devtools/webide/modules/addons.js
@@ +7,5 @@
>  const {AddonManager} = Cu.import("resource://gre/modules/AddonManager.jsm");
>  const {EventEmitter} = Cu.import("resource://gre/modules/devtools/event-emitter.js");
>  const {Services} = Cu.import("resource://gre/modules/Services.jsm");
> +const {GetJSON} = require("devtools/shared/getjson");
> +const EventEmitter = require("devtools/toolkit/event-emitter");

This seems to duplicate |EventEmitter| above, so remove whichever you don't like.
Attachment #8580841 - Flags: review?(jryans) → review+
(Assignee)

Comment 36

3 years ago
Thanks for the review!

Try shows a surprising amount of oranges, I'll have a look to make sure they're not my doing.

(In reply to J. Ryan Stinnett [:jryans] from comment #35)
> I guess you really did move the file correctly?  It's quite odd that it
> shows the new name in the splinter review mode, but the diff view and actual
> patch don't show the file move...

As discussed on IRC, don't trust Splinter or Diff or Unified Diff. The only source of truth is the raw patch, which you can see by clicking on the attachment name directly (and it shows that the file was moved properly).

Our review tools are quite bad at showing file moves.

> > +exports.GetJSON = function (prefName, bypassCache) {
> 
> I am not quite sure why Paul started these methods with an upper case
> letter, as it's kind of an unusual style choice I think...  I would prefer
> methods on an object to start with a lower case letter, like they do in
> nearly every other case.  If you really like this style though, I guess it's
> okay.

I believe it's a convention somewhere for stand-alone static methods, but judging from `git grep "exports\.g"` versus `git grep "exports\.G"`, you're right about it not being common in devtools. I'll fix it in my patch, since I don't care that much.

> Also, methods that take boolean arguments are very confusing to read later
> at the call site[1].  I think it would be better to pass an object for the
> params here.  You can still leave out |bypassCache| for the default use
> case, but at least when it's used you are forced to type the words at the
> call site.  Or, use a different approach if you like, but I would really
> prefer to not end up with call sites that say |getJSON(..., true)|, since
> that's quite confusing to read.
> 
> I realize it was already this way before, but I think it should be a pretty
> simple improvement!
> 
> [1]: http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html

You do make a good point, but I think "very confusing" is an exaggeration, especially since it's an optional parameter. I'm not fond of making the code structure more complex just for the sake of forcing call sites to be more verbose/explicit, and since this is not an overly public API I'll keep the code as is.

> >    let xhr = new XMLHttpRequest();
> 
> I bet this function is so much simpler with the new |fetch| API! ;) Just
> thinking out loud, don't need to change anything...

You're probably right, but I'll leave it as is for now.

> > +// Download remote resources early
> > +GetJSON("devtools.webide.addonsURL", true);
> 
> Hmm, did you mean to add this?  It doesn't seem like were actually loading
> the add-ons here before...

That was on purpose. I think it makes sense to grab the list of addons early, and since the comments says "remote resources" (plural) I suppose that line was simply forgotten originally.

> >  const {AddonManager} = Cu.import("resource://gre/modules/AddonManager.jsm");
> >  const {EventEmitter} = Cu.import("resource://gre/modules/devtools/event-emitter.js");
> >  const {Services} = Cu.import("resource://gre/modules/Services.jsm");
> > +const {GetJSON} = require("devtools/shared/getjson");
> > +const EventEmitter = require("devtools/toolkit/event-emitter");
> 
> This seems to duplicate |EventEmitter| above, so remove whichever you don't
> like.

Nice catch! It was a mistake while rewriting history, the removal of the first line ended up in a later commit locally.
(Assignee)

Comment 37

3 years ago
Looks like all the oranges were just about the duplicate `const EventEmitter`.
(Assignee)

Comment 38

3 years ago
Created attachment 8581096 [details] [diff] [review]
Move getjson from webide/ to shared/.

:jryans' r+
Attachment #8580841 - Attachment is obsolete: true
Attachment #8581096 - Flags: review+
(Assignee)

Comment 39

3 years ago
Created attachment 8581097 [details] [diff] [review]
Make devices.js use a CDN.

:ochameau's r+
Attachment #8580843 - Attachment is obsolete: true
Attachment #8581097 - Flags: review+
(Assignee)

Comment 40

3 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25e54a4f8521

(Not sure how the 2 unrelated changes snuck in, let's hope they behave.)
(Assignee)

Comment 41

3 years ago
Created attachment 8581110 [details] [diff] [review]
Move getjson from webide/ to shared/.

Renamed a function originally also called "getJSON".

Still :jryans' r+, new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1192b381bb06
Attachment #8581096 - Attachment is obsolete: true
Attachment #8581110 - Flags: review+
(Assignee)

Comment 42

3 years ago
All good now!

Sheriffs, please land the "getjson" patch first, and the "devices.js" patch after.
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Component: Simulator → Developer Tools
Product: Firefox OS → Firefox
(Assignee)

Comment 43

3 years ago
Created attachment 8581764 [details] [diff] [review]
Add method to find a device by its properties.
(Assignee)

Comment 44

3 years ago
Late addition, sheriffs please land the two previous patches (see comment 42) while the "find a device" patch is being reviewed.
Keywords: leave-open
(Assignee)

Comment 45

3 years ago
Comment on attachment 8581764 [details] [diff] [review]
Add method to find a device by its properties.

Alex, please have a look. I didn't open a separate bug because I was lazy, but I'll do it if you complain.
Attachment #8581764 - Flags: review?(poirot.alex)
(Assignee)

Comment 46

3 years ago
Comment on attachment 8581764 [details] [diff] [review]
Add method to find a device by its properties.

Actually, nevermind. I need something that doesn't return a promise.
Attachment #8581764 - Flags: review?(poirot.alex)
(Assignee)

Comment 47

3 years ago
Comment on attachment 8581764 [details] [diff] [review]
Add method to find a device by its properties.

No need to land this.
Attachment #8581764 - Flags: review-
(Assignee)

Updated

3 years ago
Keywords: leave-open
https://hg.mozilla.org/integration/fx-team/rev/d462c6963156
https://hg.mozilla.org/integration/fx-team/rev/3ec9f77647f6
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d462c6963156
https://hg.mozilla.org/mozilla-central/rev/3ec9f77647f6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.