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 banner controller 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 some possible 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.

ext.centralNotice.choiceData JSON with data needed to choose a campaign and a banner on the client.

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

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

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

ext.centralNotice.display/deviceChecker Device checking. (See considerations, below.)

ext.centralNotice.kvStore/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/geoIP Sets window.Geo. May be moved to a different extension or to MW core. (See the related Phabricator task.)

ext.centralNotice.testing/testingBannerLoader Small module for loading banners for testing purposes. Loads dynamically via mw.loader.load when a banner parameter is included in the URL.

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

Considerations
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.

RL module deprecation
Several modules currently mentioned in calls to  in scripts in cached HTML will be deprecated. This cached HTML can continue to be served for up to one month. Here's a tentative rollout and deprecation plan: Modules used for the administration UI will not be changed (other than the location of their resources in the CN source tree).
 * All deprecated modules will have no content (JS, CSS, templates) associated with them.
 * Two deprecated modules (one for desktop and the other for mobile) will depend on the new  module, which will itself bring in everything else.
 * We'll start adding the new  module in HTML on all platforms immediately.
 * Once the cache has cleared, deprecated modules will be removed.

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:
 * 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.
 * 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.

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

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

Boolean property; default false. Should be set from the  hook.

Boolean property; default false. Should be set from the  hook.

Available key-value storage contexts

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)