Bug 144499 - Postpone caption style sheet creation
Summary: Postpone caption style sheet creation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-05-01 13:58 PDT by Eric Carlson
Modified: 2015-05-01 18:16 PDT (History)
1 user (show)

See Also:


Attachments
Proposed patch. (9.42 KB, patch)
2015-05-01 17:19 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2015-05-01 13:58:08 PDT
CaptionUserPreferences::updateCaptionStyleSheetOverride() is called when a <video> element is created. This is a fairly expensive operation and the style sheet it generates is only needed when the <video> has captions, so it should be postponed as long as possible.
Comment 1 Eric Carlson 2015-05-01 13:58:34 PDT
rdar://problem/20691225
Comment 2 Eric Carlson 2015-05-01 17:19:51 PDT
Created attachment 252203 [details]
Proposed patch.
Comment 3 Simon Fraser (smfr) 2015-05-01 17:32:56 PDT
Comment on attachment 252203 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=252203&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:480
> +    if (m_haveRegisteredForCaptionPreferenceChanges)
> +        document.unregisterForCaptionPreferencesChangedCallbacks(this);

Does this need to set m_haveRegisteredForCaptionPreferenceChanges=false? Or maybe my suggested rename would clarify.

> Source/WebCore/html/HTMLMediaElement.h:888
> +    bool m_haveRegisteredForCaptionPreferenceChanges { false };

Would this be better as m_hasHadTextTrack?
Comment 4 Eric Carlson 2015-05-01 17:41:28 PDT
Comment on attachment 252203 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=252203&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:480
>> +        document.unregisterForCaptionPreferencesChangedCallbacks(this);
> 
> Does this need to set m_haveRegisteredForCaptionPreferenceChanges=false? Or maybe my suggested rename would clarify.

No, because we want to re-register if the element is inserted back into the DOM (like the layout test does). I will rename it.
Comment 5 Simon Fraser (smfr) 2015-05-01 17:55:01 PDT
Comment on attachment 252203 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=252203&action=review

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:200
> +        // Generating and registering the caption stylesheet can be expensive and this method is called indirectly when the parser creates an audio or

Maybe early return on if (m_listeningForPreferenceChanges) before this whole block.
Comment 6 Eric Carlson 2015-05-01 18:16:01 PDT
Committed r183705: https://trac.webkit.org/r183705