723178 - Re-add performance metrics to AMO ping

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch for TB&SM [Checkin: Comment 8]

+++ This bug was initially created as a clone of Bug #716736 +++

Bug 682356 landed ~2 months ago, removing the performance data from the metadata ping to AMO (effectively backing out bug 623950), after bug 668392 added the list of addons to telemetry. However, it turns out that the data from Telemetry is insufficient for AMO's needs. 

So bug 623950 needs to be re-landed (ie, undo the change bug 682356 did), but with the performance data only being sent when it's part of the periodic background update check.

Note that bug 682356 landed before the previous merge - so this fix will need to land on Aurora too.

Comment 1

[Justin Wood (:Callek)]

Comment on attachment 593502 [details] [diff] [review]
patch for TB&SM [Checkin: Comment 8]

I reviewed both patches, But I do feel that for at least branch landing of the TB ones, we need mark's rs or review. Approvals also require the dependent patch to land of the Firefox branches.

My one required change is to expand the %APP% into the app strings, since its what Firefox's patch did and I don't see any test from my glance here that does the APP expand for this pref.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 593502 [details] [diff] [review]
patch for TB&SM [Checkin: Comment 8]

r=Standard8. Please definitely make the %APP% -> thunderbird (and seamonkey) as Callek requested.

Comment 3

[Mark Banner (:standard8)]

I know it doesn't quite fit, but lets move this to mailnews core where we can track it landing for both apps.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 593502 [details] [diff] [review]
patch for TB&SM [Checkin: Comment 8]

I wondered if that might happen.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Justin Wood (:Callek) from comment #1)
> My one required change is to expand the %APP% into the app strings, since
> its what Firefox's patch did and I don't see any test from my glance here
> that does the APP expand for this pref.

(In reply to Mark Banner (:standard8) from comment #2)
> r=Standard8. Please definitely make the %APP% -> thunderbird (and seamonkey)
> as Callek requested.

Bah, two reviewers and it's me who needs to figure out the details in order for you to allow me to not repeat FF's mistakes... Well, here we go:

* I created the new extensions.getAddons.getWithPerformance.url pref as a copy of the *existing* extensions.getAddons.get.url pref (just one line above!), which *already and still does* use %APP% - for SM and TB, but not FF.
** If you look at the strings you'll easily see that %APP% is used in the exact same places (mind you: copy!)
** If the %APP% expansion wouldn't work, then extensions.getAddons.get.url would be broken, too!
** If I make the change you requested, we'll have an inconsistency and thus a greater chance for an unnoticed breakage.

Note: extensions.getAddons.get.url (and thus the copy, too) uses the variables %LOCALE%, %APP%, %API_VERSION%, %IDS%, %OS%, and %VERSION%.

So, what happens? (all Toolkit/Core code)
1. _beginGetAddons uses a local variable called "params" which unconditionally defines the variables API_VERSION and IDS, and conditionally TIME_MAIN, TIME_FIRST_PAINT and TIME_SESSION_RESTORED.
2. _beginGetAddons calls _formatURLPref with the "params" variable.
3. _formatURLPref substitutes anything from "params", then calls nsURLFormatterService.formatURLPref. What's left at this point is %LOCALE%, %APP%, %OS% and %VERSION%.
4. formatURLPref delegates to formatURL, which substitutes anything that member variable "_defaults" contains, among them LOCALE, APP, OS, and VERSION.

Blame will happily tell you that the "APP" line in "_defaults" has been changed the last time in 2008, but was present even before that. So it's not trunk-only and for sure on Aurora and Beta, too. Did I mention that extensions.getAddons.get.url would be broken, too, if that was not the case?

Now if I didn't convince you then it's of course your right to make the changes you required upon checkin, but in that case, please use your own name for the patch author field. I don't want to put my name under that change.

Comment 6

[Justin Wood (:Callek)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #5)
> Now if I didn't convince you then it's of course your right to make the
> changes you required upon checkin, but in that case, please use your own
> name for the patch author field. I don't want to put my name under that
> change.

As for me, I still think matching Firefox is better than diverging here, (for these metrics bugs we have traditionally matched firefox char for char except where we need to switch, like s/firefox/seamonkey/) Though given your investigation I won't bikeshed on this particular issue, so you can land that as-is. I just know I want to see this landed!

I can't speak for mark though.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Mark: ping.

Comment 8

[Mark Banner (:standard8)]

Ok, I just did a bit more digging. The Firefox prefs used to be %APP%, it was bug 650246 that changed them.

Given that we use %APP% at the moment, lets be consistent. I don't see a need to port that bug unless requested.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 593502 [details] [diff] [review]
patch for TB&SM [Checkin: Comment 8]

http://hg.mozilla.org/comm-central/rev/01e1c291493e
http://hg.mozilla.org/releases/comm-aurora/rev/6b8a03850000
http://hg.mozilla.org/releases/comm-beta/rev/092136562289