Last Comment Bug 755136 - Implement Social API sidebar
: Implement Social API sidebar
Status: RESOLVED FIXED
[Fx16][strings:2]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
:
Mentors:
Depends on: 764241 766622 771826 775380
Blocks: 770695 773351 775336
  Show dependency treegraph
 
Reported: 2012-05-14 17:14 PDT by Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
Modified: 2013-12-27 14:31 PST (History)
7 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Early WIP patch (2.60 KB, patch)
2012-07-06 11:30 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
sidebar patch (9.15 KB, patch)
2012-07-10 17:26 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
sidebar patch (13.26 KB, patch)
2012-07-11 18:11 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
sidebar patch (13.30 KB, patch)
2012-07-12 10:40 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
sidebar patch (12.75 KB, patch)
2012-07-12 16:35 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
sidebar patch (14.76 KB, patch)
2012-07-13 14:01 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
updated patch (15.25 KB, text/plain)
2012-07-15 23:24 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details
sidebar patch (13.84 KB, patch)
2012-07-17 15:05 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
mixedpuppy: review+
jaws: feedback+
Details | Diff | Review
rebased, with test and Jared's comments addressed (23.16 KB, patch)
2012-07-18 12:09 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
rebased, with test and Jared's comments addressed (27.55 KB, patch)
2012-07-18 13:47 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
gavin.sharp: review+
mixedpuppy: feedback+
Details | Diff | Review
Mockup: Blank sidebars on Windows and OSX (1.47 MB, image/png)
2012-07-18 17:30 PDT, Jennifer Morrow [:Boriss] (UX)
no flags Details
updated patch (32.18 KB, patch)
2012-07-19 20:46 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
interdiff (32.18 KB, patch)
2012-07-19 20:48 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
interdiff (7.38 KB, patch)
2012-07-19 20:48 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review

Description Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-05-14 17:14:53 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-02 14:09:47 PDT
Similar to the bookmarks sidebar, new windows should maintain the foreground window sidebar state.
Comment 2 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-06 11:30:57 PDT
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.
Comment 3 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-10 17:26:51 PDT
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.
Comment 4 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-11 18:11:44 PDT
Created attachment 641300 [details] [diff] [review]
sidebar patch

updated with test
Comment 5 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-12 10:40:36 PDT
Created attachment 641527 [details] [diff] [review]
sidebar patch

update of patch against current m-c
Comment 6 Adam Muntner [:adamm] (use NEEDINFO) 2012-07-12 14:52:10 PDT
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) [on leave 5/16-7/16] 2012-07-12 16:35:26 PDT
Created attachment 641649 [details] [diff] [review]
sidebar patch

updated against latest patches
Comment 8 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-13 14:01:48 PDT
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
Comment 9 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-15 23:24:40 PDT
Created attachment 642493 [details]
updated patch
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-17 15:05:40 PDT
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.
Comment 11 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-17 16:06:12 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-18 11:22:36 PDT
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: gavin@gavinsharp.com] 2012-07-18 12:05:55 PDT
(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: gavin@gavinsharp.com] 2012-07-18 12:09:02 PDT
Created attachment 643493 [details] [diff] [review]
rebased, with test and Jared's comments addressed
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-18 13:47:12 PDT
Created attachment 643568 [details] [diff] [review]
rebased, with test and Jared's comments addressed

(forgot to add the test file)
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-18 14:21:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/be1e9d076b8a
Comment 17 Jennifer Morrow [:Boriss] (UX) 2012-07-18 17:30:15 PDT
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.
Comment 18 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-18 22:22:03 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-19 08:21:24 PDT
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: gavin@gavinsharp.com] 2012-07-19 19:28:08 PDT
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: gavin@gavinsharp.com] 2012-07-19 20:46:38 PDT
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.
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-19 20:48:21 PDT
Created attachment 644163 [details] [diff] [review]
interdiff
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-19 20:48:41 PDT
Created attachment 644164 [details] [diff] [review]
interdiff
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-20 01:21:11 PDT
https://hg.mozilla.org/mozilla-central/rev/3a05d298599e

Note You need to log in before you can comment on or make changes to this bug.