Bug 60379

Created attachment 92597 [details] Patch

Attachment 92597 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/track/CueParserPrivate.h:49: More than one command on the same line [whitespace/newline] [4] Source/WebCore/html/TextTrackPrivate.h:42: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

@annacc and I are beginning to upstream our work on <track>! We have coded a functional demo privately on our computers, but because the code is too large to review all at once we will be uploading pieces of it in a series of patches. This patch simply adds some of the core header components. Here are some links that may provide more information and context: * Complete code for demo (not to be committed): https://bugs.webkit.org/show_bug.cgi?id=60312 * Class diagram: http://bit.ly/mMdPB9 * Design doc (a bit out of date): http://bit.ly/lXS37N

Comment on attachment 92597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92597&action=review I am not an official reviewer but here goes some thoughts. > Source/WebCore/WebCore.gypi:3002 > + 'html/LoadableTextTrack.cpp', > + 'html/LoadableTextTrack.h', > + 'html/LoadableTextTrackImpl.cpp', > + 'html/LoadableTextTrackImpl.h', Is this code chromium specific? I could not conclude from the information that I found in this bug. If it is not then you will need to add this files to all the build systems. We have many of them! :) > Source/WebCore/html/LoadableTextTrack.h:42 > + static PassRefPtr<LoadableTextTrack> create(ScriptExecutionContext* context, const String& kind, const String& label, const String& language, bool isDefault) Boolean parameters reduce readability. Would it not be possible to use an enumeration? > Source/WebCore/html/LoadableTextTrack.h:51 > + LoadableTextTrack(ScriptExecutionContext*, const String& kind, const String& label, const String& language, bool isDefault); The same here. > Source/WebCore/html/LoadableTextTrackImpl.h:46 > + static PassOwnPtr<LoadableTextTrackImpl> create(ScriptExecutionContext* context, const String& kind, const String& label, const String& language, bool isDefault) And here. > Source/WebCore/html/LoadableTextTrackImpl.h:68 > + LoadableTextTrackImpl(ScriptExecutionContext*, const String& kind, const String& label, const String& language, bool isDefault); Ditto. > Source/WebCore/html/TextTrackCue.cpp:37 > +TextTrackCue::TextTrackCue(ScriptExecutionContext* context, const String& id, const double start, const double end, const String& content, const String& settings, const bool pauseOnExit) Ditto. > Source/WebCore/html/TextTrackCue.h:43 > + static PassRefPtr<TextTrackCue> create(ScriptExecutionContext* context, const String& id, double start, double end, const String& content, const String& settings, bool pauseOnExit) Same thing. > Source/WebCore/html/TextTrackCue.h:72 > + TextTrackCue(ScriptExecutionContext*, const String& id, double start, double end, const String& content, const String& settings, bool pauseOnExit); Same.

Comment on attachment 92597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92597&action=review >> Source/WebCore/WebCore.gypi:3002 >> + 'html/LoadableTextTrackImpl.h', > > Is this code chromium specific? I could not conclude from the information that I found in this bug. > If it is not then you will need to add this files to all the build systems. We have many of them! :) Track is not chromium-specific. I'll chat with annacc to determine which other files I will update! >> Source/WebCore/html/LoadableTextTrack.h:42 >> + static PassRefPtr<LoadableTextTrack> create(ScriptExecutionContext* context, const String& kind, const String& label, const String& language, bool isDefault) > > Boolean parameters reduce readability. Would it not be possible to use an enumeration? Sure, we can use enums instead. Can you give me an example of the typical naming convention for bools-turned-to-enums? Would it be enum Default {IS_DEFAULT, IS_NOT_DEFAULT}? >> Source/WebCore/html/TextTrackCue.cpp:37 >> +TextTrackCue::TextTrackCue(ScriptExecutionContext* context, const String& id, const double start, const double end, const String& content, const String& settings, const bool pauseOnExit) > > Ditto. Oops, also these primitive parameters are const for some reason. Don't think I did that anywhere else. I'll change that as well!

