906212 - socialapi startup ui regressions

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Alice0775 White]

Attachment: screen shot

Steps To Reproduce:
1. Start Firefox with newly created clean profile
2. Alt > View > Sidebar

Actual Results:
Garbage menuitem 

Expected Results:
Should not

Comment 1

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/72bc1aebb5d0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130812125540
Bad:
http://hg.mozilla.org/mozilla-central/rev/95df3ec3bb70
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130812144341
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=72bc1aebb5d0&tochange=95df3ec3bb70


Regression window(fx)
Good:
http://hg.mozilla.org/integration/fx-team/rev/afffd4f5742d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130812120541
Bad:
http://hg.mozilla.org/integration/fx-team/rev/051daf7b6d7e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130812120741
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=afffd4f5742d&tochange=051daf7b6d7e

Regressed by:
051daf7b6d7e	Shane Caraveo — bug 899671 avoid execution in socialapi during startup if not enabled, r=jaws

This is similar to Bug 847825. 
But this is recent regression.
Please provide AUTOMATIC TEST to avoid similar bug again.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

This looks like a revisit of bug 846075 which does state in test-suite as being + 
Mark is there another test that needs adding to catch this issue in other manifestations?

Tracking this regression for FF26.

Comment 3

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

(In reply to [e-mail] [:lsblakk] from comment #2)
> Mark is there another test that needs adding to catch this issue in other
> manifestations?

It would appear so, yes :)  Shane has offered to knock up a patch that both fixes the issue and adds another test for this new case.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: hide the menu

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

Comment 5

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

Comment on attachment 795761 [details] [diff] [review]
hide the menu

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

LGTM, but as discussed in IRC, let's make sure every social command appears in CheckSocialUI.

Comment 6

[Shane Caraveo (:mixedpuppy)]

bug 899671 caused a number of startup regressions that were not properly caught in our tests, usurping this bug for the general issue.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: fix ui startup regressions

This should still avoid any startup costs when social is not enabled, while making sure social ui is enabled when the user has any enabled providers.

Comment 8

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=87a4bc1dfccc

Comment 9

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

Comment on attachment 796960 [details] [diff] [review]
fix ui startup regressions

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

r+ with the test comments addressed.

::: browser/base/content/browser-social.js
@@ +45,5 @@
>  
>      if (!Social.initialized) {
>        Social.init();
> +    } else if (Social.providers.length > 0) {
> +      // if we have providers enabled then initialize the UI

nit: please reword this entire comment (eg, turn it into 2 proper sentences or similar)

::: browser/base/content/test/social/head.js
@@ +198,5 @@
>    win = win || window;
>    let doc = win.document;
>    let provider = Social.provider;
>    let enabled = win.SocialUI.enabled;
> +  let active = SocialService.hasEnabledProviders && !win.SocialUI._chromeless &&

in all the tests touched by this patch, I think the check they make should be Social.providers.length, as that is the same check made by the front-end.  In the block below, checking hasEnabledProviders matches that state is fine though.

(IOW, hasEnabledProviders() should remain a concept private to Social.jsm almost everywhere other than maybe 1 check here that they match)

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: fix ui startup regressions

feedback added

Comment 11

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/a2de1265566f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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