In #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop, based on lots of UX discussion, the current goal is to allow users to navigate the complete Management menu without intermediary page loads (like Admin menu, though not relying on hover for touch devices). This brings up two performance concerns:
- Building and rendering a large menu tree on each page request is expensive server-side. Some mitigation of this is being explored in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.
- Even if server-side caching in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees is worked out, it's still a lot of bytes to add to every page, so especially for bandwidth-constrained mobile devices, a client-side cache would be beneficial.
If we implement a client-side cache anyway, there's less Toolbar-specific need for some of the work being explored in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, though that issue can still be useful for other menus.
This patch implements a client-side cache with JSONP. For each page, we add a SCRIPT tag with a src that contains the hash of a JSON array containing a rendered html subtree for each top-level toolbar link. Since a hash specifies its hashed content uniquely, the entire JSONP response is cached in Drupal's page cache. This allows efficient caching: multiple users who have the same Management links available to them (e.g., have the same roles, and none of the Management links are per-user access controlled or customized) can reuse the same cached JSONP data. A {cache_toolbar} bin is added to keep track of the mapping from user to hash. The per-user hash mapping and hash to page cache approach is the same as what's employed by Admin menu 3.x, though I simplified some of the implementation details in this patch.
Comment | File | Size | Author |
---|---|---|---|
#23 | toolbar-cache-23.patch | 8.77 KB | effulgentsia |
#23 | interdiff.txt | 1.44 KB | effulgentsia |
#22 | toolbar-cache-22.patch | 8.28 KB | effulgentsia |
#21 | toolbar-cache-21.patch | 8.23 KB | effulgentsia |
#20 | toolbar-cache-20.patch | 7.42 KB | effulgentsia |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedThe 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.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedAdded update function.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedHave you considered the security implications? This *must not* be loadable cross-domain, so JSONP is very much a no-go.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedWhat's the cross domain risk? Is this access checking not sufficient?
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe hash seems unpredictable enough, so this looks ok.
Comment #7
JohnAlbinI'm going to review this today or tomorrow while at BADCamp. :-)
This issue is also highly dependent on the solution we use for #1827296: Mark up the currently active menu trail in the menu list since a PHP-based build of the menu + active trail requires a different cache strategy from a JS-based lets-just-decorate-the-static-menu-tree approach.Edit: I should really read the issues summary better. :-)Comment #8
JohnAlbinHmmm…
I applied the patch, re-installed Drupal and I can verify that it is loading a script tag pointing at a page-cache-enabled url.
However, when looking at that /toolbar/subtrees page, all I saw was this:
So the patch is failing to load the sub-trees properly.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedMenu 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.
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedActually, 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.Comment #12
JohnAlbin#9: toolbar-cache-9.patch queued for re-testing.
Comment #13
JohnAlbinThe caching strategy is quite nice here. In the cache_toolbar table it is storing the hash of the full, expanded menu for each user. A specific user will actually share a hash with any user who has the same permission set. The hash is used in the URL to uniquely identify a page which contains a json response containing the full menu. Drupal will use normal page caching to cache that json response. And the browser will read the cache headers sent by Drupal to prevent it from ever checking to see if the page has been updated (it will never be updated because the hash itself is unique per unique versions of the menu.)
Really nice.
I've applied the patch and used a browser inspector to verify that the DOM elements are being loaded into the page. And I did a thorough review at the code level as well.
One should note that there's a @TODO that says:
which makes perfect sense to me. We'll definitely need to revisit that later.
RTBC!
Comment #14
webchickProbably makes sense for catch to review this one.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #16
jessebeach CreditAttribution: jessebeach commentedI'd like to roll this into #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop once the cacheing strategy is approved. We'll need to rework the JavaScript to match the current stage of the ToolBar patch. I'll work with effulgentsia to do that.
Comment #17
catchI have a few comments on the actual code, but the strategy itself seems good to me.
Not sure what status this should be at...
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedMaybe better yet, moving this to the sandbox queue.
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedRebased to #1137920-297: Fix toolbar on small screen sizes and redesign toolbar for desktop. Doesn't work yet, so leaving at "needs work".
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedComment #22
effulgentsia CreditAttribution: effulgentsia commentedRebased to #1137920-301: Fix toolbar on small screen sizes and redesign toolbar for desktop.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedRemoved one todo and added another one.
Comment #24
jessebeach CreditAttribution: jessebeach commentedClosing.
This patch has been rolled into #1137920-303: Fix toolbar on small screen sizes and redesign toolbar for desktop.
Comment #24.0
jessebeach CreditAttribution: jessebeach commentedUpdated issue summary.