Extension:CentralNotice/Notes/Banner controller refactoring

Here's a place to keep some rough notes about reorganizing CentralNotice banner controller code. See also Campaign-associated mixins and banner history. Work should go on the  branch.

Goals

 * Improve modularity and separation of concerns in banner controller code.
 * Adapt the client-side API as needed for new campaign-associated mixins.
 * Improve site performance when banners are shown and when they're not.

Requirements

 * Separate concerns in a way that makes sense for selective loading of components.
 * Follow typical and best-practice MW patterns.
 * Remove cruft.
 * Prepare to move out code that has uses beyond CentralNotice (GeoIP cookie stuff, maybe mobile device detection).

Proposal
Following are modules, objects and their responsibilities. Format is: RLModule/object(where applicable).

ext.centralNotice.startup Checks  and the presence of a banner URL param, and starts the appropriate processes. This module is added via the HTML, and brings in   and    as dependencies.

ext.centralNotice.choiceData JSON with data needed to choose a campaign and a banner on the client. If there are possible campaigns available, this module will depend on display, as well as any mixin modules required by the campaings, and dependencies of those modules.

ext.centralNotice.display/mw.centralNotice Coordinates other objects, displays banners and provides an API for outside code, including banners and campaign-associated mixins.

ext.centralNotice.display/mw.centralNotice.internal.state Stores data for campaign/banner processing and data related to the state of that processing.

ext.centralNotice.display/mw.centralNotice.internal.chooser Selects a campaign and a banner based on.

ext.centralNotice.display/mw.centralNotice.internal.bucketer Retrieves, stores, chooses and processes buckets.

ext.centralNotice.display/mw.centralNotice.internal.hide Retrieve, process and store 'hide' cookies, which prevent banners from showing in certain circumstances.

ext.centralNotice.kvStore/mw.centralNotice.internal.kvStore Stores and retrieves items for different contexts (campaign, category, global). Should be declared as a dependency by mixin modules, as needed. See also the existing KV store patch.

ext.centralNotice.geoIP/mw.geoIP Sets window.Geo. May be moved to a different extension or to MW core. (See the related Phabricator task.)

We'll use dynamic RL dependencies of  to select the RL modules needed.

Considerations: mobile-only code
Currently the code for checking the device is in a mobile-only module, so it's only sent to mobile clients. However, it's only a small bit of code, and it by putting it in bannerManager we're able to send it only when a client has possible campaigns in. This will benefit mobile users more than the current setup and will not affect desktop users noticeably. It would be possible to keep it mobile-only and make it available only when there are campaign choices by creating a mobile-only version of ; however, this added complication does not seems worth it, especially if, in the longer term, we can work towards having device data available server-side.

Rollout proposal: RL module deprecation and queue changes
Calls to load ResourceLoader modules are embedded  tags in the main HTML. This HTML can be cached for up to one month for anonymous users. As a result, both deprecating modules and moving them to a different queue can take a while.

In addition, given the size of the current change, we need to keep open the option of reverting the main code rollout.

Here is a multi-stage proposal to deprecate current RL modules, move new modules to a different queue, and separate GeoIP code from CentralNotice.

Modules as currently deployed
Summary: Everything is top-loaded. GeoIP data is processed right away, and everything else waits for the DOM to be ready.

Stage 1: New modules only as dependencies, rollback is possible
Summary: Old modules are emptied of code and only bring in new modules as dependencies. The old modules are still added to the HTML; this should make it easier to roll back this step if necessary. Everything is still top-loaded (since the new stuff is a dependency of top-loaded old stuff). GeoIP data is processed right away, and everything else waits for the DOM to be ready.

Stage 2: New modules added to cached HTML
Summary: Old modules are no longer added to the HTML, but will still be loaded from cached HTML.

Page views with the new HTML: GeoIP is top-loaded and runs right away. CentralNotice campaign and banner selection is bottom-loaded and waits for the DOM to be ready.

Page views with the old HTML: Just as in the previous stage, everything is still top-loaded, GeoIP data is processed right away, and everything else waits for the DOM to be ready.

Stage 3: Improve performance for banners display, old modules ready to be removed
Summary: One month after Stage 2 is deployed, we should be able to completely remove the old RL modules. GeoIP is top-loaded and runs right away. CentralNotice campaign and banner selection is bottom-loaded but begins processing right away. Only actual banner insertion waits for the DOM. We may deploy CSS/JS trickery to avoid or minimize a page bump when a banner is displayed.

