Closed
Bug 846013
Opened 11 years ago
Closed 11 years ago
about:blank subframe entries in session restore can cause perf problems
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 22
People
(Reporter: Margaret, Assigned: Margaret)
Details
Attachments
(1 file)
1.92 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
This was fixed in desktop a while ago (bug 705597), and seems like a worthwhile perf win, even if sessionstore.js never gets as big as it could on desktop.
Attachment #719179 -
Flags: review?(bnicholson)
Comment 1•11 years ago
|
||
Comment on attachment 719179 [details] [diff] [review] patch Review of attachment 719179 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/components/SessionStore.js @@ -668,5 @@ > let child = aEntry.GetChildAt(i); > - if (child) > - entry.children.push(this._serializeHistoryEntry(child)); > - else // to maintain the correct frame order, insert a dummy entry > - entry.children.push({ url: "about:blank" }); It's not clear what this bit here was for, so we should watch out for session restore regressions.
Attachment #719179 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #1) > Comment on attachment 719179 [details] [diff] [review] > patch > > Review of attachment 719179 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/components/SessionStore.js > @@ -668,5 @@ > > let child = aEntry.GetChildAt(i); > > - if (child) > > - entry.children.push(this._serializeHistoryEntry(child)); > > - else // to maintain the correct frame order, insert a dummy entry > > - entry.children.push({ url: "about:blank" }); > > It's not clear what this bit here was for, so we should watch out for > session restore regressions. Reading the comments in bug 705597, it looks like this is a historical artifact that's no longer needed. Cc'ing ttaubert in case he remembers what the implications of removing this might be.
Comment 3•11 years ago
|
||
Starting with bug 705597 comment #33 nobody really seemed to know why we were inserting about:blank entries "to maintain the correct frame order" in the first place. I removed that part and I can't recall any problems we had with it. Wait... I just read bug 705597 comment #43 and I recall the .childCount attribute being wrong when dynamic history entries (e.g. iframes) have been removed. That lead to GetEntryAtIndex() failing. ... and I fixed that in bug 707862. So that patch by Margaret should be good :)
Assignee | ||
Comment 4•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd374908d03
Comment 5•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fcd374908d03
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•