755136 - Implement Social API sidebar

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

moved from: https://github.com/mozilla/socialapi-dev/issues/25

I think having the sidebar pop open on every new window will lead to annoyance. Let's default to minimized.

Comment 1

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

Similar to the bookmarks sidebar, new windows should maintain the foreground window sidebar state.

Comment 2

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

Attachment: Early WIP patch

This patch just adds the sidebar and maintains the visibility state from its opener as well as its width. There is no visible way to enable the sidebar yet, so to test out this patch you'll need to use DOM Inspector to remove the 'hidden' attribute from the sidebar and splitter.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: sidebar patch

implements sidebar loading and toggling of sidebar visibility, builds on top of patch from bug 771826.  for preliminary review, still needs tests.

as well, this does not hookup the navigator.mozSocial api, so communication with the worker is not available yet.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: sidebar patch

updated with test

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: sidebar patch

update of patch against current m-c

Comment 6

[Adam Muntner [:adamm] (use NEEDINFO)]

I set https://bugzilla.mozilla.org/show_bug.cgi?id=766622 as a blocker

"visual cue for security of sidebar"

We can discuss whether it should be a blocker or not. My take is yes, as I see bug 766622 as setting a design/security requirement which this bug (755136) should meet. 

I am open to alternative approaches to setting bug 766622 as a blocker, keeping in mind that this was one of the issues of greatest concern to arise from the last round of secreview, and it looks like UX is being engaged on this one for the relevent pieces.

For the security objective, see comment 5 on bug 766622: https://bugzilla.mozilla.org/show_bug.cgi?id=766622#c5

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: sidebar patch

updated against latest patches

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: sidebar patch

new patch includes some fixing of persisting sidebar state, moves to pref for persistence rather than persisting broadcaster attributes

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: updated patch

Comment 10

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

Attachment: sidebar patch

Here's a modified version of the patch, which I think is functionally equivalent. I removed the splitter styling changes, we'll need to followup on polishing that.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 643157 [details] [diff] [review]
sidebar patch

are you sure Social.enabled should be removed?   
Can socialSidebarBroadcaster be something like socialSidebarVisible or socialSidebarState?

Comment 12

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

Comment on attachment 643157 [details] [diff] [review]
sidebar patch

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

Not much feedback from me, I think this generally looks OK but I didn't spend enough time to get really familiar with the patch.

::: browser/base/content/browser-social.js
@@ +259,5 @@
> +    this.updateSidebar();
> +  },
> +
> +  // Whether the sidebar can be shown for this window.
> +  get enabled() {

I would rather that this be named |canShowSidebar()|

@@ +272,5 @@
> +           docElem.getAttribute('chromehidden').indexOf("extrachrome") >= 0;
> +  },
> +
> +  // Whether the user has toggled the sidebar on (for windows where it can appear)
> +  get desired() {

I would rather that this be named |enabled|.

Comment 13

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> are you sure Social.enabled should be removed?

It's currently unused, so this seems fine. We can always re-add it later if needed.
   
> Can socialSidebarBroadcaster be something like socialSidebarVisible or
> socialSidebarState?

Describing what it is ("broadcaster") seems more useful in an ID than describing what it does. We can revisit if its purpose needs to change later.

Comment 14

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

Attachment: rebased, with test and Jared's comments addressed

Comment 15

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

Attachment: rebased, with test and Jared's comments addressed

(forgot to add the test file)

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/be1e9d076b8a

Comment 17

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Blank sidebars on Windows and OSX

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)
> Created attachment 643568 [details] [diff] [review]
> rebased, with test and Jared's comments addressed
> 
> (forgot to add the test file)

Gavin, would it be possible to get a screenshot of what this is doing?  I'm attaching a mockup of blank sidebars on Windows and OSX.

Comment 18

[(no longer active)]

This seemed to have caused a perma-orange in form of leaks on debug builds across the board, so I backed it out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/614017b8bd95

Comment 19

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

Comment on attachment 643568 [details] [diff] [review]
rebased, with test and Jared's comments addressed

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

::: browser/base/content/test/browser_social_sidebar.js
@@ +67,5 @@
> +  ok(!Services.prefs.getBoolPref("social.sidebar.open"), "sidebar open pref should be false");
> +  is(sidebar.firstChild.getAttribute('src'), "about:blank", "sidebar url should not be set");
> +
> +  // Remove the test provider
> +  SocialService.removeProvider(Social.provider.origin, finish);

about:blank is the page that is leaking. Should removeProvider trigger a close on the sidebar and destroy the <browser>?

Comment 20

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

I filed bug 775779 on the fake-leak issue, and am just going to disable the test in debug builds for now.

Comment 21

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

Attachment: updated patch

This disables the test in debug builds per bug 775779, and makes a few tweaks to other existing tests.

Comment 22

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

Attachment: interdiff

Comment 23

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

Attachment: interdiff

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/3a05d298599e