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)
:
: 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 User image 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 User image Shane Caraveo (:mixedpuppy) 2012-07-24 18:25:33 PDT
Created attachment 645616 [details] [diff] [review]
sidebar visibility patch
Comment 2 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Shane Caraveo (:mixedpuppy) 2012-08-01 16:35:32 PDT
*** Bug 755131 has been marked as a duplicate of this bug. ***
Comment 13 User image :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 User image 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 User image :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 User image Shane Caraveo (:mixedpuppy) 2012-08-03 14:43:45 PDT
Created attachment 648863 [details]
sidebar visibility patch

updated with latest comments
Comment 17 User image :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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 15:32:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1c8194da40
Comment 19 User image 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.