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

Objects, architecture and rollout

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 choiceData and the presence of a banner URL param, and starts the appropriate processes. This module is added via the HTML, and brings in choiceData and GeoIP 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 choiceData.

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 ext.centralNotice.choiceData 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 choiceData. 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 CNBannerChoiceDataResourceLoaderModule; 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 <script> 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.

Module	Included in site HTML	Contents	Dependencies
`ext.centralNotice.bannerController`	Desktop	Reams o' code	`jquery.cookie`, `json`, `mediawiki.Uri`, `ext.centralNotice.bannerChoiceData`, `ext.centralNotice.bannerController.lib`
`ext.centralNotice.bannerController.mobile`	Mobile	Reams o' code (same as `bannerController`)	`jquery.cookie`, `json`, `mediawiki.Uri`, `ext.centralNotice.bannerChoiceData`, `ext.centralNotice.bannerController.lib`, `ext.centralNotice.bannerController.mobiledevice`
`ext.centralNotice.bannerController.mobiledevice`	Mobile	Device categorization
`ext.centralNotice.bannerChoiceData`	Mobile	JSON with available campaign and banner choices
`ext.centralNotice.bannerController.lib`	Mobile	Reams o' code

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 executes as soon as possible (unless it accesses the DOM).

Module	Included in site HTML	Contents	Dependencies
`ext.centralNotice.bannerController`	Desktop	empty	`ext.centralNotice.startUp`, `ext.CentralNotice.geoIP`
`ext.centralNotice.bannerController.mobile`	Mobile	empty	`ext.centralNotice.startUp`, `ext.CentralNotice.geoIP`
`ext.centralNotice.bannerController.mobiledevice`	Mobile	empty
`ext.centralNotice.bannerChoiceData`	Mobile	empty
`ext.centralNotice.bannerController.lib`	Mobile	empty
`ext.centralNotice.startUp`		Minimal startup logic	`ext.centralNotice.choiceData`, `mediawiki.util`
`ext.centralNotice.goeIP`		CN-independent GeoIP processing
`ext.centralNotice.choiceData`		JSON with available campaign and banner choices	Only as needed: `ext.centralNotice.display`, campaign-associated mixin modules, and the mixin modules' dependencies dependencies.
`ext.centralNotice.display`		All campaign- and banner-selection logic	`ext.centralNotice.geoIP`, `jquery.cookie`, `json`, `mediawiki.Uri`

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. As in the previous stage, everything executes as soon as possible (unless it accesses the DOM).

Module	Included in site HTML	Contents	Dependencies
`ext.centralNotice.bannerController`		empty	`ext.centralNotice.startUp`, `ext.CentralNotice.geoIP`
`ext.centralNotice.bannerController.mobile`		empty	`ext.centralNotice.startUp`, `ext.CentralNotice.geoIP`
`ext.centralNotice.bannerController.mobiledevice`		empty
`ext.centralNotice.bannerChoiceData`		empty
`ext.centralNotice.bannerController.lib`		empty
`ext.centralNotice.startUp`	Desktop and mobile	Minimal startup logic	`ext.centralNotice.choiceData`, `mediawiki.util`
`ext.centralNotice.goeIP`	Desktop and mobile	CN-independent GeoIP processing
`ext.centralNotice.choiceData`		JSON with available campaign and banner choices	Only as needed: `ext.centralNotice.display`, campaign-associated mixin modules, and their dependencies.
`ext.centralNotice.display`		All campaign- and banner-selection logic	`ext.centralNotice.geoIP`, `jquery.cookie`, `json`, `mediawiki.Uri`

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.

Module	Included in site HTML	Contents	Dependencies
`ext.centralNotice.bannerController`
`ext.centralNotice.bannerController.mobile`
`ext.centralNotice.bannerController.mobiledevice`
`ext.centralNotice.bannerChoiceData`
`ext.centralNotice.bannerController.lib`
`ext.centralNotice.startUp`	Desktop and mobile	Minimal startup logic	`ext.centralNotice.choiceData`, `mediawiki.util`
`ext.centralNotice.goeIP`	Desktop and mobile	CN-independent GeoIP processing
`ext.centralNotice.choiceData`		JSON with available campaign and banner choices	Only as needed: `ext.centralNotice.display`, campaign-associated mixin modules, and their dependencies.
`ext.centralNotice.display`		All campaign- and banner-selection logic	`ext.centralNotice.geoIP`, `jquery.cookie`, `json`, `mediawiki.Uri`

Expected future changes

GeoIP processing should move to a different extension or to Mediawiki core.
We may make other changes to improve performance and/or mitigate the "banner bump".

API

Requirements

