fix addon install panel location

VERIFIED FIXED in Firefox 22

Status

()

Firefox
SocialAPI
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

unspecified
Firefox 22
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
The 3rd party addon window should appear as a dropdown sheet on osx, however it is appearing as a dialog.
(Assignee)

Comment 1

5 years ago
Created attachment 724707 [details] [diff] [review]
fix parent for addon panel
Assignee: nobody → mixedpuppy
Attachment #724707 - Flags: review?(mhammond)
(Assignee)

Comment 2

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

messed up this patch, fixing it
Attachment #724707 - Attachment is obsolete: true
Attachment #724707 - Flags: review?(mhammond)
(Assignee)

Comment 3

5 years ago
Created attachment 724723 [details] [diff] [review]
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.
Attachment #724723 - Flags: review?(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?
Attachment #724723 - Flags: review?(mhammond) → review+
(Assignee)

Comment 5

5 years ago
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.
Attachment #724723 - Flags: review?(felipc)
(Assignee)

Updated

5 years ago
Blocks: 836452
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
(Assignee)

Comment 7

5 years ago
(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.
Attachment #724723 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/b0c7a4c13f29
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Verified fixed in Firefox 22.0b5.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.