214797 - remove nsFileSpec uses from search

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[cls]

nsInternetSearchService still uses nsFileSpec instead of using nsILocalFile.

Comment 1

[cls]

Attachment: v1.0

Comment 2

[jag (Peter Annema)]

Comment on attachment 129041 [details] [diff] [review]
v1.0

+		   temp.Append(NS_ConvertASCIItoUCS2(extensions[ext_count]));

AppendASCIItoUTF16(temp, extensions[ext_count]);

(needs nsReadableUtils.h)

+		   if (foundIconFlag == PR_TRUE) 

Instead of that, just say |if (foundIconFlag)|.

+	 rv = ((nsIFile*)localFile)->GetFileSize(&contentsLen);

That cast there shouldn't be needed, since nsILocalFile inherits from nsIFile.
Same in other places.

It's good that you're using spaces instead of tabs, though at least two lines
mix tabs and spaces, leading to weird spacing issues. Could you fix those?

Other than that, sr=jag

Comment 3

[Darin Fisher]

Comment on attachment 129041 [details] [diff] [review]
v1.0

>Index: xpfe/components/search/src/nsInternetSearchService.cpp

>+        nsCOMPtr<nsILocalFile> engineFile;
>+        rv = NS_NewNativeLocalFile(nsDependentCString(baseFilename), PR_TRUE, getter_AddRefs(engineFile));
>+        if (NS_FAILED(rv)) return rv;
> 
> 	nsString	data;
>+	rv = ReadFileContents(engineFile, data);

looks like indentation inconsistencies here.


>-			if (pngIconFile.IsFile())
>-			{
>-				iconSpec = pngIconFile;
>-				foundIconFlag = PR_TRUE;
>-			}
>-		}
>+                static const char *extensions[] = {
>+                  ".gif",
>+                  ".jpg",

and here you are changing the indentation style.  maybe try to keep
things consistent?  or does the file already mostly use 2 space
indents?


>+                  ".jpeg",
>+                  ".png",
>+                  0,
>+                };

nit: use nsnull instead of 0 :)


>+
>+                int ext_count = 0;
>+                while (extensions[ext_count] != 0) {

nit:
		  while (extensions[ext_count]) {
or
		  while (extensions[ext_count] != nsnull) {


>+                  uri.Left(temp, uri.Length()-4);

nit: it is generally encouraged to use Substring instead of Left,Mid,Right


> 	char				*contents;
> 
>+        NS_ENSURE_ARG_POINTER(localFile);
>+
> 	sourceContents.Truncate();
> 
>-	contentsLen = fileSpec.GetFileSize();
>+        rv = ((nsIFile*)localFile)->GetFileSize(&contentsLen);
>+        if (NS_FAILED(rv)) return rv;
> 	if (contentsLen > 0)

indentation


>+                        while (total < contentsLen) {
>+                          	rv = inputStream->Read(contents+total, 
>+                                                       PRUint32(contentsLen),
>+                                                       &howMany);
>+                                if (NS_FAILED(rv)) {
>+                                  delete [] contents;

indentation


r=darin with nits fixed

Comment 4

[cls]

Attachment: v1.1

This patch fixes the nits with the exception of the
.Append(NS_ConvertASCIIToUCS2) one.  LXR shows no AppendASCIIToUTF16 function. 
Btw, most of the indentation issues come from existing tabs in the file but I
didn't want to touch all of those spots.

Comment 5

[cls]

Comment on attachment 129767 [details] [diff] [review]
v1.1

Transferring r= and seeking sr= for the nit that couldn't be resolved.

Comment 6

[jag (Peter Annema)]

Comment on attachment 129767 [details] [diff] [review]
v1.1

sr=jag. Feel free to go with this, or with AppendUTF8toUTF16.

Comment 7

[Simon Paquet [:sipaq]]

Attachment: v1.1.1

de-bitrotted v1.1 patch

Comment 8

[[e-mail]]

I did not check in sipaq's patch because he changed spaces to tabs,
instead I reupdated cls's original patch and checked it in.

Comment 9

[Matthew A. McGuigan]

I could be wrong, but I think this patch caused this bug:

http://bugzilla.mozilla.org/show_bug.cgi?id=231203