gBrowser.addTab not treat null/undefined Uri as blank tab leading to extra work at least when restoring session

VERIFIED FIXED in Firefox 3.7a1

Status

()

defect
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: tabmix.onemen, Assigned: dao)

Tracking

({perf})

Trunk
Firefox 3.7a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

If we call gBrowser.addTab(); then in the method addTab
var blank = (aURI == "about:blank") is false if also for cases that aURI is null or undefined.

The result is unnecessary call to loadURIWithFlags, and that slowdown the process of opening new tabs, especially when we open many tabs at once.

For example nsSessionStore first add new tabs with addTab() and just after that it call to restore tab history.

One solution is to replace everywhere addTab() with addTab("about:blank") or
to replace in gBrowser.addTab:

-   var blank = (aURI == "about:blank");
-   var blank = (aURI != null) ? (aURI == "about:blank") : true;
For the reference:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1203
1203             var blank = (aURI == "about:blank");

http://mxr.mozilla.org/mozilla-central/search?string=.addTab%28%29&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Keywords: perf
Summary: gBrowser.addTab not treat null/undefined Uri as blank tab → gBrowser.addTab not treat null/undefined Uri as blank tab leading to extra work at least when restoring session
Whiteboard: [good first bug]
Version: unspecified → Trunk
I fixed this in bug 320989, but it might be a good idea to do this independently.
Posted patch patchSplinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #405665 - Flags: review?(mano)
Whiteboard: [good first bug]
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4f6351d943ef
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Tests have been updated. So I assume those would cover any regression. Marking verified fixed based on no broken tests.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Comment on attachment 405665 [details] [diff] [review]
patch

"approval1.9.2.1=?":
No risk, little perf win.
Attachment #405665 - Flags: approval1.9.2.1?
Comment on attachment 405665 [details] [diff] [review]
patch

No risk, small perf win, 1.9.2.3+
Attachment #405665 - Flags: approval1.9.2.2? → approval1.9.2.3+
Attachment #405665 - Flags: approval1.9.2.3+ → approval1.9.2.3?
Comment on attachment 405665 [details] [diff] [review]
patch

Sorry for the bugspam - this should have been moved to a ?, not a +. I think we should take this in 1.9.2.3, but the tree isn't yet open for those checkins, so we'll circle back around and + once it is.
Attachment #405665 - Flags: approval1.9.1.10?
Attachment #405665 - Flags: approval1.9.2.3?
Attachment #405665 - Flags: approval1.9.2.3+
Attachment #405665 - Flags: approval1.9.1.10?
Attachment #405665 - Flags: approval1.9.1.10-
Comment on attachment 405665 [details] [diff] [review]
patch

Approved for 1.9.2.3, a=dveditz for release-drivers

not approved for 1.9.1, a minor perf win isn't worth it.
Backed out because it was triggering an orange in browser_keyevents_during_autoscrolling.js.

(I get the same failures when I run that test directly even without the patch, so I suspect it's an underlying issue with the test that this just further exposed.)
Comment on attachment 405665 [details] [diff] [review]
patch

Missed the 1.9.2.4 release, removing approval.
Attachment #405665 - Flags: approval1.9.2.4+ → approval1.9.2.4-
You need to log in before you can comment on or make changes to this bug.