Closed Bug 1415461 Opened 2 years ago Closed 2 years ago

Consolidate the life cycle of the channel managed by ChannelSuspendAgent

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/dom/media/ChannelMediaResource.cpp#1042,1056

We should suspend/resume the channel after OnStartRequest() and before OnStopRequest() is fired.

It doesn't make sense to suspend/resume a channel after it is closed. Likewise, it doesn't make much sense either to suspend/resume a channel before OnStartRequest() is received.
Assignee: nobody → jwwang
Blocks: 1369263
Priority: -- → P3
Attachment #8927118 - Flags: review?(bechen)
Comment on attachment 8927118 [details]
Bug 1415461 - consolidate the life cycle of the channel managed by ChannelSuspendAgent.

https://reviewboard.mozilla.org/r/198340/#review203582

So this patch actually change the behavior of "suspend comes before OnStartRequest". Do we have testcases? Or should we add some assert?
Attachment #8927118 - Flags: review?(bechen) → review+
Comment on attachment 8927118 [details]
Bug 1415461 - consolidate the life cycle of the channel managed by ChannelSuspendAgent.

https://reviewboard.mozilla.org/r/198340/#review203582

No, it is just a case we want to avoid as stated in comment 0. Consider the following case:
suspend -> OnChannelRedirect -> OnStartRequest

Without this change, we would suspend the old channel without resuming it again.
Attachment #8927118 - Flags: review?(gsquelart)
Comment on attachment 8927118 [details]
Bug 1415461 - consolidate the life cycle of the channel managed by ChannelSuspendAgent.

https://reviewboard.mozilla.org/r/198340/#review203646

::: dom/media/ChannelMediaResource.cpp:1076
(Diff revision 1)
> -  if (!mIsChannelSuspended && IsSuspended()) {
> +  MOZ_ASSERT(aChannel);
> +  MOZ_ASSERT(!mChannel, "The previous channel not closed.");
> +  MOZ_ASSERT(!mIsChannelSuspended);
> +
> +  mChannel = aChannel;
> +  // Ensure the suspend status of the channel match our suspend count.

'match' -> 'matches'

::: dom/media/ChannelMediaResource.cpp:1092
(Diff revision 1)
> -void
> -ChannelSuspendAgent::NotifyChannelClosing()
> -{
> -  MOZ_ASSERT(NS_IsMainThread());
> -  MOZ_ASSERT(mChannel);
>    // Before close the channel, it need to be resumed to make sure its internal

Since you're modifying nearby code: 'close' -> 'closing', 'need' -> 'needs'
Attachment #8927118 - Flags: review?(gsquelart) → review+
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/056e41a8b982
consolidate the life cycle of the channel managed by ChannelSuspendAgent. r=bechen,gerald
https://hg.mozilla.org/mozilla-central/rev/056e41a8b982
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.