Preload social api panels

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: jaws)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx17])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

5 years ago
similar to bug 753448, we want to preload the documents for the status panels and use docshell swapping so we do not have slow loading on every panel open.
(Reporter)

Comment 1

5 years ago
Created attachment 646772 [details] [diff] [review]
swapping docshells
(Reporter)

Comment 2

5 years ago
Created attachment 646773 [details] [diff] [review]
deck of status browsers
(Reporter)

Comment 3

5 years ago
two approaches, one using docShell swapping, the other using a stack of browsers.  Trying to consider which approach is better for this.

One thought that just came to mind after attaching the patches...

With docShell swapping the status browsers could be attached to the hidden window, thus only requiring up to 3 status browsers for the whole app instance, rather than 3 per window with the deck.  I'm inclined to go that direction.
(Reporter)

Comment 4

5 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> With docShell swapping the status browsers could be attached to the hidden
> window, thus only requiring up to 3 status browsers for the whole app
> instance, rather than 3 per window with the deck.  I'm inclined to go that
> direction.

iiuc, we have to add an html iframe on the hidden window (as we did in the frameworker, in order to work cross platform).  Problem is, I cannot swap the docshell from that to the xul browser, I just get:

WARNING: NS_ENSURE_TRUE(otherEl) failed: file /Users/shanec/moz/mozilla-central/content/xul/content/src/nsXULElement.cpp, line 1420

Which seems to be saying that both elements must be xul elements.
(Reporter)

Comment 5

5 years ago
gavin, jaws,

I have two approaches here.  The docshell swapping mostly works (first time fails, but I figure that is fixable), and then there is simply using a deck of browser elements.  

To get the docshell swapping to work, I have to attach browser elements into the window.  It would be great if this could be attached to the hiddenWindow, but when I tried that approach I got failures (see above).

Otherwise, I think the deck approach is better since it requires less browsers per window than the docshell technique (deck uses 3, docshell swap uses 4) and there is no need to do a docshell swap.  Is there another reason for using docshell swapping that outweighs the additional browser element?
Comment on attachment 646772 [details] [diff] [review]
swapping docshells

It seems that the <deck> solution will give us what we want without having to hand-roll our own swapping implementation.
Attachment #646772 - Flags: feedback-
Comment on attachment 646773 [details] [diff] [review]
deck of status browsers

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

This looks good. I'll test it out and upload an updated patch with a couple tweaks for review.
Attachment #646773 - Flags: feedback+
Assignee: mixedpuppy → jaws
Status: NEW → ASSIGNED
(Reporter)

Updated

5 years ago
Blocks: 779923
Created attachment 648559 [details] [diff] [review]
Patch

This patch uses a <xul:deck> of browsers to keep the panel contents loaded. It shows very good snappiness over the current trunk. I also fixed the panels so that they properly size to content.

