Closed Bug 1114888 Opened 5 years ago Closed 5 years ago

simplify HTMLMediaElement::AddRemoveSelfReference to remove consideration of mDelayingLoadEvent

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(1 file, 1 obsolete file)

The only time when the delaying-the-load-event flag is set but networkState is
not NETWORK_LOADING is in step 4 of the resource selection algorithm while awaiting a stable state [1]. During this time, a runnable holds a reference to the media element.  Therefore keeping a self-reference while NETWORK_LOADING makes it unnecessary to consider the delaying-the-load-event flag in the self-reference logic.

[1] http://www.w3.org/TR/2014/REC-html5-20141028/embedded-content-0.html#concept-media-load-algorithm
Not requesting review yet, until I confirm this is consistent with a solution
for bug 1114885.
Comment on attachment 8542411 [details] [diff] [review]
no need to consider delaying load event flag in AddRemoveSelfReference r?

Review of attachment 8542411 [details] [diff] [review]:
-----------------------------------------------------------------

The patch seems tricky to me. Although the invariant is still kept that a self-reference is kept as long as delaying-the-load-event flag is true, it is obscure to notice the self-reference is kept by the runnable. What is the patch trying to fix?
(In reply to JW Wang [:jwwang] from comment #3)
> What is the patch trying to fix?

Just simplifying, because there are fewer things to track, before making things more complicated in bug 1114885.  It is common for runnables to hold references.
Comment on attachment 8542411 [details] [diff] [review]
no need to consider delaying load event flag in AddRemoveSelfReference r?

Review of attachment 8542411 [details] [diff] [review]:
-----------------------------------------------------------------

I think the change to move AddRemoveSelfReference into ChangeNetworkState is a good one and we should take that. However, I agree with JW that we should keep the check for mDelayLoadEvent, that's clearer.
Attachment #8542411 - Flags: review?(roc) → review-
Attachment #8540546 - Attachment is obsolete: true
ChangeDelayLoadStatus is usually called after ChangeNetworkState.  Moving AddRemoveSelfReference() from ChangeDelayLoadStatus() to ChangeNetworkState() and retaining mDelayingLoadEvent logic in AddRemoveSelfReference() would not give the desired result due to testing mDelayingLoadEvent before it is modified.

I expect the ordering of ChangeDelayLoadStatus() and ChangeNetworkState() could usually be changed, but that wasn't the purpose of this bug.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
(In reply to Karl Tomlinson (:karlt) from comment #0)
> The only time when the delaying-the-load-event flag is set but networkState
> is not NETWORK_LOADING is in step 4 of the resource selection algorithm while
> awaiting a stable state [1]. During this time, a runnable holds a reference
> to the media element.

FWIW HTMLMediaElement.cpp doesn't follow this algorithm.  QueueSelectResourceTask() doesn't set mDelayingLoadEvent until SelectResource() runs in stable state, but the runnable keeps the HTMLMediaElement alive.
I don't know whether or not the load event could be queued before the stable state.
You need to log in before you can comment on or make changes to this bug.