Separate Settings and Help from History and Extensions when settings-in-

chrome/browser/resources/uber/uber_frame.js

Index: chrome/browser/resources/uber/uber_frame.js
diff --git a/chrome/browser/resources/uber/uber_frame.js b/chrome/browser/resources/uber/uber_frame.js
index 8c5bb3bfe5c6be5c5c40373a6532f72a720c66d0..d82c83eb7b95de8846cbb5b7d16a5c434ee72b6d 100644
--- a/chrome/browser/resources/uber/uber_frame.js
+++ b/chrome/browser/resources/uber/uber_frame.js
@@ -77,6 +77,7 @@ cr.define('uber_frame', function() {
     var navItem =
         document.querySelector('li[controls="' + params.pageId + '"]');
     setSelection(navItem);
+    showNavItems();

[Dan Beam, 2014/06/06 21:21:23]

shouldn't this only be invoked if IsSettingsWindowEnabled()?

[michaelpg, 2014/06/07 02:12:15]

I've made the nav items hidden to prevent the flicker you'd see in 
--enable-settings-window. Otherwise all nav items would show and then
two would disappear.

Because of that, showNavItems needs to be called at least once.
changeSelection is called the first time the page loads to set the
initial selection.

The alternative is to
1. use .style.display instead of hidden attribute
2. set display attributes to "none" if --enable-settings-window 
3. add "isSettingsWindow" to the data source for uber_frame.js
4. if !isSettingsWindow call showNavItems from changeSelection

which in the case of !isSettingsWindow would be faster for some
definition of speed. But this is simpler and I didn't think I should
be concerned about iterating over a document with 4 <li>s.

   }
 
   /**
@@ -93,6 +94,19 @@ cr.define('uber_frame', function() {
   }
 
   /**
+   * Shows nav items belonging to the same group as the selected item.
+   */
+  function showNavItems() {
+    var navItems = document.querySelectorAll('li');
+    var selectedNavItem = document.querySelector('li.selected');

[Dan Beam, 2014/06/06 21:21:23]

what happens if this returns null?

[Dan Beam, 2014/06/06 21:21:23]

nit: make a

  function getSelectedNavItem() {
    return document.querySelector('li.selected');
  }

a re-use here and other places

[michaelpg, 2014/06/07 02:12:15]

Done.

[michaelpg, 2014/06/07 02:12:15]

bad things.. but.. it can't because of the ordering in changeSelection.
if the selected navItem didn't exist, setSelection would have thrown
TypeError before this call.

+
+    for (var i = 0; i < navItems.length; ++i) {
+      navItems[i].hidden = (navItems[i].getAttribute('group') !=
+                            selectedNavItem.getAttribute('group'));

[Dan Beam, 2014/06/06 21:21:23]

nit: needless ()

[Dan Beam, 2014/06/06 21:21:23]

you don't ever actually use selectedNavItem itself, I'd just recommend

  var selectedNavItem = getSelectectedNavItem();
  if (!selectedNavItem)  // or assert(selectedNavItem)
    return;

  var selectedGroup = selectedNavItem.getAttribute('group');
  // use |selectedGroup|

[michaelpg, 2014/06/07 02:12:15]

Done.

+    }
+  }
+
+  /**
    * Adjusts this frame's content to scrolls from the outer frame. This is done
    * to obscure text in RTL as a user scrolls over the content of this frame (as
    * currently RTL scrollbars still draw on the right).