Closed
Bug 776928
Opened 12 years ago
Closed 12 years ago
When doing lazy tab restoration, set the <browser>s in question to display:none until restored
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
VERIFIED
FIXED
Firefox 20
People
(Reporter: bzbarsky, Assigned: ttaubert)
References
Details
(Keywords: addon-compat, dev-doc-needed, Whiteboard: [MemShrink:P1])
Attachments
(1 file, 1 obsolete file)
3.33 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
Kyle and I were looking for about:memory for a large session with tons of about:blank in to-be-restored tabs, and a large component of it is the memory needed for UA stylesheet stuff for all those pages. Simply setting unrestored browsers to display:none will prevent them having to create a presentation and all that jazz, at least for now, saving a good bit of memory.
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [MemShrink]
Version: unspecified → Trunk
Assignee | ||
Comment 1•12 years ago
|
||
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #645318 -
Flags: review?(gavin.sharp)
Attachment #645318 -
Flags: review?(dao)
Comment 2•12 years ago
|
||
Comment on attachment 645318 [details] [diff] [review] patch v1 You shouldn't need the tabbrowser-browser class. You can just style 'browser' in tabbrowser.css. Also please document this with a comment along the lines of comment 0.
Attachment #645318 -
Flags: review?(dao) → review-
Comment 3•12 years ago
|
||
Comment on attachment 645318 [details] [diff] [review] patch v1 >@@ -4351,16 +4353,19 @@ let SessionStoreInternal = { > let browser = aTab.linkedBrowser; > > // Keep the tab's previous state for later in this method > let previousState = browser.__SS_restoreState; > > // The browser is no longer in any sort of restoring state. > delete browser.__SS_restoreState; > >+ aTab.removeAttribute("pending"); >+ browser.removeAttribute("pending"); Is it a bug that the 'pending' attribute wasn't removed from the tab here?
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #3) > >+ aTab.removeAttribute("pending"); > >+ browser.removeAttribute("pending"); > > Is it a bug that the 'pending' attribute wasn't removed from the tab here? Yeah, kind of. When we're interrupting the restoration of a pending tab by restoring a completely different session (i.e. by overwriting the current session), then the pending attribute might not be removed. This wasn't really a problem before but would be now. Also, there are other situations where _resetTabRestoringState() is called to clear a tab's restore state.
Attachment #645318 -
Attachment is obsolete: true
Attachment #645318 -
Flags: review?(gavin.sharp)
Attachment #645720 -
Flags: review?(dao)
Updated•12 years ago
|
Attachment #645720 -
Flags: review?(dao) → review+
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/48bfee9fa89d
Whiteboard: [MemShrink:P1] → [MemShrink:P1][fixed-in-fx-team]
Assignee | ||
Comment 6•12 years ago
|
||
That simple patch didn't go so well... Backed out. https://hg.mozilla.org/integration/fx-team/rev/52be83165f95
Whiteboard: [MemShrink:P1][fixed-in-fx-team] → [MemShrink:P1]
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/48bfee9fa89d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Target Milestone: --- → Firefox 17
Comment 8•12 years ago
|
||
(oops, backed out) https://hg.mozilla.org/mozilla-central/rev/52be83165f95
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 9•12 years ago
|
||
> That simple patch didn't go so well... Backed out.
What went wrong?
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #9) > > That simple patch didn't go so well... Backed out. > > What went wrong? There seems to be a lot of code that accesses browser.contentWindow and the like and just assumes that there's a docShell, etc.
Reporter | ||
Comment 11•12 years ago
|
||
Hmm. And the issue was that the XBL binding didn't get attached? I wonder if we could just force XBL binding attachment here even though we're display:none to work around that issue.
Reporter | ||
Comment 12•12 years ago
|
||
Though I would have thought the binding would have gotten attached when the scripted access happened anyway. Did that not happen for some reason?
Reporter | ||
Comment 13•12 years ago
|
||
Oh, nevermind. This isn't a cloned element, so no binding via precreate. Let me see what I can do about it.
Reporter | ||
Comment 14•12 years ago
|
||
Except we _do_ load bindings for display:none nodes. What's really going on here is that the handling the "type" attribute for <xul:browser> lives in nsSubDocumentFrame.
Reporter | ||
Comment 15•12 years ago
|
||
Local testing indicates that once bug 787131 is fixed we can land this. Pushing the combination to try now.
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Comment 16•12 years ago
|
||
Bug 787131 just landed on inbound, so we should be able to land this on inbound now; on other branches once inbound merges there.
Comment 17•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ef9db325d8 I moved the CSS to tabbrowser.css as advised in comment 2.
Target Milestone: Firefox 17 → Firefox 18
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e5ef9db325d8
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•12 years ago
|
Keywords: addon-compat
Comment 19•12 years ago
|
||
Disabled on 18/19 per bug 789037: https://hg.mozilla.org/releases/mozilla-aurora/rev/add4aedc517f https://hg.mozilla.org/releases/mozilla-beta/rev/ff4b7cf32d5e
status-firefox18:
--- → disabled
status-firefox19:
--- → disabled
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 20•12 years ago
|
||
This is still on trunk, at least for now.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 21•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #0) > Simply setting > unrestored browsers to display:none will prevent them having to create a > presentation and all that jazz, at least for now, saving a good bit of > memory. Can you please tell me how to test this?
Reporter | ||
Comment 22•11 years ago
|
||
Compare about:memory output for a session as described in comment 0 before/after this patch?
Comment 23•11 years ago
|
||
Which section in about:memory should I look at?
Reporter | ||
Comment 24•11 years ago
|
||
The about:blank windows, specifically the layout memory.
Comment 25•11 years ago
|
||
Thanks Boris. Indeed, major improvements with 1000 about:blank tabs: nightly 2012-09-04 - 115.68 MB (95.78%) -- layout nightly 2012-09-06 - 2.60 MB (34.09%) -- layout 19b6 (backout) - 133.24 MB (94.97%) -- layout
Status: RESOLVED → VERIFIED
Comment 26•11 years ago
|
||
We need to get this on the "Firefox 20 for add-on developers" doc at least. Jorge, is this something that would merit a post on the add-ons blog as well?
Keywords: dev-doc-needed
Comment 27•11 years ago
|
||
We will definitely include it in the Firefox 20 compat docs, and I might add it to a separate compat post that I have planned for next week.
Target Milestone: Firefox 18 → Firefox 20
You need to log in
before you can comment on or make changes to this bug.
Description
•