Fast Fetch Doubleclick experiment that clears correlator on page pause #10708
Conversation
| @@ -269,6 +287,8 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A { | |||
|
|
|||
| /** @private {number} */ | |||
| this.ifi_ = 0; | |||
|
|
|||
| this.viewer_ = Services.viewerForDoc(this.getAmpDoc()); | |||
There was a problem hiding this comment.
This is only used in buildCallback, can you just make it a const within that?
| height: '250', | ||
| width: '320', | ||
| }); | ||
| document.body.appendChild(element); |
There was a problem hiding this comment.
You need an afterEach that deletes this element
There was a problem hiding this comment.
Should have been referencing fixture.doc which requires no cleanup as its within frame
| }); | ||
|
|
||
| it('clears if in experiment', () => { | ||
| impl.win.ampAdPageCorrelator = 12345; |
There was a problem hiding this comment.
can delete this line, it's in the beforeEach. I'd just replace with an expect for what it should be.
| impl.buildCallback(); | ||
| expect(onVisibilityChangedHandler).to.be.ok; | ||
| onVisibilityChangedHandler(); | ||
| expect(isInExperiment(element, CORRELATOR_CLEAR_EXP_BRANCHES.CONTROL)) |
There was a problem hiding this comment.
You don't actually check the value of impl.win.ampAdPageCorrelator in this test.
| height: '250', | ||
| width: '320', | ||
| }); | ||
| document.body.appendChild(elem2); |
There was a problem hiding this comment.
you also need to delete this at the end of the test.
There was a problem hiding this comment.
Modified to use fixture document
| impl.win.experimentBranches[CORRELATOR_CLEAR_EXP_NAME] === undefined); | ||
| }); | ||
|
|
||
| it('only registers one on viewability change listener', () => { |
There was a problem hiding this comment.
what is this testing? is this supposed to be 'only register one onVisibilityChangedHandler' ?
There was a problem hiding this comment.
Yep, updated name
|
looking... |
| // TODO(keithwrightbos,glevitzy) - determine behavior for correlator | ||
| // interaction with refresh. | ||
| const experimentInfoMap = | ||
| /** @type {!Object<string, !ExperimentInfo>} */ ({}); |
| /** @override */ | ||
| buildCallback() { | ||
| super.buildCallback(); | ||
| if (this.win['dbclk_a4a_viz_change']) { |
There was a problem hiding this comment.
Overall, I'd prefer an ampdoc service b/c (a) it already supports one-per-doc logic, (b) it extends well to PWA/shadow docs, and (c) could be a good placeholder for other state instead of adding more stuff to global window (e.g. ampCorrelator).
There was a problem hiding this comment.
Will address in a subsequent PR as this is a larger change. I agree that having a dedicate service for state is a good idea.
| expect(onVisibilityChangedHandler).to.be.ok; | ||
| onVisibilityChangedHandler(); | ||
| expect( | ||
| impl.win.experimentBranches[CORRELATOR_CLEAR_EXP_NAME] !== undefined); |
There was a problem hiding this comment.
This test has been silently failing, and I commented it out in #13424
Do we actually expect impl.win.experimentBranches[CORRELATOR_CLEAR_EXP_NAME] to be defined here?
(I haven't looked at the code in detail)
Doubleclick Delayed Fetch clears window state when page is swiped between SERP viewer including correlator. However, this is not the case for Fast Fetch as the window state is maintained. Start 2%, client-side diverted experiment which clears correlator on pause page visibility change.
/cc @ampproject/a4a