Comment on attachment 92597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92597&action=review I know it is a pain to split big patches, but there are a lot of basic errors here so I think it would be useful for your initial patches to be quite a bit smaller so you can integrate comments into subsequent patches before posting them. > Source/WebCore/html/CueIndex.cpp:40 > +CueSet CueSet::difference(const CueSet& other) const > +{ > + return CueSet(); > +} Is this implementation temporary? If so, please add a FIXME comment. Ditto for the rest of the methods in this file. > Source/WebCore/html/CueIndex.h:47 > + void add(TextTrackCue*); > + bool contains(TextTrackCue*) const; > + void remove(TextTrackCue*); Can you use constant references instead of mutable pointers? > Source/WebCore/html/LoadableTextTrack.cpp:45 > +void LoadableTextTrack::load(const String& url) > +{ > +} FIXME? > Source/WebCore/html/LoadableTextTrackImpl.cpp:41 > +LoadableTextTrackImpl::LoadableTextTrackImpl(ScriptExecutionContext* context, const String& kind, const String& label, const String& language, bool isDefault) > + : m_kind(kind) > + , m_label(label) > + , m_language(language) > +{ > +} Don't you need to store "default"? > Source/WebCore/html/LoadableTextTrackImpl.cpp:74 > +void LoadableTextTrackImpl::setMode(TextTrack::Mode mode) > +{ > +} "m_mode = mode"? > Source/WebCore/html/LoadableTextTrackImpl.cpp:96 > +TextTrackCueList* LoadableTextTrackImpl::cues() const > +{ > + return 0; > +} > + > +TextTrackCueList* LoadableTextTrackImpl::activeCues() const > +{ > + return 0; > +} > + > +void LoadableTextTrackImpl::popNewestCues(Vector<TextTrackCue*>& output) > +{ > +} > + > +void LoadableTextTrackImpl::load(const String& url) > +{ > +} > + > +void LoadableTextTrackImpl::newCuesLoaded() > +{ > +} FIXMEs? > Source/WebCore/html/MutableTextTrack.cpp:46 > +void MutableTextTrack::addCue(TextTrackCue* cue) > +{ > +} > +void MutableTextTrack::removeCue(TextTrackCue* cue) > +{ > +} FIXMEs. We usually put a blank line between methods. > Source/WebCore/html/MutableTextTrack.h:46 > + void addCue(TextTrackCue*); > + void removeCue(TextTrackCue*); Is it necessary to pass a mutable pointer here, could you use "const TextTrackCue&" instead? > Source/WebCore/html/MutableTextTrackImpl.cpp:55 > +void MutableTextTrackImpl::addCue(TextTrackCue* cue) > +{ > +} > + > +void MutableTextTrackImpl::removeCue(TextTrackCue* cue) > +{ > +} FIXME. > Source/WebCore/html/MutableTextTrackImpl.cpp:84 > +void MutableTextTrackImpl::setMode(TextTrack::Mode mode) > +{ > +} Should this set m_mode? > Source/WebCore/html/MutableTextTrackImpl.cpp:102 > +TextTrackCueList* MutableTextTrackImpl::cues() const > +{ > + return 0; > +} > + > +TextTrackCueList* MutableTextTrackImpl::activeCues() const > +{ > + return 0; > +} > + > +void MutableTextTrackImpl::newCuesLoaded() > +{ > +} > + > +void MutableTextTrackImpl::popNewestCues(Vector<TextTrackCue*>& output) > +{ > +} FIXME. > Source/WebCore/html/TextTrack.cpp:44 > +TextTrack::TextTrack(ScriptExecutionContext* context) > +{ > +} Is "context" not needed? If not, why the argument? If it is just not being used yet, add a comment and an UNUSED_PARAM() so it doesn't break the compile. > Source/WebCore/html/TextTrack.cpp:53 > +String TextTrack::kind() const > +{ > + return m_private->kind(); > +} Can m_private never be NULL? Ditto for the rest of the methods that use it. > Source/WebCore/html/TextTrack.cpp:77 > +void TextTrack::setMode(unsigned short mode, ExceptionCode& ec) > +{ > +} Does this method not call through to m_private? Named but unused parameters will break the build. > Source/WebCore/html/TextTrack.cpp:83 > +TextTrackCueList* TextTrack::cues() const > +{ > + return 0; > + > +} Blank line not needed. Does this method not call through to m_private? > Source/WebCore/html/TextTrack.cpp:88 > +TextTrackCueList* TextTrack::activeCues() const > +{ > + return 0; > +} Does this method not call through to m_private? > Source/WebCore/html/TextTrack.h:71 > + TextTrackCueList* cues() const; > + TextTrackCueList* activeCues() const; I am not sure of ownership of these return values or who will use them, but is it correct to return naked pointers? >>> Source/WebCore/html/TextTrackCue.cpp:37 >>> +TextTrackCue::TextTrackCue(ScriptExecutionContext* context, const String& id, const double start, const double end, const String& content, const String& settings, const bool pauseOnExit) >> >> Ditto. > > Oops, also these primitive parameters are const for some reason. Don't think I did that anywhere else. I'll change that as well! "context" is unused, should it be? > Source/WebCore/html/TextTrackCue.cpp:58 > +void TextTrackCue::setTrack(TextTrack* track) > +{ > +} "m_track = track"? > Source/WebCore/html/TextTrackCue.cpp:83 > +String TextTrackCue::direction() const > +{ > + return ""; > +} FIXME here and for the rest of the unimplemented methods in this file. > Source/WebCore/html/TextTrackCue.h:63 > + String direction() const; > + bool snapToLines() const; > + long linePosition() const; > + long textPosition() const; > + long size() const; > + String alignment() const; These aren't in the W3C spec, they all WebVTT specific. Don't they belong in a derived class? > Source/WebCore/html/TextTrackCueList.cpp:53 > +TextTrackCueList::TextTrackCueList(ScriptExecutionContext* context) > +{ > +} > + > +TextTrackCue* TextTrackCueList::getCueById(const String& id) const > +{ > + return 0; > +} > + > +void TextTrackCueList::append(Vector<PassRefPtr<TextTrackCue> >& newCues) > +{ > +} > + > +void TextTrackCueList::append(const PassRefPtr<TextTrackCue>& cue) > +{ > +} > + > +void TextTrackCueList::remove(const PassRefPtr<TextTrackCue>& cue) > +{ > +} Add FIXMEs. This won't compile without UNUSED_PARAMs everywhere. >> Source/WebCore/html/TextTrackPrivate.h:42 >> + WTF_MAKE_NONCOPYABLE(TextTrackPrivateInterface); WTF_MAKE_FAST_ALLOCATED; > > More than one command on the same line [whitespace/newline] [4] What she said. > Source/WebCore/loader/CueLoader.h:43 > + virtual void takeNewCuesFrom(CueLoader*) = 0; I am not wild about the name "takeNewCuesFrom" - take them where, and from what? > Source/WebCore/loader/CueLoader.h:55 > + virtual void popNewestCues(Vector<TextTrackCue*>& outCues) = 0; "pop" doesn't seem right, doesn't this method "get" the newest cues? The "out" prefix isn't necessary. > Source/WebCore/platform/track/CueParser.cpp:57 > +void CueParser::load(ScriptExecutionContext* context, const String& url) > +{ > +} > + > +void CueParser::didReceiveData(const char* data, int len) > +{ > +} > + > +void CueParser::depleteParsedCues(Vector<PassRefPtr<TextTrackCue> >& outputCues) > +{ > +} FIXME + UNUSED_PARAM > Source/WebCore/platform/track/CueParser.h:54 > + void didReceiveData(const char* data, int dataLength); Parameter names are not necessary. > Source/WebCore/platform/track/CueParser.h:57 > + void depleteParsedCues(Vector<PassRefPtr<TextTrackCue> >& outputCues); Ditto. > Source/WebCore/platform/track/CueParserPrivate.h:54 > + enum Format { WebVTT, XML }; I don't think we plan to support cues in generic XML, so this name isn't correct. Actually we probably don't need an enum at all until someone starts working on a new format. > Source/WebCore/platform/track/CueParserPrivate.h:57 > + virtual void parseBytes(const char* bytes, int length) = 0; Parameter names not necessary. > Source/WebCore/platform/track/CueParserPrivate.h:60 > + // Transfers ownership of last parsed cues to caller. > + virtual void depleteParsedCues(Vector<PassRefPtr<TextTrackCue> >& outputCues) = 0; "deplete" doesn't seem right, like "popNewestCues" doesn't this method "get" the newest cues? The "output" prefix isn't necessary.

