Separate VTTCue from VTTCueBox and LayoutVTTCue

Separate VTTCue from VTTCueBox and LayoutVTTCue

Both VTTCueBox and LayoutVTTCue had a VTTCue member, which made for
tricky lifetime considerations, in particular the VTTCue destructor
was removing the VTTCueBox from the DOM, so that the stale VTTCue*
pointers would never be accessed.

Instead, let VTTCue provide VTTCueBox with the information it needs,
which will in turn provide it to LayoutVTTCue. Stale information
should not be a problem, as all relevant changes to VTTCue will call
cueDidChange(), whichs ends up removing and re-inserting the cue.

Refactoring only, no web-observable changes are intended.

BUG=511174, 509911
R=fs@opera.com

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

[philipj_slow, 2015-07-17 12:02:58]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-17 12:03:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1240433007/1

[philipj_slow, 2015-07-17 12:03:57]

There's still some ugly around regions, but I'll just leave that for now.

[commit-bot: I haz the power, 2015-07-17 12:05:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-17 12:05:25]

Dry run: Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/63239)

(exceeded global retry quota)

[philipj_slow, 2015-07-17 12:12:36]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-17 12:12:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1240433007/20001

[fs, 2015-07-17 14:31:48]

LGTM w/ some comments/notes

https://codereview.chromium.org/1240433007/diff/20001/Source/core/html/track/...
File Source/core/html/track/vtt/VTTCue.cpp (right):

https://codereview.chromium.org/1240433007/diff/20001/Source/core/html/track/...
Source/core/html/track/vtt/VTTCue.cpp:827: if (regionId().isEmpty()) {
Maybe you could add a "VTTRegion* region() { return track() &&
track()->regions() ? ... : nullptr; }" and use that (here and all the other
places that accesses the region. IIRC that's what the spec looks like now too
right?). Anyway, can be done later, since is equivalent to what's there already
IIUC.

https://codereview.chromium.org/1240433007/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutVTTCue.h (right):

https://codereview.chromium.org/1240433007/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.h:42: float snapToLinesPosition() { return
m_snapToLinesPosition; }
Any particular reason for not doing something like:

float snapToLinesPosition() const
{
    return toVTTCueBox(*node()).snapToLinesPosition();
}

?

Hmm, I guess we might not have a way to id a VTTCueBox, and adding one would
s*ck I suppose... Oh well.

(I really think that we shouldn't be recreating theses fragments at a whim...)

[commit-bot: I haz the power, 2015-07-17 15:28:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-17 15:28:23]

Dry run: This issue passed the CQ dry run.

[philipj_slow, 2015-07-20 07:33:30]

Thanks for review! I haven't made any changes, but if there's something that
comes to mind (when you get back) just follow up :)

https://codereview.chromium.org/1240433007/diff/20001/Source/core/html/track/...
File Source/core/html/track/vtt/VTTCue.cpp (right):

https://codereview.chromium.org/1240433007/diff/20001/Source/core/html/track/...
Source/core/html/track/vtt/VTTCue.cpp:827: if (regionId().isEmpty()) {
On 2015/07/17 14:31:48, fs (ooo) wrote:
> Maybe you could add a "VTTRegion* region() { return track() &&
> track()->regions() ? ... : nullptr; }" and use that (here and all the other
> places that accesses the region. IIRC that's what the spec looks like now too
> right?). Anyway, can be done later, since is equivalent to what's there
already
> IIUC.

Yes, I changed the spec such that it should be possible to have an id->region
mapping while parsing and then after that simply let a VTTCue point to a
VTTRegion. I didn't want to change any behavior here, though.

https://codereview.chromium.org/1240433007/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutVTTCue.h (right):

https://codereview.chromium.org/1240433007/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.h:42: float snapToLinesPosition() { return
m_snapToLinesPosition; }
On 2015/07/17 14:31:48, fs (ooo) wrote:
> Any particular reason for not doing something like:
> 
> float snapToLinesPosition() const
> {
>     return toVTTCueBox(*node()).snapToLinesPosition();
> }
> 
> ?
> 
> Hmm, I guess we might not have a way to id a VTTCueBox, and adding one would
> s*ck I suppose... Oh well.
> 
> (I really think that we shouldn't be recreating theses fragments at a whim...)

Precisely, an isVTTCueBox would require an extra virtual on Node, and I didn't
think it was worth it, nor did static_cast<VTTCueBox*> seem appealing despite
currently being safe.

I was also tempted to use the style system to pass this float, by adding some
inline style that isn't actually used, but that seemed to clever. If passing
this extra information through the constructor turns out to be a problem (or
deemed ugly) either an explicit internal style property or an is/toVTTCueBox
would be OK.

[philipj_slow, 2015-07-20 07:41:07]

philipj@opera.com changed reviewers:
+ sigbjornf@opera.com

[philipj_slow, 2015-07-20 07:41:08]

sof, can you please check this for Oilpan sanity? Not sure if the removal of the
destructor means some other simplification is also made possible...

[sof, 2015-07-20 08:15:39]

lgtm oilpan-wise.

https://codereview.chromium.org/1240433007/diff/20001/Source/core/html/track/...
File Source/core/html/track/vtt/VTTCue.cpp (left):

https://codereview.chromium.org/1240433007/diff/20001/Source/core/html/track/...
Source/core/html/track/vtt/VTTCue.cpp:246: VTTCue::~VTTCue()
It would be preferable to keep an empty destructor, to avoid synthesizing &
inlining a larger one.

https://codereview.chromium.org/1240433007/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutVTTCue.h (right):

https://codereview.chromium.org/1240433007/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.h:38: explicit LayoutVTTCue(ContainerNode*,
float snapToLinesPosition);
nit: can drop explicit now.

[philipj_slow, 2015-07-20 08:34:16]

Thanks sof!

https://codereview.chromium.org/1240433007/diff/20001/Source/core/html/track/...
File Source/core/html/track/vtt/VTTCue.cpp (left):

https://codereview.chromium.org/1240433007/diff/20001/Source/core/html/track/...
Source/core/html/track/vtt/VTTCue.cpp:246: VTTCue::~VTTCue()
On 2015/07/20 08:15:39, sof wrote:
> It would be preferable to keep an empty destructor, to avoid synthesizing &
> inlining a larger one.

Done.

https://codereview.chromium.org/1240433007/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutVTTCue.h (right):

https://codereview.chromium.org/1240433007/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutVTTCue.h:38: explicit LayoutVTTCue(ContainerNode*,
float snapToLinesPosition);
On 2015/07/20 08:15:39, sof wrote:
> nit: can drop explicit now.

Done, and also added explicit to the VTTCueBox constructor which now has just
one argument.

[philipj_slow, 2015-07-20 08:34:27]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-07-20 08:34:27]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
fs@opera.com
Link to the patchset: https://codereview.chromium.org/1240433007/#ps40001
(title: "empty constructor, explicit keyword")

[commit-bot: I haz the power, 2015-07-20 08:34:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1240433007/40001

[commit-bot: I haz the power, 2015-07-20 10:00:38]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199160