564092 - Make the fragment a map of {guid: add-on name} in the Discovery Pane

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jeff Balogh (:jbalogh)]

Or the other way around.  The direction doesn't matter.

I need the add-on names for the Support section of about:addons[1].  It displays all the user's add-ons with a link to a support page on AMO.  We can't ask AMO for a guid => name map because that hurts privacy.

[1]: http://docs.google.com/Doc?docid=0Acwo2Bn17-PrZGZudHRobnJfNDJ0cTV0NjRnZA&hl=en&pli=1 (at the bottom)

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Took me awhile to figure out this is for the Discover pane.

Moving it over to the meta bug for that.

Comment 2

[Justin Scott [:fligtar]]

A lot of the Discovery Pane is ready now, but not having this hash makes a lot of the testing we were hoping to do with the scaffolding moot.

When do you think this will land?

Also, it's probably a good idea to include more than just the GUID and name. Let's do name, version, and status for each GUID.

Comment 3

[Dave Townsend [:mossop]]

So do you want this as a map of id: {name, version, status} or an array of {id, name, version, status}?

Also what is status? The status we send with update requests or something else?

Comment 4

[Justin Scott [:fligtar]]

Up to Jeff but I'd think the former. And status is the same as update requests, such as userEnabled, blocklisted, etc.

Comment 5

[Jeff Balogh (:jbalogh)]

(In reply to comment #3)
> So do you want this as a map of id: {name, version, status} or an array of {id,
> name, version, status}?

{<guid>: {name: <name>, version: <version>, ...}} please.  Thanks!

Are there any other details that you think might be interesting to us?  I'd like to get everything we can up front before we're stuck with what's in Firefox 4.

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #4)
> Up to Jeff but I'd think the former. And status is the same as update requests,
> such as userEnabled, blocklisted, etc.

Rather than replicating the status string and then presumably having JS in the page to parse it into something useful I think it would be easier both on our and your code for us to include specific properties in the object, userDisabled, isCompatible, isBlocklisted.

(In reply to comment #2)
> When do you think this will land?

Going to try to make sure it is done by the end of the week.

Comment 7

[Dave Townsend [:mossop]]

The patch I have working for this is generating URLs like this:

https://services.addons.mozilla.org/en-US/firefox/discovery/3.7a5pre/Darwin#{[e-mail]%22:{%22name%22:%22DOM%20Inspector%22,%22version%22:%222.0.0%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},[e-mail]%22:{%22name%22:%22Timer%20Fire%22,%22version%22:%221.5%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{75739dec-72db-4020-aa9a-6afa6744759b}%22:{%22name%22:%22Extension%20Developer%22,%22version%22:%220.3.0.20060726%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{8620c15f-30dc-4dba-a131-7c5d20cf4a29}%22:{%22name%22:%22Nightly%20Tester%20Tools%22,%22version%22:%222.0.3%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{F8A55C97-3DB6-4961-A81D-0DE0080E53CB}%22:{%22name%22:%22Download%20Manager%20Tweak%22,%22version%22:%220.9.2%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{972ce4c6-7e08-4474-a285-3208198ce6fd}%22:{%22name%22:%22Default%22,%22version%22:%223.7a5pre%22,%22type%22:%22theme%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{94f5cfb8-19e0-bf48-8f9f-ac53e023daef}%22:{%22name%22:%22Test%20Plug-in%22,%22version%22:%221.0.0.0%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{c6148506-96cc-ba40-800c-55ce6c07ef49}%22:{%22name%22:%22Default%20Plugin%22,%22version%22:%222.0%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{a8d57c58-0ace-2e44-89db-449f30980e93}%22:{%22name%22:%22Facebook%20Plug-In%22,%22version%22:%221.0%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{f14e330a-7b2b-a146-b395-1a9cd989a734}%22:{%22name%22:%22iPhotoPhotocast%22,%22version%22:%227.0%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{20bf7604-a61e-4c4d-ba95-503ff62e15ed}%22:{%22name%22:%22Silverlight%20Plug-In%22,%22version%22:%223.0.50106.0%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{7b1ee460-2e83-bb47-a666-6d899ffca5c4}%22:{%22name%22:%22Flip4Mac%20Windows%20Media%20Plugin%202.3.2%20%22,%22version%22:%222.3.2.6%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{91bf27f7-c749-914f-a088-8dcbe83c2e5c}%22:{%22name%22:%22Shockwave%20Flash%22,%22version%22:%2210.1.53.38%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{df72c059-2261-c044-9fc1-d00d9a633c08}%22:{%22name%22:%22QuickTime%20Plug-in%207.6.6%22,%22version%22:%227.6.6%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false}}

You'll note that there is a little encoding going on there, not sure there is an easy way around that on my end and it's probably easy to solve on your side. It also contains all types of add-ons, plugins included so if you don't care about those you'll need to filter out only the ones you do care about.

Does this look ok?

Comment 8

[Wil Clouser [:clouserw]]

URLs like that are going to make our front end caches pretty much worthless.  Do we really need all this info and can we load it via AJAX instead?

Comment 9

[Wil Clouser [:clouserw]]

(In reply to comment #8)
> URLs like that are going to make our front end caches pretty much worthless. 
> Do we really need all this info and can we load it via AJAX instead?

Jeff points out this is all behind a hash so I take back everything bad that I said. :)

Comment 10

[Jeff Balogh (:jbalogh)]

(In reply to comment #7)
> The patch I have working for this is generating URLs like this:
> Does this look ok?

That's beautiful. JSON.parse(location.hash.slice(1)) handles it fine.

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Fairly straightforward stuff, just appends the data to the url we load.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 445225 [details] [diff] [review]
patch rev 1

That is an epic hash :)

Do we want to eventually force a refresh/change of hash when a new addon is installed (from there or another tab) while the Discover pane is the active pane?

Comment 13

[Jeff Balogh (:jbalogh)]

(In reply to comment #12)
> (From update of attachment 445225 [details] [diff] [review])
> That is an epic hash :)

Is there any limit on the length of the hash/url that we could run into?

> Do we want to eventually force a refresh/change of hash when a new addon is
> installed (from there or another tab) while the Discover pane is the active
> pane?

I wouldn't mind (that would trigger onhashchange, right?).  If the discovery pane knows the add-ons have changed, it can offer to update the personal recommendations.  Otherwise we'll be looking for changes next time the page is reloaded.  How often would that happen?

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 445225 [details] [diff] [review] [details])
> > That is an epic hash :)
> 
> Is there any limit on the length of the hash/url that we could run into?