Remain compatible with existing banners. Certain methods, properties and hooks should remain available. We can use JS get to warn when deprecate properties are accessed. The following should stick around:
- mw.centralNotice.data (though it'll now be read-only)
- mw.centralNotice.bannerData
- mw.centralNotice.bannerData.alterImpressionData
- mw.centralNotice.events.bannerLoaded
- mw.centralNotice.insertBanner()
- mw.centralNotice.hideBanner()
Here are things that we don't need to keep publicly accessible outside CentralNotice:
- mw.centralNotice.recordImpression()
- mw.centralNotice.impressionData
- mw.centralNotice.alreadyRan
- mw.centralNotice.deferredObjs
- mw.centralNotice.loadBanner()
- mw.centralNotice.chooseRandomBanner()
- mw.centralNotice.toggleNotice()
- mw.centralNotice.getBucket() (already deprecated)
- mw.centralNotice.storeBucket() (already deprecated)
- mw.centralNotice.initialize()
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.

Proposal

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.

Component	Functionality	PS44
`geoIP`	When a GeoIP cookie is present and contains valid data, the data is parsed and placed in a window.Geo object, no background requests are made, and the deferred object is resolved right away.
`geoIP`	When a GeoIP cookie is present, but it contains invalid data of any kind, a background request is made that sets window.Geo, a cookie is set, and the deferred object is resolved.
`geoIP`	If a background geoIP lookup request returns an error, appropriately flagged data is stored in the cookie (an object with the value `"vx"` for the `af` property), and the deferred object is rejected. The flagged cookie data is in the right format to cause another background lookup on the next pageview.
`display`	A banner is injected into the DOM by `mw.centralNotice.insertBanner()`	✓QUnit
`display`	Calling `mw.centralNotice.displayTestingBanner()` fetches the banner indicated by the `banner` URL param.	✓QUnit
`display`	The `randomcampaign` URL param determines campaign selection when more than one campaign is allocated.	✓QUnit
`display`	The `randombanner` URL param determines banner selection when more than one banner is allocated.	✓QUnit
`display`	`chooseAndMaybeDisplay()` runs pre-banner mixin hook handlers with the correct parameters, after a campaign and bucket are chosen.
`display`	`chooseAndMaybeDisplay()` runs post-banner mixin hook handlers with the correct parameters,if a banner is canceled, or if the chosen campaign doesn't have a banner for this user.
`display`	`chooseAndMaybeDisplay()` calls the `alterImpressionData` function if it exists and if `bannerNotGuaranteedToDisplay` is set, and that function is able to cancel the banner and provide a reason. Note: this should be smoke tested, but unit tests are probably not worth the effort, since the feature may well be deprecated.
`display`	`chooseAndMaybeDisplay()` calls `Special:RecordImpression` at the designated sample rate in all cases if a banner is chosen. Note: this should be smoke tested, but unit tests are probably not worth the effort, since the feature may well be deprecated.
`display`	Calls to `Special:RecordImpresion` provide the correct legacy params as per the documentation. Note: many parameters there are out of scope for this code, since they're currently provided by in-banner JS. Also note: this should be smoke tested, but unit tests are probably not worth the effort, since the feature may well be deprecated.
`display.state`	`setUp()` pulls in initial data and makes it available from `getData()` as appropriate.
`display.state`	`setUp()` sets device class correctly based on `userAgent`.
`display.bucketer`	`process()` recognizes when a bucket is available from a cookie and doesn't change it.
`display.bucketer`	`process()` chooses a random bucket if none was available in the cookie, and saves it to the cookie.
`display.bucketer`	`process()` updates a campaign's end date in the cookie if necessary.
`display.bucketer`	`process()` purges expired buckets from the cookie.
`display.bucketer`	`loadBuckets()` deals correctly with the absence of a cookie and the presence of a corrupted one.
`display.bucketer`	`getBucket()` retrieves the bucket correctly.
`display.bucketer`	`setBucket()` sets the bucket and stores it in the cookie.
`display.bucketer`	`storeBucket()` sets the cookie expiry date correctly.
`display.hide`	If a hide cookie is present for the category indicated by setCategory(), and the duration for the hide reason indicated by the cookie has not yet elapsed, then after `processCookie()` is called, `shouldHide()` will return `true`.
`display.hide`	If a hide cookie is present for the category indicated by setCategory(), and the duration for the hide reason indicated by the cookie has elapsed, then after `processCookie()` is called, `shouldHide()` will return `false`.
`display.hide`	`setHideWithCloseButtonCookies()` sets a cookie with the appropriate values.
`display.hide`	`setHideWithCloseButtonCookies()` adds `img` tags to the DOM to bring on the hide cookie swarm, as appropriate.
`display.chooser`	Campaigns and banners correctly allocated for diverse targeting scenarios and contexts.	✓QUnit
`kvStore`	`setItem()` correctly stores an item with the right key for the context indicated.
`kvStore`	`setItem()` adds an error to the error log when something goes awry.
`kvStore`	The error log stores errors as a FIFO queue.
`kvStore`	`getErrorLog()` retrieves the error log.
`kvStore`	`getItem()` gets an item correctly for the context indicated.
`kvStore`	`removeItem()` removes an item correctly for the context indicated.

Issuez and considerations

Reducing the size of top-loading RL modules

Does the fact that RL modules are cached in localStorage somewhat mitigate performance gains from doing this?

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)