394826 - McCoy initial landing

Status: RESOLVED FIXED

(Other Applications Graveyard :: McCoy, defect)

Description

[Dave Townsend [:mossop]]

To track the review and other bits needed for the initial release of McCoy.

Not sure if we're going to create a new bugzilla component for this so just filing here for the time being.

Comment 1

[Dave Townsend [:mossop]]

Attachment: initial implementation

This is the initial implementation. Not perfect but close to pre-release worthy.

Since this includes a binary component, the only way I have right now to build it is as part of an xulrunner build. Extract it into mozilla/xulrunner/examples/mccoy and add mccoy to the DIRS in Makefile.in above that. Do the build and it should spit out in <OBJDIR>/dist/xpi-stage/mccoy. It requires a recent XULRunner version (last few days).

Rather than get the UI spot on, let's just work over any issues with the code itself then pick up any UI issues we need before a release in other bugs, need to get this into svn soon so we can start making incremental changes.

To any extension authors watching, I'd advise against building this version yourselves and releasing extensions secured with this, if we have to make changes to the signing process then you might find you've released an extension that can never be updated. Hopefully it shouldn't be long before we can do at least an pre-release of this.

Comment 2

[Benjamin Smedberg]

If you put a build.mk in (maybe need a confvars.sh also) you can build this against the pre-built XR SDK, instead of needing a full build tree.

e.g. unzip your stuff to mozilla/mccoy and then build with --enable-application=mccoy --with-libxul-sdk=/path/to/sdk

Comment 3

[Alex Vincent [:WeirdAl]]

Can someone on the inside clue the masses as to what McCoy is?  :-)

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #2)
> If you put a build.mk in (maybe need a confvars.sh also) you can build this
> against the pre-built XR SDK, instead of needing a full build tree.
> 
> e.g. unzip your stuff to mozilla/mccoy and then build with
> --enable-application=mccoy --with-libxul-sdk=/path/to/sdk

Unfortunately it requires NSS libraries which don't appear to be in the SDK.

(In reply to comment #3)
> Can someone on the inside clue the masses as to what McCoy is?  :-)

Care to read the component description?

Comment 5

[Phil Ringnalda (:philor)]

> what McCoy is?

It's not a magician, just an old country doctor.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

These are some notes from a first-pass review of the "components" folder:

