432421 - UI update

Status: RESOLVED FIXED

(Other Applications Graveyard :: McCoy, defect)

Description

[Dave Townsend [:mossop]]

This bug is for the UI update to McCoy changing to a task oriented approach and allowing for the addition of new tasks including ones unrelated to the signing process that was the original goal of McCoy

Comment 1

[Dave Townsend [:mossop]]

The following patches are a series that implement the new UI and make all existing functionality work in it, as well as adding mochitest support and a series of tests for what is implemented here. The are mercurial patches so might look a bit odd in some cases, especially since a couple contain file moves. I'll try to call those out.

Comment 2

[Dave Townsend [:mossop]]

Attachment: keymanager rev 1

This is the only patch that leaves McCoy unusable, but it is necessary as it moves the current main UI out of the way, essentially it renames mccoy.xul (and js and dtd and properties) to keymanager.xul and removes all the signing and other functionality leaving just the ability to add, remove and rename keys.

No additional functionality is added here just a lot removed (to be picked up in later patches).

Comment 3

[Dave Townsend [:mossop]]

Attachment: branding rev 1

I have some updates to the main icon to land, never comes out so well in a patch. Madhava, can you take a quick look at the icons in the archive?

Comment 4

[Dave Townsend [:mossop]]

Attachment: mainui rev 1

This adds the main structure of the new UI. Like the add-ons manager and preferences in Firefox is uses styled radiogroups to provide the pane selector and also for the task list which appears on the left. A main iframe is used to display the actual task UI which allows for nice scoping and not having to worry about conflicts. The frame defaults to an intro pane. The beginnings of support for testing whether navigating away from the task is allowed is there but not yet completed.

The default styling for the main UI is essentially like windows but using all system colors. Then windows, linux and mac have overrides to customise the styles for them.

The UI provides a set of widgets and default stylings that all task panels should use including a file picker and a key picker.

Added icons come from the tango icon set, let me know if you want me to package them up for review.

Comment 5

[Dave Townsend [:mossop]]

Attachment: tests rev 1

This adds the mochitesting for the app. The binary name has to change a little for what the in-tree automation.py expects and we have to add the testing/mochitest target followed by our mccoy/tests which overwrites the runtests.py script.

The testing is done as regular chrome mochitests which I think work better than the chrome-tests as they exist in their own window avoiding all scope conflicts yet can easily get at the main window. All that is necessary is a special overlay that opens the testing window and the testing window has to have some special parts to ensure you can browse to individual tests and back again.

This includes a set of tests for the binary keyservice component. Currently it checks key generation and deletion and then performs some random signing and verification, for now this is about the limit until we can import known keys into the testing profile.

The runtests.py is near identical to the in-tree version just with a lot of the unnecessary parts removed leaving only chrome tests. The test server is also removed as it does not work for an app built against the sdk at the moment. One other change is that the main chrome url to overlay is preprocessed in so it can potentially be used for multiple apps.

Comment 6

[Dave Townsend [:mossop]]

Attachment: modules rev 1

This adds some shared javascript modules for use by the tasks. It includes the beginnings of an API for working with the install.rdf and update.rdf formats. The goal is that tasks should not have to wrry about talking rdf or whatever the underlying format is and just talk to the API. This make changing to a potential future new format of manifest easier as well as allowing for changing to a better rdf serializer or whatever without changing every task. This API is very basic right now but should grow as new tasks are added to accomodate their needs.

The rdfserializer.js file which generates the data to sign for an update manifest is moved to modules/UpdateDataSerializer.jsm and then a few tweaks are made.

The API includes tests for that which is now implemented and includes a basic rdf comparator which can take two rdf files and check that their contents is identical from an rdf point of view, ignoring the actual serialization.

Comment 7

[Dave Townsend [:mossop]]

Attachment: addkey rev 1

This implements the task to add an update key to the install manifest. It displays as a simple file chooser and key chooser to pick the manifest and key and then a save button lets you choose where to save the updated manifest.

By using the API the javascript for this is quite simple.

Comment 8

[Dave Townsend [:mossop]]

Attachment: signupdate rev 1

This adds the task to sign an update manifest, quite simple just a file picker and a key picker and then a save button.

Comment 9

[Dave Townsend [:mossop]]

Attachment: verifysign rev 1

This adds the signature verification task. User can select to verify against an install manifest that contains an updateKey or directly against an encryption key.

Comment 10

[Dave Townsend [:mossop]]

Attachment: uitests rev 1

Last one!

This adds a mochitest checking that the main UI updates display properly as various items are clicked.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 319954 [details] [diff] [review]
modules rev 1

Pike, feel like taking a look over the rdf comparer in shared.js?

Comment 12

[Axel Hecht]