Comment on attachment 92597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92597&action=review > Source/WebCore/html/LoadableTextTrackImpl.h:72 > + String m_language; Many of these private vars can go in TextTrackPrivateInterface and getter methods for kind(), label(), language() can be defined there (reducing redundancy). > Source/WebCore/html/MutableTextTrackImpl.h:73 > + String m_language; Ditto for private vars moved into TextTrackPrivateInterface (same as in LoadableTextTrackImpl)

Created attachment 96650 [details] Patch

Comment on attachment 92597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92597&action=review >>> Source/WebCore/WebCore.gypi:3002 >>> + 'html/LoadableTextTrackImpl.h', >> >> Is this code chromium specific? I could not conclude from the information that I found in this bug. >> If it is not then you will need to add this files to all the build systems. We have many of them! :) > > Track is not chromium-specific. I'll chat with annacc to determine which other files I will update! I think I've added all new files to existing build systems, but would certainly appreciate a double-check :). >> Source/WebCore/html/CueIndex.cpp:40 >> +} > > Is this implementation temporary? If so, please add a FIXME comment. > > Ditto for the rest of the methods in this file. Done. >> Source/WebCore/html/CueIndex.h:47 >> + void remove(TextTrackCue*); > > Can you use constant references instead of mutable pointers? Yes, I think you are right, TextTrackCueList will have ownership of the TextTrackCues, so const ref makes the most sense here. >> Source/WebCore/html/LoadableTextTrack.cpp:45 >> +} > > FIXME? Done.