> interface nsIKeyPair : nsISupports
> {
>   attribute string name;

I was thinking that this should be an |ACString| instead of |string| and when I looked at the implementation of KeyPair::GetName, I believe it more.

> KeyPair::GetName(char * *aName)
> {
>     *aName = PK11_GetPrivateKeyNickname(mPrivateKey);

GetPrivateKeyNickname allocates the value returned (aName in this case) which needs to be PROT_Free later. But we can't since its an XPCOM return value. I think you should use ACString as the param type, assign the key nickname return value then free it before returning.

> interface nsIKeyService : nsISupports
> {
>   nsISimpleEnumerator enumerateKeys();

Looks like this could be implemented using nsCOMArray and NS_NewArrayEnumerator. Then you could remove KeyPairEnumerator.h/.cpp

non-nit: KeyService.cpp uses "canceled" in several places and I was going to say it was spelled incorrectly, but "canceled" appears elsewhere in our code (as does "cancelled" which I had believed was the correct en-US spelling)

I wasn't able to help much on the nitty-gritty PK_* and SEC* stuff.

I'll move on to "content" next

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Note that my "nits" are really up to you. Don't bother with them if you don't want to.

application.ini :
> ID={c8a67ca0-cc27-4f45-91d9-dae18ebadabf}

Can we switch to a new style ID? "[e-mail]" maybe

General comment for most of the XUL files:

For accessibility, we should at least use <label control="xxx" accesskey="&xxx.yyy;" /> when labels are paired with UI widgets like textboxes.

Also accesskey on <menu> and <menuitem>


keywizard.js :

>  if (gRandom.length < RANDOMLENGTH) {
>    document.documentElement.canAdvance = false;
>  }

Other places you don't use braces for single line blocks

>  else {
>    alert("You must enter a name for the key.");
>    return false;
>  }

Might want to use .properties

nit: You don't need the else-block since the if-block returns

mccoy.js : 

> alert("...")
Might want to use .properties

>    case "cmd_about":
>      var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>               getService(Ci.nsIWindowMediator);
>      var win = wm.getMostRecentWindow("McCoy:About");
>      if (win)
>        win.focus();
>      else
>        window.openDialog("chrome://mccoy/content/about.xul", "",
>                          "chrome,dialog,centerscreen");

nit: Making the dialog modal would simplify this

>    }
>    else
>      alert("No signature exists in the update manifest");
>  }
>  else
>    alert("Install manifest is invalid or does not contain a public key");

Use braces since the if-blocks require braces and use .properties

mccoy.xul :

>  <toolbox>
>    <toolbar mode="icons">

You're using mode="icons", but it doesn't seem to be working. I had to force it in some of my apps. This can be fixed later, just wanted to point it out.


Nice code and its was easy to follow. r=mfinkle with the non-nits addressed

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 279986 [details]
initial implementation

We should also move this out of "mozilla/xulrunner/examples/mccoy".

"mozilla/mccoy" seems better

Comment 9

[Dave Townsend [:mossop]]

Just providing any answers to these as I come across them, will post the updated patch later

(In reply to comment #6)
> These are some notes from a first-pass review of the "components" folder:
> 
> > interface nsIKeyPair : nsISupports
> > {
> >   attribute string name;
> 
> I was thinking that this should be an |ACString| instead of |string| and when I
> looked at the implementation of KeyPair::GetName, I believe it more.

Agreed

> > interface nsIKeyService : nsISupports
> > {
> >   nsISimpleEnumerator enumerateKeys();
> 
> Looks like this could be implemented using nsCOMArray and
> NS_NewArrayEnumerator. Then you could remove KeyPairEnumerator.h/.cpp

Thanks for the reminder. I had originally developed this against the 1.8 branch where NS_NewArrayEnumerator wasn't available. That simplifies things now.

> non-nit: KeyService.cpp uses "canceled" in several places and I was going to
> say it was spelled incorrectly, but "canceled" appears elsewhere in our code
> (as does "cancelled" which I had believed was the correct en-US spelling)

According to the OED cancelled is the correct spelling but canceled is also acceptable, so changed to cancelled throughout ;)

I've also changed the code for passing ACString data to PK11 methods to using PromiseFlatCString rather than BeginReading, it makes hte code more readable I believe.

Comment 10

[Reed Loden [:reed]]

(In reply to comment #9)
> According to the OED cancelled is the correct spelling but canceled is also
> acceptable, so changed to cancelled throughout ;)

"cancelled" is wrong in en-US. "canceled" is the correct spelling. Silly Brits.

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #7)
> keywizard.js :
> 
> >  if (gRandom.length < RANDOMLENGTH) {
> >    document.documentElement.canAdvance = false;
> >  }

I've dropped the keywizard now and filed bug 396009 for adding additional
entropy in the future if necessary.

> >    case "cmd_about":
> >      var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
> >               getService(Ci.nsIWindowMediator);
> >      var win = wm.getMostRecentWindow("McCoy:About");
> >      if (win)
> >        win.focus();
> >      else
> >        window.openDialog("chrome://mccoy/content/about.xul", "",
> >                          "chrome,dialog,centerscreen");
> 
> nit: Making the dialog modal would simplify this

So it looks like about dialogs tend to be non-modal on OSX and modal elsewhere. I'll leave this for now but there are a couple of other things that should behave differently on different platforms.

> mccoy.xul :
> 
> >  <toolbox>
> >    <toolbar mode="icons">
> 
> You're using mode="icons", but it doesn't seem to be working. I had to force it
> in some of my apps. This can be fixed later, just wanted to point it out.

I had forgotton I put that in, I think I kinda like it with the text too so
removed it for now.

Taken the rest.

Comment 12

[Dave Townsend [:mossop]]

Landed with a big update to the build system so we can build and package on any platform barring universal OSX (though I can do those manually for the time being until I can get something that works better). Still a couple of other issues to take care of before a first release, will track those in bugs targetting M1

Adding         Makefile.in
Adding         app
Adding         app/Makefile.in
Adding         app/application.ini
Adding         app/default-prefs.js
Adding         app/macbuild
Adding         app/macbuild/Contents
Adding         app/macbuild/Contents/Info.plist.in
Adding         app/macbuild/Contents/Resources
Adding         app/macbuild/Contents/Resources/English.lproj
Adding         app/macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in
Adding         build.mk
Adding         chrome
Adding         chrome/Makefile.in
Adding         chrome/branding
Adding         chrome/branding/Makefile.in
Adding         chrome/branding/brand.dtd
Adding         chrome/branding/brand.properties
Adding  (bin)  chrome/branding/icon48.png
Adding  (bin)  chrome/branding/icon64.png
Adding         chrome/branding/jar.mn
Adding         chrome/content
Adding         chrome/content/Makefile.in
Adding         chrome/content/about.xul
Adding         chrome/content/jar.mn
Adding         chrome/content/mccoy.js
Adding         chrome/content/mccoy.xml
Adding         chrome/content/mccoy.xul
Adding         chrome/content/rdfserializer.js
Adding         chrome/locales
Adding         chrome/locales/Makefile.in
Adding         chrome/locales/en-US
Adding         chrome/locales/en-US/about.dtd
Adding         chrome/locales/en-US/mccoy.dtd
Adding         chrome/locales/en-US/mccoy.properties
Adding         chrome/locales/jar.mn
Adding         chrome/theme
Adding         chrome/theme/Makefile.in
Adding  (bin)  chrome/theme/database_key.png
Adding  (bin)  chrome/theme/help.png
Adding         chrome/theme/jar.mn
Adding  (bin)  chrome/theme/key.png
Adding  (bin)  chrome/theme/key_add.png
Adding  (bin)  chrome/theme/key_delete.png
Adding  (bin)  chrome/theme/key_delete_disabled.png
Adding  (bin)  chrome/theme/key_go.png
Adding  (bin)  chrome/theme/key_go_disabled.png
Adding         chrome/theme/mccoy.css
Adding  (bin)  chrome/theme/pencil-disabled.png
Adding  (bin)  chrome/theme/pencil.png
Adding  (bin)  chrome/theme/rosette.png
Adding  (bin)  chrome/theme/rosette_disabled.png
Adding  (bin)  chrome/theme/script_key.png
Adding  (bin)  chrome/theme/script_key_disabled.png
Adding  (bin)  chrome/theme/tick.png
Adding  (bin)  chrome/theme/tick_disabled.png
Adding         components
Adding         components/Makefile.in
Adding         components/public
Adding         components/public/Makefile.in
Adding         components/public/nsIKeyService.idl
Adding         components/src
Adding         components/src/KeyPair.cpp
Adding         components/src/KeyPair.h
Adding         components/src/KeyService.cpp
Adding         components/src/KeyService.h
Adding         components/src/KeyUtils.cpp
Adding         components/src/KeyUtils.h
Adding         components/src/Makefile.in
Adding         confvars.sh
Adding         installer
Adding         installer/Makefile.in
Adding         makefiles.sh
Transmitting file data ..........................................................
Committed revision 6790.