Comment on attachment 319954 [details] [diff] [review]
modules rev 1

I think that compareGraph should be called differently, it's really just a helper for two trees, and it could use a comment to that extent.

On to the code, ios in compareRDF isn't used, AFAICT.

I think that you need to exclude purely anonyous circles from the graph, too. Those would still satisfy the single-in-arc restriction, but you'd never actually test them. Not sure if there's a sane way to detect those, and if it's necessary.

Otherwise, the restriction in terms of acceptable graphs and the test itself looks cool, r=me.

Comment 13

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

Comment on attachment 319946 [details] [diff] [review]
keymanager rev 1

does what you said it would.

In the jar.mn, don't you need to add the second column for these items:

+* content/keymanager.xul
+  content/keymanager.js
+* content/mccoy.xml
+  content/rdfserializer.js
+* content/about.xul
+  content/about.js

Comment 14

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

Comment on attachment 319951 [details] [diff] [review]
mainui rev 1


>+/**
>+ * This adjusts the default protocol handler actions to just open with the
>+ * system default app
>+ */
>+function fixProtocolHandlers() {
>+  var hs = Cc["@mozilla.org/uriloader/handler-service;1"].
>+           getService(Ci.nsIHandlerService);
>+  var extps = Cc["@mozilla.org/uriloader/external-protocol-service;1"].
>+              getService(Ci.nsIExternalProtocolService);
>+
>+  var httpHandler = extps.getProtocolHandlerInfo("http");
>+  httpHandler.preferredAction = Ci.nsIHandlerInfo.useSystemDefault;
>+  httpHandler.alwaysAskBeforeHandling = false;
>+  hs.store(httpHandler);
>+}

Crap, I was hoping we could stop doing this

>+++ b/chrome/theme/mac/mccoy.css
>@@ -0,0 +1,79 @@
>+window {
>+  -moz-binding: url("chrome://global/skin/globalBindings.xml#unifiedWindow");
>+}

Huh, I would have thought that was the default

>+
>+#paneSelector {
>+  background-color: #9F9F9F;
>+  background-image: url("chrome://global/skin/toolbar/toolbar-background.gif");
>+  background-repeat: repeat-x;
>+  padding: 4px 0 8px 0;
>+  margin: 0;
>+  -moz-box-pack: center;
>+  border-bottom: 1px solid #404040;
>+}
>+
>+window:not([active="true"]) #paneSelector {
>+  background-image: url("chrome://global/skin/toolbar/toolbar-background-inactive.png");
>+  background-color: #cfcfcf;
>+  border-bottom: 1px solid rgba(0,0,0,0.35);
>+}

Nicely done! Looks great

Comment 15

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

Comment on attachment 319953 [details] [diff] [review]
tests rev 1

Too bad we couldn't reuse runtests.py from the tree - your changes are the MAIN_CHROME_URL / __CHROME_URL__ and the app-overlay.xul stuff?

Comment 16

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

Comment on attachment 319954 [details] [diff] [review]
modules rev 1

modules and tests look good - RDF isn't my strongest area

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #15)
> (From update of attachment 319953 [details] [diff] [review])
> Too bad we couldn't reuse runtests.py from the tree - your changes are the
> MAIN_CHROME_URL / __CHROME_URL__ and the app-overlay.xul stuff?

Yeah that and removing the http server, without that runtests just fails because it cannot find xpcshell. Gavin was talking about making runtests more useable for other applications, I'll point him to this and probably submit a patch or two.

Comment 18

[Madhava Enros [:madhava]]

Comment on attachment 319947 [details]
branding rev 1

we can clean this up a little at some point, but this works for the time being

Comment 19

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

Comment on attachment 319956 [details] [diff] [review]
addkey rev 1

/development/addkey.js

