850947 - fix addon install panel location

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

The 3rd party addon window should appear as a dropdown sheet on osx, however it is appearing as a dialog.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: fix parent for addon panel

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 724707 [details] [diff] [review]
fix parent for addon panel

messed up this patch, fixing it

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: fix parent for addon panel

fixes the previous patch where I missed the installprovider change in the blocklist test, also does some minor cleanup there.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 724723 [details] [diff] [review]
fix parent for addon panel

Review of attachment 724723 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, but untested as to whether it passes all tests and whether it actually fixes the described problem (TBH, I'm not even sure what the described problem actually means, but that's OK ;)

::: browser/base/content/test/social/browser_blocklist.js
@@ +116,5 @@
> +          Social.installProvider(doc, manifest_bad, function(addonManifest) {
> +            gBrowser.removeTab(tab);
> +            finish(false);
> +          });
> +        } catch(e) {

While it isn't directly related to the patch, can we do something simple to check that the exception being thrown is what we expect?

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 724723 [details] [diff] [review]
fix parent for addon panel

Felipe, can you give a secondary review on this from an osx perspective?  This should land for 23, bug 836452 will likely be blocked on this as well.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 724723 [details] [diff] [review]
fix parent for addon panel

Review of attachment 724723 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/Social.jsm
@@ -177,5 @@
>      }
>      return null;
>    },
>  
> -  installProvider: function(origin ,sourceURI, data, installCallback) {

Changes looks good, but this previous code is odd-looking: was this broken before? there are 4 params here but the function was called with only three

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to :Felipe Gomes from comment #6)
> Comment on attachment 724723 [details] [diff] [review]
> fix parent for addon panel
> 
> Review of attachment 724723 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/Social.jsm
> @@ -177,5 @@
> >      }
> >      return null;
> >    },
> >  
> > -  installProvider: function(origin ,sourceURI, data, installCallback) {
> 
> Changes looks good, but this previous code is odd-looking: was this broken
> before? there are 4 params here but the function was called with only three

It was wrong before, only worked because the params were in the right order otherwise.

Comment 8

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=4e2732c6ac3a

Comment 9

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0c7a4c13f29

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0c7a4c13f29

Comment 11

[u279076]

Verified fixed in Firefox 22.0b5.