Closed Bug 784238 Opened 8 years ago Closed 8 years ago

make isVisible API work for notification panels / chat windows

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 17

People

(Reporter: Gavin, Assigned: mixedpuppy)

References

Details

(Whiteboard: [Fx17])

Attachments

(1 file, 7 obsolete files)

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.
Whiteboard: [Fx17]
since the larger part of this is the chatbox, I'll take it.
Assignee: nobody → mixedpuppy
Attached patch panel/chat isVisible support (obsolete) — Splinter Review
a. always set before socialFrameShow/Hide events. 
b. set on load
Attachment #653876 - Flags: review?(jaws)
Attachment #653876 - Flags: review?(gavin.sharp)
Depends on: 783691
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;
Attachment #653876 - Flags: review?(jaws) → review+
Attached patch updated w/comments (obsolete) — Splinter Review
Attachment #653876 - Attachment is obsolete: true
Attachment #653876 - Flags: review?(gavin.sharp)
Attachment #653941 - Flags: review?(jaws)
Attachment #653941 - Flags: review?(gavin.sharp)
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.
Attachment #653941 - Flags: review?(jaws)
Attachment #653941 - Flags: review?(gavin.sharp)
Attachment #653941 - Flags: review+
Attached patch events patch (obsolete) — Splinter Review
updated with comments
Attachment #653941 - Attachment is obsolete: true
Attachment #654415 - Flags: review?(jaws)
Attached patch flyout-v2 (obsolete) — Splinter Review
updated from feedback comments
Attachment #654415 - Attachment is obsolete: true
Attachment #654415 - Flags: review?(jaws)
Attachment #654416 - Flags: review?(jaws)
Attached patch events patch (obsolete) — Splinter Review
sigh, get the right patch here
Attachment #654416 - Attachment is obsolete: true
Attachment #654416 - Flags: review?(jaws)
Attachment #654426 - Flags: review?(jaws)
Attached patch panel/chat isVisible support (obsolete) — Splinter Review
rebased
Attachment #654426 - Attachment is obsolete: true
Attachment #654426 - Flags: review?(jaws)
Attachment #654705 - Flags: review?(jaws)
Attachment #654705 - Flags: review?(gavin.sharp)
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?
Attachment #654705 - Flags: review?(jaws)
Attachment #654705 - Flags: review?(gavin.sharp)
Attachment #654705 - Flags: review-
(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.
Attached patch panel/chat isVisible support (obsolete) — Splinter Review
add missing isActive=true
Attachment #654705 - Attachment is obsolete: true
Attachment #654849 - Flags: review?(jaws)
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"?
Attachment #654849 - Flags: review?(jaws) → review+
using popuphidden
Attachment #654849 - Attachment is obsolete: true
Attachment #654865 - Flags: review?(jaws)
Attachment #654865 - Flags: review?(jaws) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5734825a4f1f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.