Closed Bug 987076 Opened 6 years ago Closed 6 years ago

Construct HTMLMediaElement::mTextTrackManager lazily

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: roc, Assigned: reyre)

References

Details

Attachments

(2 files, 3 obsolete files)

Right now we construct a TextTrackManager for every HTMLMediaElement ... which goes on to create a bunch of objects of its own. This is unnecessary 99% of the time. Please fix it.
I've removed the TextTrackManager's initialization from the MediaElement's ctor and added a getter that creates it if we don't have it yet. Functions on the MediaElement that are TextTrack specific now use this getter.
Attachment #8395691 - Flags: review?(roc)
I've removed the TextTrackManager's initialization from the MediaElement's ctor and added a getter that creates it if we don't have it yet. The public functions exposed on the MediaElement's IDL now call this getter.
Attachment #8395691 - Attachment is obsolete: true
Attachment #8395691 - Flags: review?(roc)
Attachment #8395708 - Flags: review?(roc)
Comment on attachment 8395708 [details] [diff] [review]
v2: Construct HTMLMediaElement::mTextTrackManager lazily r=roc

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

::: content/html/content/public/HTMLMediaElement.h
@@ +870,5 @@
>    void PopulatePendingTextTrackList();
>  
> +  // Gets a reference to the MediaElement's TextTrackManager. If the
> +  // MediaElement doesn't yet have one then it will create it.
> +  TextTrackManager* CreateOrGetTextTrackManager();

GetOrCreate is the more common ordering, so call it GetOrCreateTextTrackManager.
Attachment #8395708 - Flags: review?(roc) → review+
Thanks for review Robert.

Carrying forward r=roc.
Attachment #8395708 - Attachment is obsolete: true
Forgot to update the commit message to reflect the new function name.

Carrying forward r=roc.
Attachment #8395909 - Attachment is obsolete: true
Try looks green.

We also need to land this in Gecko 29 and 30 for bug 960224, correct Roc?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/85a509905f70
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
RyanVM pointed out that we should probably land this on Beta as well since it's landing in Aurora/B2G28. It should improve desktop as well.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 865407
User impact if declined: Adds an extra 50ms of load time to videos
Testing completed (on m-c, etc.): Completed on the base patch, not backported one.
Risk to taking this patch (and alternatives if risky): Will add 50ms of load time to pages with videos.
String or IDL/UUID changes made by this patch: N/A
Attachment #8397252 - Flags: approval-mozilla-beta?
Rick, I don't see an uplift request for aurora.
Flags: needinfo?(rick.eyre)
[:sylvestre] from comment #11)
> Rick, I don't see an uplift request for aurora.

That work is being done in bug 960224 as it was the original bug for the regression. This bug was to get it into FF31, and 29 now since bug 960224 is only putting it into FF30.(In reply to Sylvestre Ledru
Flags: needinfo?(rick.eyre)
OK. Thanks for the information.

Can you evaluate the risk of approving this patch ?
For example, low, medium, high, thisisgoingtocrashfirefox, etc :)
Flags: needinfo?(rick.eyre)
(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> OK. Thanks for the information.
> 
> Can you evaluate the risk of approving this patch ?
> For example, low, medium, high, thisisgoingtocrashfirefox, etc :)

I would say the risk of approving this patch is low. I'm 99% sure it won't crash Firefox, if it did I would be _very_ surprised. All the entry points into the code that is now being lazy loaded has guards on it and are confined to the MediaElement so even if there was an instance where we hadn't loaded it yet, but it was called, it wouldn't crash.
Flags: needinfo?(rick.eyre)
Comment on attachment 8397252 [details] [diff] [review]
v1: Construct HTMLMediaElement::mTextTrackManager lazily r=roc (back-ported to FF29)

OK. Thanks! Approving then.
Attachment #8397252 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
No worries, thank you! :)
You need to log in before you can comment on or make changes to this bug.