Move VTT functionality from TextTrackCue.* to VTTCue.*

Move VTT functionality from TextTrackCue.* to VTTCue.*

BUG=270340
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162550

[acolwell GONE FROM CHROMIUM, 2013-11-20 18:38:44]

https://codereview.chromium.org/77853002/diff/1/Source/core/html/shadow/Media...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/77853002/diff/1/Source/core/html/shadow/Media...
Source/core/html/shadow/MediaControlElements.cpp:738: if (cue->isVTTCue()) {
It feels like this logic should be inside VTTCue. Could we refactor this so the
code here would look something like

cue->display(m_videoDisplaySize.size(), this);

Then we theoretically wouldn't need to modify this code anymore for different
types of cues.

https://codereview.chromium.org/77853002/diff/1/Source/core/html/track/TextTr...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/77853002/diff/1/Source/core/html/track/TextTr...
Source/core/html/track/TextTrackCue.h:96: virtual bool hasDisplayTree() const {
return false; }
Do we really need to make all these virtual? If we don't think they will be
needed by other formats then it seems like we should refactor the code so we
only need a few virtual methods.

https://codereview.chromium.org/77853002/diff/1/Source/core/rendering/RenderT...
File Source/core/rendering/RenderTextTrackCue.h (right):

https://codereview.chromium.org/77853002/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTextTrackCue.h:35: class RenderTextTrackCue : public
RenderBlockFlow {
I think we should just remove this object and have the RenderVTTCue just derive
from RenderBlockFlow. This object isn't really providing any value right now. We
can always add it back if a need presents itself.

[gasubic, 2013-11-21 03:07:37]

https://codereview.chromium.org/77853002/diff/1/Source/core/html/shadow/Media...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/77853002/diff/1/Source/core/html/shadow/Media...
Source/core/html/shadow/MediaControlElements.cpp:738: if (cue->isVTTCue()) {
On 2013/11/20 18:38:45, acolwell wrote:
> It feels like this logic should be inside VTTCue. Could we refactor this so
the
> code here would look something like
> 
> cue->display(m_videoDisplaySize.size(), this);
> 
> Then we theoretically wouldn't need to modify this code anymore for different
> types of cues.

Done

https://codereview.chromium.org/77853002/diff/1/Source/core/html/track/TextTr...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/77853002/diff/1/Source/core/html/track/TextTr...
Source/core/html/track/TextTrackCue.h:96: virtual bool hasDisplayTree() const {
return false; }
On 2013/11/20 18:38:45, acolwell wrote:
> Do we really need to make all these virtual? If we don't think they will be
> needed by other formats then it seems like we should refactor the code so we
> only need a few virtual methods.

Eliminated three: hasDisplayTree, getDisplayTree, element. However, remaining
three (updateDisplayTree, removeDisplayTree,
notifyRegionWhenRemovingDisplayTree) require more work, and would be better done
in a separate CL.

https://codereview.chromium.org/77853002/diff/1/Source/core/rendering/RenderT...
File Source/core/rendering/RenderTextTrackCue.h (right):

https://codereview.chromium.org/77853002/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTextTrackCue.h:35: class RenderTextTrackCue : public
RenderBlockFlow {
On 2013/11/20 18:38:45, acolwell wrote:
> I think we should just remove this object and have the RenderVTTCue just
derive
> from RenderBlockFlow. This object isn't really providing any value right now.
We
> can always add it back if a need presents itself.

Done.

[acolwell GONE FROM CHROMIUM, 2013-11-21 19:21:59]

lgtm % nits.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:585: if (cue->isVTTCue()) {
nit: It seems like this should be handled by a virtual ToString() method on
TextTrackCue/VTTCue as well.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
Source/core/html/track/TextTrackCue.h:96: virtual void updateDisplay(const
IntSize& videoSize, HTMLDivElement* container) { }
nit: I think this should this be pure virtual so that derived classes have to
explicitly express their cue display intent.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
Source/core/html/track/TextTrackCue.h:98: virtual void updateDisplayTree(double)
{ }
nit: Please file a bug and add a FIXME for the necessary refactoring to remove
these.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.h (right):

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.h:66: static PassRefPtr<VTTCue>
create(Document& document, double startTime, double endTime, const String& text)
nit: Remove document param name since it doesn't add any extra info. Presubmit
usually complains about instances of the param name matching the param type.
That is why it was missing in the original code.

[acolwell GONE FROM CHROMIUM, 2013-11-21 19:22:00]

lgtm % nits.

[esprehn, 2013-11-22 04:52:29]

lgtm, can you add the argument names for the simple types before landing? An
interface with DoStuff(bool) is difficult to know how to implement since no one
knows what the bool is for.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControlElements.cpp:738:
cue->updateDisplay(m_videoDisplaySize.size(), this);
You probably want *this and to use references.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
Source/core/html/track/TextTrackCue.h:98: virtual void updateDisplayTree(double)
{ }
Please add the argument name for the double. What is it?

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
Source/core/html/track/TextTrackCue.h:100: virtual void
notifyRegionWhenRemovingDisplayTree(bool) { }
Same thing, what is this bool? Add argument names for primitive types please.

[gasubic, 2013-11-22 15:27:33]

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:585: if (cue->isVTTCue()) {
On 2013/11/21 19:22:00, acolwell wrote:
> nit: It seems like this should be handled by a virtual ToString() method on
> TextTrackCue/VTTCue as well.

Done.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControlElements.cpp:738:
cue->updateDisplay(m_videoDisplaySize.size(), this);
On 2013/11/22 04:52:30, esprehn wrote:
> You probably want *this and to use references.

Done.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
Source/core/html/track/TextTrackCue.h:96: virtual void updateDisplay(const
IntSize& videoSize, HTMLDivElement* container) { }
On 2013/11/21 19:22:00, acolwell wrote:
> nit: I think this should this be pure virtual so that derived classes have to
> explicitly express their cue display intent.

Done.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
Source/core/html/track/TextTrackCue.h:98: virtual void updateDisplayTree(double)
{ }
On 2013/11/21 19:22:00, acolwell wrote:
> nit: Please file a bug and add a FIXME for the necessary refactoring to remove
> these.

Done.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
Source/core/html/track/TextTrackCue.h:98: virtual void updateDisplayTree(double)
{ }
On 2013/11/22 04:52:30, esprehn wrote:
> Please add the argument name for the double. What is it?

Done.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
Source/core/html/track/TextTrackCue.h:98: virtual void updateDisplayTree(double)
{ }
On 2013/11/21 19:22:00, acolwell wrote:
> nit: Please file a bug and add a FIXME for the necessary refactoring to remove
> these.

Done.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/T...
Source/core/html/track/TextTrackCue.h:100: virtual void
notifyRegionWhenRemovingDisplayTree(bool) { }
On 2013/11/22 04:52:30, esprehn wrote:
> Same thing, what is this bool? Add argument names for primitive types please.

Done.

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.h (right):

https://codereview.chromium.org/77853002/diff/100001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.h:66: static PassRefPtr<VTTCue>
create(Document& document, double startTime, double endTime, const String& text)
On 2013/11/21 19:22:00, acolwell wrote:
> nit: Remove document param name since it doesn't add any extra info. Presubmit
> usually complains about instances of the param name matching the param type.
> That is why it was missing in the original code.

I guess you didn't notice that document is referenced here in the inline body.

[commit-bot: I haz the power, 2013-11-22 16:08:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/glenn@chromium.org/77853002/330001

[commit-bot: I haz the power, 2013-11-22 16:50:43]

Change committed as 162550