Caching strategy for D8 toolbar

+++ b/core/modules/toolbar/toolbar.css
@@ -127,3 +127,10 @@ body.toolbar-drawer {
+/**
+ * @todo Hide or show submenus based on some logic (e.g., hover)
+ */
+#toolbar-menu ul {
+  display: none;
+}
+++ b/core/modules/toolbar/toolbar.js
@@ -35,6 +35,13 @@ Drupal.toolbar.init = function() {
+  // Add subtrees.
+  if (Drupal.toolbar.subtrees) {
+    for (var id in Drupal.toolbar.subtrees) {
+      $('#toolbar-link-' + id).after(Drupal.toolbar.subtrees[id]);
+    }
+  }
+

The reason this patch has no visual impact to the current toolbar is because of this. The JSONP that's returned is a rendered subtree of each top-level link. For no-js users, this means the toolbar is just the same as it is in HEAD. For js users, the code above adds the HTML to its respective place in the DOM, but the CSS above hides it. The @todo indicates we'll want something more useful than that, but that's the job of #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop. This issue, meanwhile, is focused on addressing the performance/caching concerns only.

Note that the reason the JSON structure is per subtree is to enable further front-end optimizations, like only calling $().after() on demand per top-level link, and not adding lots of stuff to the DOM until the user needs it, which could help with memory/battery usage on phones.

Added update function.

+++ b/core/modules/toolbar/toolbar.module
@@ -60,6 +62,13 @@ function toolbar_menu() {
+  $items['toolbar/subtrees/%'] = array(
+    'page callback' => 'toolbar_subtrees_jsonp',
+    'page arguments' => array(2),
+    'access callback' => '_toolbar_subtrees_access',
+    'access arguments' => array(2),
+    'type' => MENU_CALLBACK,
+  );
   return $items;
 }
 
@@ -77,6 +86,68 @@ function toolbar_toggle_page() {
 }
 
 /**
+ * Access callback: Returns if the user has access to the rendered subtree requested by the hash.
+ *
+ * @see toolbar_menu().
+ */
+function _toolbar_subtrees_access($hash) {
+  return user_access('access toolbar') && ($hash == _toolbar_get_subtree_hash());
+}

What's the cross domain risk? Is this access checking not sufficient?

Menu name changed from 'management' to 'admin', so here's a fix for that. CNR for bot, but I still want to implement some cache header feedback I got from msonnabaum.

+++ b/core/modules/toolbar/toolbar.module
@@ -77,6 +86,68 @@ function toolbar_toggle_page() {
+  $max_age = 3600 * 24 * 365;
+  drupal_add_http_header('Expires', gmdate(DATE_RFC1123, REQUEST_TIME + $max_age));
+  drupal_add_http_header('Cache-Control', 'private, max-age=' . $max_age);

Actually, turns out these cache headers are fine as-is. And when I look at the net tab in firebug, I get confirmation that the browser does not re-request. It only does when I start an xdebug session (which is what prompted me to think something was wrong), because that causes the cookie to change. In this case, we don't need Vary: Cookie, but drupal_serve_page_from_cache() doesn't allow the Vary header to be removed: something we might want to fix elsewhere, but doesn't really cause a problem here, since under normal operation, the user's cookies don't change often.

I don't think it makes sense to commit this on its own, as all it does is add code to give the browser an expanded toolbar that is display:none. But, leaving it at RTBC to get feedback from catch (and others) on whether this is an approved approach. If it is, I can roll it in to #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop.

I think duplicate is fine then. Next step is to roll this in to #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop and code reviews can happen there.

Maybe better yet, moving this to the sandbox queue.

Rebased to #1137920-297: Fix toolbar on small screen sizes and redesign toolbar for desktop. Doesn't work yet, so leaving at "needs work".

Rebased to #1137920-301: Fix toolbar on small screen sizes and redesign toolbar for desktop.

Removed one todo and added another one.