Skip to content

🚀 [Story video] Use the inlined video response instead of issuing an XHR request, for the 1st video of the 1st web story page #37499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
coreymasanto merged 25 commits into from
Feb 14, 2022
Merged

🚀 [Story video] Use the inlined video response instead of issuing an XHR request, for the 1st video of the 1st web story page #37499

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
163b8f7
For the first video of the first web story page, use the inlined vide…
coreymasanto Jan 26, 2022
0819baf
Fix merge conflict
coreymasanto Jan 26, 2022
cf79773
Refactor to prevent unnecessary XHR-related function calls
coreymasanto Jan 26, 2022
16ba4e8
Fix promise
coreymasanto Jan 26, 2022
ad8a9a0
Syntax fix
coreymasanto Jan 27, 2022
455d45e
Allow the XHR request to be sent if the inline video is unexpectedly …
coreymasanto Jan 27, 2022
4b3a02f
Lint
coreymasanto Jan 27, 2022
73c67c1
Remove unnecessary variable from shouldUseInlineVideoResponse and upd…
coreymasanto Jan 28, 2022
c92c2e7
Wrap JSON.parse() in a try/catch
coreymasanto Jan 28, 2022
6164988
Pull out a new requestCachedVideoSources() method from the fetchCache…
coreymasanto Jan 28, 2022
7c760e0
Clean up preexisting lengthy if-statement
coreymasanto Jan 28, 2022
b4093ea
Merge branch 'main' into inlinedVideo
coreymasanto Feb 8, 2022
5640b66
Simplify JSON parsing logic in requestCachedVideoSources
coreymasanto Feb 8, 2022
8787143
Add a check for the 'sources' key
coreymasanto Feb 8, 2022
d2465cc
Lint fix
coreymasanto Feb 8, 2022
bc73828
Add initial, untested version of tests for inline video response logic
coreymasanto Feb 11, 2022
697dd6b
Merge branch 'main' into inlinedVideo
coreymasanto Feb 11, 2022
b477b97
Add a describe grouping for the set of inline video response tests
coreymasanto Feb 11, 2022
2d108a6
Update logic for determining whether the inline response should be used
coreymasanto Feb 11, 2022
a5487d2
Get tests passing
coreymasanto Feb 11, 2022
c328d21
Lint fixes
coreymasanto Feb 11, 2022
e0e6de7
Merge branch 'main' into inlinedVideo
coreymasanto Feb 11, 2022
e0e8b85
Merge branch 'main' into inlinedVideo
coreymasanto Feb 11, 2022
d4ed4ec
Merge branch 'main' into inlinedVideo
coreymasanto Feb 14, 2022
db458ea
Merge branch 'main' into inlinedVideo
coreymasanto Feb 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Get tests passing
  • Loading branch information
