Status

()

defect
RESOLVED FIXED
17 years ago
2 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Required changes to RegXPCOM:

Use XPCOM Glue and Frozen Interfaces only.

Allowed to be run from any directory (such as /temp) and specify which XPCOM
registry this regristration attempt coosponds to.

RegXPCOM should allow GRE/build independency.  There shouldn’t be multiple
version of RegXPCOM.

Code clean up and reduction

Clean up console output – some goes to stderr, other goes to stdout.

Add Help option
Blocks: 168052
Posted patch patch v.1Splinter Review
Comment on attachment 119112 [details] [diff] [review]
patch v.1

i would like to land this for 1.4b.  If these changes are unacceptable for the
1.4 release, we may want to think about creating a new replacement application.
 The reason I say this is that we want to make sure that we don't bust anyone
using the current version of regxpcom and at the same time we don't want to
continue to block people that have these new registration requirements.
Attachment #119112 - Flags: superreview?(dveditz)
Attachment #119112 - Flags: review?(alecf)
Comment on attachment 119112 [details] [diff] [review]
patch v.1

+    char* xpcomPath = (char*) malloc(len + sizeof(XPCOM_DLL) +
sizeof(XPCOM_FILE_PATH_SEPARATOR) + 1);
+    sprintf(xpcomPath, "%s" XPCOM_FILE_PATH_SEPARATOR XPCOM_DLL,
gXPCOMLocation);

um, did you mean %s%s here? why not just use strcpy/strcat? don't forget to
check for OOM
+Mozilla RegXPCOM - a registration tool for xpcom components		       
\n\
+									       
\n\
+Usage: regxpcom [options] [file-or-directory]				       
\n\

do this:
printf(
"foo	    \n"
"bar	    \n"

super long lines are difficult to manage/indent in some editors - let C join
the strings instead

other than that, this looks great, doesn't seem to risky for 1.4 to me..lets
just get it in soon. r=alecf
Attachment #119112 - Flags: review?(alecf) → review+
> um, did you mean %s%s here? why not just use strcpy/strcat? don't forget to

Nope.  Same thing as you suggest in the next comment.  The second parameter to
sprintf looks like this after the preprocessor does its thing:

"%s" "\\" "xpcom.dll"

"difficult to manage/indent in some editors"  Bah.  Use Emacs!! :-)  seriously
though.... i will fix it ;-)

Comment on attachment 119112 [details] [diff] [review]
patch v.1

darin, got time to take a look?
Attachment #119112 - Flags: superreview?(dveditz) → superreview?(darin)
Comment on attachment 119112 [details] [diff] [review]
patch v.1

>+#include "stdlib.h"

#include <stdlib.h>

maybe some build environment cares??


>+  const char* fileLocation = nsnull;
...
>+  else if(strcmp(prop, NS_XPCOM_XPTI_REGISTRY_FILE) == 0 && gXPTIDatLocation)
>+  {
>+    fileLocation = gXPTIDatLocation;
>+  }
>+
>+  if (!fileLocation)
>+    return NS_ERROR_FAILURE;

hmm... why don't you just add an |else| clause above, like so:

  else if(strcmp(prop, NS_XPCOM_XPTI_REGISTRY_FILE) == 0 && gXPTIDatLocation)
  {
    fileLocation = gXPTIDatLocation;
  }
  else
    return NS_ERROR_FAILURE;

or can some of the g-prefixed variables be null?


>+  return localFile->QueryInterface(NS_GET_IID(nsIFile), (void**)_retval);

hmm... no query interface needed here since nsILocalFile inherits from nsIFile.

    NS_ADDREF(*_retval = localFile);
    return NS_OK;


>+  if (gXPCOMLocation) {
>+    int len = strlen(gXPCOMLocation);
>+    char* xpcomPath = (char*) malloc(len + sizeof(XPCOM_DLL) + sizeof(XPCOM_FILE_PATH_SEPARATOR) + 1);
>+    sprintf(xpcomPath, "%s" XPCOM_FILE_PATH_SEPARATOR XPCOM_DLL, gXPCOMLocation);

nit: sizeof(XPCOM_DLL) includes room for null byte, so you're allocating
two extra bytes.


>+    gPathEnvString = PR_smprintf("%s=%s;%s",
>+                                 XPCOM_SEARCH_KEY,
>+                                 gXPCOMLocation,
>+                                 path);

nit: check for memory allocation failure here?


>+    spec = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
...
>+    rv = spec->InitWithNativePath(nsEmbedCString(path));

nit: why not use NS_NewNativeLocalFile?


>+          case 'q':
>+            gQuiet = PR_TRUE;
>+            break;
>+
>+        case 'a':

nit: indentation


sr=darin w/ nits addressed.
Attachment #119112 - Flags: superreview?(darin) → superreview+
all fixed but:

>+  return localFile->QueryInterface(NS_GET_IID(nsIFile), (void**)_retval);

> hmm... no query interface needed here since nsILocalFile inherits from nsIFile.

compiler wants it.
really?  can't you just use a static cast?
possible. i will give that a try - if it works that why i will go with that.
xpcom newsgroup posting regarding these changes:

news://news.mozilla.org:119/b6ag4n$8jt1@ripley.netscape.com

Checking in Makefile.in;
/cvsroot/mozilla/xpcom/tools/registry/Makefile.in,v  <--  Makefile.in
new revision: 1.29; previous revision: 1.28
done
Checking in regxpcom.cpp;
/cvsroot/mozilla/xpcom/tools/registry/regxpcom.cpp,v  <--  regxpcom.cpp
new revision: 1.24; previous revision: 1.23
done

Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Small fix needed: "behavoir"->"behavior" in the regxpcom help text.
You need to log in before you can comment on or make changes to this bug.