675762 - ACR and AMO should work together on compatibility checking

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect, P3)

Description

[Justin Scott [:fligtar]]

If a user is browsing AMO with the Add-on Compatibility Reporter, AMO should not make it so difficult to install incompatible add-ons, e.g. they shouldn't be faded, should have install buttons, etc.

Comment 1

[Brian King [:kinger]]

One idea .. use custom DOM events:

https://developer.mozilla.org/en/Code_snippets/Interaction_between_privileged_and_non-privileged_pages#Sending_data_from_chrome_to_unprivileged_document

Comment 2

[Christopher Van Wiemeersch [:cvan]]

Brian, can we trigger an event from the extension itself? Just a simple boolean will suffice - and let me know the name of it so I can listen for it from the compatibility-checking JS.

Comment 3

[David McNamara [:mackers]]

OK. We intend to implement this functionality in the ACR extension as so:

1. when the user navigates to any addons.mozilla.org page, we will wait for the page's "load" event, then
2. the extension will send the page a custom event - "ACRInstalled" - to inform the page that is installed.

The extension code will look something like this:

var el = targetDoc.body;
var event = targetDoc.createEvent("Event")
event.initEvent("ACRInstalled", true, true);
el.dispatchEvent(event);

For an AMO page to catch this event, it should add an event listener for "ACRInstalled" as early as possible - e.g. during the onload handler.

I think the page's load handlers are called and finished before the browser (and extensions) are notified of the page load.

Comment 4

[David McNamara [:mackers]]

An alternative implementation for this would be for the extension to set a permanent cookie "ACRInstalled" on addons.mozilla.org. The AMO JS then checks for this cookie and updates the dispaly as appropriate.

Comment 5

[Brian King [:kinger]]

(In reply to David McNamara [:mackers] from comment #4)
> An alternative implementation for this would be for the extension to set a
> permanent cookie "ACRInstalled" on addons.mozilla.org. The AMO JS then
> checks for this cookie and updates the dispaly as appropriate.

By 'permanent', you also mean we will delete it when the ACR is disabled/removed, right? ;)

Comment 6

[David McNamara [:mackers]]

(In reply to Brian King (Briks) [:kinger] from comment #5)
> By 'permanent', you also mean we will delete it when the ACR is
> disabled/removed, right? ;)

Yep.

So do you think we should implement the event or the cookie approach? My gut feeling is to go the cookie route, as there is less chance of concurrency problems.

Also, on reflection I think the event/cookie should have a more general name, e.g. "ShowIncompatibleAddons".

Comment 7

[Wil Clouser [:clouserw]]

AMO is using a library that cvan wrote that first attempts to use local storage and then falls back to a cookie.  Sounds like we should use that in the add-on too and set the flag with it.

Comment 8

[David McNamara [:mackers]]

r93983

ACR now sets a permanent (until uninstall) cookie on addons.mozilla.org: 

ShowIncompatibleAddons=1

Comment 9

[Wil Clouser [:clouserw]]

Are you using local storage first?  Cookies should be a last resort

Comment 10

[Brian King [:kinger]]