Expected future changes

 * GeoIP processing should move to a different extension or to Mediawiki core.
 * should move to the "middle" queue.

Requirements

 * Remain compatible with existing banners. Certain methods, properties and hooks should remain available. We can use JS  to warn when deprecate properties are accessed. The following should stick around:
 * (though it'll now be read-only)
 * Here are things that we don't need to keep publicly accessible outside CentralNotice:
 * (already deprecated)
 * (already deprecated)
 * New stuff the API will offer for campaign-associated mixins:
 * Retrieve bucket.
 * Set bucket.
 * A flag that can be set by a mixin to signal that banners in a campaign are guaranteed to display to the user if loaded. This will disable Special:RecordImpression for such campaigns and add a parameter to the call to Special:BannerLoader to confirm that impressions may be counted using logs to that endpoint.
 * A flag that can be set by a mixin to cancel the display of banners from this campaign.
 * Access to the Key-Value store methods.
 * Everything should work smoothly with existing cached HTML.
 * (already deprecated)
 * (already deprecated)
 * New stuff the API will offer for campaign-associated mixins:
 * Retrieve bucket.
 * Set bucket.
 * A flag that can be set by a mixin to signal that banners in a campaign are guaranteed to display to the user if loaded. This will disable Special:RecordImpression for such campaigns and add a parameter to the call to Special:BannerLoader to confirm that impressions may be counted using logs to that endpoint.
 * A flag that can be set by a mixin to cancel the display of banners from this campaign.
 * Access to the Key-Value store methods.
 * Everything should work smoothly with existing cached HTML.
 * A flag that can be set by a mixin to signal that banners in a campaign are guaranteed to display to the user if loaded. This will disable Special:RecordImpression for such campaigns and add a parameter to the call to Special:BannerLoader to confirm that impressions may be counted using logs to that endpoint.
 * A flag that can be set by a mixin to cancel the display of banners from this campaign.
 * Access to the Key-Value store methods.
 * Everything should work smoothly with existing cached HTML.

Proposal
See also the WIP patch.

Get the bucket for this campaign. Guaranteed to be available in the  hook.

Will influence which banner is selected when called from the  hook.

Sets a boolean flag; default false.

Sets boolean flag and string. Should be called from the  hook.

Sets boolean flag and string.

Set the sample rate for calling Special:RecordImpression. Default is.

Available key-value storage contexts

Smoke and unit testing
Following are bits of functionality that we need to verify in the new code. The "PS..." columns are for patchs set in the Gerrit change. Functionality covered by unit tests should be marked as such.

Why should any CentralNotice code be top-loaded?
Discussion

The sooner the bannerController runs, the less of a page bump there will be when a banner is shown. Also, completely eliminating CentralNotice top-loaded modules would increase the number of round trips across the board. —AGreen (WMF) (talk)
 * I guess this only applies to traditional banners. Banners that overlay the content, and animate wouldn't have this issue. I think ideally you want to minimise what is loaded on top to just be the function - doINeedToRenderABanner, this can decide the answer and load a banner placeholder at the top of the page with a nice ajax loader bar (a bit like the ones they have in VE). Then the code at the bottom takes over when it can and says whichBannerDoINeed and howDoIRenderThatBanner and hey presto top loaded RL JavaScript is much tinier. —Jdlrobson (talk) 19 June 2015‎
 * Thanks! I agree that if banners were required to always overlay page content, CentralNotice could be more performant pretty easily. However, that seems like a broad proposal that would need more discussion elsewhere.
 * I think there's a lot of room to improve the performance of the current system, without moving to banners that must overlay content. As it stands, the bulk of the processing is to answer precisely the question you mentioned, doINeedToRenderABanner. If we can start to answer this question on the server for a larger percentage of users (as explained in this Phabricator task, then a lot less users will receive the banner selection code.
 * Maybe some benchmarking of the newly refactored code would help set priorities.
 * Regarding the idea of showing a placeholder while the actual banner loads (I think that suggestion is for a scenario in which banners don't overlay content?), the code that decides whether or not to inject the placeholder, and determines its size, would still have to run as early as possible, again to avoid a page bump, no?
 * Also, remember that it's not possible to fully determine on the server whether a banner should be shown to a user, since some of the criteria are only available in the user's browser. —AGreen (WMF) (talk) 03:00, 9 July 2015 (UTC)