Closed Bug 116149 Opened 23 years ago Closed 23 years ago

review new DB code for FreeType checkin

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: bstell, Assigned: bstell)

References

Details

Attachments

(1 file, 7 obsolete files)

corrected to use nsILocalFile.h
1.11 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
this is to help manage the checkin of the new DB code for the FreeType2 code
Attached patch patch; mozilla/gfx/public (obsolete) — — Splinter Review
Attached patch patch; mozilla/gfx/src (obsolete) — — Splinter Review
both this and attachment 62303 [details] [diff] [review] are required. I separated the gfx/public and gfx/src parts since the patch command has problems with multiple Makefile.in files
Blocks: 116147
Few comments: 1) I think we need to call PromiseFlatCString() before InitWithPath() +PRBool +nsNameValuePairDB::OpenTmpForWrite(const char* aCatalogName) +{ + nsCAutoString name(aCatalogName); + name.Append(".tmp"); + nsCOMPtr<nsILocalFile> local_file = new nsLocalFile(); >add >>>>+ const nsAFlatCString& filename = PromiseFlatCString(name); >change>>+ local_file->InitWithPath(filename.get()); and other places where we call InitWithPath() (eg. + current_file->InitWithPath(current_name.get());) since InitWithPath() expects |char *| 2) instead of + char *parent_path; + dir->GetPath(&parent_path); how about + nsXPIDLCString parent_path; + dir->GetPath(getter_Copies(parent_path)); nit: + if (GetNextElement(name, value)<=0) { space before & after '<='
about the recommendation 1), can we use nsPromiseFlatCString instead of nsCAutoString?
used nsPromiseFlatCString with InitwithPath() used nsXPIDLCString with GetPath() added space to test
Attachment #62304 - Attachment is obsolete: true
Comment on attachment 62473 [details] [diff] [review] nsNameValuePairDB.cpp with updates Looks good. /r=yokoyama
Attachment #62473 - Flags: review+
Comment on attachment 62303 [details] [diff] [review] patch; mozilla/gfx/public Sorry, I did review this patch yesterday; but forgot to mark it. Here is my /r=yokoyama
Attachment #62303 - Flags: review+
reassign to you, brian
Assignee: yokoyama → bstell
cc'ing db buddies. Sorry I haven't reviewed this yet, I'll get to it by week end. /be
Status: NEW → ASSIGNED
any news?
Comment on attachment 62473 [details] [diff] [review] nsNameValuePairDB.cpp with updates I shouldn't whine about references for out params, but doesn't this GetNextElement call read as though name and value are in params? >+ while (GetNextElement(name, value) > 0) { >+ if (*name == '\0') // ignore comments >+ continue; >+ if (strcmp(name, "Version")==0) { >+ foundVersion = PR_TRUE; >+ num = sscanf(value, "%d.%d.%d", &major, &minor, &rev); >+ if (num != 3) { >+ NVPDB_PRINTF(("failed to parse version number (%s)", value)); >+ return PR_FALSE; >+ } >+ >+ // NVPDB_VERSION_MAJOR >+ // It is presumed that major versions are not backwards compatibile. >+ if (major != NVPDB_VERSION_MAJOR) { >+ NVPDB_PRINTF(("version major %d != %d", major, NVPDB_VERSION_MAJOR)); >+ return PR_FALSE; >+ } >+ >+ // NVPDB_VERSION_MINOR >+ // If there is a backwards compatibility with different >+ // minor versions put the test here. >+ >+ // NVPDB_VERSION_REV >+ // if there should not be backwards compatibility issues >+ // with different minor versions. Is "minor" above different from "minor versions" in the previous comment-paragraph? REV => dot-release, but what's the right word here? major/minor/miniscule? >+ >+ mMajorNum = major; >+ mMinorNum = minor; >+ mRevNum = rev; >+ } >+ } >+ if (!foundVersion) >+ return PR_FALSE; >+ >+ return PR_TRUE; Why not just 'return foundVersion;' instead of the last four lines? >+} >+ >+// >+// Re-get an element. Used if the element is bigger than >+// the buffer that was first passed in >+// >+// PRInt32 GetCurrentElement(const char* &aName, const char* &aValue, >+// char *aBuffer, PRUint32 aBufferLen); >+// >+// to implement this the GetNextElement methods need to save >+// the file position so this routine can seek backward to it. >+// >+ >+PRInt32 >+nsNameValuePairDB::GetNextElement(const char* &aName, const char* &aValue) >+{ >+ return GetNextElement(aName, aValue, mBuf, FC_BUF_LEN); I need to see the .h file, is it attached elsewhere? I'm wondering whether mBuf is an array or a pointer -- if array of size FC_BUF_LEN, use sizeof mBuf instead of the constant. >+ // >+ // Check we got a complete line >+ // >+ len = strlen(line); >+ if ((!len) || (line[len-1] != '\n')) { /me blinks at all the extra parentheses :-) But seriously, maybe len == 0 is better here. >+ len++; // space for the line terminator >+ while(1) { A space after while as for other control keywords? >+ int val = getc(mFile); >+ len ++; >+ if ((val == '\n') || (val == EOF)) >+ return -len; Should EOF count in len? I'm still looking for uses of the negative r.v. >+ } >+ } >+ len--; >+ line[len] = '\0'; >+ //NVPDB_PRINTF(("line = (%s)", line)); >+ >+ // >+ // Check the group number >+ // >+ num = sscanf(line, "%u ", &groupNum); >+ if ((num != 1) || (groupNum != (unsigned)mCurrentGroup)) >+ return NVPDB_END_OF_GROUP; >+ >+ // >+ // Get the name >+ // >+ name = strchr(line, ' '); >+ if ((!name) || (name[1]=='\0')) >+ return NVPDB_GARBLED_LINE; >+ name++; >+ >+ // >+ // If it is a comment >+ // return a blank name (strlen(aName)==0) >+ // return the comment in the value field >+ // >+ if (*name == '#') { >+ aValue = name; >+ return 1; >+ } Do you really want to require a group number and two (right?) spaces before any comment? >+// HACK: should the input param be a nsReadableCString& ? Is that a hack, or just good minimal C++ API impedence? >+PRBool >+nsNameValuePairDB::OpenForRead(const char* aCatalogName) >+{ >+ nsDependentCString name(aCatalogName); >+ const nsAFlatCString& filename = PromiseFlatCString(name); >+ >+ nsCOMPtr<nsILocalFile> local_file = new nsLocalFile(); >+ local_file->InitWithPath(filename.get()); >+ local_file->OpenANSIFileDesc("r", &mFile); >+ if (mFile == nsnull) >+ goto OpenForRead_Error; >+ >+ // Check the header >+ if (!CheckHeader()) >+ goto OpenForRead_Error; >+ >+ return PR_TRUE; Avoid the gotos via 'if (mFile && CheckHeader()) return PR_TRUE;' and let failure fall into the error code. >+ >+OpenForRead_Error: >+ mError = PR_TRUE; >+ NVPDB_PRINTF(("OpenForRead error")); >+ return PR_FALSE; >+} >+ char buf[64]; >+ PutElement("", "########################################"); Oh I see -- you do want to restrict comments to the "value" field. Still, aren't you requiring two spaces after the group number, earlier? >+ PutElement("", "# #"); >+ PutElement("", "# Name Value Pair DB #"); >+ PutElement("", "# #"); >+ PutElement("", "# This is a program generated file #"); >+ PutElement("", "# #"); >+ PutElement("", "# Do not edit #"); >+ PutElement("", "# #"); >+ PutElement("", "########################################"); >+ sprintf(buf, "%d.%d.%d", NVPDB_VERSION_MAJOR, NVPDB_VERSION_MINOR, >+ NVPDB_VERSION_REV); Although 64 is manifestly big enough, sprintf makes me nervous (Morris worm style exploits). Use PR_snprintf. >+PRBool >+nsNameValuePairDB::PutElement(const char* aName, const char* aValue) >+{ >+ if (mAtEndOfGroup) >+ goto PutElement_Error; >+ >+ if ((!*aName) && (*aValue == '#')) >+ fprintf(mFile, "%u %s\n", mCurrentGroup, aValue); >+ else >+ fprintf(mFile, "%u %s=%s\n", mCurrentGroup, aName, aValue); >+#ifdef DEBUG >+ fflush(mFile); >+#endif >+ return PR_TRUE; >+ >+PutElement_Error: Single goto label suggests eliminating the goto and putting this code up top, instead of the goto. >+ mError = PR_TRUE; >+ NVPDB_PRINTF(("PutElement_Error")); >+ return PR_FALSE; >+} >+ >+PRBool >+nsNameValuePairDB::PutEndGroup(const char* aType) >+{ >+ if (mAtEndOfGroup) >+ goto PutEndGroup_Error; >+ >+ mAtEndOfGroup = PR_TRUE; >+ fprintf(mFile, "%u end=%s\n", mCurrentGroup, aType); >+#ifdef DEBUG >+ fflush(mFile); >+#endif >+ return PR_TRUE; >+ >+PutEndGroup_Error: Ditto. >+ mError = PR_TRUE; >+ NVPDB_PRINTF(("PutEndGroup_Error")); >+ return PR_FALSE; >+} >+ >+PRBool >+nsNameValuePairDB::PutStartGroup(const char* aType) >+{ >+ if (!mAtEndOfGroup) >+ goto PutStartGroup_Error; >+ >+ mAtEndOfGroup = PR_FALSE; >+ mCurrentGroup++; >+ fprintf(mFile, "%u begin=%s\n", mCurrentGroup, aType); >+#ifdef DEBUG >+ fflush(mFile); >+#endif >+ return PR_TRUE; >+ >+PutStartGroup_Error: And again. >+ mError = PR_TRUE; >+ NVPDB_PRINTF(("PutStartGroup_Error")); >+#ifdef DEBUG >+ fflush(mFile); >+#endif >+ return PR_FALSE; >+} >+ >+PRBool >+nsNameValuePairDB::RenameTmp(const char* aCatalogName) >+{ >+ nsresult rv; >+ nsCOMPtr<nsILocalFile> dir; >+ PRBool exists = PR_FALSE; >+ nsCAutoString old_name(aCatalogName); >+ nsDependentCString current_name(aCatalogName); >+ nsCAutoString tmp_name(aCatalogName); >+ nsCAutoString old_name_tail; >+ nsCAutoString current_name_tail; >+ nsCOMPtr<nsILocalFile> old_file; >+ nsCOMPtr<nsILocalFile> current_file; >+ nsCOMPtr<nsILocalFile> tmp_file; >+ nsCAutoString parent_dir; >+ nsXPIDLCString parent_path; >+ nsXPIDLCString cur_path; >+ >+ // >+ // Split the parent dir and file name >+ // >+ PRInt32 slash = 0, last_slash = -1; >+ nsCAutoString aFontDirName(aCatalogName); This variable is named as if it were an argument. >+ // RFindChar not coded so do it by hand >+ while ((slash=((nsAReadableCString&)aFontDirName).FindChar('/', slash))>=0) { >+ last_slash = slash; >+ slash++; >+ } >+ if (last_slash < 0) >+ goto Rename_Error; >+ >+ aFontDirName.Left(parent_dir, last_slash); Lots of copying into autostrings instead of using dependent strings and iterators, but when I see Left, I call for jag or dbaron to suggest a better way. I need to get some sleep, then I'll review the rename code better. Otherwise, let me know what I've hit and missed above. Thanks, /be
Jag, could you suggest alternatives to Left and Right in the rename function at the bottom of the patch? Thanks, /be
Comment on attachment 62473 [details] [diff] [review] nsNameValuePairDB.cpp with updates >+// HACK: should the input param be a nsReadableCString& ? Actually, use const nsACString&, which is what nsAReadableCString typedefs to. >+PRBool >+nsNameValuePairDB::OpenForRead(const char* aCatalogName) >+{ >+ nsDependentCString name(aCatalogName); Then you could get rid of the above line. >+ const nsAFlatCString& filename = PromiseFlatCString(name); I would inline this below. The combination as is doesn't need a PromiseFlatCString btw, nsDependentCString is already flat. If you use |const nsAFlatCString&| as your input parameter you won't need it either, but of course that's less generic. >+ nsCOMPtr<nsILocalFile> local_file = new nsLocalFile(); >+ local_file->InitWithPath(filename.get()); >+ local_file->OpenANSIFileDesc("r", &mFile); ... >+PRBool >+nsNameValuePairDB::OpenTmpForWrite(const char* aCatalogName) >+{ >+ nsCAutoString name(aCatalogName); >+ name.Append(".tmp"); >+ nsCOMPtr<nsILocalFile> local_file = new nsLocalFile(); >+ local_file->InitWithPath(PromiseFlatCString(name).get()); No PromiseFlatCString needed, a nsCAutoString is already flat. >+PRBool >+nsNameValuePairDB::RenameTmp(const char* aCatalogName) >+{ ... >+ PRInt32 slash = 0, last_slash = -1; >+ nsCAutoString aFontDirName(aCatalogName); >+ // RFindChar not coded so do it by hand >+ while ((slash=((nsAReadableCString&)aFontDirName).FindChar('/', slash))>=0) { Why the cast? >+ last_slash = slash; >+ slash++; >+ } >+ if (last_slash < 0) >+ goto Rename_Error; >+ >+ aFontDirName.Left(parent_dir, last_slash); >+ dir = new nsLocalFile(); >+ dir->InitWithPath(PromiseFlatCString(parent_dir).get()); Until InitWithPath takes nsACString&, we need to provide it with a flat(tened) string, so Dependent Substrings which aren't guaranteed to be nul-terminated are out of the question, so this Left call is as good as anything. That said, the PromiseFlatString isn't needed since your copied string is already flat. >+ dir->GetPath(getter_Copies(parent_path)); >+ >+ if (!mAtEndOfGroup || mError) >+ goto Rename_Error; >+ >+ // >+ // check that we have a tmp copy >+ // >+ tmp_name.Append(".tmp"); >+ tmp_file = new nsLocalFile(); >+ tmp_file->InitWithPath(PromiseFlatCString(tmp_name).get()); It's already flat. And no out-of-memory check (I'm sure I missed a few of these nits earlier). >+ tmp_file->Exists(&exists); >+ if (!exists) >+ goto Rename_Error; >+ >+ // >+ // get rid of any old copy >+ // >+ old_name.Append(".old"); >+ old_file = new nsLocalFile(); >+ old_file->InitWithPath(PromiseFlatCString(old_name).get()); Same comments. >+ old_file->Exists(&exists); >+ if (exists) { >+ rv = old_file->Remove(PR_FALSE); >+ } >+ >+ // >+ // Check we have a current copy >+ // >+ current_file = new nsLocalFile(); >+ current_file->InitWithPath(PromiseFlatCString(current_name).get()); >+ current_file->Exists(&exists); Same comments. >+ if (exists) { >+ // >+ // Rename the current copy to old >+ // >+ current_file->GetPath(getter_Copies(cur_path)); >+ old_name.Right(old_name_tail, old_name.Length() - last_slash - 1); >+ rv = current_file->MoveTo(dir, PromiseFlatCString(old_name_tail).get()); Same commments.
Attachment #62473 - Flags: needs-work+
> ... [comments about] references for out params Changed all "char *&" refs to "char **" > >+ if (major != NVPDB_VERSION_MAJOR) { ... > >+ // NVPDB_VERSION_MINOR ... > >+ // NVPDB_VERSION_REV ... > Is "minor" above different from "minor versions" in the previous > comment-paragraph? REV => dot-release, but what's the right word here? > major/minor/miniscule? Changed to: // NVPDB_VERSION_MAJOR // It is presumed that major versions are not backwards compatibile. ... // NVPDB_VERSION_MINOR // It is presumed that minor versions are backwards compatible // but will have additional features. // Put any tests related to minor versions here. // NVPDB_VERSION_MINISCULE // It is presumed that miniscule versions are backwards compatible, // have no new features, but can have bug fixes. // Put any tests related to miniscule versions here. > >+ if (!foundVersion) > >+ return PR_FALSE; > >+ > >+ return PR_TRUE; > Why not just 'return foundVersion;' instead of the last four lines? I intended this call, CheckHeader(), to test if the data in the DB is readable (compatible) with the current code. Since the upper layer has no way to change the current code the return is a boolean and not the version. > >+ return GetNextElement(aName, aValue, mBuf, FC_BUF_LEN); > I need to see the .h file, is it attached elsewhere? Sometimes I've had problems before with patch applying changes to the wrong Makefile.in so since this bug makes changes to gfx/public/Makefile.in and gfx/src/Makefile.in I split the patch into 2 parts. The *.h file is in the same bug, see attachment 62303 [details] [diff] [review]. > I'm wondering whether mBuf is an array or a pointer -- if array of > size FC_BUF_LEN, use sizeof mBuf instead of the constant. Good suggestion. Done. > >+ if ((!len) || (line[len-1] != '\n')) { > /me blinks at all the extra parentheses :-) If you wish I will remove the extra parentheses but this is an area where in trying to be careful I am consciously pedantic. > But seriously, maybe len == 0 is better here. Done. Should I make similar changes for null tests? > >+ while(1) { > A space after while as for other control keywords? Done. Thanks for catching that. > > >+ int val = getc(mFile); > >+ len ++; > >+ if ((val == '\n') || (val == EOF)) > >+ return -len; > > Should EOF count in len? No, fixed. I'm still looking for uses of the negative r.v. The intent of the negative lenght is to allow us to implement this function (hinted at in the *.h file) in the future: + // implement this to re-read an element if it is larger than the buffer + // PRInt32 GetCurrentElement(const char* &aName, const char* &aValue, + // char *aBuffer, PRUint32 aBufferLen); By returning the lenght the caller could reread it with a bigger (malloc'd) buffer when/if we implement this function. > >+ // Check the group number > >+ // > >+ num = sscanf(line, "%u ", &groupNum); ... > >+ // Get the name > >+ // > >+ name = strchr(line, ' '); ... > >+ if (*name == '#') { > >+ aValue = name; > >+ return 1; > >+ } > > Do you really want to require a group number and two (right?) spaces > before any comment? I definitely want a group number before comments to make the parsing regular. I also feel that comments are a valid part of a group and that the app may want to look them. Only one space is required. I assume the question is related to the strchr call. It's there to move beyond the variable length group number. Kindly let me know if I am I missing something here. > >+// HACK: should the input param be a nsReadableCString& ? > Is that a hack, or just good minimal C++ API impedence? For the life of me I still find our string classes seriously under documented. More to follow in the response to jag's comments. > >+nsNameValuePairDB::OpenForRead(const char* aCatalogName) ... > >+ if (mFile == nsnull) > >+ goto OpenForRead_Error; > >+ > >+ // Check the header > >+ if (!CheckHeader()) > >+ goto OpenForRead_Error; > >+ > >+ return PR_TRUE; > Avoid the gotos via 'if (mFile && CheckHeader()) return PR_TRUE;' > and let failure fall into the error code. Done, thank you. > >+ PutElement("", "########################################"); > Oh I see -- you do want to restrict comments to the "value" field. > Still, aren't you requiring two spaces after the group number, earlier? No. I hope I'm not missing something here. > >+ sprintf(buf, "%d.%d.%d", NVPDB_VERSION_MAJOR, NVPDB_VERSION_MINOR, > Although 64 is manifestly big enough, sprintf makes me nervous (Morris worm > style exploits). Use PR_snprintf. As you wish (although for the life of me I cannot figure out how this might be exploited since I control the bufferk the sprintf, and the data). > >+nsNameValuePairDB::PutElement(const char* aName, const char* aValue) > >+{ > >+ if (mAtEndOfGroup) > >+ goto PutElement_Error; ... > >+PutElement_Error: > Single goto label suggests eliminating the goto and putting this code > up top, instead of the goto. Done. > >+nsNameValuePairDB::PutEndGroup(const char* aType) ... > >+PutEndGroup_Error: > Ditto. Done > >+nsNameValuePairDB::PutStartGroup(const char* aType) ... > >+PutStartGroup_Error: > And again. Done > >+nsNameValuePairDB::RenameTmp(const char* aCatalogName) ... > >+ nsCAutoString aFontDirName(aCatalogName); > > This variable is named as if it were an argument. Thanks for catching this. > >+ // RFindChar not coded so do it by hand > >+ while ((slash=((nsAReadableCString&)aFontDirName).FindChar('/', slash))>=0) { ... > >+ aFontDirName.Left(parent_dir, last_slash); > Lots of copying into autostrings instead of using dependent strings and > iterators, but when I see Left, I call for jag or dbaron to suggest a better > way. I tried to do what I understand to be correct per jag's comments but if I missed something kindly let me know. > I need to get some sleep, then I'll review the rename code better. Thanks for working on this (and so late at night). I do apologize for pestering so much. > >+// HACK: should the input param be a nsReadableCString& ? > Actually, use const nsACString&, which is what nsAReadableCString typedefs to. Done. > >+ nsDependentCString name(aCatalogName); > Then you could get rid of the above line. Done. > >+ const nsAFlatCString& filename = PromiseFlatCString(name); > I would inline this below. Done. > The combination as is doesn't need a PromiseFlatCString btw, nsDependentCString > is already flat. > If you use |const nsAFlatCString&| as your input parameter you won't need it > either, but of course that's less generic. I change the input param to nsACString. Kindly let me know if you would like to suggest any changes. > >+nsNameValuePairDB::OpenTmpForWrite(const char* aCatalogName) > >+{ > >+ nsCAutoString name(aCatalogName); > >+ name.Append(".tmp"); > >+ nsCOMPtr<nsILocalFile> local_file = new nsLocalFile(); > >+ local_file->InitWithPath(PromiseFlatCString(name).get()); > No PromiseFlatCString needed, a nsCAutoString is already flat. Fixed. > >+nsNameValuePairDB::RenameTmp(const char* aCatalogName) ... > >+ while ((slash=((nsAReadableCString&)aFontDirName).FindChar('/', slash))>=0) { > Why the cast? My confusion. Removed. > >+ dir->InitWithPath(PromiseFlatCString(parent_dir).get()); > ... That said, > the PromiseFlatString isn't needed since your copied string is already flat. Removed. > >+ tmp_file->InitWithPath(PromiseFlatCString(tmp_name).get()); > It's already flat. And no out-of-memory check (I'm sure I missed a few of these > nits earlier). I searched for all new operations and put in tests. > >+ old_file->InitWithPath(PromiseFlatCString(old_name).get()); > Same comments. > >+ current_file->InitWithPath(PromiseFlatCString(current_name).get()); > Same comments. > >+ rv = current_file->MoveTo(dir, PromiseFlatCString(old_name_tail).get()); > Same commments. Fixed.
Attachment #62303 - Attachment is obsolete: true
Attachment #62473 - Attachment is obsolete: true
Comment on attachment 67677 [details] [diff] [review] reworked per Brendan's and jag's review, both gfx/public and gfx/src >+ PRInt32 mCurrentGroup; >+ PRBool mAtEndOfGroup; >+ PRBool mAtEndOfCatalog; >+ PRBool mError; PRPackedBool, so long as no one takes the address and passes it where PRBool* is expected. >+ // NVPDB_VERSION_MINISCULE Sorry, was half-joking about the name. Perhaps MAINTENANCE is better in light of: >+ // It is presumed that miniscule versions are backwards compatible, >+ // have no new features, but can have bug fixes. >+ // Put any tests related to miniscule versions here. >+ if (!foundVersion) >+ return PR_FALSE; >+ >+ return PR_TRUE; [I wrote] > > >+ if (!foundVersion) > > >+ return PR_FALSE; > > >+ > > >+ return PR_TRUE; > > Why not just 'return foundVersion;' instead of the last four lines? [You replied] > I intended this call, CheckHeader(), to test if the data in the DB > is readable (compatible) with the current code. Since the upper > layer has no way to change the current code the return is a boolean > and not the version. Cool, I didn't mean to change the return value to a version number -- I thought foundVersion was a PRBool, so why not return it here. The effect is the same, and it takes less code than the if (!foundVersion) return PR_FALSE; return PR_TRUE; sequence. >+ if ((len == 0) || (line[len-1] != '\n')) { Just FYI, I found (!len) both overparenthesized (because !, as all unary operators do, binds more tightly than &&, ||, and other binary operators) and obfuscated because len is not a boolean. This version's cool, but still overparenthesized (perhaps to your taste; no worries). >+ len++; // space for the line terminator >+ while (1) { >+ int val = getc(mFile); >+ if (val == EOF) >+ return -len; >+ len ++; Space before ++ not prevailing style. >+ if (val == '\n') >+ return -len; >+ } >+ } >+ len--; >+ line[len] = '\0'; >+ //NVPDB_PRINTF(("line = (%s)", line)); >+ >+ // >+ // Check the group number >+ // >+ num = sscanf(line, "%u ", &groupNum); Ah, I was thrown by the trailing space in the format string. It will cause sscanf to waste cycles skipping one or more spaces in the line. >+ if ((num != 1) || (groupNum != (unsigned)mCurrentGroup)) >+ return NVPDB_END_OF_GROUP; >+ >+ // >+ // Get the name >+ // >+ name = strchr(line, ' '); >+ if ((!name) || (name[1]=='\0')) >+ return NVPDB_GARBLED_LINE; >+ name++; I repeat that ! is never lower precedence than ||, so I don't see why ((!name) || ...) should be written here, but I'm picking a nit. To answer your question about null testing, I prefer if (p) or if (!p) to if (p != nsnull) or if (p == nsnull). There are a couple of reasons, including conciseness, lack of = for == typo phobia leading to nsnull on the left, and lack of precedence concerns (once you know that ! is higher precedence than any boolean). >+ while (GetNextElement(&name, &value) > 0) >+ continue; Overindented continue. >+// HACK: should the input param be a nsReadableCString& ? >+PRBool >+nsNameValuePairDB::OpenForRead(const nsACString & aCatalogName) The HACK comment can go now, right? >+{ >+ nsCOMPtr<nsILocalFile> local_file = new nsLocalFile(); Don't you need do_CreateInstance here? >+ if (local_file) { >+ local_file->InitWithPath(PromiseFlatCString(aCatalogName).get()); >+ local_file->OpenANSIFileDesc("r", &mFile); >+ if ((mFile) && CheckHeader()) (I definitely don't understand the parenthesization of mFile here -- no operator, on possibility of confusion -- but ok.) >+ return PR_TRUE; >+ } >+ >+ mError = PR_TRUE; >+ NVPDB_PRINTF(("OpenForRead error")); >+ return PR_FALSE; >+} >+ >+PRBool >+nsNameValuePairDB::OpenTmpForWrite(const nsACString& aCatalogName) >+{ >+ nsCAutoString name(aCatalogName); >+ name.Append(".tmp"); >+ nsCOMPtr<nsILocalFile> local_file = new nsLocalFile(); Ditto on do_CreateInstance. >+ // >+ // get rid of any old copy >+ // >+ old_name.Append(".old"); >+ old_file = new nsLocalFile(); >+ if (!old_file) >+ goto Rename_Error; >+ old_file->InitWithPath(old_name.get()); >+ old_file->Exists(&exists); >+ if (exists) { >+ rv = old_file->Remove(PR_FALSE); >+ } Won't rename(2) unlink any existing directory entry? /be
Attachment #67677 - Flags: needs-work+
Attached patch revised per Brendan's comments (obsolete) — — Splinter Review
> >+ PRBool mAtEndOfGroup; > >+ PRBool mAtEndOfCatalog; > >+ PRBool mError; > PRPackedBool, so long as no one takes the address and passes it > where PRBool* > is expected. Changed. > >+ // NVPDB_VERSION_MINISCULE > Sorry, was half-joking about the name. Perhaps MAINTENANCE is > better ... Changed. > >+ if (!foundVersion) > >+ return PR_FALSE; > >+ > >+ return PR_TRUE; > > [much text ...] Changed to avoid the unnecessary logic and just return the variable (Just call me thick headed). > >+ if ((len == 0) || (line[len-1] != '\n')) { > Just FYI, I found (!len) both overparenthesized (because !, as all unary > operators do, binds more tightly than &&, ||, and other binary operators) and > obfuscated because len is not a boolean. This version's cool, but still > overparenthesized (perhaps to your taste; no worries). Good point about the len not being a boolean. Regarding parenthesizes: I expect that code will be viewed/modified by developers of varing skill. While highly experienced developers can clearly read this with or without the parenthesizes I try to write my code to be clear to a large range of developers. Hence the the use of parenthesizes for explicit grouping. I will gladly change it if you like. > >+ len ++; > Space before ++ not prevailing style. Thank you. Fixed. > >+ num = sscanf(line, "%u ", &groupNum); > Ah, I was thrown by the trailing space in the format string. It will cause > sscanf to waste cycles skipping one or more spaces in the line. Good point. Removed. > >+ if ((!name) || (name[1]=='\0')) > I repeat that ! is never lower precedence than ||, so I don't see why ((!name) > || ...) should be written here, but I'm picking a nit. Comments as above. I will gladly change this if you like. > >+ while (GetNextElement(&name, &value) > 0) > >+ continue; > Overindented continue. Fixed. Thank you. > >+// HACK: should the input param be a nsReadableCString& ? > The HACK comment can go now, right? Oops, yes of course. Removed. (It has completed its job of getting me advice on this.) > > >+{ > >+ nsCOMPtr<nsILocalFile> local_file = new nsLocalFile(); > > Don't you need do_CreateInstance here? Thank you educating me on this. All "new nsLocalFile()" replaced with do_CreateInstance(NS_LOCALFILEOUTPUTSTREAM_CONTRACTID, &result); > >+ if ((mFile) && CheckHeader()) > (I definitely don't understand the parenthesization of mFile here -- no > operator, on possibility of confusion -- but ok.) Clearly not needed. Thank you. Removed. > >+ if (exists) { > >+ rv = old_file->Remove(PR_FALSE); > >+ } > Won't rename(2) unlink any existing directory entry? Yes, I was being a bit fastidious in cleaning up ahead of time if an error occured. It is not important to me either way and is not needed. Removed. As always, thanks for working on this.
Attachment #67677 - Attachment is obsolete: true
Comment on attachment 67936 [details] [diff] [review] revised per Brendan's comments sr=brendan@mozilla.org. If you agree, pick nits like eliminating singly-used gotos (goto error_return seems especially capricious, because the second error check is inverted into a success check governing an early true return -- that suggests inverting the first failure test and nesting the "success" code in it, letting failures fall through to where the error_return: label lies). Can you remind me how this code will be used? Does the client code read all elements into an in-memory O(1) map of some sort? This was r='d already, right? /be
Attachment #67936 - Flags: superreview+
> pick nits like ... Will do. > Does the client code read all elements into an in-memory O(1) map of > some sort? The caller passes in a pointer to a buffer for the results. If the return is postive it is the number of bytes used in the buffer. If the return is negative it is the needed lenght of the buffer and the data in the buffer is truncated (and null terminated). There is an unimplemented fuction prototype to re-get the same line. This would allow the caller to alloc a bigger buffer and re-get the data. The code is very memory conservative. It might make getting the elements from the group much easier for the caller if this code used a bit more memory (at the callers request) and the group were put in a hash or array for easier access. But perhaps not. There is room for thinking here. Thanks
That reminds me: I'm not sure fgets can return an empty string (pointer to a char array of the form {'\0'}) -- shouldn't it return null for EOF, and a non-empty string otherwise? Anyway, my point is that it looks to me as though any empty string return causes a getc, and if that returns EOF, then the return valueu from GetNextElement is -1. Is this right? It might be better to assert if fgets returns an empty string, and return an error as a backstop in non-DEBUG builds. My question about O(1) lookups of data mapped by an NVDB was really answered by the patch over in bug 116154, but thanks for the detailed reply on this patch. /be /be
(It took me a while to wind into the issue I believe you are addressing. Would you kindly let me know if I am still missing your point.) > I'm not sure fgets can return an empty string (pointer to a > char array of the form {'\0'}) -- shouldn't it return null > for EOF, and a non-empty string otherwise? The Redhat 6.2 fgets man pages says: gets() and fgets() return s on success, and NULL on error or when end of file occurs while no characters have been read. I wonder what fgets does if there is a '\0' in the file? + line = fgets(aBuffer, aBufferLen, mFile); + if (!line) { ... + } + + len = strlen(line); + if ((len == 0) || (line[len-1] != '\n')) { + len++; // space for the line terminator + while (1) { + int val = getc(mFile); + if (val == EOF) + return -len; + len++; + if (val == '\n') + return -len; + } + } > Anyway, my point is that it looks to me as though any empty > string return causes a getc, and if that returns EOF, then > the return valueu from GetNextElement is -1. Is this right? Right, assuming fgets() could return an empty string instead of an EOF. > It might be better to assert if fgets returns an empty string, > and return an error as a backstop in non-DEBUG builds. Would it be okay if I add the assertion and if the string is empty return NVPDB_GARBLED_LINE?
>Would it be okay if I add the assertion and if the string is >empty return NVPDB_GARBLED_LINE? Sure. /be
checked in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Um, there's a problem here. This file #includes nsLocalFileUnix.h. Non unix platforms build with Makefile.in, including Windows and OS/2. If the intent is for this file to be unix only, it needs an if in the makefile, if tis file is for other platforms, it needs an #ifdef in the file.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Attached patch remove unneed include (obsolete) — — Splinter Review
There has been discussion of using this simple DB for other uses than just fonts so I'd like to try to make this work on as many platforms as possible. I find that the include file is no longer needed and it appears to be because in the previous patch Brendan had me change: nsCOMPtr<nsILocalFile> local_file = new nsLocalFile(); to nsCOMPtr<nsILocalFile> local_file = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID,&result); Michael: could you try this patch?
Attached patch revised per dbaron's suggestion (obsolete) — — Splinter Review
removed depricated nsIFileSpec.h replaced nsLocalFileUnix.h with nsLocalFile.h added nsString.h to nsNameValuePairDB.h as it uses nsACString
Attachment #67936 - Attachment is obsolete: true
Attachment #69870 - Attachment is obsolete: true
Michael: could you try attachment 69872 [details] [diff] [review] ?
Comment on attachment 69872 [details] [diff] [review] revised per dbaron's suggestion >Index: gfx/public/nsNameValuePairDB.h >=================================================================== >RCS file: /cvsroot/mozilla/gfx/public/nsNameValuePairDB.h,v >retrieving revision 1.1 >diff -u -r1.1 nsNameValuePairDB.h >--- gfx/public/nsNameValuePairDB.h 16 Feb 2002 08:07:44 -0000 1.1 >+++ gfx/public/nsNameValuePairDB.h 16 Feb 2002 18:50:05 -0000 >@@ -40,6 +40,8 @@ > #ifndef NSNAMEVALUEPAIRDB_H > #define NSNAMEVALUEPAIRDB_H > >+#include "nsString.h" >+ > #define FC_BUF_LEN 1024 > > #define NVPDB_MIN_BUFLEN 100 >Index: gfx/src/nsNameValuePairDB.cpp >=================================================================== >RCS file: /cvsroot/mozilla/gfx/src/nsNameValuePairDB.cpp,v >retrieving revision 1.1 >diff -u -r1.1 nsNameValuePairDB.cpp >--- gfx/src/nsNameValuePairDB.cpp 16 Feb 2002 08:09:11 -0000 1.1 >+++ gfx/src/nsNameValuePairDB.cpp 16 Feb 2002 18:50:05 -0000 >@@ -40,9 +40,8 @@ > #include "nspr.h" > #include "nsCOMPtr.h" > #include "nsAppDirectoryServiceDefs.h" >-#include "nsIFileSpec.h" > #include "nsNameValuePairDB.h" >-#include "nsLocalFileUnix.h" >+#include "nsLocalFile.h" > > #define NVPDB_VERSION_MAJOR 1 > #define NVPDB_VERSION_MINOR 0
Something's weird with some keys in textareas, but what I was trying to say was that the #include "nsLocalFile.h" should be #include "nsILocalFile.h". (I'm not sure why nsLocalFile.h and nsLocalFileUnix.h are even exported.)
I filed bug 125912 on the implementation headers being exported. Also, you should be able to tell from tinderbox whether it fixes the problem -- mkaply shouldn't need to test it for you. Both the OS/2 and Windows+gmake tinderboxes on SeaMonkey-Ports are red because of this.
dbaron: thanks for helping
Attachment #69872 - Attachment is obsolete: true
Comment on attachment 69878 [details] [diff] [review] corrected to use nsILocalFile.h r=dbaron
Attachment #69878 - Flags: review+
Yeah, when I commented on using do_CreateInstance instead of new nsLocalFile, I meant "don't include nsLocalFileUnix.h" -- sorry if I wasn't clear enough. /be
Sorry to jump into this late, but is this Yet Another DB in Mozilla? Don't we already have nsPersistentProperties for name/value pairs? don't we already have mork for more complex databases?
Before I began this (4 months ago) I talked with Vidur, Bienvenu, Brendan about this. Vidur felt that XML was a bit much for this and performance would be an issue. I talked with Bienvenu about mork. Mork while being an impressive DB has never been used outside of mail and is primarily known and maintained by 2 people. If mork were in general use and had a simple API I would have felt it was a better match for use as a font summary DB. I believe Brendan agreed that mork was not a good fit. We discussed prefs but the data is only read once then stored in a font summary table. Using prefs would give a big unneeded memory footprint. I'n not that familiar with nsPersistentProperties. Perhaps Brendan or someone else would care to comment.
wrong! Mork is use constantly outside of mail - in that it's used for your global history, and this is accessed every time CSS tries to color a link as "visited" or "not visited" Mork has a somewhat obscure, but not horribly complex API, and nsGlobalHistory is a perfect simple consumer of that API. Mork buys you things like smart-atomization of strings, journaling and basically no corruption (reports of global history or mail summary corruptions are few and far between) - its also quite performant. I agree prefs would be a very bad call. nsPersistentProperties is basically: 1) a file format of name=value pairs, read from disk as UTF8 2) an API for getting and setting these name value pairs (see nsIPersistentProperties in http://lxr.mozilla.org/) nsPersistentProperties is what we use for string bundles - so like mork, a well-trodden piece of code. You would use nsIPersistentProperties AS your font summary table (i.e. instead of using it and then populating your own data with it) I'm just trying to promote code reuse and reduce risk - we are long past the days of fitting on a floppy disk (remember that?) but we've got to stop reinventing the wheel in every module!
URL: http://http://
I plead guilty for not thinking of nsPersistentProperties -- bstell, was there something it couldn't do that you need? If not, can you switch to using it? We can figure out the timing after we figure out how much of a stretch it is to use, but obviously it would be better to switch now. /be
I will try to look at it in a few hours. I should have a couple of hours between mignight and about 3 am while I watching the tree. :)
in bug 48888 alecf suggested that nsIPresistentProperties is only one of several tools we might consider.
One way to speed up startup is to not load all the font summary info. Loading 200 font summaries takes about 70 milliseconds on my system. If only the information needed at startup were loaded it would be much faster. When I modified the data to just have the minimal info it reduced the time by about 90% to around 7 milliseconds. To avoid reading all the data use a font summary per font. Put the font summary files in a subdir. Public in /usr/local/mozilla/fonts/summaries Private in $HOME/.mozilla/fonts/summaries Abbreviated Font Summaries (AFS) file read at startup to get font list minimal font info to keep reading it as fast as possible one each per font ================= font.<major-minor>.timestamp=<timestamp> font.<major-minor>.name=<family name> font.<major-minor>.langInfo=<lang info> Regular Fonts Summaries file name=<font name> filename=<font filename> timestamp=<timestamp> langInfo=<lang info> IsValid=<is valid> num_ccmap_pages=<number of ccmap pages> ccmap.<number>=<ccmap line>
overview of the steps: At Startup ======================================================== Startup is very time critical get list of dirs foreach dir in list of dirs { get abbreviated font summaries (AFS) for the dir foreach file in dir { lookup file in AFS if (missing or (timestamp changed)) { note AFS needs update scan font write its font summary } note that this entry has been checked if (!IsValid) continue; foreach lang in the font's lang groups add font-lang to font nodes } check if all entries have been checked if !(all entries checked) fontsPruned = true if (fontsPruned || (AFS needs update)) write new AFS } When font requested ======================================================== lookup file in AFS load font summary file
Target Milestone: --- → mozilla1.0
*** Bug 96768 has been marked as a duplicate of this bug. ***
There is an on going discussion about the reusing code for the DB. This bug was about reviewing the code for checkin and checkin has happend so marking this bug fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
QA Contact: teruko → bstell
the code has been checked in so I'm closing out this "needs review" bug
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: