As part of investigating bug 1009746 and bug 1012214, I discovered that the settings tests listed in dom/settings/tests/chrome.ini weren't being run. Well, dom/settings/tests/moz.build has: if CONFIG['MOZ_B2G']: MOCHITEST_CHROME_MANIFESTS += ['chrome.ini'] But when I look through TBPL, I see no "oth" tests shown for any of the B2G builds. So this means, that effectively the settings chrome tests aren't being run.

chrome.ini should just have [test_settings.xul], not the .js file

Comment on attachment 8424480 [details] [diff] [review] patch I think you need a build guru to tell you how to get the testing you want.

This is also going to need to add SettingsService to the browser package manifest, otherwise the tests fail on desktop

https://tbpl.mozilla.org/?tree=Try&rev=cc0b1933c80d Yes the permissions test failed here, but it at least shows that we /are/ running the tests which is what this patch is expected to do, and it no longer fails e10s like the old patch did either.

I don't think we want to expose the SettingsService to all desktop builds. We have to find another solution here.

That should just be a matter of turning the or's in the moz.build files into and's, right? MOZ_B2G and ENABLE_TESTS?

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #9) > That should just be a matter of turning the or's in the moz.build files into > and's, right? MOZ_B2G and ENABLE_TESTS? According to bent we ship production firefox with ENABLE_TESTS. We have to double check with some build folks.

That'd be fine though, because we don't ship it with MOZ_B2G also, right?

So do we want to taking the Settings API out of non B2G/Mulet-desktop completely?

Oops, too broad. Do we want to taking the SettingsManager out of non B2G/Mulet-desktop completely?

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #13) > Oops, too broad. Do we want to taking the SettingsManager out of non > B2G/Mulet-desktop completely? The SettingsManager is only accessed via the DOM API and we don't expose it for web content. It should be included in B2G/Mulet. The SettingsService is accessed in our chrome code and we shouldn't use it for our Desktop build.

Fixed test, try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1bc81cff45a, though this patch also contains the required clobber to fix OS X builds.

Pushed at a=TEST-ONLY since :gwagner is apparently too managerial to get his code reviews done in under a year now. :) https://hg.mozilla.org/integration/mozilla-inbound/rev/756fcc79ab98

Annnnnnnnd backed out due to OS X bundle building bustage. If only all of my tree burns were this alliterative. Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b0acc875712

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #18) > Annnnnnnnd backed out due to OS X bundle building bustage. If only all of my > tree burns were this alliterative. > > Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b0acc875712 seems https://treeherder.mozilla.org/logviewer.html#?job_id=10649739&repo=mozilla-inbound is also a real failure from this patch (its also on the try run)

and https://treeherder.mozilla.org/logviewer.html#?job_id=10651623&repo=mozilla-inbound seems to be real too

Ok, turns out that geolocation uses the existence of SettingsService to tell whether or not it should use settings so it totally breaks on desktop if I add SettingsService everywhere. Now ifdef'd it for B2G, but unfortunately that means this is no longer a=TEST-ONLY. :(

Try passing on all platforms other than windows due to me pulling a bad inbound: https://treeherder.mozilla.org/#/jobs?repo=try&revision=842c824eeec6 Try on windows with valid inbound: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1b6e4c13e7

Before I review and after a quick look to the patches, allow me to ask a dumb question: why? Why were those tests not being run?

The reason why I hesitated to review this patch is that we were running in situations where people start using the settings service on desktop by accident. We have to make sure nobody is using the settings service on desktop.

Would a pref be enough of a guard for that?

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #26) > Would a pref be enough of a guard for that? How would the pref work?

I could imagine some flag that we have to set when we run the tests. When we don't set the flag and request the settingsService we should assert.

Oh, hmm, I guess this is created via factory and is xpcom, so we can't easily pref off creation of the service. Should we just run this test on B2G then?

Comment on attachment 8621775 [details] [diff] [review] Patch 1 (v5) - User Timing API Implementation Review of attachment 8621775 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/installer/package-manifest.in @@ +535,5 @@ > @RESPATH@/components/messageWakeupService.manifest > @RESPATH@/components/SettingsManager.js > @RESPATH@/components/SettingsManager.manifest > +@RESPATH@/components/SettingsService.js > +@RESPATH@/components/SettingsService.manifest Why do we package on browser ?

Comment on attachment 8621775 [details] [diff] [review] Patch 1 (v5) - User Timing API Implementation Sorry, meant to turn off review on this. We're probably just going with running this on ICS emulator only, meaning it'll be a TEST-ONLY patch.

And back to test only we go. Passing chrome ICS: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7d856a67fce