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)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: Margaret, Assigned: Margaret)

Details

Attachments

(1 file)

Attached patch patchSplinter 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 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+
(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.
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 :)
https://hg.mozilla.org/mozilla-central/rev/fcd374908d03
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: