Last Comment Bug 779360 - Implement mozSocial.isVisible API
: Implement mozSocial.isVisible API
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Jared Wein [:jaws] (please needinfo? me)
Depends on: 777177 785676
Blocks: 774520 779686
  Show dependency treegraph
Reported: 2012-07-31 17:01 PDT by Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
Modified: 2015-11-03 16:49 PST (History)
5 users (show) in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (1.66 KB, patch)
2012-08-07 16:46 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Review
Patch v2 (4.76 KB, patch)
2012-08-08 15:03 PDT, Jared Wein [:jaws] (please needinfo? me) feedback-
Details | Diff | Review
Patch v3 (5.81 KB, patch)
2012-08-08 15:38 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
Details | Diff | Review
Tests for patch (reduced Makefile) (16.09 KB, patch)
2012-08-18 23:02 PDT, Jared Wein [:jaws] (please needinfo? me) feedback+
Details | Diff | Review
test fixes (2.69 KB, patch)
2012-08-20 09:22 PDT, :Gavin Sharp [email:]
jaws: review+
Details | Diff | Review
rollup patch with panel changes removed (13.13 KB, patch)
2012-08-20 18:51 PDT, :Gavin Sharp [email:]
felipc: feedback+
Details | Diff | Review
pref_key_personalize_bg[1].jpg (43.04 KB, image/jpeg)
2015-11-02 17:37 PST, alecia.dennord80
no flags Details

Description Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 17:01:51 PDT
consider if we need a boolean value for whether the sidebar is visible or not.
Comment 1 Jared Wein [:jaws] (please needinfo? me) 2012-08-07 16:46:14 PDT
Created attachment 649879 [details] [diff] [review]

This patch adds a sidebarVisible function to mozSocial, although I'm having a hard time testing it with the scatchpad:
> document.getElementById("social-sidebar-browser").contentWindow.navigator.mozSocial is undefined
but Ctrl+I inspecting it shows the object exists.

Also, I'm not sure if this is the right place for functionality like this. This doesn't look like it is easily accessible from whitelisted content (which as I understand it is the goal).

On a smaller nit, I think this should probably be a getter instead of a function, but I've left that change out for now.
Comment 2 :Gavin Sharp [email:] 2012-08-07 16:49:57 PDT
You probably need:
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-08-07 17:19:16 PDT
Comment on attachment 649879 [details] [diff] [review]

Thanks, wrappedJSObject was what I needed. This works fine, when I switch it to a getter then it seems to stick to returning true even when the sidebar is hidden. I'm not sure why, but this patch works so maybe we'll just leave it as a function (maybe just rename it to isSidebarVisible() to make it clear that there is no side effects).
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-08-07 17:42:41 PDT
I got some help from Felipe and we found that the reason the getter isn't working is because of the way that we create the propList in MozSocialAPI.jsm. The getter is run once during propList construction and its return value is stored instead of the getter.

I think we should leave this as a function to keep the code simpler and rename it to isSidebarVisible().
Comment 5 Mark Hammond [:markh] 2012-08-08 04:24:32 PDT
It looks like defineProperties does allow property getters, and I think a 'sidebarVisible' property is much nicer...
Comment 6 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-08-08 11:41:14 PDT
We're going to need a visibility api for chat windows, bug 779686.  I'd like to propose that this api becomes isVisible and reflects the visibility state of each particular widget that this api is injected into.
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-08-08 15:03:57 PDT
Created attachment 650333 [details] [diff] [review]
Patch v2

This patch adds a getter named 'isVisible', which returns true if the calling widget is visible. Shane, can you test this out with the chatbrowser work you're doing?
Comment 8 :Gavin Sharp [email:] 2012-08-08 15:22:03 PDT
Comment on attachment 650333 [details] [diff] [review]
Patch v2

>diff --git a/toolkit/components/social/MozSocialAPI.jsm b/toolkit/components/social/MozSocialAPI.jsm