@coreymasanto
coreymasanto committed Feb 11, 2022
commit a5487d2e7a68d3b973add7242971d17769b42674
55 changes: 31 additions & 24 deletions extensions/amp-video/0.1/test/test-video-cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,36 +496,38 @@ describes.realWin('amp-video cached-sources', {amp: true}, (env) => {

describe('web stories: inlined video', async () => {
it('should use the inlined source for the first video in the story instead of sending an XHR request', async () => {
console.log('test #1');

// Set up an inlined source response for the first video in the story
const storyEl = createStoryForInlineResponseTesting();
const storyEl = createStoryForInlineVideoTesting();
env.win.document.body.appendChild(storyEl);
setUpInlinedVideoResponse();

const xhrSpy = env.sandbox.spy(xhrService, 'fetch');

// Fetch the sources for the first video in the story
const videoEl = env.win.document.getElementById('video1');
const videoEl = storyEl.querySelectorAll('amp-video')[0];
await fetchCachedSources(videoEl, env.ampdoc);

const addedSources = videoEl.querySelectorAll('source');
expect(xhrSpy).to.have.not.been.called();
expect(addedSources).to.have.lengthOf(1); // obtained from inlined source
expect(xhrSpy).to.have.not.been.called;
const inlinedSources = videoEl.querySelectorAll('source[src="inlined_video_response.mp4"]');
expect(inlinedSources).to.have.lengthOf(1);
});

it('should send an XHR request for any video that is not the very first one within the story', async () => {
// Set up an inlined source response for the first video in the story
const storyEl = createStoryForInlineResponseTesting();
const storyEl = createStoryForInlineVideoTesting();
env.win.document.body.appendChild(storyEl);
setUpInlinedVideoResponse();

const xhrSpy = env.sandbox.spy(xhrService, 'fetch');

// Fetch the sources for the second video on the first story page
const videoEl2 = env.win.document.getElementById('video2');
// Fetch the sources for video #2: the 2nd video on the first story page
const videoEl2 = storyEl.querySelectorAll('amp-video')[1];
await fetchCachedSources(videoEl2, env.ampdoc);

// Fetch the sources for the first video on the second story page
const videoEl3 = env.win.document.getElementById('video3');
// Fetch the sources for video #3: the 1st video on the 2nd story page
const videoEl3 = storyEl.querySelectorAll('amp-video')[2];
await fetchCachedSources(videoEl3, env.ampdoc);

expect(xhrSpy).to.have.been.calledWith(
Expand All @@ -538,13 +540,13 @@ describes.realWin('amp-video cached-sources', {amp: true}, (env) => {

it('should send XHR request if inline config not provided', async () => {
// Create story without setting an inlined source response
const storyEl = createStoryForInlineResponseTesting();
const storyEl = createStoryForInlineVideoTesting();
env.win.document.body.appendChild(storyEl);

const xhrSpy = env.sandbox.spy(xhrService, 'fetch');

// Fetch the sources for the first video in the story
const videoEl = env.win.document.getElementById('video1');
const videoEl = storyEl.querySelectorAll('amp-video')[0];
await fetchCachedSources(videoEl, env.ampdoc);

expect(xhrSpy).to.have.been.calledWith(
Expand All @@ -553,15 +555,19 @@ describes.realWin('amp-video cached-sources', {amp: true}, (env) => {
});

it('should send XHR request if inlined video response fails to parse', async () => {
// Set up an inlined source response for the first video in the story
const storyEl = createStoryForInlineResponseTesting();
// Set up an improperly configured response for the first video in the story
const storyEl = createStoryForInlineVideoTesting();
env.win.document.body.appendChild(storyEl);
setUpInlinedVideoResponse(true /* responseShouldFailToParse */);
const scriptEl = createElementWithAttributes(env.win.document, 'script', {
'id': 'amp-google-video-cache-response',
'type': 'application/json',
});
scriptEl.textContent = '{"faulty": [{}]}';

const xhrSpy = env.sandbox.spy(xhrService, 'fetch');

// Fetch the sources for the first video in the story
const videoEl = env.win.document.getElementById('video1');
const videoEl = storyEl.querySelectorAll('amp-video')[0];
await fetchCachedSources(videoEl, env.ampdoc);

expect(xhrSpy).to.have.been.calledWith(
Expand All @@ -587,7 +593,7 @@ describes.realWin('amp-video cached-sources', {amp: true}, (env) => {
return videoEl;
}

function createStoryForInlineResponseTesting() {
function createStoryForInlineVideoTesting() {
const storyEl = env.win.document.createElement('amp-story');
const storyPageEl1 = env.win.document.createElement('amp-story-page');
const storyPageEl2 = env.win.document.createElement('amp-story-page');
Expand All @@ -597,37 +603,38 @@ describes.realWin('amp-video cached-sources', {amp: true}, (env) => {
// Place two videos on the first page. video #1 is nested more deeply than
// video #2, but it should still be considered the first video on the page.
const gridLayerEl = env.win.document.createElement('amp-story-grid-layer');
const videoEl1 = createVideo([{id: 'video1', src: 'video1.mp4'}]);
const videoEl1 = createVideo([{src: 'video1.mp4'}]);
gridLayerEl.appendChild(videoEl1);
storyPageEl1.appendChild(gridLayerEl);

// Place video #2 on the first page.
const videoEl2 = createVideo([{id: 'video2', src: 'video2.mp4'}]);
const videoEl2 = createVideo([{src: 'video2.mp4'}]);
storyPageEl1.appendChild(videoEl2);

// Place video #3 on the second page.
const videoEl3 = createVideo([{id: 'video3', src: 'video3.mp4'}]);
const videoEl3 = createVideo([{src: 'video3.mp4'}]);
storyPageEl2.appendChild(videoEl3);

return storyEl;
}

function setUpInlinedVideoResponse(responseShouldFailToParse = false) {
function setUpInlinedVideoResponse() {
const scriptEl = createElementWithAttributes(env.win.document, 'script', {
'id': 'amp-google-video-cache-response',
'type': 'application/json',
});
scriptEl.textContent = `
{
${responseShouldFailToParse ? "wrong_key" : "sources"}: [
"sources": [
{
"url": "video1.mp4",
"url": "inlined_video_response.mp4",
"codec": "h264",
"type": "video/mp4",
"bitrate_kbps": 400
},
}
],
"has_audio": false
}`;
env.win.document.head.appendChild(scriptEl);
}
});
3 changes: 1 addition & 2 deletions extensions/amp-video/0.1/video-cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,6 @@ function requestCachedVideoSources(videoEl, ampdoc) {
*/
function shouldUseInlineVideoResponse(videoEl, win) {
// Google video cache inlines the first video of the first web story page.
const firstVid =
win.document.querySelector('amp-story-page:first-of-type amp-video');
const firstVid = win.document.querySelector('amp-story-page:first-of-type amp-video');
return videoEl === firstVid;
}