838883 - fix updating provider list

Status: RESOLVED INVALID

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

The part 2 patch from bug 821262 contained a couple changes that are necessary still.  The patch to be attached will contain those.  It fixes updating the ui when add/removeProvider are called, and is necessary for the addon manager bug 755126.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: fix provider updates

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Was this not covered by bug 837027?

Comment 3

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> Was this not covered by bug 837027?

It wasn't.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> (In reply to :Gavin Sharp (use [e-mail] for email) from comment
> #2)
> > Was this not covered by bug 837027?
> 
> It wasn't.

ie. the refactor patch changes more stuff, making these changes also necessary.

Rather than continuing to change the refactor patch, we thought it would be simpler to review an additional patch.

Comment 5

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

Comment on attachment 711045 [details] [diff] [review]
fix provider updates

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

I removed this part of the patch from bug 837027 as it wasn't strictly necessary there.

The only way in aurora to add a new provider is via activation, and only the "current" one can be removed - meaning the provider-set (and/or pref-changed) observer would still get called in those cases.  The only way I could hit the condition that required this new observer was via tests, which *did* test deactivating a non-current provider.  As this couldn't be provoked via the UI, it was dropped from the 837027 patch in the interests of keeping it as small as possible.

However, with the addons manager and related stuff, it becomes possible via the UI to add providers without making them current, and also to remove a non-current one - so it is no longer a test-only issue.  It could possibly be argued therefore that this change either (a) includes the activation tests which initially demonstrated the problem, or (b) land with the addons stuff.  (a) sounds right to me.

So f+ from me with the activation tests - Shane - would you like me to add them to this patch?

::: browser/base/content/browser-social.js
@@ +737,5 @@
>    },
>  
>    // Called when the Social.provider changes
>    updateProvider: function () {
> +    let provider = Social.provider || Social.defaultProvider;

I think this specific change should be in the "refactor" patch, as that refactor is what forces it rather than the new observer.

Comment 6

[Shane Caraveo (:mixedpuppy)]

IMO this change isn't related to activation, it's about ui updates after a provider is added/removed.  e.g. The activation tests are not checking to see if the added provider is listed in the menu.

I think we can add the activation tests at any time separately, and they specifically benefit bug 786133, as well as just testing our current activation.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: fix provider updates

another small fix that caused test failures

Comment 8

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> IMO this change isn't related to activation, it's about ui updates after a
> provider is added/removed.  e.g. The activation tests are not checking to
> see if the added provider is listed in the menu.

No, but the activation tests hit this condition IIUC.  I guess I was saying that this patch needs a test which demonstrates the need for the change; the activation tests qualify, but if you'd prefer a specific new test for this case, I'd be fine with that too.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #8)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> > IMO this change isn't related to activation, it's about ui updates after a
> > provider is added/removed.  e.g. The activation tests are not checking to
> > see if the added provider is listed in the menu.
> 
> No, but the activation tests hit this condition IIUC.  I guess I was saying
> that this patch needs a test which demonstrates the need for the change; the
> activation tests qualify, but if you'd prefer a specific new test for this
> case, I'd be fine with that too.

The activation tests pass without the changes in this patch.  I've created another bug to add those tests.

Comment 10

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

Rolling this up into bug 821262