Closed Bug 838883 Opened 9 years ago Closed 9 years ago

fix updating provider list

Categories

(Firefox Graveyard :: SocialAPI, defect)

19 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch fix provider updates (obsolete) — Splinter Review
Assignee: nobody → mixedpuppy
Attachment #711045 - Flags: review?(gavin.sharp)
Attachment #711045 - Flags: feedback?(mhammond)
Was this not covered by bug 837027?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)
> Was this not covered by bug 837027?

It wasn't.
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 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.
Attachment #711045 - Flags: feedback?(mhammond) → feedback+
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.
another small fix that caused test failures
Attachment #711045 - Attachment is obsolete: true
Attachment #711045 - Flags: review?(gavin.sharp)
Attachment #711116 - Flags: review?(gavin.sharp)
(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.
(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.
Attachment #711116 - Flags: review?(gavin.sharp)
Rolling this up into bug 821262
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.