The page previews test is finished for now, but we may want to revisit this for another test at some point. Sam shared the following code which might help:

One thing I meant to say was you could use mw.loader.using here:

// Has the Popups codebase been sent to the user?
if ( mw.loader.getState( 'ext.popups' ) === 'ready' ) {
  mw.loader.using( 'ext.popups', () => {
    callback( mw.popups.isEnabled() ); // Or resolve a promise or whatever...
  } );
}

This would mean that you wouldn't have to hardcode a timeout.

It does seem like the most likely cause is that sometimes the banner loads before ext.popups, and sometimes it happens afterwards. We could verify by making the in-banner code register something like 'popupTestStatusUnkown' if mw.popups is not available... Not sure if it's worth it, though...

Indeed @phuedx did warn that a race might be an issue here. Many apologies for missing that...

Assuming that is the problem, neither the timeouts nor mw.loader.using will help, since we call beacon/impression right after the banner loads (and in-banner JS runs). (See reallyInsertBanner() and processAfterBannerFetch() in ext.centralNotice.display.js.) Currently no facilities for async delay. So, while the call to mediaWiki.centralNotice.registerTest() should succeed, that information won't be included in the beacon/impression request.

The only solution I can think of would be to add a feature to let in-banner JS delay the call to beacon/impression. Any thoughts? Is this important to do before the year-end fundraising campaign in English-speaking countries?

Thanks!!! :D

@Pcoombe when would you like to test this again?

@AndyRussG how tough would it be to address this?

In T176334#3704997, @DStrine wrote:

@AndyRussG how tough would it be to address this?

I think it shouldn't be tough, but it would add a tidbit of not-tested-at-scale-or-for-very-long complexity at the wrong time...

In T176334#3704960, @AndyRussG wrote:

It does seem like the most likely cause is that sometimes the banner loads before ext.popups, and sometimes it happens afterwards. We could verify by making the in-banner code register something like 'popupTestStatusUnkown' if mw.popups is not available... Not sure if it's worth it, though...

<snip />

The only solution I can think of would be to add a feature to let in-banner JS delay the call to beacon/impression. Any thoughts? Is this important to do before the year-end fundraising campaign in English-speaking countries?

If you were to do this, I'd recommend a hybrid approach: you add the feature to allow a banner to delay the call to the analytics backend but include the notion of the unknown state so that you can set a limit on the delay, e.g.

function rejectIn( delay ) {
  const deferred = $.Deferred();

  setTimeout(
    () => deferred.reject(),
    delay
  );

  return deferred.promise();
}

const MAX_DELAY = 250; // ms
const isEnabledPromise = mw.loader.using( 'ext.popups' )
  .then( () => mw.popups && mw.popups.isEnabled() )

// If it takes longer than 250 ms to load and evaluate the Popups codebase (for whatever reason), then mark its state as unknown.
$.when( isEnabledPromise, rejectIn( MAX_DELAY ) )
  .then( ( isEnabled ) => mw.centralNotice.registerTest( isEnabled ? 'popupsEnabled' : 'popupsDisabled' ) )
  .fail( () => mw.centralNotice.registerTest( 'popupsUnknown' ) );

In T176334#3706870, @phuedx wrote:

The only solution I can think of would be to add a feature to let in-banner JS delay the call to beacon/impression. Any thoughts? Is this important to do before the year-end fundraising campaign in English-speaking countries?

If you were to do this, I'd recommend a hybrid approach: you add the feature to allow a banner to delay the call to the analytics backend but include the notion of the unknown state so that you can set a limit on the delay, e.g.

Yea, great suggestion! Thanks much!!! :D

@DStrine According to @jrobell It's possible we would want to test this during Big English, although it depends on the Reading team's own timeline for deployment of the feature.

That's right. @DStrine @Pcoombe
We do want to run a test during Big English to test the impact on Hovercards during a longer time period for a small number of readers. The reading team is planning to roll out the feature in January so this gives us the opportunity to test before the roll out.

In T176334#3709047, @jrobell wrote:

We do want to run a test during Big English to test the impact on Hovercards during a longer time period for a small number of readers. The reading team is planning to roll out the feature in January so this gives us the opportunity to test before the roll out.

I've just checked with @ovasileva and I can confirm that this is correct.

Change 386883 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@master] Add API to delay call to record impression

https://gerrit.wikimedia.org/r/386883

Here's how to use the provided patch from JS in a banner:

mw.centralNotice.requestRecordImpressionDelay(
    mw.loader.using( 'ext.popups' )
).done( function () {
    if ( mw.popups ) {
        mw.centralNotice.registerTest( ( mw.popups.isEnabled() ? 'popupsEnabled' : 'popupsDisabled' ) );
    } else {
        mw.centralNotice.registerTest( 'popupsUnknown' );
    }
} );

In this example, you'd get 'popupsUnkown' only when the request to load the popups module didn't return before the timeout to call beacon/impression. Any handlers set on the promise returned by mw.centralNotice.requestRecordImpressionDelay() are guaranteed to run before beacon/impression is called. This ensures that any data they set will be included in the call.

On how many clients do we expect the popups extension itself to be loaded (whether or not it's enabled)? Now, I'm worried we'll be adding a lot of extra bandwidth use... I think it's definitely important to avoid this code on any platforms where it's never loaded (like mobile?).

The approach is a bit clunkier than what @phuedx suggested--the promise returned by mw.centralNotice.requestRecordImpressionDelay() doesn't fail if we call beacon/impression because we reached the timeout rather than the promise sent as an argument resolving. This is just because the code has to live in two places (CN client-side API and a banner), and I thought it'd be best to keep the API and its implementation quite simple. Could still be changed, though.

Thanks much!! :D

Thanks @AndyRussG, that code looks good to me. We can definitely add it only where needed i.e. English desktop banners.

If it's possible to get this change deployed before 16 November, when Reading are planning to end their current Page Previews test (T178500) that would be really helpful.

Change 386883 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Add API to delay call to record impression

https://gerrit.wikimedia.org/r/386883

Change 391208 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@wmf_deploy] Add API to delay call to record impression

