Summary: | In-band text tracks infrastructure | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | feature-media-reviews, gtk-ews, gyuyoung.kim, ojan, pnormand, rakuco, silviapf, webkit-bug-importer, webkit.review.bot, xan.lopez | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 102708 | ||||||||
Attachments: |
|
Description
Eric Carlson
2012-11-20 12:30:07 PST
Created attachment 175269 [details]
Proposed patch
Attachment 175269 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/html/track/InbandTextTrack.h:50: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.h:60: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.cpp:30: You should add a blank line after implementation file's own header. [build/include_order] [4]
Source/WebCore/html/track/InbandTextTrack.cpp:32: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/html/track/InbandTextTrack.cpp:40: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Total errors found: 5 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 175269 [details] Proposed patch Attachment 175269 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14916711 Nice! Looks generally good to me.
> Source/WebCore/html/track/InbandTextTrackPrivate.h:57
> + virtual Kind kind() const { return none; }
Spec says that the default kind is "subtitles":
"The missing value default is the subtitles state.".
You might want to return "subtitles" here instead of "none".
Created attachment 175276 [details]
Updated patch
Updated for style problems and Silvia's suggestion. I don't understand the GTK build issue so this may not build yet.
(In reply to comment #4) > Nice! Looks generally good to me. > > > Source/WebCore/html/track/InbandTextTrackPrivate.h:57 > > + virtual Kind kind() const { return none; } > > Spec says that the default kind is "subtitles": > "The missing value default is the subtitles state.". > > You might want to return "subtitles" here instead of "none". Good suggestion, thanks! Looks like the GTK failure was unrelated. This patch is ready for review. Comment on attachment 175276 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=175276&action=review Looks pretty good! I just have a few nits to consider before landing. > Source/WebCore/ChangeLog:42 > + (TextTrackList::invalidateTrackIndexesAfterTrack): New, invalide the cached track indexes of Typo: invalide > Source/WebCore/html/track/InbandTextTrack.cpp:40 > +InbandTextTrack::InbandTextTrack(ScriptExecutionContext* context, TextTrackClient* client, PassRefPtr<InbandTextTrackPrivate> playerPrivate) Nit: playerPrivate made me think of MediaPlayerPrivate here, maybe a different name can be used, something like trackPrivate? > Source/WebCore/html/track/InbandTextTrack.cpp:94 > +void InbandTextTrack::addCue(InbandTextTrackPrivate*, double start, double end, const String& id, const String& content, const String& settings) I don't see this method used yet, unless I missed it :) Maybe the first argument is not necessary at all? We already have the InbandTextTrackPrivate pointer as a private member. (In reply to comment #8) > (From update of attachment 175276 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175276&action=review > > Looks pretty good! I just have a few nits to consider before landing. > > > Source/WebCore/ChangeLog:42 > > + (TextTrackList::invalidateTrackIndexesAfterTrack): New, invalide the cached track indexes of > > Typo: invalide > Fixed. > > Source/WebCore/html/track/InbandTextTrack.cpp:40 > > +InbandTextTrack::InbandTextTrack(ScriptExecutionContext* context, TextTrackClient* client, PassRefPtr<InbandTextTrackPrivate> playerPrivate) > > Nit: playerPrivate made me think of MediaPlayerPrivate here, maybe a different name can be used, something like trackPrivate? > Good suggestion, thanks. Fixed. > > Source/WebCore/html/track/InbandTextTrack.cpp:94 > > +void InbandTextTrack::addCue(InbandTextTrackPrivate*, double start, double end, const String& id, const String& content, const String& settings) > > I don't see this method used yet, unless I missed it :) Maybe the first argument is not necessary at all? We already have the InbandTextTrackPrivate pointer as a private member. I added an ASSEERT that it is the same as the private member. Thanks for the review, committed as r135410. |