Closed Bug 214797 Opened 21 years ago Closed 21 years ago

remove nsFileSpec uses from search

Categories

(SeaMonkey :: Search, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cls, Assigned: shliang)

References

Details

Attachments

(2 files, 1 obsolete file)

nsInternetSearchService still uses nsFileSpec instead of using nsILocalFile.
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #129041 - Flags: superreview?(jaggernaut)
Attachment #129041 - Flags: review?(alecf)
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
Attachment #129041 - Flags: superreview?(jaggernaut)
Attachment #129041 - Flags: superreview+
Attachment #129041 - Flags: review?(darin)
Attachment #129041 - Flags: review?(alecf)
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
Attachment #129041 - Flags: review?(darin) → review+
Attached patch v1.1Splinter Review
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.
Attachment #129041 - Attachment is obsolete: true
Comment on attachment 129767 [details] [diff] [review]
v1.1

Transferring r= and seeking sr= for the nit that couldn't be resolved.
Attachment #129767 - Flags: superreview?
Attachment #129767 - Flags: review+
Attachment #129767 - Flags: superreview? → superreview?(jag)
Comment on attachment 129767 [details] [diff] [review]
v1.1

sr=jag. Feel free to go with this, or with AppendUTF8toUTF16.
Attachment #129767 - Flags: superreview?(jag) → superreview+
Attached patch v1.1.1Splinter Review
de-bitrotted v1.1 patch
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.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
I could be wrong, but I think this patch caused this bug:

http://bugzilla.mozilla.org/show_bug.cgi?id=231203
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: