Closed Bug 797716 Opened 7 years ago Closed 7 years ago

tweak social panel auto-resize code

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set

Tracking

(firefox17 fixed)

RESOLVED FIXED
Firefox 18
Tracking Status
firefox17 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

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.
Attached patch As described.Splinter Review
Assignee: nobody → mhammond
Attachment #667854 - Flags: review?(felipc)
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 ?
Attachment #667854 - Flags: review?(felipc) → review+
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?
https://hg.mozilla.org/mozilla-central/rev/b287821e8d5f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Attachment #667854 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 764787
Duplicate of this bug: 797668
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.