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

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Johnny Stenback (:jst)]

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.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Make nsDocShell::mURIFixup a static member.

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

Comment 2

[Boris Zbarsky [: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....

Comment 3

[Darin Fisher]

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

sr=darin

Comment 4

[Johnny Stenback (:jst)]

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 :-)

Comment 5

[rbs]

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 {

Comment 6

[Johnny Stenback (:jst)]

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

Comment 7

[rbs]

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