>+    isVisible: {

>+        let sidebarBrowser = mainWindow.document.getElementById("social-sidebar-browser");
>+        if (targetWindow == sidebarBrowser.contentWindow) {
>+          let sidebar = mainWindow.document.getElementById("social-sidebar-box");

>+        let chatbrowsers = mainWindow.document.getElementsByTagName("chatbrowser");

Hardcoding the front-end IDs and use of "chatbrowser" in here isn't ideal, since it really ties this code to the current browser-specific implementation details. I think a better solution would be to just have this return:

and then have the front-end code set isActive appropriately (when the sidebar closes, when the panels close, etc.). That also has the benefit of making the normal !isActive docshell optimizations like timeout throttling apply to social content, when they're hidden.
Comment 9 Jared Wein [:jaws] (please needinfo? me) 2012-08-08 15:38:59 PDT
Created attachment 650343 [details] [diff] [review]
Patch v3

Thanks Gavin, I like this approach a lot better.
Comment 10 :Felipe Gomes (needinfo me!) 2012-08-10 14:59:20 PDT
forgot to mention: while you're at it, it'd be nice if you added names to the functions in mozSocialObj as they are all anonymous at the moment
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2012-08-17 23:04:26 PDT
Update on the status of this bug: I wrote a test for this bug and have hit some problems with the tests and am working on narrowing down the cause. If the new test for this bug runs before the browser_social_mozSocialAPI.js test, then the mozSocialAPI test will time out. If it runs afterwards then all is fine.
Comment 12 :Gavin Sharp [email:] 2012-08-18 18:54:21 PDT
Can you attach the latest patch here? I can try to help figure it out.
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2012-08-18 23:02:14 PDT
Created attachment 653145 [details] [diff] [review]
Tests for patch (reduced Makefile)

This is the test that I wrote for the patch. I made the much shorter to speed up the test runs, and it still reproduces the problem.
Comment 14 :Gavin Sharp [email:] 2012-08-20 09:14:22 PDT
Comment on attachment 653145 [details] [diff] [review]
Tests for patch (reduced Makefile)

I think I found the issues, will attach some additional changes that fixes the tests for me.
Comment 15 :Gavin Sharp [email:] 2012-08-20 09:22:34 PDT
Created attachment 653406 [details] [diff] [review]
test fixes

Two issues:
- exceptions under the social:pref-changed observer weren't being reported for some reason. This hid the fact that since bug 777176 landed, SocialToolbar.updateButtonHiddenState doesn't handle Social.provider being null (which happens during test runs after the tests complete). So it was failing to hide the sidebar after browser_social_sidebar.js ran.
- browser_social_isVisible.js didn't reset the sidebar open state pref, so after it ran would stay false, which would prevent the sidebar from loading during browser_social_mozSocial_API.js, which caused it to time out.

I noticed a third issue that wasn't causing failures but seems problematic as well:
- Since bug 777176, browser_social_mozSocial_API seems to rely on the sidebar loading after the pre-loaded panel, which isn't guaranteed anymore (the test used to rely on the panel only loading when it was activated, but bug 777176 obviously changed that). This will be easier to fix once we have the shown/hidden notifications for panels, which Shane has a patch for somewhere, so I'll file a followup.
Comment 16 :Gavin Sharp [email:] 2012-08-20 09:46:15 PDT
(In reply to :Gavin Sharp (use for email) from comment #15)
> SocialToolbar.updateButtonHiddenState doesn't handle Social.provider being
> null (which happens during test runs after the tests complete). So it was
> failing to hide the sidebar after browser_social_sidebar.js ran.

Rather: updateSidebar wasn't being called to hide the sidebar, because updateButtonHiddenState was throwing silently.

> I noticed a third issue that wasn't causing failures but seems problematic
> as well:

We'll fix this in bug 783691.
Comment 17 :Gavin Sharp [email:] 2012-08-20 18:51:36 PDT
Created attachment 653620 [details] [diff] [review]
rollup patch with panel changes removed

As discussed on IRC, I think the panel changes in the previous patch were wrong. Sorting out the right behavior for panels might be a bit tricky, so let's do that in a followup to avoid blocking the landing of 779686.
Comment 18 :Gavin Sharp [email:] 2012-08-20 19:01:26 PDT
Comment 19 :Gavin Sharp [email:] 2012-08-20 19:07:05 PDT
Filed bug 784238 to cover the panel/chat changes.
Comment 20 Ed Morley [:emorley] 2012-08-21 06:28:43 PDT
Comment 21 alecia.dennord80 2015-11-02 17:37:57 PST Comment hidden (spam)
Comment 22 Al Billings [:abillings] 2015-11-03 16:38:09 PST
Comment on attachment 8682276 [details]

Please don't abuse flags on attachments.

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