Last Comment Bug 797716 - tweak social panel auto-resize code
: tweak social panel auto-resize code
Status: RESOLVED FIXED
:
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)
Mentors:
: 764787 797668 (view as bug list)
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


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

Description 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 Mark Hammond [:markh] 2012-10-04 01:16:55 PDT
Created attachment 667854 [details] [diff] [review]
As described.
Comment 2 :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 :Gavin Sharp [email: gavin@gavinsharp.com] 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 Ed Morley [:emorley] 2012-10-05 03:56:57 PDT
https://hg.mozilla.org/mozilla-central/rev/b287821e8d5f
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-05 07:37:57 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/fdc325aabb0e
Comment 7 Shane Caraveo (:mixedpuppy) 2012-10-05 11:03:29 PDT
*** Bug 764787 has been marked as a duplicate of this bug. ***
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 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.