Closed Bug 755136 Opened 8 years ago Closed 8 years ago

Implement Social API sidebar

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 17

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [Fx16][strings:2])

Attachments

(3 files, 11 obsolete files)

1.47 MB, image/png
Details
32.18 KB, patch
Details | Diff | Splinter Review
7.38 KB, patch
Details | Diff | Splinter Review
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.
Summary: New windows should start with sidebar minimized (?) → New windows should maintain foreground window sidebar state
Whiteboard: [needs-ux] → [needs-ux][Fx16]
Assignee: nobody → jaws
Summary: New windows should maintain foreground window sidebar state → Implement sidebar (New windows should maintain foreground window sidebar state)
Similar to the bookmarks sidebar, new windows should maintain the foreground window sidebar state.
Status: NEW → ASSIGNED
Summary: Implement sidebar (New windows should maintain foreground window sidebar state) → Implement Social API sidebar
Depends on: 764241
Attached patch Early WIP patch (obsolete) — Splinter Review
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.
Attachment #639736 - Flags: feedback?(mixedpuppy)
Assignee: jaws → mixedpuppy
Attached patch sidebar patch (obsolete) — Splinter Review
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.
Attachment #640873 - Flags: feedback?(jaws)
Attachment #640873 - Flags: feedback?(gavin.sharp)
Attachment #639736 - Attachment is obsolete: true
Attachment #639736 - Flags: feedback?(mixedpuppy)
Depends on: 771826
Blocks: 770695
Attached patch sidebar patch (obsolete) — Splinter Review
updated with test
Attachment #640873 - Attachment is obsolete: true
Attachment #640873 - Flags: feedback?(jaws)
Attachment #640873 - Flags: feedback?(gavin.sharp)
Attachment #641300 - Flags: feedback?(jaws)
Attachment #641300 - Flags: feedback?(gavin.sharp)
Attached patch sidebar patch (obsolete) — Splinter Review
update of patch against current m-c
Attachment #641300 - Attachment is obsolete: true
Attachment #641300 - Flags: feedback?(jaws)
Attachment #641300 - Flags: feedback?(gavin.sharp)
Attachment #641527 - Flags: feedback?(jaws)
Attachment #641527 - Flags: feedback?(gavin.sharp)
Blocks: 773351
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
Attached patch sidebar patch (obsolete) — Splinter Review
updated against latest patches
Attachment #641527 - Attachment is obsolete: true
Attachment #641527 - Flags: feedback?(jaws)
Attachment #641527 - Flags: feedback?(gavin.sharp)
Attachment #641649 - Flags: feedback?(gavin.sharp)
Attached patch sidebar patch (obsolete) — Splinter Review
new patch includes some fixing of persisting sidebar state, moves to pref for persistence rather than persisting broadcaster attributes
Attachment #641649 - Attachment is obsolete: true
Attachment #641649 - Flags: feedback?(gavin.sharp)
Attachment #642035 - Flags: feedback?(gavin.sharp)
Attached file updated patch (obsolete) —
Attachment #642035 - Attachment is obsolete: true
Attachment #642035 - Flags: feedback?(gavin.sharp)
Attachment #642493 - Flags: review?(gavin.sharp)
Attached patch sidebar patch (obsolete) — Splinter Review
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.
Attachment #642493 - Attachment is obsolete: true
Attachment #642493 - Flags: review?(gavin.sharp)
Attachment #643157 - Flags: review?(mixedpuppy)
Attachment #643157 - Flags: review?(jaws)
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?
Attachment #643157 - Flags: review?(mixedpuppy) → review+
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|.
Attachment #643157 - Flags: review?(jaws) → feedback+
(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.
(forgot to add the test file)
Attachment #643493 - Attachment is obsolete: true
Attachment #643568 - Flags: review+
Attachment #643568 - Flags: feedback?(mixedpuppy)
Attachment #643568 - Flags: feedback?(mixedpuppy) → feedback+
https://hg.mozilla.org/integration/mozilla-inbound/rev/be1e9d076b8a
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 17
Flags: in-testsuite+
Blocks: 775336
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Depends on: 775380
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 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>?
I filed bug 775779 on the fake-leak issue, and am just going to disable the test in debug builds for now.
Attached patch updated patchSplinter Review
This disables the test in debug builds per bug 775779, and makes a few tweaks to other existing tests.
Attachment #643568 - Attachment is obsolete: true
Attached patch interdiffSplinter Review
Attachment #644163 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3a05d298599e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [needs-ux][Fx16] → [needs-ux][Fx16][strings:2]
Whiteboard: [needs-ux][Fx16][strings:2] → [Fx16][strings:2]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.