https://gerrit.wikimedia.org/r/391208

Change 391208 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@wmf_deploy] Add API to delay call to record impression

https://gerrit.wikimedia.org/r/391208

Thanks for the work on this @AndyRussG. I've included the code from T176334#3716446 in the banner templates (wrapped in a conditional so it only runs for English desktop banners). The ratio of impressions with popupsEnabled to popupsDisabled looks good now, but I'm seeing a very large number with popupsUnknown (more than enabled and disabled put together). Data

Testing in Chrome and Firefox I sometimes see them sending back popupsDisabled, and more often popupsUnknown. Any idea why this might be happening?

@Pcoombe thanks for testing...! :) I'll look at the popups code to see if there are any clues there...

Hi! It looks like the problem is that the mw.popups object isn't created as soon as the module is loaded, but rather in an idle callback using mw.requestIdleCallback().

Since idle callbacks should be performed in FIFO order, we can just resolve the promise used to delay the record impression call within another idle callback:

var deferred = $.Deferred();

mw.loader.using( 'ext.popups' ).done( function () {
    mw.requestIdleCallback( function () {
        deferred.resolve();
    } );
} );

mw.centralNotice.requestRecordImpressionDelay( deferred.promise() ).done( function () {
    if ( mw.popups ) {
        mw.centralNotice.registerTest( ( mw.popups.isEnabled() ? 'popupsEnabled' : 'popupsDisabled' ) );
    } else {
        mw.centralNotice.registerTest( 'popupsUnknown' );
    }
} );

I smoke tested this with a test campaign on production, and it seems to eliminate the popupsUnknown results. Before this is added in actual banners, I'd like to just add it to the banners used for cross-browser testing on the beta cluster, and re-run the browser tests...

Thanks much!!!! :)

The code above passed our browser tests on the beta cluster... :)

Nice, @AndyRussG!

Thanks @AndyRussG! I added the code above to our banners yesterday (diff), and while there's still some popupsUnknown results coming through, they are significantly reduced from previously.

This is from 08:00-09:00 UTC this morning

Ah, no wonder we're still getting a few popupsUnknown. Some of our banners still have the old code. I'll try and chase them down.

Even after updating all the banners, we continued to get popupsUnknown status on about 19-20% of all banner views recorded. It's strange, but not a big deal since we have more than enough data.

In T176334#3856864, @Pcoombe wrote:

Even after updating all the banners, we continued to get popupsUnknown status on about 19-20% of all banner views recorded. It's strange, but not a big deal since we have more than enough data.

Hi! Apologies for the delay in replying... It's possible that for those users, popups hasn't been able to load and run before the maximum delay for the record impression call has been reached. We should probably update the CN API so that the in-banner JS can tell whether or not the maximum delay was reached. We could also increase the delay (currently at 250 milliseconds). Thanks much!! :)

In T176334#3870139, @AndyRussG wrote:

We should probably update the CN API so that the in-banner JS can tell whether or not the maximum delay was reached.

Looking at this again more closely, I remembered why we didn't add that in the API originally--it's just because mw.centralNotice.requestRecordImpressionDelay() could potentially be called any number of times with different promises, and it seems like it'd be relatively a lot of code to keep track of which ones resolved in time, and which ones didn't. Instead, we could change slightly the code in the banners to check the status of the promise sent in as an argument:

var deferred = $.Deferred();

mw.loader.using( 'ext.popups' ).done( function () {
    mw.requestIdleCallback( function () {
        deferred.resolve();
    } );
} );

mw.centralNotice.requestRecordImpressionDelay( deferred.promise() ).done( function () {
    var testStr;

    if ( deferred.state() == 'pending' ) {
        testStr = 'popupsNotLoaded';
    } else if ( mw.popups ) {
        testStr = mw.popups.isEnabled() ? 'popupsEnabled' : 'popupsDisabled';
    } else {
        testStr = 'popupsUnknown';
    }
    mw.centralNotice.registerTest( testStr );
} )

A few quick notes:

• Avoid using mw.loader.using() in this way, unless you intent to be responsible for actively starting a code download that wasn't normally going to occur. This might be safe for short-lived code if (and only if) you know for sure the download happens unconditionally for all wikis, pages, skins, and users. Looking at the Popups extension code, it does not load the ext.popups module unconditionally. Which means, unless protected against, this will actively download a significant amount of unused code, which is a problem. See code in Popups.git:/includes/PopupsHooks.php

• To know whether or not a module was enqueued for this page view by PHP, use mw.loader.getState( moduleName ) !== 'registered'. If a module has been enqueued for this page, its state can be anything from loading, loaded, executing, ready or error. Depending on when the banner code runs, relative to the popups code. Once you've determined that it is enqueued, calling using() is a safe way of riding along that load and get a reliable callback for when it becomes ready and/or has failed to load. Be sure, in that case, to also attach an error handler: mw.loader.using( .. ).then( .. is loaded .., .. failed to load )...

• Outcome should be one of 1) Popups not loaded on this page, 2) Popups failed to load, 3) Popups loaded and once ready was determined to be enabled, 4) Popups loaded and once ready was determined to be disabled.