Closed
Bug 777176
Opened 12 years ago
Closed 12 years ago
Preload social api panels
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 17
People
(Reporter: mixedpuppy, Assigned: jaws)
References
Details
(Whiteboard: [Fx17])
Attachments
(1 file, 8 obsolete files)
11.99 KB,
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
Reporter | ||
Comment 3•12 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•12 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•12 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?
Assignee | ||
Comment 6•12 years ago
|
||
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-
Assignee | ||
Comment 7•12 years ago
|
||
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 | ||
Updated•12 years ago
|
Assignee: mixedpuppy → jaws
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•12 years ago
|
||
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 9•12 years ago
|
||
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•12 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.
Comment 11•12 years ago
|
||
Right, I was just suggesting that you include the content type.
Reporter | ||
Comment 12•12 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.
Assignee | ||
Comment 13•12 years ago
|
||
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)
Assignee | ||
Comment 14•12 years ago
|
||
(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>.
Assignee | ||
Comment 15•12 years ago
|
||
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)
Assignee | ||
Comment 16•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #649523 -
Flags: review?(gavin.sharp) → review?(felipc)
Assignee | ||
Updated•12 years ago
|
Summary: docshell swapping for status panels → Preload social api panels
Assignee | ||
Comment 17•12 years ago
|
||
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 18•12 years ago
|
||
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)
Assignee | ||
Comment 19•12 years ago
|
||
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 20•12 years ago
|
||
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•12 years ago
|
Whiteboard: [Fx16] → [Fx17]
Assignee | ||
Comment 21•12 years ago
|
||
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 22•12 years ago
|
||
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+
Assignee | ||
Comment 23•12 years ago
|
||
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
Comment 24•12 years ago
|
||
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 → ---
Comment 25•12 years ago
|
||
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
Comment 26•12 years ago
|
||
Bug 782901 comment 4 suggests that it was a problem with the patch (and not really related to Talos at all).
Assignee | ||
Comment 27•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cd7025e07686
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•