Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13968007:
Create WebInbandTextTrack and WebInbandTextTrackClient (Closed)
|
Created:
7 years, 8 months ago by Matthew Heaney (Chromium) Modified:
7 years, 7 months ago CC:
blink-reviews, jamesr, abarth_chromum.org, feature-media-reviews_chromium.org, Stephen Chennney, jeez, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionCreate WebInbandTextTrack and WebInbandTextTrackClient
interfaces so that WebMediaPlayer implementations can add
and remove inband text tracks from the HTMLMediaElement.
This change just provide the plumbing between the
WebMediaPlayerClient interface and the existing
InbandTextTrack infrastructure in core/platform/graphics
that HTMLMediaElement uses for inband text tracks.
BUG=230708
TEST=None. No user visible functionality has changed.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149801
Patch Set 1 #
Total comments: 62
Patch Set 2 : incorporate aaron's comments #
Total comments: 25
Patch Set 3 : incorporate aaron's comments #Patch Set 4 : rebase #
Total comments: 13
Patch Set 5 : incorporate aaron's comments #Patch Set 6 : rebase #Patch Set 7 : incorporate aaron's comments #
Total comments: 22
Patch Set 8 : rebase #Patch Set 9 : incorporated reviewers' comments #Patch Set 10 : rebase #Patch Set 11 : fixed overriding declaration #Patch Set 12 : rebase #Messages
Total messages: 19 (0 generated)
Here are some initial comments. https://codereview.chromium.org/13968007/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/13968007/diff/1/PRESUBMIT.py#newcode45 PRESUBMIT.py:45: license_header=license_header)) Why is this needed? It should probably be in a separate patch. https://codereview.chromium.org/13968007/diff/1/Source/WebCore/html/track/Inb... File Source/WebCore/html/track/InbandTextTrack.cpp (right): https://codereview.chromium.org/13968007/diff/1/Source/WebCore/html/track/Inb... Source/WebCore/html/track/InbandTextTrack.cpp:29: #include "GenericCueData.h" nit: This should be in the group below in alphabetical order. https://codereview.chromium.org/13968007/diff/1/Source/WebCore/platform/graph... File Source/WebCore/platform/graphics/GenericCueData.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebCore/platform/graph... Source/WebCore/platform/graphics/GenericCueData.h:86: m_baseFontSize = baseFontSize; } nit: Don't line wrap here. Blink style doesn't enforce the 80-column rule for cases like this. https://codereview.chromium.org/13968007/diff/1/Source/WebCore/platform/graph... Source/WebCore/platform/graphics/GenericCueData.h:89: void setRelativeFontSize(double relativeFontSize) { ditto https://codereview.chromium.org/13968007/diff/1/Source/WebCore/platform/graph... File Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebCore/platform/graph... Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:43: double start, nit: Remove extra trailing whitespace https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... File Source/WebKit/chromium/public/WebInbandTextTrack.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebInbandTextTrack.h:46: enum Mode { Disabled, Hidden, Showing }; nit: Move this enum and the Kind one up above function declarations. Add Mode & Kind prefixes to the enum values. This is another area where Blink style differs from Chromium. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebInbandTextTrack.h:47: virtual void setMode(Mode mode) = 0; nit: s/mode// . Blink doesn't specify parameter names when they don't provide any extra info. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebInbandTextTrack.h:51: // Can we use TextKind in pipeline_status.h? Yes this enum is necessary. Blink cannot have any dependencies on Chromium code. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... File Source/WebKit/chromium/public/WebMediaPlayerClient.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebMediaPlayerClient.h:34: #include "WebInbandTextTrack.h" nit: remove and forward declare. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebMediaPlayerClient.h:87: // TODO(matthewjheaney): resolve nature of pointer Remove comment? I'm assuming you have resolved the ownership relationship. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebMediaPlayerClient.h:88: virtual void addTextTrack(WebInbandTextTrack* text_track) = 0; nit: remove parameter name. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:41: WebInbandTextTrack* track) nit: Blink style doesn't require 80-col wrap for something like this. It prefers a single line for something like this. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:43: { nit: Add ASSERT(!m_track); here and remove all m_track NULL checks below since m_track never gets updated after construction. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:56: // TODO(matthewjheaney): not sure what to do here yet At a minimum you need to store the pointer so client() returns what was passed to setClient(). You should pass a WebInbandTextTrackClientImpl() that wraps client here if client is non-NULL and pass 0 otherwise. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:70: return NULL; Update to return m_client. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:85: const WebInbandTextTrack::Mode m = mode_map[mode]; nit: Use a static_cast and there is no need for a local variable. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:105: const T::Mode result = mode_map[m_track->mode()]; ditto https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:127: const T::Kind result = kind_map[m_track->kind()]; ditto https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h:46: explicit InbandTextTrackPrivateImpl(WebInbandTextTrack* track); nit: remove param name. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h:50: void setClient(WebCore::InbandTextTrackPrivateClient* client); nit: ditto https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h:53: virtual void setMode(Mode mode); nit: ditto https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h:66: WebInbandTextTrack* m_track; Shouldn't this be OwnPtr<WebInbandTextTrack> so that the WebInbandTextTrack gets destroyed when this object gets destroyed? https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp:12: : client_(client) { nit: { on its own line and add ASSERT(client); https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp:20: const WebString& settings) { nit: { on its own line. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp:21: // TODO(matthewjheaney): resolve value of first param looks like a pointer to the InbandTextTrackPrivateImpl that created this object needs to be passed here. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h:45: WebCore::InbandTextTrackPrivateClient* client); nit: No need for line wrap or variable name here. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h:48: //virtual ~WebInbandTextTrackPrivateClientImpl() {} Yes. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h:57: WebCore::InbandTextTrackPrivateClient* client_; nit: s/client_/m_client/ https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:304: // TODO(matthewjheaney): resolve parameter type and lifetime issues Remove comment since I believe InbandTextTrackPrivateImpl should be managing ownership here. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:92: virtual void addTextTrack(WebInbandTextTrack* text_track); nit: remove param name.
Incorporated Aaron's comments. https://codereview.chromium.org/13968007/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/13968007/diff/1/PRESUBMIT.py#newcode45 PRESUBMIT.py:45: license_header=license_header)) I had a stale depot_tools. Fixed. https://codereview.chromium.org/13968007/diff/1/Source/WebCore/html/track/Inb... File Source/WebCore/html/track/InbandTextTrack.cpp (right): https://codereview.chromium.org/13968007/diff/1/Source/WebCore/html/track/Inb... Source/WebCore/html/track/InbandTextTrack.cpp:29: #include "GenericCueData.h" On 2013/04/17 00:13:56, acolwell wrote: > nit: This should be in the group below in alphabetical order. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebCore/platform/graph... File Source/WebCore/platform/graphics/GenericCueData.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebCore/platform/graph... Source/WebCore/platform/graphics/GenericCueData.h:86: m_baseFontSize = baseFontSize; } On 2013/04/17 00:13:56, acolwell wrote: > nit: Don't line wrap here. Blink style doesn't enforce the 80-column rule for > cases like this. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebCore/platform/graph... Source/WebCore/platform/graphics/GenericCueData.h:89: void setRelativeFontSize(double relativeFontSize) { On 2013/04/17 00:13:56, acolwell wrote: > ditto Done. https://codereview.chromium.org/13968007/diff/1/Source/WebCore/platform/graph... File Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebCore/platform/graph... Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:43: double start, On 2013/04/17 00:13:56, acolwell wrote: > nit: Remove extra trailing whitespace Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... File Source/WebKit/chromium/public/WebInbandTextTrack.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebInbandTextTrack.h:46: enum Mode { Disabled, Hidden, Showing }; On 2013/04/17 00:13:56, acolwell wrote: > nit: Move this enum and the Kind one up above function declarations. > > Add Mode & Kind prefixes to the enum values. This is another area where Blink > style differs from Chromium. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebInbandTextTrack.h:47: virtual void setMode(Mode mode) = 0; On 2013/04/17 00:13:56, acolwell wrote: > nit: s/mode// . Blink doesn't specify parameter names when they don't provide > any extra info. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebInbandTextTrack.h:51: // Can we use TextKind in pipeline_status.h? On 2013/04/17 00:13:56, acolwell wrote: > Yes this enum is necessary. Blink cannot have any dependencies on Chromium code. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... File Source/WebKit/chromium/public/WebMediaPlayerClient.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebMediaPlayerClient.h:34: #include "WebInbandTextTrack.h" On 2013/04/17 00:13:56, acolwell wrote: > nit: remove and forward declare. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebMediaPlayerClient.h:87: // TODO(matthewjheaney): resolve nature of pointer On 2013/04/17 00:13:56, acolwell wrote: > Remove comment? I'm assuming you have resolved the ownership relationship. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/public... Source/WebKit/chromium/public/WebMediaPlayerClient.h:88: virtual void addTextTrack(WebInbandTextTrack* text_track) = 0; On 2013/04/17 00:13:56, acolwell wrote: > nit: remove parameter name. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:41: WebInbandTextTrack* track) On 2013/04/17 00:13:56, acolwell wrote: > nit: Blink style doesn't require 80-col wrap for something like this. It prefers > a single line for something like this. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:43: { On 2013/04/17 00:13:56, acolwell wrote: > nit: Add ASSERT(!m_track); here and remove all m_track NULL checks below since > m_track never gets updated after construction. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:56: // TODO(matthewjheaney): not sure what to do here yet On 2013/04/17 00:13:56, acolwell wrote: > At a minimum you need to store the pointer so client() returns what was passed > to setClient(). > > You should pass a WebInbandTextTrackClientImpl() that wraps client here if > client is non-NULL and pass 0 otherwise. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:56: // TODO(matthewjheaney): not sure what to do here yet On 2013/04/17 00:13:56, acolwell wrote: > At a minimum you need to store the pointer so client() returns what was passed > to setClient(). > > You should pass a WebInbandTextTrackClientImpl() that wraps client here if > client is non-NULL and pass 0 otherwise. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:70: return NULL; On 2013/04/17 00:13:56, acolwell wrote: > Update to return m_client. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:85: const WebInbandTextTrack::Mode m = mode_map[mode]; On 2013/04/17 00:13:56, acolwell wrote: > nit: Use a static_cast and there is no need for a local variable. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:85: const WebInbandTextTrack::Mode m = mode_map[mode]; On 2013/04/17 00:13:56, acolwell wrote: > nit: Use a static_cast and there is no need for a local variable. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:105: const T::Mode result = mode_map[m_track->mode()]; On 2013/04/17 00:13:56, acolwell wrote: > ditto Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:127: const T::Kind result = kind_map[m_track->kind()]; On 2013/04/17 00:13:56, acolwell wrote: > ditto Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h:46: explicit InbandTextTrackPrivateImpl(WebInbandTextTrack* track); On 2013/04/17 00:13:56, acolwell wrote: > nit: remove param name. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h:50: void setClient(WebCore::InbandTextTrackPrivateClient* client); On 2013/04/17 00:13:56, acolwell wrote: > nit: ditto Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h:53: virtual void setMode(Mode mode); On 2013/04/17 00:13:56, acolwell wrote: > nit: ditto Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/In... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h:66: WebInbandTextTrack* m_track; On 2013/04/17 00:13:56, acolwell wrote: > Shouldn't this be OwnPtr<WebInbandTextTrack> so that the WebInbandTextTrack gets > destroyed when this object gets destroyed? Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp:12: : client_(client) { On 2013/04/17 00:13:56, acolwell wrote: > nit: { on its own line and add ASSERT(client); Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp:20: const WebString& settings) { On 2013/04/17 00:13:56, acolwell wrote: > nit: { on its own line. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp:21: // TODO(matthewjheaney): resolve value of first param On 2013/04/17 00:13:56, acolwell wrote: > looks like a pointer to the InbandTextTrackPrivateImpl that created this object > needs to be passed here. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h:45: WebCore::InbandTextTrackPrivateClient* client); On 2013/04/17 00:13:56, acolwell wrote: > nit: No need for line wrap or variable name here. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h:48: //virtual ~WebInbandTextTrackPrivateClientImpl() {} On 2013/04/17 00:13:56, acolwell wrote: > Yes. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h:57: WebCore::InbandTextTrackPrivateClient* client_; On 2013/04/17 00:13:56, acolwell wrote: > nit: s/client_/m_client/ Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:304: // TODO(matthewjheaney): resolve parameter type and lifetime issues On 2013/04/17 00:13:56, acolwell wrote: > Remove comment since I believe InbandTextTrackPrivateImpl should be managing > ownership here. Done. https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h (right): https://codereview.chromium.org/13968007/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:92: virtual void addTextTrack(WebInbandTextTrack* text_track); On 2013/04/17 00:13:56, acolwell wrote: > nit: remove param name. Done.
Looking pretty good. Just a few more comments. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebInbandTextTrack.h (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrack.h:41: enum Kind { KindSubtitles, KindCaptions, KindDescriptions, KindChapters, KindMetadata, KindNone }; Please add appropriate entries in Source/WebKit/chromium/src/AssertMatchingEnums.cpp to make sure these stay in sync w/ the InbandTextTrackPrivate enums. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebInbandTextTrackClient.h (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrackClient.h:48: virtual WebCore::InbandTextTrackPrivateClient* client() = 0; Remove. Interfaces in the Source/WebKit/chromium/public/ are not allowed to expose WebCore interfaces. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrackClient.h:50: virtual void addWebVTTCue(WebCore::InbandTextTrackPrivate*, ditto. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebMediaPlayerClient.h (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebMediaPlayerClient.h:88: Might as well add the removeTextTrack() method as well. You'll need it for MSE support. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:31: #define WEBKIT_IMPLEMENTATION 1 Something doesn't look right here. You should not need this #define, the ExportMacros.h below. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:38: #include "third_party/WebKit/Source/Platform/chromium/public/WebString.h" This should just be #include <public/WebString.h> and above <wtf/PassOwnPtr.h> https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:52: void InbandTextTrackPrivateImpl::setClient(WebCore::InbandTextTrackPrivateClient* client) I think things could be simplified if you just use the storage in the base class for 'client' and have this object implement WebInbandTextTrackClient instead of creating a separate WebInbandTextTrackClientImpl object. This function would then become the following { InbandTextTrackPrivate::setClient(client); m_track->setClient(this); } and you'd only have to add the following to this class. void addWebVTTCue( double start, double end, const WebString& id, const WebString& content, const WebString& settings) { client()->addWebVTTCue(this, start, end, id, content, settings); } https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:62: if (WebInbandTextTrackClient* client = m_track->client()) If you use the base class storage like I mention above, you don't even need to implement this method. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h:67: WTF::OwnPtr<WebInbandTextTrack> m_track; nit: You shouldn't need WTF here. Something is wrong if the compiler is making you put it here. See MediaSourcePrivateImpl.h & SourceBufferPrivateImpl.h for similar examples. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:305: WTF::adoptRef(new InbandTextTrackPrivateImpl(text_track))); nit: You should not need the WTF. If so then something is wrong with the includes. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:93: Add removeTextTrack() as well. https://codereview.chromium.org/13968007/diff/12001/Source/core/platform/grap... File Source/core/platform/graphics/InbandTextTrackPrivateClient.h (left): https://codereview.chromium.org/13968007/diff/12001/Source/core/platform/grap... Source/core/platform/graphics/InbandTextTrackPrivateClient.h:37: class GenericCueData { Why are these changes in Source/core/ necessary? Is this just a hold over from migrating your changes from WebKit? If so, then please revert them if they aren't needed.
https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebInbandTextTrack.h (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrack.h:41: enum Kind { KindSubtitles, KindCaptions, KindDescriptions, KindChapters, KindMetadata, KindNone }; On 2013/04/19 17:45:49, acolwell wrote: > Please add appropriate entries in > Source/WebKit/chromium/src/AssertMatchingEnums.cpp to make sure these stay in > sync w/ the InbandTextTrackPrivate enums. Done. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebInbandTextTrackClient.h (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrackClient.h:48: virtual WebCore::InbandTextTrackPrivateClient* client() = 0; I removed this function. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrackClient.h:50: virtual void addWebVTTCue(WebCore::InbandTextTrackPrivate*, Do you know what type does here instead? https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebMediaPlayerClient.h (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebMediaPlayerClient.h:88: On 2013/04/19 17:45:49, acolwell wrote: > Might as well add the removeTextTrack() method as well. You'll need it for MSE > support. Done. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:31: #define WEBKIT_IMPLEMENTATION 1 I left the <wft/ExportMacros.h> in. Wouldn't compile without it. Perhaps there's some alternative. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:38: #include "third_party/WebKit/Source/Platform/chromium/public/WebString.h" On 2013/04/19 17:45:49, acolwell wrote: > This should just be #include <public/WebString.h> and above <wtf/PassOwnPtr.h> Done. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:38: #include "third_party/WebKit/Source/Platform/chromium/public/WebString.h" On 2013/04/19 17:45:49, acolwell wrote: > This should just be #include <public/WebString.h> and above <wtf/PassOwnPtr.h> Done. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:52: void InbandTextTrackPrivateImpl::setClient(WebCore::InbandTextTrackPrivateClient* client) On 2013/04/19 17:45:49, acolwell wrote: > I think things could be simplified if you just use the storage in the base class > for 'client' and have this object implement WebInbandTextTrackClient instead of > creating a separate WebInbandTextTrackClientImpl object. > > This function would then become the following > { > InbandTextTrackPrivate::setClient(client); > m_track->setClient(this); > } > > and you'd only have to add the following to this class. > void addWebVTTCue( > double start, > double end, > const WebString& id, > const WebString& content, > const WebString& settings) > { > client()->addWebVTTCue(this, start, end, id, content, settings); > } Done. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:62: if (WebInbandTextTrackClient* client = m_track->client()) On 2013/04/19 17:45:49, acolwell wrote: > If you use the base class storage like I mention above, you don't even need to > implement this method. Done. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.h:67: WTF::OwnPtr<WebInbandTextTrack> m_track; On 2013/04/19 17:45:49, acolwell wrote: > nit: You shouldn't need WTF here. Something is wrong if the compiler is making > you put it here. See MediaSourcePrivateImpl.h & SourceBufferPrivateImpl.h for > similar examples. Done. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:305: WTF::adoptRef(new InbandTextTrackPrivateImpl(text_track))); On 2013/04/19 17:45:49, acolwell wrote: > nit: You should not need the WTF. If so then something is wrong with the > includes. Done. https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h (right): https://codereview.chromium.org/13968007/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:93: On 2013/04/19 17:45:49, acolwell wrote: > Add removeTextTrack() as well. Done. https://codereview.chromium.org/13968007/diff/12001/Source/core/platform/grap... File Source/core/platform/graphics/InbandTextTrackPrivateClient.h (left): https://codereview.chromium.org/13968007/diff/12001/Source/core/platform/grap... Source/core/platform/graphics/InbandTextTrackPrivateClient.h:37: class GenericCueData { On 2013/04/19 17:45:49, acolwell wrote: > Why are these changes in Source/core/ necessary? Is this just a hold over from > migrating your changes from WebKit? If so, then please revert them if they > aren't needed. Done.
https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebInbandTextTrackClient.h (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrackClient.h:34: // TODO(matthewjheaney): still need these? No. You shouldn't need these since they aren't referenced in this file. https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrackClient.h:51: virtual void addWebVTTCue(void*, Why do you need this parameter? https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:98: void*, Looks like this parameter isn't needed since you don't use it here. https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp:11: WebInbandTextTrackClientImpl::WebInbandTextTrackClientImpl( This file doesn't appear to be needed anymore. https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h:38: class WebInbandTextTrackClientImpl : public WebInbandTextTrackClient { This class/file doesn't look like it is used or needed anymore. https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:311: adoptRef(new InbandTextTrackPrivateImpl(text_track))); This doesn't look right to me. Are you sure this works? It looks like the client pointer won't be set properly on this new InbandTextTrackPrivateImpl() instance so the code in HTMLMediaElement::mediaPlayerDidRemoveTrack() won't behave properly. It seems like you may need some way to map the WebInbandTextTrack instance to the InbandTextTrackPrivateImpl instance created in addTextTrack(). You could probably use the a casting trick like HTMLMediaElement::mediaPlayerDidRemoveTrack() does since you know the WebInbandTextTrackClient passed to a WebInbandTextTrack is a InbandTextTrackPrivateImpl.
https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebInbandTextTrackClient.h (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrackClient.h:34: // TODO(matthewjheaney): still need these? On 2013/04/23 06:34:18, acolwell wrote: > No. You shouldn't need these since they aren't referenced in this file. Done. https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrackClient.h:51: virtual void addWebVTTCue(void*, parameter removed https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:98: void*, parameter removed https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.cpp:11: WebInbandTextTrackClientImpl::WebInbandTextTrackClientImpl( On 2013/04/23 06:34:18, acolwell wrote: > This file doesn't appear to be needed anymore. Done. https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebInbandTextTrackClientImpl.h:38: class WebInbandTextTrackClientImpl : public WebInbandTextTrackClient { On 2013/04/23 06:34:18, acolwell wrote: > This class/file doesn't look like it is used or needed anymore. Done. https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:311: adoptRef(new InbandTextTrackPrivateImpl(text_track))); For now I just added a // TODO. I'll take a look at what you're describing here.
Completed removeTextTrack, per Aaron's implementation idea. https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/24001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:311: adoptRef(new InbandTextTrackPrivateImpl(text_track))); On 2013/04/24 04:32:18, Matthew Heaney (Chromium) wrote: > For now I just added a // TODO. I'll take a look at what you're describing > here. Done.
lgtm % nits. abarth@ : Could you take quick look to make sure I didn't miss anything and also provide an actual Blink Reviewer LGTM. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:310: InbandTextTrackPrivateImpl* const text_track_private = static_cast<InbandTextTrackPrivateImpl*>(client); nit: Add a comment here similar to the one in HTMLMediaElement::mediaPlayerDidRemoveTrack() stating why this case is safe. Also the two temporaries variables here don't really buy you anything here. Just do the cast and text_track->client() call inside the removeTextTrack() call.
The description of the CL is very short. Can you please explain what this CL does and why the project should accept it? If you're implementing a web standard, please also include a link to the spec.
I'll be happy to look at the details of the CL once you've updated the description.
On 2013/04/30 18:11:34, abarth wrote: > I'll be happy to look at the details of the CL once you've updated the > description. Oops. Sorry about that. I've updated the description.
A bunch of nits below. I didn't fully understand the lifetime requirements of the client. Would you be willing to add a comment to the setClient function explaining what the lifetime requirements are? Otherwise, LGTM. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebInbandTextTrack.h (right): https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrack.h:40: public: Please use Blink style (e.g., four-space indent) https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrack.h:42: enum Mode { ModeDisabled, ModeHidden, ModeShowing }; Please put the enum values on separate lines so that it's easier to review changes to them. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrack.h:59: virtual int textTrackIndex() const = 0; Should the index be size_t ? https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebInbandTextTrackClient.h (right): https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrackClient.h:40: virtual ~WebInbandTextTrackClient() {} Please use Blinks style (e.g., four-space indent) https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp (right): https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:36: #include <wtf/PassOwnPtr.h> Please alphabetize your includes. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:41: : m_track(WTF::adoptPtr(track)) No need for the WTF:: prefix. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:103: client()->addWebVTTCue(this, start, end, id, content, settings); Four-space indent pls. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:301: void WebMediaPlayerClientImpl::addTextTrack(WebInbandTextTrack* text_track) text_track -> textTrack (Blink uses camelCase variable names) https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:304: adoptRef(new InbandTextTrackPrivateImpl(text_track))); There is no 80 col limit. You can just put these on the same line. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:311: m_mediaPlayer->removeTextTrack(text_track_private); camelCase pls
Incorporated comments. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebInbandTextTrack.h (right): https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrack.h:40: public: On 2013/04/30 18:36:16, abarth wrote: > Please use Blink style (e.g., four-space indent) Done. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrack.h:42: enum Mode { ModeDisabled, ModeHidden, ModeShowing }; On 2013/04/30 18:36:16, abarth wrote: > Please put the enum values on separate lines so that it's easier to review > changes to them. Done. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrack.h:59: virtual int textTrackIndex() const = 0; Good question. Should we also rename this method, to inbandTextTrack ? https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... File Source/WebKit/chromium/public/WebInbandTextTrackClient.h (right): https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/pu... Source/WebKit/chromium/public/WebInbandTextTrackClient.h:40: virtual ~WebInbandTextTrackClient() {} On 2013/04/30 18:36:16, abarth wrote: > Please use Blinks style (e.g., four-space indent) Done. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp (right): https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:36: #include <wtf/PassOwnPtr.h> On 2013/04/30 18:36:16, abarth wrote: > Please alphabetize your includes. Done. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:41: : m_track(WTF::adoptPtr(track)) On 2013/04/30 18:36:16, abarth wrote: > No need for the WTF:: prefix. Done. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/InbandTextTrackPrivateImpl.cpp:103: client()->addWebVTTCue(this, start, end, id, content, settings); On 2013/04/30 18:36:16, abarth wrote: > Four-space indent pls. Done. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:301: void WebMediaPlayerClientImpl::addTextTrack(WebInbandTextTrack* text_track) On 2013/04/30 18:36:16, abarth wrote: > text_track -> textTrack (Blink uses camelCase variable names) Done. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:304: adoptRef(new InbandTextTrackPrivateImpl(text_track))); On 2013/04/30 18:36:16, abarth wrote: > There is no 80 col limit. You can just put these on the same line. Done. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:310: InbandTextTrackPrivateImpl* const text_track_private = static_cast<InbandTextTrackPrivateImpl*>(client); On 2013/04/30 18:00:46, acolwell wrote: > nit: Add a comment here similar to the one in > HTMLMediaElement::mediaPlayerDidRemoveTrack() stating why this case is safe. > > Also the two temporaries variables here don't really buy you anything here. Just > do the cast and text_track->client() call inside the removeTextTrack() call. Done. https://codereview.chromium.org/13968007/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:311: m_mediaPlayer->removeTextTrack(text_track_private); On 2013/04/30 18:36:16, abarth wrote: > camelCase pls Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/13968007/5...
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_layout_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/13968007/5...
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/13968007/8...
Message was sent while issue was closed.
Change committed as 149801 |