>+function fileChosen() {
>+  var file = document.getElementById("file").file;
>+  if (file && file.exists()) {


>+function save() {
>+  var file = document.getElementById("file");

var file = document.getElementById("file").file;   ??

>+
>+  var fp = Cc["@mozilla.org/filepicker;1"].
>+           createInstance(Ci.nsIFilePicker);
>+  fp.init(window, getString("addkey.save.title"), Ci.nsIFilePicker.modeSave);
>+
>+  fp.appendFilter(getMainString("filepicker.type.rdf"), "*.rdf");
>+  fp.appendFilters(Ci.nsIFilePicker.filterAll);
>+  fp.filterIndex = 0;
>+  fp.displayDirectory = file.parent;
>+  fp.defaultString = file.leafName;

What if file is null? You check in fileChosen() which might be enough.

/development/addkey.xul

>+
>+<!DOCTYPE window [

Should this be | page | instead of | window |  ??

>+<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
>+%brandDTD;
>+<!ENTITY % windowDTD SYSTEM "chrome://mccoy/locale/development.dtd" >
>+%windowDTD;
>+]>
>+
>+<page xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

r+ with the file issue checked

Comment 20

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

Comment on attachment 319957 [details] [diff] [review]
signupdate rev 1

>+++ b/chrome/content/update/sign.js

>+
>+function save() {
>+  var file = document.getElementById("file");

Same as last patch. Should this be:
var file = document.getElementById("file").file;

and have a null check

>+++ b/chrome/content/update/sign.xul

>+
>+<!DOCTYPE window [

use | page | instead of  | window |

r+ with the file issue checked

Comment 21

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

Comment on attachment 319958 [details] [diff] [review]
verifysign rev 1

>+
>+<!DOCTYPE window [

| page |

(I don't know if this matters or not)

Comment 22

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

Comment on attachment 319959 [details] [diff] [review]
uitests rev 1

very nice to see the tests in action. so jealous

Comment 23

[Dave Townsend [:mossop]]

(In reply to comment #14)
> (From update of attachment 319951 [details] [diff] [review])
> 
> >+/**
> >+ * This adjusts the default protocol handler actions to just open with the
> >+ * system default app
> >+ */
> >+function fixProtocolHandlers() {
> >+  var hs = Cc["@mozilla.org/uriloader/handler-service;1"].
> >+           getService(Ci.nsIHandlerService);
> >+  var extps = Cc["@mozilla.org/uriloader/external-protocol-service;1"].
> >+              getService(Ci.nsIExternalProtocolService);
> >+
> >+  var httpHandler = extps.getProtocolHandlerInfo("http");
> >+  httpHandler.preferredAction = Ci.nsIHandlerInfo.useSystemDefault;
> >+  httpHandler.alwaysAskBeforeHandling = false;
> >+  hs.store(httpHandler);
> >+}
> 
> Crap, I was hoping we could stop doing this

Good catch, I had just copied that across but it seems to be no longer necessary

Comment 24

[Dave Townsend [:mossop]]

(In reply to comment #12)
> (From update of attachment 319954 [details] [diff] [review])
> I think that compareGraph should be called differently, it's really just a
> helper for two trees, and it could use a comment to that extent.

Renamed it to compareResource and added a decent comment on what it does and when it becomes recursive.

> On to the code, ios in compareRDF isn't used, AFAICT.

It is used in the mochitests elsewhere in the patch.

> I think that you need to exclude purely anonyous circles from the graph, too.
> Those would still satisfy the single-in-arc restriction, but you'd never
> actually test them. Not sure if there's a sane way to detect those, and if it's
> necessary.

Yes that's a bit bothersome. The rdf that McCoy deals with should never hit that situation so I don't believe it is important to compare those cycles, however it would be nice to spot that they are there and fail. I don't think this should hold up landing this and the rest of this bug so I'm going to push that off to bug 433684 for now.

Comment 25

[Dave Townsend [:mossop]]

keymanager committed in r13151
branding, mainui and tests committed in r13152
modules committed in r13153

Comment 26

[Dave Townsend [:mossop]]

(In reply to comment #19)
> (From update of attachment 319956 [details] [diff] [review])
> /development/addkey.js
> 
> >+function fileChosen() {
> >+  var file = document.getElementById("file").file;
> >+  if (file && file.exists()) {
> 
> 
> >+function save() {
> >+  var file = document.getElementById("file");
> var file = document.getElementById("file").file;   ??
> 

Oops yes. Though it wasn't a big issue, the file is only used for initialising the filepicker, the actual contents of the file are already loaded into gManifest

> >+
> >+  var fp = Cc["@mozilla.org/filepicker;1"].
> >+           createInstance(Ci.nsIFilePicker);
> >+  fp.init(window, getString("addkey.save.title"), Ci.nsIFilePicker.modeSave);
> >+
> >+  fp.appendFilter(getMainString("filepicker.type.rdf"), "*.rdf");
> >+  fp.appendFilters(Ci.nsIFilePicker.filterAll);
> >+  fp.filterIndex = 0;
> >+  fp.displayDirectory = file.parent;
> >+  fp.defaultString = file.leafName;
> 
> What if file is null? You check in fileChosen() which might be enough.

Well the save button is disabled until a valid file is selected so save() should only get called when file is not null so I think this is fine as is.

> /development/addkey.xul
> 
> >+
> >+<!DOCTYPE window [
> 
> Should this be | page | instead of | window |  ??

Yeah I seem to remember our xml parser is blind to such things, but best to get it right anyway.

Comment 27

[Dave Townsend [:mossop]]

Fixed those final bits and added a null check for the file where it was used on commit in r13154.

Woot!