Implement Social API sidebar

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

unspecified
Firefox 17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 11 obsolete attachments)

1.47 MB, image/png
Details
32.18 KB, patch
Details | Diff | Splinter Review
7.38 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.

Updated

5 years ago
Summary: New windows should start with sidebar minimized (?) → New windows should maintain foreground window sidebar state
Whiteboard: [needs-ux] → [needs-ux][Fx16]

Updated

5 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 764241
Created attachment 639736 [details] [diff] [review]
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.
Attachment #639736 - Flags: feedback?(mixedpuppy)
(Assignee)

Updated

5 years ago
Assignee: jaws → mixedpuppy
(Assignee)

Comment 3

5 years ago
Created attachment 640873 [details] [diff] [review]
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.
Attachment #640873 - Flags: feedback?(jaws)
Attachment #640873 - Flags: feedback?(gavin.sharp)
(Assignee)

Updated

5 years ago
Attachment #639736 - Attachment is obsolete: true
Attachment #639736 - Flags: feedback?(mixedpuppy)
(Assignee)

Updated

5 years ago
Depends on: 771826
(Assignee)

Updated

5 years ago
Blocks: 770695
(Assignee)

Comment 4

5 years ago
Created attachment 641300 [details] [diff] [review]
sidebar patch

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)
(Assignee)

Comment 5

5 years ago
Created attachment 641527 [details] [diff] [review]
sidebar patch

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)
(Assignee)

Updated

5 years ago
Blocks: 773351
Depends on: 766622
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
(Assignee)

Comment 7

5 years ago
Created attachment 641649 [details] [diff] [review]
sidebar patch

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)
(Assignee)

Comment 8

5 years ago
Created attachment 642035 [details] [diff] [review]
sidebar patch

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)
(Assignee)

Comment 9

5 years ago
Created attachment 642493 [details]
updated patch
Attachment #642035 - Attachment is obsolete: true
Attachment #642035 - Flags: feedback?(gavin.sharp)
Attachment #642493 - Flags: review?(gavin.sharp)
Created attachment 643157 [details] [diff] [review]
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.
Attachment #642493 - Attachment is obsolete: true
Attachment #642493 - Flags: review?(gavin.sharp)
Attachment #643157 - Flags: review?(mixedpuppy)
Attachment #643157 - Flags: review?(jaws)
(Assignee)

Comment 11

5 years ago
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.
Created attachment 643493 [details] [diff] [review]
rebased, with test and Jared's comments addressed
Attachment #643157 - Attachment is obsolete: true
Created attachment 643568 [details] [diff] [review]
rebased, with test and Jared's comments addressed

(forgot to add the test file)
Attachment #643493 - Attachment is obsolete: true
Attachment #643568 - Flags: review+
Attachment #643568 - Flags: feedback?(mixedpuppy)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Updated

5 years ago
Blocks: 775336
Created attachment 643691 [details]
Mockup: Blank sidebars on Windows and OSX

(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.
Created attachment 644161 [details] [diff] [review]
updated patch

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
Created attachment 644163 [details] [diff] [review]
interdiff
Created attachment 644164 [details] [diff] [review]
interdiff
Attachment #644163 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3a05d298599e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [needs-ux][Fx16] → [needs-ux][Fx16][strings:2]
Whiteboard: [needs-ux][Fx16][strings:2] → [Fx16][strings:2]
You need to log in before you can comment on or make changes to this bug.