Closed Bug 906212 Opened 9 years ago Closed 9 years ago

socialapi startup ui regressions

Categories

(Firefox :: Menus, defect)

26 Branch
x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 26
Tracking Status
firefox25 --- unaffected
firefox26 + fixed

People

(Reporter: alice0775, Assigned: mixedpuppy)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached image 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
Version: unspecified → 26 Branch
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.
Blocks: 899671
Keywords: regression
OS: Windows 7 → All
Assignee: nobody → scaraveo
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.
Flags: needinfo?(mhammond)
(In reply to lsblakk@mozilla.com [: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.
Flags: needinfo?(mhammond)
Attached patch hide the menu (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=e458e1e18bfc
Assignee: scaraveo → mixedpuppy
Attachment #795761 - Flags: review?(mhammond)
Status: NEW → ASSIGNED
Summary: Garbage menuitem in Veiw > Sidebar → Garbage menuitem in View > Sidebar
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.
Attachment #795761 - Flags: review?(mhammond) → feedback+
Summary: Garbage menuitem in View > Sidebar → socialapi startup ui regressions
bug 899671 caused a number of startup regressions that were not properly caught in our tests, usurping this bug for the general issue.
Attached patch fix ui startup regressions (obsolete) — Splinter Review
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.
Attachment #795761 - Attachment is obsolete: true
Attachment #796960 - Flags: review?(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)
Attachment #796960 - Flags: review?(mhammond) → review+
feedback added
Attachment #796960 - Attachment is obsolete: true
Attachment #796977 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a2de1265566f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.