Troubleshoot for AMP pages #11920
Troubleshoot for AMP pages #11920
Conversation
glevitzky
force-pushed
the
troubleshoot
branch
from
8b97569
to
eb53987
Compare
| * Troubleshoot UI. | ||
| */ | ||
| postTroubleshootMessage_() { | ||
| if (!this.win.opener || !/\?.*dfpdeb/.test(this.win.location.href)) { |
There was a problem hiding this comment.
Travis seems to have found a case where this.win.location is undefined
| @@ -1064,6 +1064,68 @@ describes.realWin('amp-ad-network-doubleclick-impl', realWinConfig, env => { | |||
| ['https://partner.googleadservices.com', SAFEFRAME_ORIGIN]); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('Troubleshoot', () => { | |||
There was a problem hiding this comment.
Done. Note that "Troubleshoot" is a feature name.
| }); | ||
|
|
||
| it('should not emit post message', () => { | ||
| env.win = { |
There was a problem hiding this comment.
Ah...the Travis error is because no location/href here
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| * @return {?string} | ||
| */ | ||
| getCachedAdUrl() { | ||
| return this.adUrl_; |
There was a problem hiding this comment.
I don't think you need this function. If you are only accessing getCachedAdUrl from within the doubleclick fast fetch class, you can just directly use this.adUrl_
There was a problem hiding this comment.
I'd need to make adUrl_ protected, which would require a lot more changes. Unless you have a strong objection, I'd rather expose it through a getter function.
There was a problem hiding this comment.
ah k, idk why I was thinking you could do that.
There was a problem hiding this comment.
Doesn't doubleclick impl know the ad url? After all it constructed it when it overrode getAdUrl. You could instead have a local variable that encapsulates the message to be sent (as a typdef) which you would update (e.g. set content url, creative id, line item id, etc). When in doubt, we should try not to clutter that AmpA4A API unless necessary.
| }, | ||
| }; | ||
| impl.win = env.win; | ||
| impl.postTroubleshootMessage_(); |
There was a problem hiding this comment.
this test won't catch if opener.postMessage is never called
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| * @return {?string} | ||
| */ | ||
| getCachedAdUrl() { | ||
| return this.adUrl_; |
There was a problem hiding this comment.
Doesn't doubleclick impl know the ad url? After all it constructed it when it overrode getAdUrl. You could instead have a local variable that encapsulates the message to be sent (as a typdef) which you would update (e.g. set content url, creative id, line item id, etc). When in doubt, we should try not to clutter that AmpA4A API unless necessary.
| this.creativeId_ = null; | ||
|
|
||
| /** @private {?string} */ | ||
| this.lineitemId_ = null; |
There was a problem hiding this comment.
Should this be this.lineItemId_?
| * Troubleshoot UI. | ||
| */ | ||
| postTroubleshootMessage_() { | ||
| if (!this.win.opener || !/\?.*dfpdeb/.test(this.win.location.href)) { |
There was a problem hiding this comment.
If you're looking for query string content, you can use this.win.location.search which includes the '?'... Doesn't actually save you much here but better to be explicit. Also, do you want to handle edge cases like foodfpdeb or dfpdebfoo? If so, should this be: /[?|&]dfpdeb=/
| }; | ||
| const postMessageSpy = sandbox.spy(env.win.opener, 'postMessage'); | ||
| impl.win = env.win; | ||
| return impl.postTroubleshootMessage_().then(() => |
There was a problem hiding this comment.
Let's make this visibleForTesting instead of accessing private function
* Init. * stashing changes. * Cases slotid correctly. * Added test and finalized message attributes. * Removing test code. * Clean up + negative test. * Added comments. * Fixed comments and test. * Fixing presubmit errors. * Make more obvious what negative test is testing. * Gathering all Troubleshoot data into single type/object. * Fixing docstring. * Moved troubleshootData initializations earlier into constructor. * Made postTroubleshootMessage public and visible for testing. * Moving things back to buildCallback.
* Init. * stashing changes. * Cases slotid correctly. * Added test and finalized message attributes. * Removing test code. * Clean up + negative test. * Added comments. * Fixed comments and test. * Fixing presubmit errors. * Make more obvious what negative test is testing. * Gathering all Troubleshoot data into single type/object. * Fixing docstring. * Moved troubleshootData initializations earlier into constructor. * Made postTroubleshootMessage public and visible for testing. * Moving things back to buildCallback.
Introduces mechanism for communicating with DFP Troubleshoot. Specifically, this adds a postMessage that is emitted under certain circumstances (window.opener != null and 'dfpdeb' present in page URL), and contains information about the slot.