I am told that the total url string (url + hash) must be less than 4294967295 characters. The URL here shows us taking something like 250 characters per add-on which means we might only be able to list 17179869 add-ons in the hash, do you think that is enough?

> > Do we want to eventually force a refresh/change of hash when a new addon is
> > installed (from there or another tab) while the Discover pane is the active
> > pane?
> 
> I wouldn't mind (that would trigger onhashchange, right?).  If the discovery
> pane knows the add-ons have changed, it can offer to update the personal
> recommendations.  Otherwise we'll be looking for changes next time the page is
> reloaded.  How often would that happen?

Right now we only regenerate the URL when the add-ons manager is reloaded. We can file a separate bug on updating it while the window is open. I think it's a nice to have but not super important at this stage.

Comment 15

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/1758bbc9aaeb

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100517 Minefield/3.7a5pre

With the above build the URL looks like:

https://services.addons.mozilla.org/en-US/firefox/discovery/3.7a5pre/Darwin#{%22{972ce4c6-7e08-4474-a285-3208198ce6fd}%22:{%22name%22:%22Default%22,%22version%22:%223.7a5pre%22,%22type%22:%22theme%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{8f8fe09b-0bd3-4470-bc1b-8cad42b8203a}%22:{%22name%22:%22Live%20HTTP%20headers%22,%22version%22:%220.16%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{096bcd60-2fca-3d49-804f-3f48d5a4f23c}%22:{%22name%22:%22Microsoft%20Office%20Live%20Plug-in%22,%22version%22:%2212.2.3%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{e4107ea1-f632-9f47-a378-99e5ee9b1db2}%22:{%22name%22:%22Shockwave%20Flash%22,%22version%22:%2210.0.42%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{2aa7df1b-5f6c-7048-92db-49b9aab7bf07}%22:{%22name%22:%22QuickTime%20Plug-in%207.6.6%22,%22version%22:%227.6.6%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false}}

Is there any testing needed?

Comment 17

[Dave Townsend [:mossop]]

Added tests for this in bug 603804

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #17)
> Added tests for this in bug 603804

The question here is what has be tested on the discovery pane, especially with the data we get from AOM. Probably we should move the litmus request over there?

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #18)
> (In reply to comment #17)
> > Added tests for this in bug 603804
> 
> The question here is what has be tested on the discovery pane, especially with
> the data we get from AOM. Probably we should move the litmus request over
> there?

All we can test automatically is that the pane attempts to load the correct URL, which is covered. Anything else would require network access which automated tests aren't allowed to do so that the contents of the pane renders correctly etc. needs to be checked. I don't know if AMO has automated tests verifying that already though.