162741 - revamp chrome registry

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[Alec Flett]

As a part of bug 160000, I totally revamped nsIXULChromeRegistry, switching lots
of wstring parameters over to ACString. This allows us to pass around the faster
string objects, resulting in fewer allocations of out parameters and much less
*WithConversion routines.

But wait, you say that wstring->ACString will mean dataloss?
Fear not. The parameters I'm switching over are the locale names, (like "en-US"
or "ja-JP") skin names, (i.e. "modern" or "classic") package names ("navigator"
etc) and the actual type of provider ("skin", "locale" or "content")

Patch forthcoming. I don't know who reviews chrome registry changes now that
hyatt is gone, but I'm ccing some XUL guys in hopes they will review...

Comment 1

[Alec Flett]

Attachment: revamp the chrome registry

the patch. lots of changes. good changes.

Comment 2

[Alec Flett]

oops, over to me

Comment 3

[David Hyatt]

Comment on attachment 95301 [details] [diff] [review]
revamp the chrome registry

sr=hyatt.  Looks good!

Comment 4

[timeless]

+  PRBool mUseXBLForms;
Is there any reason not to make this a PRPackedBool?

Note to self: some of the documentation in the // long paragraph near the end 
needs a bug filed to revamp it. (from from from from)

Comment 5

[Alec Flett]

yes, in this case its just easier because we pass mUseXBLForms into a PrefAPI -
wasn't worth the code overhead for the 3 bytes of space savings. I converted
every other PRBool to PRPackedBool that I could..

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 95301 [details] [diff] [review]
revamp the chrome registry

>@@ -112,11 +113,11 @@
>   nsresult FlushCaches();
> 
> private:
>-  NS_IMETHOD LoadDataSource(const nsACString &aFileName, nsIRDFDataSource **aResult,
>+  nsresult LoadDataSource(const nsACString &aFileName, nsIRDFDataSource **aResult,
>                             PRBool aUseProfileDirOnly = PR_FALSE, const char *aProfilePath = nsnull);

Fix the indenting.  There are several other places where you need to reindent
the second or third lines of parameters for function calls or prototypes, I'm
not going to list them all here.


>@@ -1616,8 +1624,29 @@
> 
> 
> /* wstring getSelectedLocale (); */
This is now the wrong idl prototype.

>@@ -2622,19 +2640,12 @@
>    return NS_GetURLSpecFromFile(userChromeDir, aFileURL);
> }
> 
>-NS_IMETHODIMP
>-nsChromeRegistry::GetInstallRoot(nsCString& aFileURL)
>+nsresult
>+nsChromeRegistry::GetInstallRoot(nsIFile** aFileURL)
> {
>-  nsresult rv;
>-  nsCOMPtr<nsIFile> appChromeDir;
>-
>   // Build a fileSpec that points to the destination
>   // (bin dir + chrome)

Fix this comment, it's not quite right now (and probably hasn't been since this
was converted to nsIFile.)

>@@ -2910,10 +2921,16 @@
> 
> nsresult nsChromeRegistry::LoadInstallDataSource()
> {
>-    nsresult rv = GetInstallRoot(mInstallRoot);
>-    if (NS_FAILED(rv)) return rv;
>-    mInstallInitialized = PR_TRUE;
>-    return AddToCompositeDataSource(PR_FALSE);
>+  nsCOMPtr<nsIFile> installRootFile;
>+  
>+  nsresult rv = GetInstallRoot(getter_AddRefs(installRootFile));
>+  if (NS_FAILED(rv)) return rv;

You could just use NS_ENSURE_SUCCESS for these types of checks.  I'll leave it
up to you.

>+        if (!contentLocale.IsEmpty()) {
>             // caller prefers locale subdir
>             nsCOMPtr<nsIFile> locProfDefaultsDir;
>             rv = profDefaultsDir->Clone(getter_AddRefs(locProfDefaultsDir));
>             if (NS_FAILED(rv)) return rv;
>         
>-            locProfDefaultsDir->Append(nsDependentString(contentLocale));
>+            locProfDefaultsDir->AppendNative(contentLocale);

Is this safe, considering that AppendNative assumes the string to be in the
filesystem charset? (I'm not sure myself)

>@@ -1133,11 +1132,10 @@
>     rv = cmdLineArgs->GetCmdLineValue(CONTENTLOCALE_CMD_LINE_ARG, getter_Copies(cmdContent));
>     if (NS_SUCCEEDED(rv)){
>         if (cmdContent) {
>-            nsAutoString ContentLocaleName;
>-            ContentLocaleName.AssignWithConversion(cmdContent);
>+            nsCAutoString ContentLocaleName(cmdContent);

minor nit, change this variable to the normal interCaps style while you're
here.


Overall, looks like really good cleanup.  r=bryner with the above
changes/explanations.

Comment 7

[Alec Flett]

ok, fixed the nits - good suggestions. Also, AppendNative is safe, because I'm
appending a 7-bit ascii name here - so its valid in all charsets.

Comment 8

[Alec Flett]

ok, fix is in. thanks guys.

Comment 9

[shrirang khanzode]

.