183075 - Improvements to regxpcom - 1.3

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

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

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Doug Turner (:dougt)]

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.

Comment 3

[Alec Flett]

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

Comment 4

[Doug Turner (:dougt)]

> 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 5

[Doug Turner (:dougt)]

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

darin, got time to take a look?

Comment 6

[Darin Fisher]

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.

Comment 7

[Doug Turner (:dougt)]

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.

Comment 8

[Darin Fisher]

really?  can't you just use a static cast?

Comment 9

[Doug Turner (:dougt)]

possible. i will give that a try - if it works that why i will go with that.

Comment 10

[Doug Turner (:dougt)]

xpcom newsgroup posting regarding these changes:

news://news.mozilla.org:119/b6ag4n$[e-mail]

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

Comment 11

[Philip K. Warren]

Small fix needed: "behavoir"->"behavior" in the regxpcom help text.

Comment 12

[Intermittent Failures Robot]

1 failures in 792 pushes (0.001 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-central: 1

Platform breakdown:
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=183075&startday=2017-11-27&endday=2017-12-03&tree=all