Last Comment Bug 777177 - Keep the social sidebar loaded, and dispatch events when it is opened/closed
: Keep the social sidebar loaded, and dispatch events when it is opened/closed
Status: RESOLVED FIXED
[Fx16]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Shane Caraveo (:mixedpuppy)
:
:
Mentors:
: 755131 (view as bug list)
Depends on:
Blocks: 779360
  Show dependency treegraph
 
Reported: 2012-07-24 17:16 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2012-08-16 17:47 PDT (History)
4 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
sidebar visibility patch (4.44 KB, patch)
2012-07-24 18:25 PDT, Shane Caraveo (:mixedpuppy)
jaws: feedback+
Details | Diff | Splinter Review
sidebar visibility patch (5.41 KB, patch)
2012-07-30 14:22 PDT, Shane Caraveo (:mixedpuppy)
gavin.sharp: review-
jaws: feedback+
Details | Diff | Splinter Review
sidebar visibility patch (5.82 KB, patch)
2012-08-03 13:40 PDT, Shane Caraveo (:mixedpuppy)
gavin.sharp: review-
Details | Diff | Splinter Review
sidebar visibility patch (5.75 KB, text/plain)
2012-08-03 14:43 PDT, Shane Caraveo (:mixedpuppy)
no flags Details
patch with minor tweak and test addition (6.59 KB, patch)
2012-08-03 15:31 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
gavin.sharp: review+
Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-07-24 17:16:58 PDT
dont unload sidebar, use page visibility api (visibility changed event).  this is a change on the provider side, we just need to verify it works
Comment 1 Shane Caraveo (:mixedpuppy) 2012-07-24 18:25:33 PDT
Created attachment 645616 [details] [diff] [review]
sidebar visibility patch
Comment 2 Shane Caraveo (:mixedpuppy) 2012-07-24 18:27:56 PDT
visibility api reference: https://developer.mozilla.org/en/DOM/Using_the_Page_Visibility_API
Comment 3 Mark Hammond [:markh] 2012-07-24 19:14:44 PDT
It seems that minimizing the browser also triggers the visibility event.  I could also imagine that in the future the event will be sent when the page is not minimized but fully obscured by other windows - it doesn't seem to do that now but the spec seems to call for it.  Last I heard, our partners are looking specifically for an event which tells them when the sidebar has been hidden as they may then re-enable certain elements on their normal page.  I'm not sure this approach will offer that.
Comment 4 Mark Hammond [:markh] 2012-07-24 21:42:46 PDT
FWIW, the patch also causes leaks to be reported.  If the approach in this patch goes ahead, I expect you will still want to fully unload the sidebar when the entire social feature is disabled.
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-07-24 22:52:29 PDT
Comment on attachment 645616 [details] [diff] [review]
sidebar visibility patch

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

::: browser/base/content/browser-social.js
@@ +424,4 @@
>      }
> +    else {
> +      sbrowser.docShellIsActive = true;
> +    }

This can be refactored to:
sbrowser.docShellIsActive = !broadcaster.hidden;

::: browser/base/content/test/browser_social_sidebar.js
@@ +16,5 @@
>    let manifest = { // normal provider
>      name: "provider 1",
> +    origin: "http://example.com",
> +    sidebarURL: "http://example.com/browser/browser/base/content/test/social_sidebar.html",
> +    workerURL: "http://example.com/browser/browser/base/content/test/social_worker.js",

Out of curiosity, was there a reason that the test switched from https to http?
Comment 6 Shane Caraveo (:mixedpuppy) 2012-07-30 14:22:17 PDT
Created attachment 647299 [details] [diff] [review]
sidebar visibility patch

based on comments from markh, I think custom events for showing/hidding the sidebar is probably better.  Here is a patch implementing that.
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-07-30 15:18:53 PDT
While I think the custom event approach is fine, does the custom event approach remove the ability for script to query the state of the sidebar?
Comment 8 Shane Caraveo (:mixedpuppy) 2012-07-30 15:21:54 PDT
(In reply to Jared Wein [:jaws] from comment #7)
> While I think the custom event approach is fine, does the custom event
> approach remove the ability for script to query the state of the sidebar?

It does.  We could add a mozSocial.sidebarVisible api.
Comment 9 Jared Wein [:jaws] (please needinfo? me) 2012-07-31 14:23:30 PDT
Comment on attachment 647299 [details] [diff] [review]
sidebar visibility patch

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

IIRC, someone mentioned in IRC #socialdev that they could also get this information to content pages via localstorage. It's not an ideal API since it isn't straightforward, but will allow us to move forward with this. I think a better solution though would be to have a property on .mozSocial since it should be more intuitive.

Shane, can you file a separate bug to add the property to mozSocial?
Comment 10 Shane Caraveo (:mixedpuppy) 2012-07-31 17:02:55 PDT
(In reply to Jared Wein [:jaws] from comment #9)

> Shane, can you file a separate bug to add the property to mozSocial?


Bug is filed, but this functionality is not needed per conversation about this change.
Comment 11 Shane Caraveo (:mixedpuppy) 2012-07-31 17:04:31 PDT
Comment on attachment 647299 [details] [diff] [review]
sidebar visibility patch

moving this to review
Comment 12 Shane Caraveo (:mixedpuppy) 2012-08-01 16:35:32 PDT
*** Bug 755131 has been marked as a duplicate of this bug. ***
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 12:37:06 PDT
Comment on attachment 647299 [details] [diff] [review]
sidebar visibility patch

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>   updateSidebar: function SocialSidebar_updateSidebar() {

>+    // Load the sidebar document
>+    let type = broadcaster.hidden ? "sidebarhide" : "sidebarshow";

nit: check hideSidebar rather than broadcaster.hidden.

Wouldn't it be simpler for providers if we fired a single event ("sidebarstatechange") with a different detail to indicate show/hide?

>+    if (sbrowser.getAttribute("origin") != Social.provider.origin) {

>+        window.setTimeout(function () {
>+          SocialSidebar.dispatchEvent(type);
>+        }, 10);

use 0

>+      }, true);

Don't you need to pass true to removeEventListener if you passed true to addEventListener? Does it need to be a capturing listener? You could just omit it here.

>     }
>+    else {

nit: cuddle else braces: } else {

I think we do actually want to unload the sidebar when hideSidebar is true because !this.enabled (as opposed to !this.canShow) - enabling/disabling social shouldn't leave the sidebar document loaded forever.
Comment 14 Shane Caraveo (:mixedpuppy) 2012-08-03 13:40:40 PDT
Created attachment 648832 [details] [diff] [review]
sidebar visibility patch

updated based on review feedback
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 13:56:52 PDT
Comment on attachment 648832 [details] [diff] [review]
sidebar visibility patch

Looking good, just a couple of tweaks needed:

- dispatchEvent is synchronous, so you don't need the setTimeout after sidebarhide.
- I think we should unload the sidebar if !this.canShow, rather than if !Social.enabled.
- use 0 for the setTimeout delay, rather than 10
Comment 16 Shane Caraveo (:mixedpuppy) 2012-08-03 14:43:45 PDT
Created attachment 648863 [details]
sidebar visibility patch

updated with latest comments
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 15:31:00 PDT
Created attachment 648877 [details] [diff] [review]
patch with minor tweak and test addition

I just made some minor style tweaks and added a test for "sidebarshow".
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 15:32:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1c8194da40
Comment 19 Ed Morley [:emorley] 2012-08-04 11:18:17 PDT
https://hg.mozilla.org/mozilla-central/rev/bc1c8194da40

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