YouTube improvements: enter 360 VR without having to first click YouTube's Fullscreen button, set automatic 360 Projection on YouTube 360 videos (fixes issue #1215 and issue #1056)

[cvan]

YouTube improvements:

• enter 360 VR without having to first click YouTube's Fullscreen button (for issue Automatically append &mozVideoProjection=360_auto to YouTube 360 URLs #1215)
• set automatic 360 Projection on YouTube 360 videos (for issue Automatically append &mozVideoProjection=360_auto to YouTube 360 URLs #1215)

---

• fixes issues Automatically append &mozVideoProjection=360_auto to YouTube 360 URLs #1215 and Hide YouTube's "Full screen is unavailable" disclaimer when Fullscreen 360 Media Controls opened #1056

[philip-lamb]

@cvan This is a fix for a 1.2 headline feature, so suggest you open this PR against release-1.2 rather than master.

[cvan]

Hmm, I must have misunderstood the release planning. Is it possible for us to continue to create a release by tagging master when the tag is ready?

That is the process I am familiar with at Mozilla, and to my knowledge other similarly sized and larger-sized projects at Mozilla use the same process.

[philip-lamb]

No, tags for release will be on the release branch. This allows non-release development to continue on master while bugs in a release candidate are addressed. I'm manually tagging release candidates on the release branch. So if you're intending your work as a fix to a release candidate, then base for your patch should be the head of the release branch.

[cvan]

This allows non-release development to continue on master while bugs in a release candidate are addressed.

Code on master should be adequately tested so it does not regress functionality or introduce new bugs. If a branch does, the PR can be reverted. Seems like a win-win to keep master clean and up to date.

[philip-lamb]

I think you've misunderstood me; committing patches on the release branch doesn't change anything about how we use feature branches to bring work onto master. By all means if a patch made to fix a release is useful on master too, you could cherry-pick it over to a second branch off master; just this process doesn't require that. Please ping me (slack or email) if you'd like me to clarify any of the process.

[cvan]

thanks! after talking offline, I think better understand now. if it's not already, we should document these engineering processes on the Wiki (and in CONTRIBUTING.md where appropriate).

[cvan]

note to reviewers: please do not merge yet. I have a couple more test scenarios (around YouTube's SPA [single-page app] navigation) I want to evaluate (and patch) before landing. will be ready for final re-review in a few hours. thanks!

[daoshengmu]

It seems like I can't auto enter 360 VR mode in Focus Plus. @cvan does it work properly in Oculus Go?

[cvan]

It seems like I can't auto enter 360 VR mode in Focus Plus. @cvan does it work properly in Oculus Go?

it worked before I just pushed again to the branch. the new fixes for YouTube's SPA navigation, I'm still working on. it's a WIP. I regressed it in the last commit, d8e6a01. will be done in an hour or so. will ping you then. thanks!

if you want to preview the functionality before, you can check out eee0654. thanks, @daoshengmu 👍

[philip-lamb]

if you want to preview the functionality before, you can check out eee0654. thanks, @daoshengmu 👍

I tested against eee0654 If I click on the video rect itself, it transitions to fullscreen nicely. I did notice that if I use the transport controls, it doesn't transition.

[cvan]

thanks, good catch, looking into it

[cvan]

I tested against eee0654 If I click on the video rect itself, it transitions to fullscreen nicely. I did notice that if I use the transport controls, it doesn't transition.

I think I understand what you're talking about: the flicker when the Transport Controls appear on click during a 360 video playback? if so, that's a pre-existing issue with the Android UI rendering code and also occurs on master/release-1.2. could you file a separate issue for that?

[cvan]

@daoshengmu: @philip-lamb: @MortimerGoro: fixed; could you retest and rereview? thanks!

[MortimerGoro]

Nice work! I tested with https://www.youtube.com/watch?v=-xNN-bJQ4vI & https://www.youtube.com/watch?v=sPyAQQklc1s and it worked correctly on both.

But it didn't work for me when entering youtube from a google search (check this video: https://www.dropbox.com/s/piw9tvpa431bmge/youtube.mp4.zip?dl=0). It seems something related to m.youtube.com, the redirection didn't work for me

[daoshengmu]

LGTM. I have tested it on Focus Plus, it works well.

[philip-lamb]

Tested on both Go and Focus. It is a great improvement in the previous user flow for youtube 360 entry, and we have positive reviews with some edge cases that I think are worthy of follow-ups (the m.youtube.com verson, and the youtube transport controls) but I'll land this now and we can build incrementally on this work.

[cvan]

thanks for reviewing and merging!

I'm in the code right now to address the m.youtube.com URLs. filed issue #1229.