Last Comment Bug 797716 - tweak social panel auto-resize code
: tweak social panel auto-resize code
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
-- normal (vote)
: Firefox 18
Assigned To: Mark Hammond [:markh]
: Shane Caraveo (:mixedpuppy)
: 764787 797668 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2012-10-04 01:14 PDT by Mark Hammond [:markh]
Modified: 2012-10-24 18:16 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

As described. (6.68 KB, patch)
2012-10-04 01:16 PDT, Mark Hammond [:markh]
felipc: review+ approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Mark Hammond [:markh] 2012-10-04 01:14:56 PDT
The resizing code has various issues:

* The existing mutation observer code is poor - it adds multiple observers to the same panel in some cases, and keeps observing even when the panel has been closed (as the document isn't actually unloaded in that case).  This has been changed so the observer starts in the panelshown events and is stopped in the panelhidden events.

* Providers are improving the style annotations in the content loaded into the panels, so the width is more reliable, so we drop the fixed width.  However, it isn't fully reliable yet, so we still adopt a minimum height and width.  More improvements to this can come over time.
Comment 1 User image Mark Hammond [:markh] 2012-10-04 01:16:55 PDT
Created attachment 667854 [details] [diff] [review]
As described.
Comment 2 User image :Felipe Gomes (needinfo me!) 2012-10-04 01:53:29 PDT
Comment on attachment 667854 [details] [diff] [review]
As described.

Review of attachment 667854 [details] [diff] [review]:

::: browser/base/content/browser-social.js
@@ +231,5 @@
> +    // Observe anything that causes the size to change.
> +    let config = {attributes: true, characterData: true, childList: true, subtree: true};
> +    this._mutationObserver.observe(doc, config);
> +    // and seeing this may be setup after the load event has fired we do an
> +    // initial resize now.

s/seeing/since ?
Comment 4 User image :Gavin Sharp [email:] 2012-10-05 02:19:07 PDT
Comment on attachment 667854 [details] [diff] [review]
As described.

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

>+  stop: function DynamicResizeWatcher_stop() {
>+    if (this._mutationObserver) {
>+      try {
>+        this._mutationObserver.disconnect();
>+      } catch (ex) {
>+        // may get "TypeError: can't access dead object" which seems strange,
>+        // but doesn't seem to indicate a real problem, so ignore it...
>+      }

This suggests that you're holding on to a mutationObserver from a document that has been unloaded, which is generally not a good thing to do (even though the leak is prevented by bug 695480). Can you file a followup to clean this up to avoid this problem?
Comment 5 User image Ed Morley [:emorley] 2012-10-05 03:56:57 PDT
Comment 6 User image :Gavin Sharp [email:] 2012-10-05 07:37:57 PDT
Comment 7 User image Shane Caraveo (:mixedpuppy) 2012-10-05 11:03:29 PDT
*** Bug 764787 has been marked as a duplicate of this bug. ***
Comment 8 User image :Gavin Sharp [email:] 2012-10-24 18:16:38 PDT
*** Bug 797668 has been marked as a duplicate of this bug. ***

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