Do not pause the Player when element is moved to a different document [chromium/src : main]
Javier Fernandez (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Message from Javier Fernandez![]( "Open in Gerrit")
- Dale Curtis
- Javier Fernandez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
paused_ = false;Play/pause state is pretty complicated. I'm not sure it's sufficient to just set this back to false. It probably at least needs `SetPaused(false)` and more likely `PlayInternal`. Can you investigate a bit more here?
Related details
- Javier Fernandez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
paused_ = false;Play/pause state is pretty complicated. I'm not sure it's sufficient to just set this back to false. It probably at least needs `SetPaused(false)` and more likely `PlayInternal`. Can you investigate a bit more here?
I see, I've been perhaps too focused on the specific test failing. I may need a more complete test case to understand all the implications of the play / pause logic. Any idea is welcome.
I'm going to investigate it further in any case. Thanks for the review.
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Message from Javier Fernandez![]( "Open in Gerrit")
- Dale Curtis
- Javier Fernandez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Javier Fernandez added 2 comments![]( "Open in Gerrit")
Hi dalecurtis@
I did an analysis of the paused / playing transition logic and a proposal to restore the "playing" state in a more solid way.
paused_ = false;Javier FernandezPlay/pause state is pretty complicated. I'm not sure it's sufficient to just set this back to false. It probably at least needs `SetPaused(false)` and more likely `PlayInternal`. Can you investigate a bit more here?
I see, I've been perhaps too focused on the specific test failing. I may need a more complete test case to understand all the implications of the play / pause logic. Any idea is welcome.
I'm going to investigate it further in any case. Thanks for the review.
It seems that SetPaused is the only, by design, way of modifying the paused_ flag. This is because SetPaused, triggers some PseudoState changes that causes the required pseudo-style invalidations.
Another consequence of the current approach is that playing_ is never restored, after being modified by ResetMediaPlayerAndMediasource method.
There is another problem related to the play promises, rejected as part of the InvokeLoadAlgorithm (Step 4.6 in the spec). Modifying the paused_ flag manually doesn't re-create those promises.
We can use PlayInternal as a way to initiate the paused->unpaused transition, which internally calls to SetPause(false) if needed. It calls ResetMediaPlayerAndMediaSource so that the promises are recreated, and UpdatePlayStte() to indicate the WebMediaPlayer to play.
The proposed approach has the drawback that it generates a "play" event, and this goes against the spec, which states that a doc-move should be transparent and shouldn't generate any event. However, invoking the load algorithm is already generating some events, like 'emptied' and 'loadstart', so I think we are already assuming that compromise.
A more spec-compliant approach would be to implement a new helper to restore the pause stated after attaching to a new frame, so that it does SetPaused(false) + SetShowPosterFlag(false) + UpdatePlayState(), but without scheduling play / playing events. However, I see this a more complex approach and more likely to cause regressions.
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 3 comments![]( "Open in Gerrit")
paused_ = false;Javier FernandezPlay/pause state is pretty complicated. I'm not sure it's sufficient to just set this back to false. It probably at least needs `SetPaused(false)` and more likely `PlayInternal`. Can you investigate a bit more here?
Javier FernandezI see, I've been perhaps too focused on the specific test failing. I may need a more complete test case to understand all the implications of the play / pause logic. Any idea is welcome.
I'm going to investigate it further in any case. Thanks for the review.
It seems that SetPaused is the only, by design, way of modifying the paused_ flag. This is because SetPaused, triggers some PseudoState changes that causes the required pseudo-style invalidations.
Another consequence of the current approach is that playing_ is never restored, after being modified by ResetMediaPlayerAndMediasource method.
There is another problem related to the play promises, rejected as part of the InvokeLoadAlgorithm (Step 4.6 in the spec). Modifying the paused_ flag manually doesn't re-create those promises.
We can use PlayInternal as a way to initiate the paused->unpaused transition, which internally calls to SetPause(false) if needed. It calls ResetMediaPlayerAndMediaSource so that the promises are recreated, and UpdatePlayStte() to indicate the WebMediaPlayer to play.
The proposed approach has the drawback that it generates a "play" event, and this goes against the spec, which states that a doc-move should be transparent and shouldn't generate any event. However, invoking the load algorithm is already generating some events, like 'emptied' and 'loadstart', so I think we are already assuming that compromise.
A more spec-compliant approach would be to implement a new helper to restore the pause stated after attaching to a new frame, so that it does SetPaused(false) + SetShowPosterFlag(false) + UpdatePlayState(), but without scheduling play / playing events. However, I see this a more complex approach and more likely to cause regressions.
This seems an okay compromise, but the wpts probably shouldn't test for this since it's not part of the spec? This does result in some additional issues though with play promises that the review agent noticed.
if (was_playing) {As currently implemented, this logic will still result in the rejection of pending play promises with AbortError. InvokeLoadAlgorithm() (at line 721) checks if !paused_ and, if so, triggers RejectPlayPromises(). Since PlayInternal() is called after the algorithm runs, it cannot resolve the promises that were already settled by the rejection logic.
If the intention is to preserve pending promises, you might want to manually set paused_ = true before calling InvokeLoadAlgorithm() to bypass step 4.6, and then let PlayInternal() handle the state restoration. Note that the WPT test doesn't catch this because it waits for the promise to resolve before moving the node.
new_dummy_page_holder->GetDocument().adoptNode(Media(), ASSERT_NO_EXCEPTION);In these tests, you are calling adoptNode() but not actually inserting the node into the new document tree. While this is sufficient for synchronous checks, adoptNode() leaves the node disconnected. This means when the OnRemovedFromDocumentTimer eventually fires, isConnected() will be false and the element will pause. To more accurately simulate a document move and verify the timer behavior, it is better to append the node to the new document's body.
Related details
- Javier Fernandez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Javier Fernandez added 4 comments![]( "Open in Gerrit")
Thanks for the review.
See my comments inline
paused_ = false;Javier FernandezPlay/pause state is pretty complicated. I'm not sure it's sufficient to just set this back to false. It probably at least needs `SetPaused(false)` and more likely `PlayInternal`. Can you investigate a bit more here?
Javier FernandezI see, I've been perhaps too focused on the specific test failing. I may need a more complete test case to understand all the implications of the play / pause logic. Any idea is welcome.
I'm going to investigate it further in any case. Thanks for the review.
Dale CurtisIt seems that SetPaused is the only, by design, way of modifying the paused_ flag. This is because SetPaused, triggers some PseudoState changes that causes the required pseudo-style invalidations.
Another consequence of the current approach is that playing_ is never restored, after being modified by ResetMediaPlayerAndMediasource method.
There is another problem related to the play promises, rejected as part of the InvokeLoadAlgorithm (Step 4.6 in the spec). Modifying the paused_ flag manually doesn't re-create those promises.
We can use PlayInternal as a way to initiate the paused->unpaused transition, which internally calls to SetPause(false) if needed. It calls ResetMediaPlayerAndMediaSource so that the promises are recreated, and UpdatePlayStte() to indicate the WebMediaPlayer to play.
The proposed approach has the drawback that it generates a "play" event, and this goes against the spec, which states that a doc-move should be transparent and shouldn't generate any event. However, invoking the load algorithm is already generating some events, like 'emptied' and 'loadstart', so I think we are already assuming that compromise.
A more spec-compliant approach would be to implement a new helper to restore the pause stated after attaching to a new frame, so that it does SetPaused(false) + SetShowPosterFlag(false) + UpdatePlayState(), but without scheduling play / playing events. However, I see this a more complex approach and more likely to cause regressions.
This seems an okay compromise, but the wpts probably shouldn't test for this since it's not part of the spec? This does result in some additional issues though with play promises that the review agent noticed.
Your concerns about the new WPT to assert non-specified behavior comes from the play-promises and pseudo-class related tests, right ? Maybe it's better remove them and use browser tests instead.
I believe the one about the absence of the 'paused" can be justified with certain part of the spec; I can elaborate on that if needed. Not to mention that there is already a pause-move-to-other-document.html test that verifies precisely that the player is not paused as a consequence of the movement to a different document.
if (was_playing) {As currently implemented, this logic will still result in the rejection of pending play promises with AbortError. InvokeLoadAlgorithm() (at line 721) checks if !paused_ and, if so, triggers RejectPlayPromises(). Since PlayInternal() is called after the algorithm runs, it cannot resolve the promises that were already settled by the rejection logic.
If the intention is to preserve pending promises, you might want to manually set paused_ = true before calling InvokeLoadAlgorithm() to bypass step 4.6, and then let PlayInternal() handle the state restoration. Note that the WPT test doesn't catch this because it waits for the promise to resolve before moving the node.
Before this patch, the SetPaused was the only way to modify the paused_ flag, which I believe is a good idea because it avoid complex issues related to the pause -> play transition, as you pointed out before.
Don't you think that this way of avoiding the promise rejection is a bit hacky ? Not to mention there is a DCHECK in the InvokeLoadAlgorithm [1] to ensure there is no pending promises if paused_ is true.
I know that all this comes from the fact of invoking the load algorithm when the media elements changes it's document, which it's not what the spec states. I understand that it's a convenient implementation detail, though, but I wonder then whether it's worth the effort of testing the issue related to the pending promises.
Maybe it's better to drop both the both the test about the pending promises and the pseudoclases. The only think that worries me is that my change to force pause_ = false didn't cause any regression; that's what motivated me provide some regression tests.
new_dummy_page_holder->GetDocument().adoptNode(Media(), ASSERT_NO_EXCEPTION);In these tests, you are calling adoptNode() but not actually inserting the node into the new document tree. While this is sufficient for synchronous checks, adoptNode() leaves the node disconnected. This means when the OnRemovedFromDocumentTimer eventually fires, isConnected() will be false and the element will pause. To more accurately simulate a document move and verify the timer behavior, it is better to append the node to the new document's body.
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
paused_ = false;Javier FernandezPlay/pause state is pretty complicated. I'm not sure it's sufficient to just set this back to false. It probably at least needs `SetPaused(false)` and more likely `PlayInternal`. Can you investigate a bit more here?
Javier FernandezI see, I've been perhaps too focused on the specific test failing. I may need a more complete test case to understand all the implications of the play / pause logic. Any idea is welcome.
I'm going to investigate it further in any case. Thanks for the review.
Dale CurtisIt seems that SetPaused is the only, by design, way of modifying the paused_ flag. This is because SetPaused, triggers some PseudoState changes that causes the required pseudo-style invalidations.
Another consequence of the current approach is that playing_ is never restored, after being modified by ResetMediaPlayerAndMediasource method.
There is another problem related to the play promises, rejected as part of the InvokeLoadAlgorithm (Step 4.6 in the spec). Modifying the paused_ flag manually doesn't re-create those promises.
We can use PlayInternal as a way to initiate the paused->unpaused transition, which internally calls to SetPause(false) if needed. It calls ResetMediaPlayerAndMediaSource so that the promises are recreated, and UpdatePlayStte() to indicate the WebMediaPlayer to play.
The proposed approach has the drawback that it generates a "play" event, and this goes against the spec, which states that a doc-move should be transparent and shouldn't generate any event. However, invoking the load algorithm is already generating some events, like 'emptied' and 'loadstart', so I think we are already assuming that compromise.
A more spec-compliant approach would be to implement a new helper to restore the pause stated after attaching to a new frame, so that it does SetPaused(false) + SetShowPosterFlag(false) + UpdatePlayState(), but without scheduling play / playing events. However, I see this a more complex approach and more likely to cause regressions.
Javier FernandezThis seems an okay compromise, but the wpts probably shouldn't test for this since it's not part of the spec? This does result in some additional issues though with play promises that the review agent noticed.
Your concerns about the new WPT to assert non-specified behavior comes from the play-promises and pseudo-class related tests, right ? Maybe it's better remove them and use browser tests instead.
I believe the one about the absence of the 'paused" can be justified with certain part of the spec; I can elaborate on that if needed. Not to mention that there is already a pause-move-to-other-document.html test that verifies precisely that the player is not paused as a consequence of the movement to a different document.
You can just have them be wpt_internal tests if we're slightly deviating from the spec. Which I thought we were per your comments about the playing event being surfaced now?
if (was_playing) {Javier FernandezAs currently implemented, this logic will still result in the rejection of pending play promises with AbortError. InvokeLoadAlgorithm() (at line 721) checks if !paused_ and, if so, triggers RejectPlayPromises(). Since PlayInternal() is called after the algorithm runs, it cannot resolve the promises that were already settled by the rejection logic.
If the intention is to preserve pending promises, you might want to manually set paused_ = true before calling InvokeLoadAlgorithm() to bypass step 4.6, and then let PlayInternal() handle the state restoration. Note that the WPT test doesn't catch this because it waits for the promise to resolve before moving the node.
Before this patch, the SetPaused was the only way to modify the paused_ flag, which I believe is a good idea because it avoid complex issues related to the pause -> play transition, as you pointed out before.
Don't you think that this way of avoiding the promise rejection is a bit hacky ? Not to mention there is a DCHECK in the InvokeLoadAlgorithm [1] to ensure there is no pending promises if paused_ is true.
I know that all this comes from the fact of invoking the load algorithm when the media elements changes it's document, which it's not what the spec states. I understand that it's a convenient implementation detail, though, but I wonder then whether it's worth the effort of testing the issue related to the pending promises.
Maybe it's better to drop both the both the test about the pending promises and the pseudoclases. The only think that worries me is that my change to force pause_ = false didn't cause any regression; that's what motivated me provide some regression tests.
Sorry, that comment was from the AI review tool. I should have checked its suggested fix more closely. I only wanted to surface that the play promise state would be incorrect. I don't have a suggested work around.
Related details
- Javier Fernandez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |