Last Comment Bug 777176 - Preload social api panels
: Preload social api panels
Status: RESOLVED FIXED
[Fx17]
:
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)
:
Mentors:
Depends on: 782901
Blocks: 774520 779686 779923
  Show dependency treegraph
 
Reported: 2012-07-24 17:15 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2012-08-21 17:17 PDT (History)
4 users (show)
jaws: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
swapping docshells (5.07 KB, patch)
2012-07-27 17:22 PDT, Shane Caraveo (:mixedpuppy)
jaws: feedback-
Details | Diff | Splinter Review
deck of status browsers (5.33 KB, patch)
2012-07-27 17:23 PDT, Shane Caraveo (:mixedpuppy)
jaws: feedback+
Details | Diff | Splinter Review
Patch (5.85 KB, patch)
2012-08-02 16:45 PDT, Jared Wein [:jaws] (please needinfo? me)
gavin.sharp: feedback+
Details | Diff | Splinter Review
Patch v2 (6.85 KB, patch)
2012-08-03 20:46 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch v2.1 (6.85 KB, patch)
2012-08-03 22:46 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch v3 (9.49 KB, patch)
2012-08-06 19:33 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch v3.1 (9.74 KB, patch)
2012-08-09 13:01 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch v3.2 (9.58 KB, patch)
2012-08-10 10:57 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
Details | Diff | Splinter Review
Patch v4 (11.99 KB, patch)
2012-08-13 17:02 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-07-24 17:15:53 PDT
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.
Comment 1 Shane Caraveo (:mixedpuppy) 2012-07-27 17:22:53 PDT
Created attachment 646772 [details] [diff] [review]
swapping docshells
Comment 2 Shane Caraveo (:mixedpuppy) 2012-07-27 17:23:34 PDT
Created attachment 646773 [details] [diff] [review]
deck of status browsers
Comment 3 Shane Caraveo (:mixedpuppy) 2012-07-27 17:27:30 PDT
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.
Comment 4 Shane Caraveo (:mixedpuppy) 2012-07-27 17:43:01 PDT
(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.
Comment 5 Shane Caraveo (:mixedpuppy) 2012-07-30 14:30:00 PDT
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 6 Jared Wein [:jaws] (please needinfo? me) 2012-07-31 16:35:13 PDT
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.
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-07-31 16:42:02 PDT
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.
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2012-08-02 16:45:54 PDT
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.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 17:34:53 PDT
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.
Comment 10 Shane Caraveo (:mixedpuppy) 2012-08-03 17:39:13 PDT
(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.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 17:40:16 PDT
Right, I was just suggesting that you include the content type.
Comment 12 Shane Caraveo (:mixedpuppy) 2012-08-03 17:43:36 PDT
(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.
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2012-08-03 20:46:12 PDT
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.
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-08-03 21:17:51 PDT
(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>.
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-08-03 22:46:29 PDT
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!
Comment 16 Jared Wein [:jaws] (please needinfo? me) 2012-08-06 19:33:29 PDT
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.
Comment 17 Jared Wein [:jaws] (please needinfo? me) 2012-08-09 13:01:01 PDT
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.
Comment 18 :Felipe Gomes (needinfo me!) 2012-08-09 20:10:25 PDT
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
Comment 19 Jared Wein [:jaws] (please needinfo? me) 2012-08-10 10:57:08 PDT
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 :)
Comment 20 :Felipe Gomes (needinfo me!) 2012-08-10 12:08:19 PDT
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
Comment 21 Jared Wein [:jaws] (please needinfo? me) 2012-08-13 17:02:59 PDT
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.
Comment 22 :Felipe Gomes (needinfo me!) 2012-08-13 17:56:20 PDT
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
Comment 24 Ed Morley [:emorley] 2012-08-15 02:51:08 PDT
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
Comment 25 Ed Morley [:emorley] 2012-08-15 03:41:12 PDT
Tests went green after the backout. Presumably something weird going on with talos - jhammel/jmaher will need to take a look in bug 782901.
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-15 09:52:25 PDT
Bug 782901 comment 4 suggests that it was a problem with the patch (and not really related to Talos at all).
Comment 27 Jared Wein [:jaws] (please needinfo? me) 2012-08-16 18:03:44 PDT
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
Comment 28 Ed Morley [:emorley] 2012-08-17 05:28:20 PDT
https://hg.mozilla.org/mozilla-central/rev/cd7025e07686

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