Closed Bug 777177 Opened 12 years ago Closed 12 years ago

Keep the social sidebar loaded, and dispatch events when it is opened/closed

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])

Attachments

(1 file, 4 obsolete files)

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
Attached patch sidebar visibility patch (obsolete) — Splinter Review
Attachment #645616 - Flags: feedback?(jaws)
Attachment #645616 - Flags: feedback?(mhammond)
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.
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.
Attachment #645616 - Attachment is patch: true
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?
Attachment #645616 - Flags: feedback?(jaws) → feedback+
Attached patch sidebar visibility patch (obsolete) — Splinter Review
based on comments from markh, I think custom events for showing/hidding the sidebar is probably better.  Here is a patch implementing that.
Attachment #645616 - Attachment is obsolete: true
Attachment #645616 - Flags: feedback?(mhammond)
Attachment #647299 - Flags: feedback?(jaws)
Attachment #647299 - Flags: feedback?(gavin.sharp)
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?
Summary: use page visibility api for sidebar → Keep the social sidebar loaded, and dispatch events when it is opened/closed
(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 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?
Attachment #647299 - Flags: feedback?(jaws) → feedback+
Blocks: 779360
(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 on attachment 647299 [details] [diff] [review]
sidebar visibility patch

moving this to review
Attachment #647299 - Flags: feedback?(gavin.sharp) → review?(gavin.sharp)
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.
Attachment #647299 - Flags: review?(gavin.sharp) → review-
Attached patch sidebar visibility patch (obsolete) — Splinter Review
updated based on review feedback
Attachment #647299 - Attachment is obsolete: true
Attachment #648832 - Flags: review?(gavin.sharp)
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
Attachment #648832 - Flags: review?(gavin.sharp) → review-
Attached file sidebar visibility patch (obsolete) —
updated with latest comments
Attachment #648832 - Attachment is obsolete: true
Attachment #648863 - Flags: review?(gavin.sharp)
I just made some minor style tweaks and added a test for "sidebarshow".
Attachment #648863 - Attachment is obsolete: true
Attachment #648863 - Flags: review?(gavin.sharp)
Attachment #648877 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1c8194da40
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/bc1c8194da40
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.