Attachment 96650 [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/platform/track/CueParserPrivate.h:49: More than one command on the same line [whitespace/newline] [4] Source/WebCore/html/TextTrackPrivate.h:41: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 96650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96650&action=review r=me with the minor cleanup suggested. > Source/WebCore/ChangeLog:28 > + (WebCore::CueSet::difference): > + (WebCore::CueSet::unionSet): > + (WebCore::CueSet::add): > + (WebCore::CueSet::contains): > + (WebCore::CueSet::remove): > + (WebCore::CueSet::isEmpty): > + (WebCore::CueSet::size): There is no need to include the names of all of the new methods, just listing the files is enough (as long as there are no comments). > Source/WebCore/html/CueIndex.cpp:40 > + // FIXME(annacc/vrk): Implement. All FIXMEs should have bug numbers rather than irc nicks, here and throughout the rest of the patch. > Source/WebCore/html/CueIndex.cpp:54 > +void CueSet::add(const TextTrackCue& cue) > +{ > + // FIXME(annacc/vrk): Implement. > + UNUSED_PARAM(cue); > +} Because "cue" can never be used, you can leave out the parameter name instead of using UNUSED_PARAM here and throughout the rest of the patch. > Source/WebCore/html/CueIndex.h:44 > + CueSet unionSet(const CueSet& other) const; You don't have "Set" or "Cue" in the other method names, so this might be better as just "union"? > Source/WebCore/html/CueIndex.h:61 > + // Returns set of cues visible at a time in seconds. > + CueSet getVisibleCues(double time) const; I don't think "get" is necessary, maybe "visibleCuesAtTime()"? This will also allow you to leave out the parameter name here. > Source/WebCore/html/MutableTextTrackImpl.cpp:65 > +String MutableTextTrackImpl::kind() const > +{ > + return m_kind; > +} MutableTextTrackImpl and LoadableTextTrackImpl have almost exactly the same right now. Is it worth factoring the common code into a shared base class? > Source/WebCore/html/TextTrack.cpp:50 > + virtual String kind() const { return ""; } > + virtual String label() const { return ""; } > + virtual String language() const { return ""; } You can use emptyString() instead of "". > Source/WebCore/html/TextTrack.cpp:101 > +void TextTrack::setMode(unsigned short mode) > +{ > + m_private->setMode(static_cast<Mode>(mode)); > +} It doesn't look like the spec says what to do if the mode is set to an invalid value, but we should at least clamp to SHOWING, and it may be worth logging an error message as well. > Source/WebCore/html/TextTrackCue.cpp:85 > +String TextTrackCue::getCueAsSource() > +{ > + // FIXME(annacc/vrk): Implement. > + return ""; > +} Use emptyString() instead of "". > Source/WebCore/html/TextTrackCue.h:74 > + bool m_pauseOnExit; > + > + bool m_isActive; > + TextTrack* m_track; Putting all of the bools at the end of the class can allow the compiler to make the structure slightly smaller. > Source/WebCore/loader/CueLoader.h:55 > + // Transfers ownership of currently loaded cues into outCues. > + virtual void fetchNewestCues(Vector<TextTrackCue*>& cues) = 0; Your comment now has the wrong parameter name.

Created attachment 97653 [details] Patch

Comment on attachment 96650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96650&action=review >> Source/WebCore/ChangeLog:28 >> + (WebCore::CueSet::size): > > There is no need to include the names of all of the new methods, just listing the files is enough (as long as there are no comments). Done. >> Source/WebCore/html/CueIndex.cpp:40 >> + // FIXME(annacc/vrk): Implement. > > All FIXMEs should have bug numbers rather than irc nicks, here and throughout the rest of the patch. Done. >> Source/WebCore/html/CueIndex.cpp:54 >> +} > > Because "cue" can never be used, you can leave out the parameter name instead of using UNUSED_PARAM here and throughout the rest of the patch. Done. >> Source/WebCore/html/CueIndex.h:44 >> + CueSet unionSet(const CueSet& other) const; > > You don't have "Set" or "Cue" in the other method names, so this might be better as just "union"? "union" is unfortunately a C keyword. >> Source/WebCore/html/CueIndex.h:61 >> + CueSet getVisibleCues(double time) const; > > I don't think "get" is necessary, maybe "visibleCuesAtTime()"? This will also allow you to leave out the parameter name here. Done. >> Source/WebCore/html/MutableTextTrackImpl.cpp:65 >> +} > > MutableTextTrackImpl and LoadableTextTrackImpl have almost exactly the same right now. Is it worth factoring the common code into a shared base class? I totally agree! Done. >> Source/WebCore/html/TextTrack.cpp:50 >> + virtual String language() const { return ""; } > > You can use emptyString() instead of "". Done. >> Source/WebCore/html/TextTrack.cpp:101 >> +} > > It doesn't look like the spec says what to do if the mode is set to an invalid value, but we should at least clamp to SHOWING, and it may be worth logging an error message as well. Done. I followed 4.8.10.12.5 Text track API: "...On setting, if the new value is not either 0, 1, or 2, the user agent must throw an INVALID_ACCESS_ERR exception." >> Source/WebCore/html/TextTrackCue.h:74 >> + TextTrack* m_track; > > Putting all of the bools at the end of the class can allow the compiler to make the structure slightly smaller. Neat. Done. >> Source/WebCore/loader/CueLoader.h:55 >> + virtual void fetchNewestCues(Vector<TextTrackCue*>& cues) = 0; > > Your comment now has the wrong parameter name. Oops, thanks.

Comment on attachment 97653 [details] Patch Thanks!

Created attachment 97667 [details] Patch

Adding an updated patch because previous one wouldn't apply. No changes other than update.

Attachment 97667 [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/platform/track/CueParserPrivate.h:49: More than one command on the same line [whitespace/newline] [4] Source/WebCore/html/TextTrackPrivate.h:41: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #17) > Attachment 97667 [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/platform/track/CueParserPrivate.h:49: More than one command on the same line [whitespace/newline] [4] > Source/WebCore/html/TextTrackPrivate.h:41: More than one command on the same line [whitespace/newline] [4] > Total errors found: 2 in 29 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Do these need to be fixed?

(In reply to comment #18) > (In reply to comment #17) > > Attachment 97667 [details] [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/platform/track/CueParserPrivate.h:49: More than one command on the same line [whitespace/newline] [4] > > Source/WebCore/html/TextTrackPrivate.h:41: More than one command on the same line [whitespace/newline] [4] > > Total errors found: 2 in 29 files > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > Do these need to be fixed? I don't think so. They are the: WTF_MAKE_NONCOPYABLE(CueParserPrivateInterface); WTF_MAKE_FAST_ALLOCATED; and I consistently see those together on the same line in other WebKit files.

(In reply to comment #19) > (In reply to comment #18) > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > > > Do these need to be fixed? > > I don't think so. They are the: > WTF_MAKE_NONCOPYABLE(CueParserPrivateInterface); WTF_MAKE_FAST_ALLOCATED; > and I consistently see those together on the same line in other WebKit files. Why don't you file a bug against check-webkit-style?

Comment on attachment 97667 [details] Patch Marking r+ but not cq+ because it is Friday night and this patch touches all of the build projects, which is often a recipe for build breakage, and I don't see annac on irc. Feel free to ping me if you are around and I missed you, or you can have someone else cq+ it if I am not around when you are.

Comment on attachment 97667 [details] Patch Clearing flags on attachment: 97667 Committed r89186: <http://trac.webkit.org/changeset/89186>

All reviewed patches have been landed. Closing bug.