784165 - Differentiate Windows 8 Metro browser vs Desktop browser

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Brian R. Bondy [:bbondy]]

We should send up telemetry filtering data differently for when Firefox is running in Immersive (metro) vs Desktop mode.

Possible places we can differentiate:
- app name
- os type
- os version

I checked the dashboard and it doesn't show up as an available app name, but maybe because we're running with debug builds mostly.  It's possible that we don't need to do anything here if it shows up as a different app name already.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/projects/elm/file/a014bd12c148/browser/metro/application.ini.in shows us using the same App ID as desktop Firefox, and a different name. I think those should both be different.

Telemetry already sends both the app name and ID, so that should be enough for differentiation.

Comment 2

[Brian R. Bondy [:bbondy]]

Maybe I have to file it to metrics to show them differently given that they have the same app id

Comment 3

[Jim Mathies [:jimm]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> https://hg.mozilla.org/projects/elm/file/a014bd12c148/browser/metro/
> application.ini.in shows us using the same App ID as desktop Firefox, and a
> different name. I think those should both be different.
> 
> Telemetry already sends both the app name and ID, so that should be enough
> for differentiation.

There was an issue with different app ids and the sharing of the same dist/bin folder. So I don't think we can change that. I checked the adu ping data which sends the name and id, so we should be ok there fi they want to split those up.

cc'ing bsmedberg on the id thing, I don't remember the exact reason why we have to have the same id for both apps.

Comment 4

[Brian R. Bondy [:bbondy]]

I think the reasoning was because we didn't want to segregate the add-on market.  I'm not sure if that applies to newer style add-ons with the sdk add-on though.

Comment 5

[Brian R. Bondy [:bbondy]]

If the appid is the issue we could send up a virtual not real appid for the metro browser.

Comment 6

[Benjamin Smedberg]

I think that with the decision to not allow addons at first and then significantly change what kinds of addons are allowed we can change the ID without a problem.

Comment 7

[Brian R. Bondy [:bbondy]]

Confirmed on #metrics that we need to change the appid.  Also I need to file a bug for them to enable the new product in the telemetry dashboard.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 660056 [details] [diff] [review]
patch

The entry in services/sync/modules/constants.js is unused, and the addition to accessible/src/jsat/Utils.jsm looks pointless without additional changes to make that harness support metro, so I think you should omit them (and maybe file a followup bug on the latter, if it makes sense).

Comment 10

[Jim Mathies [:jimm]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #9)
> Comment on attachment 660056 [details] [diff] [review]
> patch
> 
> The entry in services/sync/modules/constants.js is unused

If they start using those wouldn't it be smart to have the new id added to the list so people know about it? Alternatively maybe we could simply remove all of these assuming they are all unused.

> and the addition
> to accessible/src/jsat/Utils.jsm looks pointless without additional changes
> to make that harness support metro, so I think you should omit them (and
> maybe file a followup bug on the latter, if it makes sense).

Ok I'll file a follow up on the accessible jsat thing.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jim Mathies [:jimm] from comment #10)
> If they start using those wouldn't it be smart to have the new id added to
> the list so people know about it? Alternatively maybe we could simply remove
> all of these assuming they are all unused.

If they need to start distinguishing Metro, they can pretty easily look up its ID and add it. I don't think there's any benefit to adding it pre-emptively. I guess the FENNEC and TEST_HARNESS_ID ones are currently unused, but that's just because http://hg.mozilla.org/mozilla-central/rev/03b4cf2bdc71 forgot to remove them.

Comment 12

[Jim Mathies [:jimm]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #11)
> (In reply to Jim Mathies [:jimm] from comment #10)
> > If they start using those wouldn't it be smart to have the new id added to
> > the list so people know about it? Alternatively maybe we could simply remove
> > all of these assuming they are all unused.
> 
> If they need to start distinguishing Metro, they can pretty easily look up
> its ID and add it. I don't think there's any benefit to adding it
> pre-emptively. I guess the FENNEC and TEST_HARNESS_ID ones are currently
> unused, but that's just because
> http://hg.mozilla.org/mozilla-central/rev/03b4cf2bdc71 forgot to remove them.

Ah ok, so we do need to add this, and update the engines pref reading code too.

Comment 13

[Jim Mathies [:jimm]]

Hmm, on second though I guess I don't need to mess with this. The current pref reading code doesn't use any of these. The only id in use appears to be seamonkey.

http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/service.js#379

Comment 14

[Jim Mathies [:jimm]]

Attachment: patch

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 660829 [details] [diff] [review]
patch

Those manifest restrictions are unnecessary now, I think, but that's a separate issue that I'll file separately.

Flagging bdahl for the pdf.js changes which look pretty straightforward, but will need to be up-streamed.

Comment 16

[Jim Mathies [:jimm]]

Note, the mc parts for pdfjs will land on mc now so I don't have to track them for our elm->mc merge coming up this fall.

Comment 17

[Brendan Dahl [:bdahl]]

Opened a PR for pdf.js at https://github.com/mozilla/pdf.js/pull/2116 with the above changes except one minor difference of using '===' instead of '=='.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

jimm: m-c isn't upstream enough :) we need to land them in pdf.js's upstream github repo to avoid them being clobbered on the next pdf.js update.

bdahl: thanks!

Comment 19

[Jim Mathies [:jimm]]

just the pdfjs and sync parts, w/the '===' update:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6caf30255c

plus the app.ini change for elm:
https://hg.mozilla.org/projects/elm/rev/f9639b517a08

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb6caf30255c