这是indexloc提供的服务，不要输入任何密码
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

Jump to bottom
Merged
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

merged 25 commits into from
Feb 14, 2022

Conversation

coreymasanto
Copy link
Contributor

@coreymasanto coreymasanto commented Jan 27, 2022 •
edited
Loading

Google video cache will be inlining the 1st video in the 1st story page, removing the need for an XHR call. This PR prevents the XHR request for this video whenever the inlined response is present.

Fixes #37397

@coreymasanto coreymasanto marked this pull request as ready for review January 27, 2022 16:16
@coreymasanto
Copy link
Contributor Author

I have tested this PR on a local server by doing the following steps:

  1. Add the following script within the <head> of videos-google-cache.html.
<script id="amp-google-video-cache-response" type="application/json">
    {
      "sources":  [
        {
          "url": "http://inlined_vid.mov",
          "codec": "h264",
          "type": "video/mp4",
          "bitrate_kbps": 400
        }
      ]
    }
  </script>
  1. Add a second video child to the first <amp-story-page>
  2. Go to videos-google-cache.html and verify that only the first video on the first page uses the inlined video source, and that an XHR request is issued for all other videos in the story.

@coreymasanto
Copy link
Contributor Author

coreymasanto commented Jan 27, 2022 •
edited
Loading

I think my current logic won't work when there are multiple grid layers on the first page that each have its own video child, because I would expect such videos to all match the amp-story-page:first-of-type amp-video:first-of-type selector. Do you know which video will be inlined by Google video cache in such a scenario? I'm guessing that it will always be the video element whose ancestor appears first, such as vid1 in this example:

<amp-story-page>
    <amp-story-grid-layer>
        <div>
            <div>
                <amp-video id="vid1">...</amp-video>
            </div>
        </div>
    </amp-story-grid-layer>

    <amp-story-grid-layer>
        <amp-video id="vid2">...</amp-video>
    </amp-story-grid-layer>

    <amp-video id="vid3">...</amp-video>
</amp-story-page>

gmajoulet
gmajoulet reviewed Jan 27, 2022
View reviewed changes
gmajoulet
gmajoulet reviewed Jan 28, 2022
View reviewed changes
Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

Can you write unit tests for this feature?
https://github.com/ampproject/amphtml/blob/main/extensions/amp-video/0.1/test/test-video-cache.js

I'd love to see at least:

  • correctly adds sources from inline config
  • doesn't send the XHR request if inline config
  • sends XHR request if non first video or non first page
  • sends XHR request if inline config (not provided|fails to parse|doesn't have the right data)
  • etc

@coreymasanto coreymasanto self-assigned this Feb 1, 2022
gmajoulet
gmajoulet reviewed Feb 10, 2022
View reviewed changes
@coreymasanto coreymasanto enabled auto-merge (squash) February 11, 2022 18:23
@coreymasanto coreymasanto merged commit c8ea23b into ampproject:main Feb 14, 2022
@coreymasanto coreymasanto deleted the inlinedVideo branch February 14, 2022 18:23
@processprocess processprocess mentioned this pull request Feb 14, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

Assignees

@coreymasanto coreymasanto

Labels
PR use: In Beta / Experimental PR use: In LTS PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Story video] Use inlined video cache responses
3 participants
@coreymasanto @gmajoulet @ampprojectbot