635572 - A Performance bug quite similar to Mozilla bug 267506

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Linhai Song]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: firefox-4.0b11

RCS file: /cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v
retrieving revision 1.86
diff -u -p -r1.86 nsContentUtils.cpp
--- base/src/nsContentUtils.cpp	15 Oct 2004 16:34:58 -0000	1.86
+++ base/src/nsContentUtils.cpp	3 Nov 2004 18:58:56 -0000
@@ -1538,11 +1538,8 @@ nsContentUtils::NewURIWithDocumentCharse
                                           nsIDocument* aDocument,
                                           nsIURI* aBaseURI)
 {
-  nsCAutoString originCharset;
-  if (aDocument)
-    originCharset = aDocument->GetDocumentCharacterSet();
-
-  return NS_NewURI(aResult, NS_ConvertUCS2toUTF8(aSpec), originCharset.get(),
+  return NS_NewURI(aResult, aSpec,
+                   aDocument ? aDocument->GetDocumentCharacterSet().get() : nsnull,
                    aBaseURI, sIOService);
 }

This is a patch for Mozilla Bug 267506. 

I find two code fragments:

First: 
firefox-4.0b11.source/content/base/src/nsFrameLoader.cpp:371

const nsAFlatCString &doc_charset = doc->GetDocumentCharacterSet();
const char *charset = doc_charset.IsEmpty() ? nsnull : doc_charset.get();

Second:
firefox-4.0b11.source/content/base/src/nsImageLoadingContent.cpp:891

const nsAFlatCString &charset = aDocument->GetDocumentCharacterSet();
return NS_NewURI(aURI,
                  aSpec,
                  charset.IsEmpty() ? nsnull : charset.get(),
                  baseURL,
                  nsContentUtils::GetIOService());


According to the patch of Mozilla bug 267506, I think these two code fragments should be changed into:

const char *charset = doc->GetDocumentCharacterSet().IsEmpty() ? nsnull : doc->GetDocumentCharacterSet().get();

and 


return NS_NewURI(aURI,
                  aSpec,
                  aDocument->GetDocumentCharacterSet().IsEmpty() ? nsnull : aDocument->GetDocumentCharacterSet().get(),
                  baseURL,
                  nsContentUtils::GetIOService());


Reproducible: Always

Comment 1

[Mounir Lamouri (:mounir)]

I think this doesn't change anything. In bug 267506, we save a string copy because we save a nsCAutoString instantiation. However, the two changes you request only save a string reference creation. Given that it's a reference, it doesn't copy anything and your changes wouldn't make thing faster.

By the way, your changes a very different than the one in bug 267506 because you check for the string emptiness and pass a value to the method depending on that. Checking the emptiness already access the string. Hopefully, GetDocumentCharacterSet returns a string reference and is an inline method so the cost isn't significant. Otherwise, your change proposal would make things worse.

Comment 2

[Mounir Lamouri (:mounir)]

Marking INVALID per comment 1. Please, feel free to reopen if you do not agree.