Bug 85050

Summary: TextTrack modes are strings
Product: WebKit Reporter: Victor Carbune <vcarbune>
Component: MediaAssignee: Anna Cavender <annacc>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, annacc, edaena, eric.carlson, feature-media-reviews, ojan, silviapf, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 85035    
Attachments:
Description Flags
Patch
none
Patch for landing none

Victor Carbune
Reported 2012-04-27 04:40:10 PDT
The IDL for TextTrack has a TextTrackMode string enumeration as type for the mode attribute (instead of numeric values)
Attachments
Patch (36.61 KB, patch)
2012-08-24 16:14 PDT, Anna Cavender 		no flags
DetailsFormatted DiffDiff
Patch for landing (38.96 KB, patch)
2012-08-27 11:19 PDT, Anna Cavender 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Edaena Salinas
Comment 1 2012-06-15 09:49:04 PDT
How is string enumeration beneficial for representing the TextTrack mode?
Eric Carlson
Comment 2 2012-06-15 10:44:33 PDT
(In reply to comment #1) > How is string enumeration beneficial for representing the TextTrack mode? That is a question for the working group. Here is the relevant spec text: http://dev.w3.org/html5/spec/media-elements.html#texttrackmode
Anna Cavender
Comment 3 2012-08-24 16:14:18 PDT
Created attachment 160519 [details] Patch
Eric Carlson
Comment 4 2012-08-25 12:41:45 PDT
Comment on attachment 160519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160519&action=review > Source/WebCore/ChangeLog:18 > + * html/HTMLMediaElement.cpp: > + (WebCore::HTMLMediaElement::loadInternal): > + (WebCore::HTMLMediaElement::textTrackModeChanged): > + (WebCore::HTMLMediaElement::textTrackKindChanged): > + (WebCore::HTMLMediaElement::addTextTrack): I think it is helpful to have a brief explanation of what changed in each method.
Eric Carlson
Comment 5 2012-08-25 12:43:03 PDT
Comment on attachment 160519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160519&action=review > Source/WebCore/html/track/TextTrack.h:73 > + static const String& disabledKeyword() > + { > + DEFINE_STATIC_LOCAL(const String, open, ("disabled")); > + return open; > + } > + > + static const String& hiddenKeyword() > + { > + DEFINE_STATIC_LOCAL(const String, closed, ("hidden")); > + return closed; > + } > + > + static const String& showingKeyword() > + { > + DEFINE_STATIC_LOCAL(const String, ended, ("showing")); > + return ended; > + } The implementation of methods like these is usually in the .cpp file.
Adam Barth
Comment 6 2012-08-25 14:56:53 PDT
Comment on attachment 160519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160519&action=review > Source/WebCore/html/track/TextTrack.h:60 > + DEFINE_STATIC_LOCAL(const String, open, ("disabled")); > + return open; We should use ConstructFromLiteral rather than this pattern now: http://trac.webkit.org/wiki/EfficientStrings
Eric Carlson
Comment 7 2012-08-25 16:02:41 PDT
(In reply to comment #6) > (From update of attachment 160519 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160519&action=review > > > Source/WebCore/html/track/TextTrack.h:60 > > + DEFINE_STATIC_LOCAL(const String, open, ("disabled")); > > + return open; > > We should use ConstructFromLiteral rather than this pattern now: > > http://trac.webkit.org/wiki/EfficientStrings I wonder if DEFINE_STATIC_LOCAL can be rewritten to use ConstructFromLiteral? That could be a big win.
Adam Barth
Comment 8 2012-08-25 16:13:56 PDT
In the case of String, that seems likely. In the general case, I'm less sure.
Anna Cavender
Comment 9 2012-08-27 11:18:59 PDT
Comment on attachment 160519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160519&action=review >> Source/WebCore/ChangeLog:18 >> + (WebCore::HTMLMediaElement::addTextTrack): > > I think it is helpful to have a brief explanation of what changed in each method. Done. >>> Source/WebCore/html/track/TextTrack.h:60 >>> + return open; >> >> We should use ConstructFromLiteral rather than this pattern now: >> >> http://trac.webkit.org/wiki/EfficientStrings > > I wonder if DEFINE_STATIC_LOCAL can be rewritten to use ConstructFromLiteral? That could be a big win. I will add ConstructFromLiteral. Thanks. >> Source/WebCore/html/track/TextTrack.h:73 >> + } > > The implementation of methods like these is usually in the .cpp file. Done.
Anna Cavender
Comment 10 2012-08-27 11:19:48 PDT
Created attachment 160752 [details] Patch for landing
WebKit Review Bot
Comment 11 2012-08-27 13:12:18 PDT
Comment on attachment 160752 [details] Patch for landing Clearing flags on attachment: 160752 Committed r126786: <http://trac.webkit.org/changeset/126786>
WebKit Review Bot
Comment 12 2012-08-27 13:12:22 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.