(In reply to Wil Clouser [:clouserw] from comment #7)
> AMO is using a library that cvan wrote that first attempts to use local
> storage and then falls back to a cookie.  Sounds like we should use that in
> the add-on too and set the flag with it.

Where can we get our hands on this?

Comment 11

[Christopher Van Wiemeersch [:cvan]]

(In reply to Brian King (Briks) [:kinger] from comment #10)
> (In reply to Wil Clouser [:clouserw] from comment #7)
> > AMO is using a library that cvan wrote that first attempts to use local
> > storage and then falls back to a cookie.  Sounds like we should use that in
> > the add-on too and set the flag with it.
> 
> Where can we get our hands on this?

https://github.com/jbalogh/zamboni/blob/master/media/js/zamboni/storage.js

Comment 12

[David McNamara [:mackers]]

Ok -- ACR r94019 is using localStorage to set the following variable on addons.mozilla.org:

ShowIncompatibleAddons=true

The variable is removed when ACR is disabled or uninstalled.

Tested on another domain and is working as expected.

There is no need for a cookie fallback, as ACR is only Firefox 3.5+ compatible (as is localStorage).

Comment 13

[Jeff Balogh (:jbalogh)]

(In reply to David McNamara [:mackers] from comment #12)
> There is no need for a cookie fallback, as ACR is only Firefox 3.5+
> compatible (as is localStorage).

Some people turn off localStorage.

Comment 14

[David McNamara [:mackers]]

(In reply to Jeff Balogh (:jbalogh) from comment #13)
> Some people turn off localStorage.

Ha! Ok r94031 will use localStorage to set the ShowIncompatibleAddons variable, falling back to a cookie if localStorage not available.

This should be completely compatible with the library on AMO.

Comment 15

[Christopher Van Wiemeersch [:cvan]]

https://github.com/jbalogh/zamboni/commit/8eaff72

Comment 16

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

Some notes before I can start to verify this bug:

* Why do we set a fixed date for the lifetime of the cookie? It's not a problem in the next couple of years, but just curious.
* What happens if the user has disabled localStorage and Cookies? In this case we will not be able to notify AMO in any way that ACR is available.
* We should use [1] to get an enumerator for cookies for AMO only.
* Where can I test the integration? Has it already been pushed to a staging site?

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsICookieManager2.idl#132

Comment 17

[David McNamara [:mackers]]

(In reply to Henrik Skupin (:whimboo) from comment #16)
> * Why do we set a fixed date for the lifetime of the cookie? It's not a
> problem in the next couple of years, but just curious.

No particular reason. Do you think it would be better to just set a session cookie each time?

> * What happens if the user has disabled localStorage and Cookies? In this
> case we will not be able to notify AMO in any way that ACR is available.

In this case, incompatible addons will not be shown on AMO. We could try implemented event passing on both ends, but maybe this is too much effort for this edge case?

> * We should use [1] to get an enumerator for cookies for AMO only.

For performance reasons? Probably better, but this code is only called once on uninstall.

Comment 18

[Justin Scott [:fligtar]]

(In reply to David McNamara [:mackers] from comment #17)
> > * What happens if the user has disabled localStorage and Cookies? In this
> > case we will not be able to notify AMO in any way that ACR is available.
> 
> In this case, incompatible addons will not be shown on AMO. We could try
> implemented event passing on both ends, but maybe this is too much effort
> for this edge case?

definitely

Comment 19

[Christopher Van Wiemeersch [:cvan]]

(In reply to Henrik Skupin (:whimboo) from comment #16)

> * Where can I test the integration? Has it already been pushed to a staging
> site?

It's here behind the /i/ URL namespace:

https://addons.mozilla.org/en-US/firefox/i/extensions/

Comment 20

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

(In reply to David McNamara [:mackers] from comment #17)
> > * Why do we set a fixed date for the lifetime of the cookie? It's not a
> > problem in the next couple of years, but just curious.
> 
> No particular reason. Do you think it would be better to just set a session
> cookie each time?

Would that be much overhead? We could move this off into a new bug. It's not blocking this release of ACR.

> > * We should use [1] to get an enumerator for cookies for AMO only.
> 
> For performance reasons? Probably better, but this code is only called once
> on uninstall.

When is this code run exactly? On shutdown or startup of Firefox? I could file a new bug here too if wanted.

Comment 21

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

Marking as verified fixed with version 0.9. Tested with local storage and cookies.

Comment 22

[David McNamara [:mackers]]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> > No particular reason. Do you think it would be better to just set a session
> > cookie each time?
> 
> Would that be much overhead? We could move this off into a new bug. It's not
> blocking this release of ACR.

Please do if you think it is important. I don't think it would be much overhead. 

> > For performance reasons? Probably better, but this code is only called once
> > on uninstall.
> 
> When is this code run exactly? On shutdown or startup of Firefox? I could
> file a new bug here too if wanted.

It is run once as soon as the user uninstalls or disables ACR. I don't think there is any noticeable processing time.

Comment 23

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

(In reply to David McNamara [:mackers] from comment #22)
> > Would that be much overhead? We could move this off into a new bug. It's not
> > blocking this release of ACR.
> 
> Please do if you think it is important. I don't think it would be much
> overhead. 

Ok, filed bug 681901.

> > When is this code run exactly? On shutdown or startup of Firefox? I could
> > file a new bug here too if wanted.
> 
> It is run once as soon as the user uninstalls or disables ACR. I don't think
> there is any noticeable processing time.

So lets leave it as it is right now.