[webkit-reviews] review denied: [Bug 60379] Adding initial interfaces and stubs for track : [Attachment 92597] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu May 12 08:42:54 PDT 2011
Eric Carlson <eric.carlson at apple.com> has denied Victoria Kirst
<vrk at chromium.org>'s request for review:
Bug 60379: Adding initial interfaces and stubs for track
https://bugs.webkit.org/show_bug.cgi?id=60379
Attachment 92597: Patch
https://bugs.webkit.org/attachment.cgi?id=92597&action=review
------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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.
More information about the webkit-reviews
mailing list