Closed Bug 784165 Opened 8 years ago Closed 8 years ago

Differentiate Windows 8 Metro browser vs Desktop browser

Categories

(Toolkit :: Telemetry, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: bbondy, Assigned: jimm)

References

Details

(Whiteboard: metro-preview, completed-elm)

Attachments

(1 file, 1 obsolete file)

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.
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.
Maybe I have to file it to metrics to show them differently given that they have the same app id
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
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.
If the appid is the issue we could send up a virtual not real appid for the metro browser.
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.
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.
Blocks: 784389
Attached patch patch (obsolete) — Splinter Review
Assignee: netzen → jmathies
Attachment #660056 - Flags: review?(benjamin)
Blocks: 790212
Whiteboard: metro-preview
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).
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(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.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Attachment #660056 - Flags: review?(benjamin)
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
Attached patch patchSplinter Review
Attachment #660056 - Attachment is obsolete: true
Attachment #660829 - Flags: review?(gavin.sharp)
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.
Attachment #660829 - Flags: review?(gavin.sharp)
Attachment #660829 - Flags: review+
Attachment #660829 - Flags: feedback?(bdahl)
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.
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 '=='.
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!
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
Whiteboard: metro-preview → metro-preview, completed-elm
https://hg.mozilla.org/mozilla-central/rev/eb6caf30255c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.