Don't waste time/space with a per-docshell reference to the nsIURIFixup service.

RESOLVED FIXED

Status

()

Core
Document Navigation
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: jst, Assigned: Adam Lock)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

14 years ago
For every docshell we create (2 per window, generally) we end up looking up the
nsIURIFixup service only to store it in a docshell member when we could just
keep a single static member reference to it. Patch coming up.
(Reporter)

Comment 1

14 years ago
Created attachment 148128 [details] [diff] [review]
Make nsDocShell::mURIFixup a static member.

... and also move some members around to save a bit of memory on some 64-bit
platforms.
(Reporter)

Updated

14 years ago
Attachment #148128 - Flags: superreview?(darin)
Attachment #148128 - Flags: review?(bzbarsky)
Comment on attachment 148128 [details] [diff] [review]
Make nsDocShell::mURIFixup a static member.

>Index: docshell/base/nsDocShell.cpp
>+    if (gDocShellCount++ == 0) {

Assert that sURIFixup is null here?

r=bzbarsky, though it may be worthwhile to change that slew of PRPackedBools
into a bitfield....
Attachment #148128 - Flags: review?(bzbarsky) → review+

Comment 3

14 years ago
Comment on attachment 148128 [details] [diff] [review]
Make nsDocShell::mURIFixup a static member.

sr=darin
Attachment #148128 - Flags: superreview?(darin) → superreview+
(Reporter)

Comment 4

14 years ago
Assertion added. It might be worth making those PRPackedBool's into bitfields,
yes, but I'll wipe that under the hopefully upcoming major nsDocShell cleanup
carpet for now :-)
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 5

14 years ago
jst, looks like there is an oversight in the dangling |else| below. The first
consumer will miss sURIFixup. Shouldn't it be:

     if (!sURIFixup) {
       ...create it
     }
     if (sURIFixup) {
       ...use it
     }

=====================

+    if (!sURIFixup) {
+        // No fixup service so try and create a URI and see what happens
+        nsAutoString uriString(aURI);
+        // Cleanup the empty spaces that might be on each end.
+        uriString.Trim(" ");
+        // Eliminate embedded newlines, which single-line text fields now allow:
+        uriString.StripChars("\r\n");
+        NS_ENSURE_TRUE(!uriString.IsEmpty(), NS_ERROR_FAILURE);
 
-            rv = NS_NewURI(getter_AddRefs(uri), uriString);
-        }
-    }
-    if (mURIFixup) {
+        rv = NS_NewURI(getter_AddRefs(uri), uriString);
+    } else {
(Reporter)

Comment 6

14 years ago
Um, looks right to me. Am I missing something obvious here?

Comment 7

14 years ago
Ah, ok. I now see that's what you meant.
http://bugzilla.mozilla.org/attachment.cgi?id=148128&action=diff#mozilla/docshell/base/nsDocShell.cpp_sec3
You need to log in before you can comment on or make changes to this bug.