Last Comment Bug 784238 - make isVisible API work for notification panels / chat windows
: make isVisible API work for notification panels / chat windows
Status: RESOLVED FIXED
[Fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Shane Caraveo (:mixedpuppy)
:
: Shane Caraveo (:mixedpuppy)
Mentors:
Depends on: 783691
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-20 19:06 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-08-25 08:45 PDT (History)
4 users (show)
markh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
panel/chat isVisible support (6.41 KB, patch)
2012-08-21 11:45 PDT, Shane Caraveo (:mixedpuppy)
jaws: review+
Details | Diff | Splinter Review
updated w/comments (6.41 KB, patch)
2012-08-21 14:13 PDT, Shane Caraveo (:mixedpuppy)
jaws: review+
Details | Diff | Splinter Review
events patch (6.22 KB, patch)
2012-08-22 16:28 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
flyout-v2 (22.74 KB, patch)
2012-08-22 16:31 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
events patch (6.22 KB, patch)
2012-08-22 16:47 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
panel/chat isVisible support (5.87 KB, patch)
2012-08-23 11:17 PDT, Shane Caraveo (:mixedpuppy)
jaws: review-
Details | Diff | Splinter Review
panel/chat isVisible support (6.25 KB, patch)
2012-08-23 16:51 PDT, Shane Caraveo (:mixedpuppy)
jaws: review+
Details | Diff | Splinter Review
panel/chat isVisible support (6.45 KB, patch)
2012-08-23 17:20 PDT, Shane Caraveo (:mixedpuppy)
jaws: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-20 19:06:29 PDT
Bug 779360 implemented the mozSocial.isVisible API, but only made it work for the sidebar. We should also make it work for chat windows and the notification panel windows.

For notification panels, we need to reflect whether the notification is currently being displayed. For chat windows, we should probably reflect whether the window is maximized? Not sure about this.
Comment 1 Shane Caraveo (:mixedpuppy) 2012-08-21 11:44:00 PDT
since the larger part of this is the chatbox, I'll take it.
Comment 2 Shane Caraveo (:mixedpuppy) 2012-08-21 11:45:57 PDT
Created attachment 653876 [details] [diff] [review]
panel/chat isVisible support

a. always set before socialFrameShow/Hide events. 
b. set on load
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-08-21 12:37:46 PDT
Comment on attachment 653876 [details] [diff] [review]
panel/chat isVisible support

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

r=me with the following issues addressed.

::: browser/base/content/socialchat.xml
@@ +23,5 @@
>      <implementation implements="nsIDOMEventListener">
>        <field name="iframe" readonly="true">
>          document.getAnonymousElementByAttribute(this, "anonid", "iframe");
>        </field>
> +      

nit: trailing whitespace

@@ +35,5 @@
> +            this.isActive = false;
> +          } else {
> +            this.removeAttribute("minimized");
> +            this.isActive = true;
> +          }

this.isActive = !val;
if (val)
  this.setAttribute(...);
else
  this.removeAttribute(...);

@@ +44,5 @@
> +        <getter>
> +          return this.iframe.docShell.isActive;
> +        </getter>
> +        <setter>
> +          this.iframe.docShell.isActive = val;

this.iframe.docShell.isActive = !!val;
Comment 4 Shane Caraveo (:mixedpuppy) 2012-08-21 14:13:50 PDT
Created attachment 653941 [details] [diff] [review]
updated w/comments
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-08-22 12:01:45 PDT
Comment on attachment 653941 [details] [diff] [review]
updated w/comments

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

::: browser/base/content/browser-social.js
@@ +480,5 @@
>      panel.addEventListener("popupshown", function onpopupshown() {
>        panel.removeEventListener("popupshown", onpopupshown);
>        SocialToolbar.button.setAttribute("open", "true");
>        if (notifBrowser.contentDocument.readyState == "complete") {
> +        notifBrowser.docShell.isActive = true;

Can you move |notifBrowser.docShell.isActive = true;| to before the if-statement above? I think it's OK if we mark the document as active before it has finished loading, and it will be similar to the patch for bug 784198.
Comment 6 Shane Caraveo (:mixedpuppy) 2012-08-22 16:28:39 PDT
Created attachment 654415 [details] [diff] [review]
events patch

updated with comments
Comment 7 Shane Caraveo (:mixedpuppy) 2012-08-22 16:31:43 PDT
Created attachment 654416 [details] [diff] [review]
flyout-v2

updated from feedback comments
Comment 8 Shane Caraveo (:mixedpuppy) 2012-08-22 16:47:24 PDT
Created attachment 654426 [details] [diff] [review]
events patch

sigh, get the right patch here
Comment 9 Shane Caraveo (:mixedpuppy) 2012-08-23 11:17:09 PDT
Created attachment 654705 [details] [diff] [review]
panel/chat isVisible support

rebased
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-08-23 16:39:27 PDT
Comment on attachment 654705 [details] [diff] [review]
panel/chat isVisible support

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

::: browser/base/content/browser-social.js
@@ +441,5 @@
>  
>      panel.addEventListener("popupshown", function onpopupshown() {
>        panel.removeEventListener("popupshown", onpopupshown);
>        SocialToolbar.button.setAttribute("open", "true");
>        notificationFrame.docShell.isAppTab = true;

I thought you were going to add |notificationFrame.docShell.isActive = true;| here?
Comment 11 Shane Caraveo (:mixedpuppy) 2012-08-23 16:49:39 PDT
(In reply to Jared Wein [:jaws] from comment #10)
> Comment on attachment 654705 [details] [diff] [review]
> panel/chat isVisible support
> 
> Review of attachment 654705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-social.js
> @@ +441,5 @@
> >  
> >      panel.addEventListener("popupshown", function onpopupshown() {
> >        panel.removeEventListener("popupshown", onpopupshown);
> >        SocialToolbar.button.setAttribute("open", "true");
> >        notificationFrame.docShell.isAppTab = true;
> 
> I thought you were going to add |notificationFrame.docShell.isActive =
> true;| here?

that's odd, I thought I had.
Comment 12 Shane Caraveo (:mixedpuppy) 2012-08-23 16:51:23 PDT
Created attachment 654849 [details] [diff] [review]
panel/chat isVisible support

add missing isActive=true
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2012-08-23 17:06:28 PDT
Comment on attachment 654849 [details] [diff] [review]
panel/chat isVisible support

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

::: browser/base/content/browser-social.js
@@ +432,5 @@
>        notificationFrame.contentDocument.documentElement.dispatchEvent(evt);
>      }
>  
>      panel.addEventListener("popuphiding", function onpopuphiding() {
>        panel.removeEventListener("popuphiding", onpopuphiding);

Can you change this to use "popuphidden"?
Comment 14 Shane Caraveo (:mixedpuppy) 2012-08-23 17:20:19 PDT
Created attachment 654865 [details] [diff] [review]
panel/chat isVisible support

using popuphidden
Comment 15 Shane Caraveo (:mixedpuppy) 2012-08-24 17:27:15 PDT
https://tbpl.mozilla.org/?tree=Try&rev=77badc2b71dc
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-08-25 08:45:43 PDT
https://hg.mozilla.org/mozilla-central/rev/5734825a4f1f

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