Skip to content

add url + sourceUrl to the channel_open message (amp-viewer-integration)#7466

Merged
chenshay merged 7 commits intoampproject:masterfrom
chenshay:message
Feb 16, 2017
Merged

add url + sourceUrl to the channel_open message (amp-viewer-integration)#7466
chenshay merged 7 commits intoampproject:masterfrom
chenshay:message

Conversation

@chenshay
Copy link
Copy Markdown
Contributor

close #7300

@chenshay chenshay requested a review from dvoytenko February 10, 2017 00:26
dev().fine(TAG, 'Channel has been opened!');
this.setup_(messaging, viewer);
});
const messaging = new Messaging(this.win, pipe);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename pipe to port. I think it will haunt me forever now :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

const messaging = new Messaging(this.win, pipe);
const ampDoc = getAmpDoc(this.win.document);
return messaging.sendRequest(RequestNames.CHANNEL_OPEN, {
url: ampDoc.getUrl(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a unit test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jridgewell
Copy link
Copy Markdown
Contributor

@chenshay: Can you update the PR title to something more descriptive?

@chenshay chenshay changed the title url + sourceUrl add url + sourceUrl to the channel_open message (amp-viewer-integration) Feb 13, 2017
ampViewerIntegration.openChannelAndStart_(viewer, ampDocSrc, messaging);
expect(sendRequestSpy).to.have.been.calledWith('channelOpen', {
sourceUrl: 'http://localhost:9876/test/fixtures/served/ampdoc-with-messaging.html',
url: '/test/fixtures/served/ampdoc-with-messaging.html',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This is not an absolute URL? They should all be absolute...

Copy link
Copy Markdown
Contributor Author

@chenshay chenshay Feb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what's returned from getSourceUrl(ampdocUrl)

@chenshay chenshay merged commit 39b1640 into ampproject:master Feb 16, 2017
@chenshay chenshay deleted the message branch February 16, 2017 20:58
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
…on) (ampproject#7466)

* url + sourceUrl

* rename

* added test

* remove only

* fix

* test fix

* test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add url and sourceUrl field to the channelOpen message

3 participants