Make share-tracking extension to get incoming/outcoming fragments #4019
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| <!doctype html> | ||
| <html ⚡> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>Share tracking examples</title> | ||
| <link rel="canonical" href="amps.html" > | ||
| <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"> | ||
| <link href='https://fonts.googleapis.com/css?family=Questrial' rel='stylesheet' type='text/css'> | ||
| <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> | ||
| <script async src="https://cdn.ampproject.org/v0.js"></script> | ||
| <script async src="https://cdn.ampproject.org/v0/amp-share-tracking-0.1.js"></script> | ||
| </head> | ||
| <body> | ||
| <amp-share-tracking data-href='http://localhost:8000/share-tracking/get-outgoing-fragment'> | ||
| </amp-share-tracking> | ||
|
|
||
| </body> | ||
| </html> | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| <!doctype html> | ||
| <html ⚡> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>Share tracking examples</title> | ||
| <link rel="canonical" href="amps.html" > | ||
| <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"> | ||
| <link href='https://fonts.googleapis.com/css?family=Questrial' rel='stylesheet' type='text/css'> | ||
| <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> | ||
| <script async src="https://cdn.ampproject.org/v0.js"></script> | ||
| <script async src="https://cdn.ampproject.org/v0/amp-share-tracking-0.1.js"></script> | ||
| </head> | ||
| <body> | ||
| <amp-share-tracking> | ||
| </amp-share-tracking> | ||
|
|
||
| </body> | ||
| </html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,43 +15,113 @@ | |
| */ | ||
|
|
||
| import {isExperimentOn} from '../../../src/experiments'; | ||
| import {xhrFor} from '../../../src/xhr'; | ||
| import {viewerFor} from '../../../src/viewer'; | ||
| import {Layout} from '../../../src/layout'; | ||
| import {dev, user} from '../../../src/log'; | ||
|
|
||
| /** @private @const {string} */ | ||
| const TAG = 'amp-share-tracking'; | ||
|
|
||
| /** | ||
| * @visibleForTesting | ||
| */ | ||
| export class AmpShareTracking extends AMP.BaseElement { | ||
| /** | ||
| * @return {boolean} | ||
| * @private | ||
| */ | ||
| isExperimentOn_() { | ||
| return isExperimentOn(this.win, TAG); | ||
| } | ||
|
|
||
| /** @override */ | ||
| isLayoutSupported(layout) { | ||
|
There was a problem hiding this comment. Would you mind copying this to There was a problem hiding this comment. I created a separate PR since this has nothing to do with share-tracking. |
||
| return layout == Layout.NODISPLAY || layout == Layout.CONTAINER; | ||
| } | ||
|
|
||
| /** @override */ | ||
| buildCallback() { | ||
| user.assert(this.isExperimentOn_(), `${TAG} experiment is disabled`); | ||
|
|
||
| /** @private {string} */ | ||
| this.vendorHref_ = this.element.getAttribute('data-href'); | ||
| dev.fine(TAG, 'vendorHref_: ', this.vendorHref_); | ||
|
|
||
| /** @private {!Promise<!Object>} */ | ||
| this.shareTrackingFragments_ = Promise.all([ | ||
| this.getIncomingFragment_(), | ||
| this.getOutgoingFragment_()]).then(results => { | ||
| dev.fine(TAG, 'incomingFragment: ', results[0]); | ||
| dev.fine(TAG, 'outgoingFragment: ', results[1]); | ||
| return { | ||
| incomingFragment: results[0], | ||
| outgoingFragment: results[1], | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Get the incoming share-tracking fragment from the viewer | ||
| * @return {!Promise<string>} | ||
| * @private | ||
| */ | ||
| getIncomingFragment_() { | ||
| return viewerFor(this.win).getFragment().then(fragment => { | ||
| const match = fragment.match(/\.([^&]*)/); | ||
| return match ? match[1] : ''; | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Get an outgoing share-tracking fragment | ||
| * @return {!Promise<string>} | ||
| * @private | ||
| */ | ||
| getOutgoingFragment_() { | ||
| if (this.vendorHref_) { | ||
| return this.getOutgoingFragmentFromVendor_(this.vendorHref_); | ||
| } | ||
| return this.getOutgoingRandomFragment_(); | ||
| } | ||
|
|
||
| /** | ||
| * Get an outgoing share-tracking fragment from vendor | ||
| * by issueing a post request to the url the vendor provided | ||
| * @param {string} vendorUrl | ||
| * @return {!Promise<string>} | ||
| * @private | ||
| */ | ||
| getOutgoingFragmentFromVendor_(vendorUrl) { | ||
| const postReq = { | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| requireAmpResponseSourceOrigin: true, | ||
| body: {}, | ||
| }; | ||
| return xhrFor(this.win).fetchJson(vendorUrl, postReq).then(response => { | ||
| if (response.fragment) { | ||
| return response.fragment; | ||
| } | ||
| user.error(TAG, 'The response from [' + vendorUrl + '] does not ' + | ||
| 'have a fragment value.'); | ||
| return ''; | ||
| }).catch(error => { | ||
|
There was a problem hiding this comment. Nitpick: This callback can be the second param to |
||
| user.error(TAG, 'The request to share-tracking endpoint failed:' + error); | ||
| return ''; | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Get a random outgoing share-tracking fragment | ||
| * @return {!Promise<string>} | ||
| * @private | ||
| */ | ||
| getOutgoingRandomFragment_() { | ||
| // TODO(yuxichen): Generate random outgoing fragment | ||
| const randomFragment = 'rAmDoM'; | ||
| return Promise.resolve(randomFragment); | ||
|
There was a problem hiding this comment. couldn't we just return cc @jridgewell There was a problem hiding this comment. although this would change the return type of There was a problem hiding this comment. I prefer making the return type of There was a problem hiding this comment. I think returning a promise here is better, since |
||
| } | ||
| } | ||
|
|
||
| AMP.registerElement('amp-share-tracking', AmpShareTracking); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| /** | ||
| * Copyright 2016 The AMP HTML Authors. All Rights Reserved. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the 'License'); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an 'AS-IS' BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import {createIframePromise} from '../../../../testing/iframe'; | ||
| import {AmpShareTracking} from '../amp-share-tracking'; | ||
| import {Viewer} from '../../../../src/service/viewer-impl'; | ||
| import {Xhr} from '../../../../src/service/xhr-impl'; | ||
| import {toggleExperiment} from '../../../../src/experiments'; | ||
| import * as sinon from 'sinon'; | ||
|
|
||
| describe('amp-share-tracking', () => { | ||
| let sandbox; | ||
| let viewerForMock; | ||
| let xhrMock; | ||
|
|
||
| beforeEach(() => { | ||
| sandbox = sinon.sandbox.create(); | ||
| viewerForMock = sandbox.stub(Viewer.prototype, 'getFragment'); | ||
| xhrMock = sandbox.stub(Xhr.prototype, 'fetchJson'); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| sandbox.restore(); | ||
| }); | ||
|
|
||
| function getAmpShareTracking(optVendorUrl) { | ||
| return createIframePromise().then(iframe => { | ||
| toggleExperiment(iframe.win, 'amp-share-tracking', true); | ||
| const el = iframe.doc.createElement('amp-share-tracking'); | ||
| if (optVendorUrl) { | ||
| el.setAttribute('data-href', optVendorUrl); | ||
| } | ||
| const ampShareTracking = new AmpShareTracking(el); | ||
| ampShareTracking.buildCallback(); | ||
| return ampShareTracking; | ||
| }); | ||
| } | ||
|
|
||
| it('should get incoming fragment starting with dot', () => { | ||
| viewerForMock.onFirstCall().returns(Promise.resolve('.12345')); | ||
| const mockJsonResponse = {fragment: '54321'}; | ||
| xhrMock.onFirstCall().returns(Promise.resolve(mockJsonResponse)); | ||
| return getAmpShareTracking('http://foo.bar').then(ampShareTracking => { | ||
| expect(ampShareTracking.vendorHref_).to.equal('http://foo.bar'); | ||
| return ampShareTracking.shareTrackingFragments_.then(fragments => { | ||
| expect(fragments.incomingFragment).to.equal('12345'); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('should get incoming fragment starting with dot and ignore ' + | ||
| 'other parameters', () => { | ||
| viewerForMock.onFirstCall().returns(Promise.resolve('.12345&key=value')); | ||
| const mockJsonResponse = {fragment: '54321'}; | ||
| xhrMock.onFirstCall().returns(Promise.resolve(mockJsonResponse)); | ||
| return getAmpShareTracking('http://foo.bar').then(ampShareTracking => { | ||
| expect(ampShareTracking.vendorHref_).to.equal('http://foo.bar'); | ||
| return ampShareTracking.shareTrackingFragments_.then(fragments => { | ||
| expect(fragments.incomingFragment).to.equal('12345'); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('should ignore incoming fragment if it is empty', () => { | ||
| viewerForMock.onFirstCall().returns(Promise.resolve('')); | ||
| const mockJsonResponse = {fragment: '54321'}; | ||
| xhrMock.onFirstCall().returns(Promise.resolve(mockJsonResponse)); | ||
| return getAmpShareTracking('http://foo.bar').then(ampShareTracking => { | ||
| expect(ampShareTracking.vendorHref_).to.equal('http://foo.bar'); | ||
| return ampShareTracking.shareTrackingFragments_.then(fragments => { | ||
| expect(fragments.incomingFragment).to.equal(''); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('should ignore incoming fragment if it does not start with dot', () => { | ||
| viewerForMock.onFirstCall().returns(Promise.resolve('12345')); | ||
| const mockJsonResponse = {fragment: '54321'}; | ||
| xhrMock.onFirstCall().returns(Promise.resolve(mockJsonResponse)); | ||
| return getAmpShareTracking('http://foo.bar').then(ampShareTracking => { | ||
| expect(ampShareTracking.vendorHref_).to.equal('http://foo.bar'); | ||
| return ampShareTracking.shareTrackingFragments_.then(fragments => { | ||
| expect(fragments.incomingFragment).to.equal(''); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('should get outgoing fragment randomly if no vendor url ' + | ||
| 'is provided', () => { | ||
| viewerForMock.onFirstCall().returns(Promise.resolve('.12345')); | ||
| return getAmpShareTracking().then(ampShareTracking => { | ||
| expect(ampShareTracking.vendorHref_).to.be.null; | ||
| return ampShareTracking.shareTrackingFragments_.then(fragments => { | ||
| expect(fragments.outgoingFragment).to.equal('rAmDoM'); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('should get outgoing fragment from vendor if vendor url is provided ' + | ||
| 'and the response format is correct', () => { | ||
| viewerForMock.onFirstCall().returns(Promise.resolve('.12345')); | ||
| const mockJsonResponse = {fragment: '54321'}; | ||
| xhrMock.onFirstCall().returns(Promise.resolve(mockJsonResponse)); | ||
| return getAmpShareTracking('http://foo.bar').then(ampShareTracking => { | ||
| expect(ampShareTracking.vendorHref_).to.equal('http://foo.bar'); | ||
| return ampShareTracking.shareTrackingFragments_.then(fragments => { | ||
| expect(fragments.outgoingFragment).to.equal('54321'); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('should get empty outgoing fragment if vendor url is provided ' + | ||
| 'but the response format is NOT correct', () => { | ||
| viewerForMock.onFirstCall().returns(Promise.resolve('.12345')); | ||
| const mockJsonResponse = {foo: 'bar'}; | ||
| xhrMock.onFirstCall().returns(Promise.resolve(mockJsonResponse)); | ||
| return getAmpShareTracking('http://foo.bar').then(ampShareTracking => { | ||
| expect(ampShareTracking.vendorHref_).to.equal('http://foo.bar'); | ||
| return ampShareTracking.shareTrackingFragments_.then(fragments => { | ||
| expect(fragments.outgoingFragment).to.equal(''); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('should call fetchJson with correct request when getting outgoing' + | ||
| 'fragment', () => { | ||
| viewerForMock.onFirstCall().returns(Promise.resolve('.12345')); | ||
| const mockJsonResponse = {fragment: '54321'}; | ||
| xhrMock.onFirstCall().returns(Promise.resolve(mockJsonResponse)); | ||
| return getAmpShareTracking('http://foo.bar').then(ampShareTracking => { | ||
| expect(ampShareTracking.vendorHref_).to.equal('http://foo.bar'); | ||
| const xhrCall = xhrMock.getCall(0); | ||
| expect(xhrCall.args[0]).to.equal('http://foo.bar'); | ||
| const config = xhrCall.args[1]; | ||
| expect(config.method).to.equal('POST'); | ||
| expect(config.credentials).to.equal('include'); | ||
| expect(config.requireAmpResponseSourceOrigin).to.be.true; | ||
| return ampShareTracking.shareTrackingFragments_.then(fragments => { | ||
| expect(fragments.outgoingFragment).to.equal('54321'); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('should get empty outgoing fragment if vendor url is provided ' + | ||
| 'but the xhr fails', () => { | ||
| viewerForMock.onFirstCall().returns(Promise.resolve('.12345')); | ||
| xhrMock.onFirstCall().returns(Promise.reject('404')); | ||
| return getAmpShareTracking('http://foo.bar').then(ampShareTracking => { | ||
| expect(ampShareTracking.vendorHref_).to.equal('http://foo.bar'); | ||
| return ampShareTracking.shareTrackingFragments_.then(fragments => { | ||
| expect(fragments.outgoingFragment).to.equal(''); | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there no
layout=nodisplay?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I return true in
isLayoutSupported. So I don't needlayout=nodisplay. What's the correct way?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not enough. It's best to declare
layout=nodisplay. Alternatively, we can addamp-share-tracking {display: none;}in main CSS (amp.css). I can be talked into it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amp-analyticsandamp-experimentboth return true inisLayoutSupportedwithoutdisplay: noneinamp.cssnorlayout=nodisplayin the html element. Shall I change those two?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added
display: noneinamp.cssThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isLayoutSupportedcan only allownodisplayandcontainerfor both elements.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the
isLayoutSupportedin the latest commit