This includes a good chunk of Shane's earlier patch.
Attachment #646772 - Attachment is obsolete: true
Attachment #646773 - Attachment is obsolete: true
Attachment #648559 - Flags: review?(gavin.sharp)
Comment on attachment 648559 [details] [diff] [review]
Patch

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>   updateButton: function SocialToolbar_updateButton() {

>+        notifBrowser.setAttribute('src', "data:;charset=utf-8,");

data:text/html;charset=utf-8, ?

>   showAmbientPopup: function SocialToolbar_showAmbientPopup(iconContainer) {

>+    let panelContent = document.getAnonymousElementByAttribute(panel, "class", "panel-arrowcontent");

looks unused

>     panel.addEventListener("popuphiding", function onpopuphiding() {
>       panel.removeEventListener("popuphiding", onpopuphiding);
>-      // unload the panel
>       SocialToolbar.button.removeAttribute("open");
>-      notifBrowser.setAttribute("src", "about:blank");

Like in bug 777177, I think we need to unload these documents when social is entirely disabled. As far as I can tell, with this patch, once you enable social in the browser once these pages will stick around forever.

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+      <deck id="social-notification-deck" flex="1">
>+        <browser type="content"/>
>+        <browser type="content"/>
>+        <browser type="content"/>
>+      </deck>

I wonder whether we shouldn't just be using <iframe>s here. Seems like they'd be lighter weight, and sufficient.
Attachment #648559 - Flags: review?(gavin.sharp) → feedback+
(Reporter)

Comment 10

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Comment on attachment 648559 [details] [diff] [review]
> Patch
> 
> >diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js
> 
> >   updateButton: function SocialToolbar_updateButton() {
> 
> >+        notifBrowser.setAttribute('src', "data:;charset=utf-8,");
> 
> data:text/html;charset=utf-8, ?

A trick from addonsdk:

https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/lib/panel.js#L149

Certainly worth a comment, potentially better than about:blank, but not sure.
Right, I was just suggesting that you include the content type.
(Reporter)

Comment 12

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #11)
> Right, I was just suggesting that you include the content type.

sigh...missed that, time to step away from the computer.
Created attachment 648943 [details] [diff] [review]
Patch v2

This patch addresses the feedback from the previous patch, with the exception about replacing the <browser> elements with <iframe> elements.

When I replaced the <browser>s with <iframe>s, the browser hit an assert upon loading the iframes:
> Assertion failure: equal, at /dom/base/nsJSEnvironment.cpp:1463

I'm not sure why it was asserting, but when I attached the debugger to it the script that it was executing when it hit that assertion was script from the social provider.
Attachment #648559 - Attachment is obsolete: true
Attachment #648943 - Flags: review?(gavin.sharp)
(In reply to Jared Wein [:jaws] from comment #13)
> I'm not sure why it was asserting, but when I attached the debugger to it
> the script that it was executing when it hit that assertion was script from
> the social provider.

D'oh! I just realized that it is probably because I forgot the HTML namespace on the <iframe>.
Created attachment 648950 [details] [diff] [review]
Patch v2.1

Same patch as before but this one is using xul:iframes. The problem I had earlier was that I had removed the type="content" attribute. Thanks Gavin!
Attachment #648943 - Attachment is obsolete: true
Attachment #648943 - Flags: review?(gavin.sharp)
Attachment #648950 - Flags: review?(gavin.sharp)
Created attachment 649523 [details] [diff] [review]
Patch v3

This patch dynamically creates the elements for the iframes as well as the icon containers. I continued to use the icon names here because they provide a unique identifier for each type of notification.
Attachment #648950 - Attachment is obsolete: true
Attachment #648950 - Flags: review?(gavin.sharp)
Attachment #649523 - Flags: review?(gavin.sharp)
Attachment #649523 - Flags: review?(gavin.sharp) → review?(felipc)
Summary: docshell swapping for status panels → Preload social api panels
Created attachment 650652 [details] [diff] [review]
Patch v3.1

This is the same as the previous patch but uses a document fragment to chunk all of the DOM adjustments together.
Attachment #649523 - Attachment is obsolete: true
Attachment #649523 - Flags: review?(felipc)
Attachment #650652 - Flags: review?(felipc)
Comment on attachment 650652 [details] [diff] [review]
Patch v3.1

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

::: browser/base/content/browser-social.js
@@ +350,5 @@
> +        if (icon.counter) {
> +          iconCounter.appendChild(document.createTextNode(icon.counter));
> +        }
> +        iconCounter = iconContainer.appendChild(iconCounter);
> +        iconContainer = iconContainers.appendChild(iconContainer);

nit: add some spacing in this block when you change the element you're handling to make it a bit easier to read

@@ +386,5 @@
> +          browserIter.style.width = "0px";
> +          browserIter.style.height = "0px";
> +        }
> +        browserIter = browserIter.nextElementSibling;
> +      }

given that is necessary to do this, the <deck> is not providing any value. You can just use a <box> and set .hidden on each panel that you'd be setting the width/height to 0

@@ +388,5 @@
> +        }
> +        browserIter = browserIter.nextElementSibling;
> +      }
> +
> +      let [height, width] = [body.firstChild.offsetHeight, 330];

body.firstChild.offsetHeight || 300
Attachment #650652 - Flags: review?(felipc)
Created attachment 650945 [details] [diff] [review]
Patch v3.2

Thanks for the quick review. I've made the requested changes. I like the .hidden better than the width/height=0px too :)
Attachment #650652 - Attachment is obsolete: true
Attachment #650945 - Flags: review?(felipc)
Comment on attachment 650945 [details] [diff] [review]
Patch v3.2

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

::: browser/base/content/browser-social.js
@@ +367,5 @@
>      let iconImage = iconContainer.firstChild;
>      let panel = document.getElementById("social-notification-panel");
> +    let notifBox = document.getElementById("social-notification-box");
> +    let notifBrowser = document.getElementById(iconImage.getAttribute("notifBrowserId"));
> +    notifBox.selectedPanel = notifBrowser;

you can remove this line as it was meant for the deck
Attachment #650945 - Flags: review?(felipc) → review+
(Reporter)

Updated

5 years ago
Whiteboard: [Fx16] → [Fx17]
Created attachment 651576 [details] [diff] [review]
Patch v4

The previous patch failed to update the ambient notification area if the already applied buttons had some of their attributes changed.
Attachment #650945 - Attachment is obsolete: true
Attachment #651576 - Flags: review?(felipc)
Comment on attachment 651576 [details] [diff] [review]
Patch v4

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

::: browser/base/content/browser-social.js
@@ +410,2 @@
>        sizePanelToContent();
> +    }, false);

if it doesn't break anything, let's remove this load listener as it seems it's not necessary anymore since the panel is never loaded here, just displayed
Attachment #651576 - Flags: review?(felipc) → review+
(Reporter)

Updated

5 years ago
Blocks: 774520
Try run:
https://tbpl.mozilla.org/?tree=Try&rev=0067a381cbb0

https://hg.mozilla.org/integration/mozilla-inbound/rev/34d187fac5f7
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 17
Version: unspecified → Trunk
Backed out at Jared's suggestion, on the off-chance this caused bug 782901 (I'm not quite sure how it could have done, but it's worth a shot):
https://hg.mozilla.org/integration/mozilla-inbound/rev/99b0a4f5de33
Target Milestone: Firefox 17 → ---
Tests went green after the backout. Presumably something weird going on with talos - jhammel/jmaher will need to take a look in bug 782901.
Depends on: 782901
Bug 782901 comment 4 suggests that it was a problem with the patch (and not really related to Talos at all).
(Reporter)

Updated

5 years ago
Blocks: 779686
Green try run, with talos this time:
https://tbpl.mozilla.org/?tree=Try&rev=9240325bd84e

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7025e07686
https://hg.mozilla.org/mozilla-central/rev/cd7025e07686
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.