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

()

Firefox
Tabbed Browser
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: tabmix.onemen, Assigned: dao)

Tracking

({perf})

Trunk
Firefox 3.7a1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
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;

Comment 1

8 years ago
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
(Assignee)

Comment 2

8 years ago
I fixed this in bug 320989, but it might be a good idea to do this independently.
(Assignee)

Comment 3

8 years ago
Created attachment 405665 [details] [diff] [review]
patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #405665 - Flags: review?(mano)
(Assignee)

Updated

8 years ago
Whiteboard: [good first bug]
Comment on attachment 405665 [details] [diff] [review]
patch

r=mano
Attachment #405665 - Flags: review?(mano) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 5

8 years ago
http://hg.mozilla.org/mozilla-central/rev/4f6351d943ef
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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+
Blocks: 536940
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.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/74a86bea13ae
status1.9.2: --- → .4-fixed
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.)
status1.9.2: .4-fixed → ---
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.