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)
Firefox Graveyard
SocialAPI
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 17
People
(Reporter: mixedpuppy, Assigned: mixedpuppy)
References
Details
(Whiteboard: [Fx16])
Attachments
(1 file, 4 obsolete files)
6.59 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #645616 -
Flags: feedback?(jaws)
Assignee | ||
Updated•12 years ago
|
Attachment #645616 -
Flags: feedback?(mhammond)
Assignee | ||
Comment 2•12 years ago
|
||
visibility api reference: https://developer.mozilla.org/en/DOM/Using_the_Page_Visibility_API
Comment 3•12 years ago
|
||
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•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #645616 -
Attachment is patch: true
Comment 5•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
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)
Comment 7•12 years ago
|
||
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
Assignee | ||
Comment 8•12 years ago
|
||
(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•12 years ago
|
||
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+
Assignee | ||
Comment 10•12 years ago
|
||
(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.
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 647299 [details] [diff] [review] sidebar visibility patch moving this to review
Attachment #647299 -
Flags: feedback?(gavin.sharp) → review?(gavin.sharp)
Comment 13•12 years ago
|
||
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-
Assignee | ||
Comment 14•12 years ago
|
||
updated based on review feedback
Attachment #647299 -
Attachment is obsolete: true
Attachment #648832 -
Flags: review?(gavin.sharp)
Comment 15•12 years ago
|
||
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-
Assignee | ||
Comment 16•12 years ago
|
||
updated with latest comments
Attachment #648832 -
Attachment is obsolete: true
Attachment #648863 -
Flags: review?(gavin.sharp)
Comment 17•12 years ago
|
||
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+
Comment 18•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1c8194da40
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 17
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc1c8194da40
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•