♻️✅ Simplify rotate-to-fullscreen/integration test combo#15480
♻️✅ Simplify rotate-to-fullscreen/integration test combo#15480alanorozco merged 9 commits intoampproject:masterfrom
Conversation
| }); | ||
| removeElement(animation); | ||
| removeElement(mask); | ||
| video.signals().signal(VideoServiceSignals.USER_INTERACTED); |
There was a problem hiding this comment.
I think this method needs to listen to USER_INTERACTED instead of firing it (and having click handlers above fire it).
This event will be helpful for #9055 where we want AMP actions such as play(), pause() to be considered user-interactions which then we can easily do by triggering this event ( so maybe move the event definition to where other events like PLAYING, etc.. are in video-interface?)
There was a problem hiding this comment.
Welp, the signal is permanent, so the state would be the same regardless as to where the actual event is handled. I propose removing userInteractedWithAutoplay in favor of the signal (signals.get(USER_INTERACTED)), which should reconcile #9055.
There was a problem hiding this comment.
I see what you mean about triggering on other actions, though. Changes to this flow coming in a separate PR :)
There was a problem hiding this comment.
I guess I am suggesting that we want to call this method ( that hides he EQ icon, etc.. ) when the signal is received rather than having it fire it. To fix #9055, we would need registerCommonActions_ to also fire this signal but then this method won't run unless it listens to it (we would want this method to run for #9055)
There was a problem hiding this comment.
Yup, on the same page!
src/service/video-manager-impl.js
Outdated
| if (this.isPlaying_ | ||
| && this.playCalledByAutoplay_ | ||
| && !this.userInteractedWithAutoPlay_) { | ||
| && !this.userInteractedWithAutoPlay()) { |
There was a problem hiding this comment.
Rename to userInteracted() ?
| }); | ||
| removeElement(animation); | ||
| removeElement(mask); | ||
| video.signals().signal(VideoServiceSignals.USER_INTERACTED); |
There was a problem hiding this comment.
I guess I am suggesting that we want to call this method ( that hides he EQ icon, etc.. ) when the signal is received rather than having it fire it. To fix #9055, we would need registerCommonActions_ to also fire this signal but then this method won't run unless it listens to it (we would want this method to run for #9055)
3b75547 to
13a94f7
Compare
|
Test is failing on Travis and I haven't been able to repro locally. Skipping for now, #15511 |
Partial for #9055.
This was originally only an integration test PR, but after much trying to deflake, I realized I had to change strategies:
Simplifies by no longer keeping track of visible videos. This was an unnecessary optimization, since:
Sorts videos on all relevant signals (user interacted, play, etc.). Scroll shouldn't be a necessary signal, as manual playback is more relevant. Since the amount of videos should be small, and this happens only on relevant events, a linear search to filter before sort should be ok.
Adds integration test.
Cleans up the sort method somewhat.
Overall this is less code, has fewer failure points and is better tested.