bug 441794: address review comments

mobile/chrome/content/browser-ui.js

@@ -1,3 +1,4 @@
+// -*- Mode: js2; tab-width: 2; indent-tabs-mode: nil; js2-basic-offset: 2; js2-skip-preprocessor-directives: t; -*-
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
@@ -47,6 +48,7 @@ const PANELMODE_BOOKMARKLIST      = 4;
 const PANELMODE_SIDEBAR           = 5;
 const PANELMODE_TABLIST           = 6;
 const PANELMODE_FULL              = 7;
+const PANELMODE_SHORTCUTS         = 8;
 
 var BrowserUI = {
   _panel : null,
@@ -151,7 +153,6 @@ var BrowserUI = {
     getBrowser().addEventListener("DOMLinkAdded", this, true);
 
     Shortcuts.restore();
-    Shortcuts.test();
 
     Browser.content.addEventListener("overpan", this, false);
     Browser.content.addEventListener("pan", this, true);
@@ -316,26 +317,47 @@ var BrowserUI = {
     sidebar.left = toolbar.width = container.boxObject.width;
     sidebar.height = tablist.height = container.boxObject.height - toolbar.boxObject.height;
 
-    if (aMode == PANELMODE_URLVIEW || aMode == PANELMODE_SIDEBAR ||
+    function doTheStuff()
-        aMode == PANELMODE_TABLIST || aMode == PANELMODE_FULL)
     {
-      toolbar.setAttribute("mode", "view");
-      toolbar.top = 0;
-
         this._edit.hidden = true;
         this._edit.reallyClosePopup();
         this._caption.hidden = false;
-      bookmark.hidden = true;
+    }
-      urllist.hidden = true;
+
+    function viewURL() { toolbar.setAttribute("mode", "view"); }
+    function editURL() { toolbar.setAttribute("mode", "edit"); }
+
+    function showToolbar() { toolbar.top = 0; }
+    function hideToolbar() { toolbar.top = -toolbar.boxobject.height; }
+
+    function showSidebar() { sidebar.left = toolbar.boxObject.width - sidebar.boxObject.width; }
+    function hideSidebar() { sidebar.left = toolbar.boxObject.width; }
+
+    function showTablist() { tablist.left = 0; }
+    function hideTablist() { tablist.left = -tablist.boxObject.width; }
+
+    function showPane(id)
+    {
+        var nodes = container.childNodes;
+        Array.forEach(container.childNodes, function(n)
+                                            {
+                                              var nid = n.getAttribute("id");
+                                              if (nid != "browser")
+                                                n.hidden = !(nid == id);
+                                            });
+    }
+
+    if (aMode == PANELMODE_URLVIEW || aMode == PANELMODE_SIDEBAR ||
+        aMode == PANELMODE_TABLIST || aMode == PANELMODE_FULL)
+    {
+      doTheStuff();
+      viewURL();
+      showToolbar();
 
-      let sidebarTo = toolbar.boxObject.width;
-      let tablistTo = -tablist.boxObject.width;
       if (aMode == PANELMODE_SIDEBAR || aMode == PANELMODE_FULL)
-        sidebarTo -= sidebar.boxObject.width;
+        showSidebar();
       if (aMode == PANELMODE_TABLIST || aMode == PANELMODE_FULL)
-        tablistTo = 0;
+        showTablist();
-      sidebar.left = sidebarTo;
-      tablist.left = tablistTo;
     }
     else if (aMode == PANELMODE_URLEDIT) {
       toolbar.setAttribute("mode", "edit");
@@ -378,6 +400,19 @@ var BrowserUI = {
       urllist.width = container.boxObject.width;
       urllist.height = container.boxObject.height - toolbar.boxObject.height;
     }
+    else if (aMode == PANELMODE_SHORTCUTS) {
+      this._edit.hidden = true;
+      this._edit.reallyClosePopup();
+      this._caption.hidden = false;
+
+      bookmark.hidden = true;
+      urllist.hidden = true;
+      toolbar.top = -toolbar.boxObject.height;
+      sidebar.left = toolbar.boxObject.width;
+      tablist.left = -tablist.boxObject.width;
+
+      document.getElementById("shortcuts-container").hidden = false;
+    }
     else if (aMode == PANELMODE_NONE) {
       toolbar.top = -toolbar.boxObject.height;
       sidebar.left = toolbar.boxObject.width;

mobile/chrome/content/browser.xul

@@ -66,10 +66,13 @@
   <stringbundleset id="stringbundleset">
     <stringbundle id="bundle_browser" src="chrome://browser/locale/browser.properties"/>
     <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
-    <stringbundle id="bundle-keys" src="chrome://global/locale/keys.properties"/>
+    <stringbundle id="bundle_keys" src="chrome://global/locale/keys.properties"/>
-    <stringbundle id="bundle-platformKeys" src="chrome://global-platform/locale/platformKeys.properties"/>
+    <stringbundle id="bundle_platformKeys" src="chrome://global-platform/locale/platformKeys.properties"/>
   </stringbundleset>
 
+  <!-- if your extension adds a new command, overlay a new string
+  bundle into this set, and the shortcut editor will look in it for a
+  proper name to call the command. -->
   <stringbundleset id="shortcut-bundles">
     <stringbundle src="chrome://browser/locale/shortcuts.properties"/>
   </stringbundleset>

mobile/chrome/content/shortcuts.js

@@ -82,7 +82,7 @@ function ShortcutEditor()
         return command in keys && keys[command];
     }
 
-    function findCommandForKey(keySpec)
+    function findKeyForSpec(keySpec)
     {
         // TODO: This is a bit simplistic as yet. For example, we should match
         //       a key with an optional modifier even if that modifier isn't
@@ -98,6 +98,8 @@ function ShortcutEditor()
                 keys[i].getAttribute("keycode") == keySpec.keycode)
                 return keys[i];
         }
+
+        return null;
     }
 
     function addKey(command, keySpec)
@@ -114,7 +116,7 @@ function ShortcutEditor()
         var key = findKeyForCommand(command);
         if (keySpec.exists)
         {
-            if (findCommandForKey(keySpec))
+            if (findKeyForSpec(keySpec))
                 return null;
 
             if (key)
@@ -306,7 +308,7 @@ function ShortcutEditor()
     // TODO: write some tests
 
     // first, we need to look up the right names of the various modifier keys.
-    var platformBundle = document.getElementById("bundle-platformKeys");
+    var platformBundle = document.getElementById("bundle_platformKeys");
     function doGetString(n) { try { return platformBundle.getString(n); } catch (ex) { dump(">>"+ex+"\n"); return undefined; } };
     var platformKeys = {
         shift: doGetString("VK_SHIFT") || "Shift",
@@ -344,7 +346,7 @@ function ShortcutEditor()
             return "";
 
         var accel = [];
-        var keybundle = document.getElementById("bundle-keys");
+        var keybundle = document.getElementById("bundle_keys");
 
         // this is sorta dumb, but whatever
         var modifiers = [], i = 1;
@@ -411,11 +413,6 @@ function ShortcutEditor()
     {
         tree = document.getElementById("shortcuts");
 
-        var nodes = document.getElementById("browser-container").childNodes;
-        Array.forEach(nodes, function(n) { if (n.getAttribute("id") != "browser-container") n.hidden = true; });
-        document.getElementById("shortcuts-container").hidden = false;
-        fillShortcutList();
-
         var textbox = document.getAnonymousElementByAttribute(tree, "anonid", "input");
         textbox.addEventListener("keypress", keyListener, true);
         textbox.addEventListener("reset", resetListener, true);
@@ -522,7 +519,7 @@ function ShortcutEditor()
     this.test = function()
     {
         // TODO: write a mochitest .xul file, and add it to the build
-        // TODO: test findCommandForKey() and findKeyForCommand()
+        // TODO: test findKeyForSpec() and findKeyForCommand()
         function eq(a, b)
         {
             for (p in a)
@@ -556,6 +553,7 @@ function ShortcutEditor()
          [["meta shift alt control", undefined, "VK_A"],  {exists: true,  modifiers: 15, key: false, keycode: "VK_A"},     "Ctrl+Alt+Meta+Shift+A"],
          [[],                                             {exists: false, modifiers: 0,  key: false, keycode: false},      ""],
          [["control"],                                    {exists: false, modifiers: 2,  key: false, keycode: false},      ""],
+         [["foobar", "a"],                                {exists: true,  modifiers: 2,  key: "a",   keycode: false},      "A"],
          [["alt", "α"],                                   {exists: true,  modifiers: 1,  key: "α",   keycode: false},      "Alt+Α"],
          [["alt", "א"],                                   {exists: true,  modifiers: 1,  key: "א",   keycode: false},      "Alt+א"]
         ].forEach(function doTests(t)