846075 - Social UI not initialized correct when social is disabled.

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

Bug 833292 caused a regression when starting Fx when social is "active" but "disabled".

To repro:
* Turn off social features while leaving providers in place.
* Restart firefox

Expected:
* The "default provider" icon is shown in the toolbar, displaying the dropdown allowing social to be turned back on.

Actual:
* There is no social UI shown at all.

*sigh* - we seem to keep regressing here.  I've a fix, but I'm working on tests which could help prevent these regressions in the future.

Comment 1

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

Attachment: More UI tweaks and lots of tests

So one thing led to another... :)  This patch has even more simplification of the social UI, and should be more efficient in what it does (a fair bit of duplication in first-time init() has been removed).

Of note:

* There is a new Social.initialized boolean.  Social.init() guarded against multiple initializations via 'if (Social.providers) ...' but this is unreliable - Social.providers is only set in callbacks, so 2 quick calls would do the wrong thing.  browser-social.js also relied on this and also could be made to fail.

* browser-social no longer has a listener for social:pref-changed - it relies purely on social:provider-changed - seeing social:provider-changed was (almost) called each time social was disabled, we might as well not bother with social:pref-changed.  This cleans-up browser-social quite a bit.

* Social.init() was the one exception to the above - if social.enabled was false, social:provider-changed wasn't called on startup - it is now.

* Lotsa tests of the window state.  The tests have a few hacks to force Social re-initialization, but uncovered a few issues (eg, the chatbar not always being hidden)

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 719440 [details] [diff] [review]
More UI tweaks and lots of tests

I'd like to see Social.init happen during the final-ui-startup notification, rather than each window checking on whether it should initialize it.

Also, the ui checking tests failed for me, you can see the fix I did in bug 786133, where I have Social:ToggleNotifications hidden by default.  I'm not sure why you're not hitting that problem, perhaps only the activation test was running that check without any providers installed.

Comment 3

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

Comment on attachment 719440 [details] [diff] [review]
More UI tweaks and lots of tests

nice cleanup! love the test coverage too.

Comment 4

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

Comment on attachment 719440 [details] [diff] [review]
More UI tweaks and lots of tests

Re moving Social.init() - I'd rather do that as a followup as I believe it will be fragile and/or have a performance impact (if we do it earlier than when it currently runs, that means it must be done before the first chrome paint is complete, so must therefore add time before that first paint.  If it happens after that paint, it will be done after we need it to be done.  Right now we know it is done at exactly the correct time).

I applied Shane's patch from bug 786133 and after resolving a few minor conflicts, all the tests pass for me.  There is a try run with this patch, plus Shane's rebased patch at https://tbpl.mozilla.org/?tree=Try&rev=c2856d7dac64

Note that while this isn't urgent, it would be great if it can have a priority, as Nightly users who happen to restart Fx while social is turned off will not be able to turn it back on again without hitting about:config!

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 719440 [details] [diff] [review]
More UI tweaks and lots of tests

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

Sorry for the delayed review. This looks good to me.

::: browser/base/content/browser-social.js
@@ +767,1 @@
>        while (tbi.lastChild != tbi.firstChild)

While you're in here changing this, can you add a check to make sure that tbi is not null? It's been cropping up in some customization work that I've been doing and the check here wouldn't hurt (but this can also be done as a separate patch).

::: browser/base/content/test/social/browser_social_window.js
@@ +18,5 @@
> +
> +let createdWindows = [];
> +
> +function openWindowAndWaitForInit(callback) {
> +  // this notification tells is SocialUI.init() has been run...

s/ is/ us/

::: browser/modules/Social.jsm
@@ +101,3 @@
>        return;
>      }
> +    this.initialized = true;

Can you add an "initialized" property to Social that default to false?

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/388dafae7c9c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/388dafae7c9c

Comment 9

[u279076]

Please advise if regression testing is needed here.