Bug 79751 – Display a TextTrackCue when snap-to-lines flag is set

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Victor Carbune]

Substeps 10.13.1 - 10.13.19 (with snap-to-lines flag set), from section 3.5 of the WebVTT rendering rules:
http://dev.w3.org/html5/webvtt/#webvtt-cue-text-rendering-rules

"10.13 If cue's text track cue snap-to-lines flag is set [...]"

[Victor Carbune]

Created attachment 147993 [details]
Draft patch

Better design in progress

[Build Bot]

Comment on attachment 147993 [details]
Draft patch

Attachment 147993 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12961941

[Victor Carbune]

Created attachment 157876 [details]
Updated implementation and added tests

The better design involves an independent renderer for cues, rather than the whole container

[Victor Carbune]

Hmm, I meant to only have text dumps of the render tree. Not sure exactly what's the proper way to do this without generating png pixel dumps as well.

[WebKit Review Bot]

Comment on attachment 157876 [details]
Updated implementation and added tests

Attachment 157876 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13475657

New failing tests:
http/tests/media/media-source/video-media-source-abort.html
http/tests/media/media-source/video-media-source-objects.html
http/tests/media/media-source/video-media-source-event-attributes.html
http/tests/media/media-source/video-media-source-play.html
http/tests/media/media-source/video-media-source-state-changes.html
http/tests/media/video-referer.html
http/tests/media/video-cancel-load.html
http/tests/media/media-source/video-media-source-errors.html
http/tests/appcache/video.html
http/tests/media/video-play-stall-before-meta-data.html
http/tests/media/pdf-served-as-pdf.html
http/tests/media/media-source/video-media-source-seek.html
http/tests/media/video-useragent.html
http/tests/security/contentSecurityPolicy/media-src-allowed.html
fast/canvas/webgl/shader-precision-format.html
http/tests/media/text-served-as-text.html
http/tests/media/video-served-as-text.html
http/tests/media/video-load-suspend.html
http/tests/media/media-source/video-media-source-add-and-remove-buffers.html
http/tests/media/video-query-url.html
http/tests/media/video-error-abort.html
http/tests/media/remove-while-loading.html
http/tests/security/video-cross-origin-readback.html
http/tests/media/video-throttled-load-metadata.html
http/tests/media/video-load-twice.html
http/tests/security/contentSecurityPolicy/media-src-blocked.html
http/tests/media/media-document.html

[WebKit Review Bot]

Created attachment 157877 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 157876 [details]
Updated implementation and added tests

Attachment 157876 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13478608

[Victor Carbune]

Created attachment 157897 [details]
Updated patch

Fixed minor mistake that triggered some test fails

[WebKit Review Bot]

Comment on attachment 157897 [details]
Updated patch

Attachment 157897 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13484393

New failing tests:
media/track/track-cue-rendering-horizontal.html
media/track/track-cue-rendering-vertical.html

[WebKit Review Bot]

Created attachment 157901 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 157897 [details]
Updated patch

Attachment 157897 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13489003

[Early Warning System Bot]

Comment on attachment 157897 [details]
Updated patch

Attachment 157897 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13475955

[Early Warning System Bot]

Comment on attachment 157897 [details]
Updated patch

Attachment 157897 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13481697

[Eric Carlson]

Comment on attachment 157897 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157897&action=review

I don't know enough about rendering to r+ this so I am only making minor style comments.

> Source/WebCore/html/track/TextTrack.cpp:302
> +    bool renderedKind = m_kind == captionsKeyword() || m_kind == subtitlesKeyword();
> +    bool renderedMode = m_mode == SHOWING || m_showingByDefault;
> +
> +    return renderedKind && renderedMode;

This would could also be done with early returns:

if (m_kind != captionsKeyword() && m_kind != subtitlesKeyword())
    return false;
if (m_mode != SHOWING && !m_showingByDefault)
    return false;

return true;

> Source/WebCore/html/track/TextTrackCue.cpp:227
> -    m_displayWritingModeMap[VerticalGrowingLeft] = CSSValueVerticalLr;
> -    m_displayWritingModeMap[VerticalGrowingRight] = CSSValueVerticalRl;
> +    m_displayWritingModeMap[VerticalGrowingLeft] = CSSValueVerticalRl;
> +    m_displayWritingModeMap[VerticalGrowingRight] = CSSValueVerticalLr;

Good fix!

> Source/WebCore/html/track/TextTrackCue.cpp:552
> +    // If cue is not associated with a text track, return â1 and abort these
> +    // steps.

"â1" -> "-1"

> Source/WebCore/rendering/RenderTextTrackCue.cpp:35
> +    LayoutStateMaintainer statePusher(
> +            view(),
> +            this,
> +            locationOffset(),
> +            hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode());

Nit: the extra line breaks are not necessary.

> Source/WebCore/rendering/RenderTextTrackCue.cpp:131
> +        if (cue->vertical() == Horizontal
> +            && ((step < 0 && top < 0) || (step > 0 && bottom >
> +                    parentBlock->absoluteBoundingBoxRect().height())))
> +            jumpToSwitchDirection = true;

I don't think the second line break helps readablilty. WebKit style is to use braces in a case where the condition takes more than one line.

> Source/WebCore/rendering/RenderTextTrackCue.cpp:142
> +        if (cue->vertical() != Horizontal
> +            && ((step < 0 && left < 0) || (step > 0 && right >
> +                    parentBlock->absoluteBoundingBoxRect().width())))
> +            jumpToSwitchDirection = true;

Ditto.

[Victor Carbune]

Created attachment 158095 [details]
Minor changes

[Victor Carbune]

(In reply to comment #14)
> (From update of attachment 157897 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157897&action=review
> 
> I don't know enough about rendering to r+ this so I am only making minor style comments.
CC'ed Ojan and Tony, hopefully they will have some time to look over this bug.

> > Source/WebCore/html/track/TextTrack.cpp:302
> > +    bool renderedKind = m_kind == captionsKeyword() || m_kind == subtitlesKeyword();
> > +    bool renderedMode = m_mode == SHOWING || m_showingByDefault;
> > +
> > +    return renderedKind && renderedMode;
> 
> This would could also be done with early returns:
> 
> if (m_kind != captionsKeyword() && m_kind != subtitlesKeyword())
>     return false;
> if (m_mode != SHOWING && !m_showingByDefault)
>     return false;
> 
> return true;
I actually did this initially, not sure why I changed. Done.

> > Source/WebCore/html/track/TextTrackCue.cpp:227
> > -    m_displayWritingModeMap[VerticalGrowingLeft] = CSSValueVerticalLr;
> > -    m_displayWritingModeMap[VerticalGrowingRight] = CSSValueVerticalRl;
> > +    m_displayWritingModeMap[VerticalGrowingLeft] = CSSValueVerticalRl;
> > +    m_displayWritingModeMap[VerticalGrowingRight] = CSSValueVerticalLr;
> 
> Good fix!
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:552
> > +    // If cue is not associated with a text track, return â1 and abort these
> > +    // steps.
> 
> "â1" -> "-1"
Done.
 
> > Source/WebCore/rendering/RenderTextTrackCue.cpp:35
> > +    LayoutStateMaintainer statePusher(
> > +            view(),
> > +            this,
> > +            locationOffset(),
> > +            hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode());
> 
> Nit: the extra line breaks are not necessary.
Done.

> > Source/WebCore/rendering/RenderTextTrackCue.cpp:131
> > +        if (cue->vertical() == Horizontal
> > +            && ((step < 0 && top < 0) || (step > 0 && bottom >
> > +                    parentBlock->absoluteBoundingBoxRect().height())))
> > +            jumpToSwitchDirection = true;
> 
> I don't think the second line break helps readablilty. WebKit style is to use braces in a case where the condition takes more than one line.
> 
> > Source/WebCore/rendering/RenderTextTrackCue.cpp:142
> > +        if (cue->vertical() != Horizontal
> > +            && ((step < 0 && left < 0) || (step > 0 && right >
> > +                    parentBlock->absoluteBoundingBoxRect().width())))
> > +            jumpToSwitchDirection = true;
> 
> Ditto.
Done.

I have temporarily marked the tests as IMAGE fail, they will anyway need rebaseline if they remain like this (the image diffs returned by the bots were 0%).

The Mac and Qt bots might still fail the build as I need to add the file properly to XCode, but I will leave this after some more reviews.

[Victor Carbune]

Created attachment 158099 [details]
Rebased patch

[Build Bot]

Comment on attachment 158099 [details]
Rebased patch

Attachment 158099 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13492241

[Early Warning System Bot]

Comment on attachment 158099 [details]
Rebased patch

Attachment 158099 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13492255

[Early Warning System Bot]

Comment on attachment 158099 [details]
Rebased patch

Attachment 158099 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13486646

[Tony Chang]

Comment on attachment 158099 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158099&action=review

r- since there are some compile errors on qt and mac.

> Source/WebCore/html/track/TextTrackCue.cpp:165
> +        String translateX = "-" + String::number(position.first) + "%";
> +        String translateY = "-" + String::number(position.second) + "%";
> +        String webkitTransformTranslateValue = "translate(" + translateX + "," + translateY + ")";

Can we use a StringBuilder here?  It should be a little bit faster.  Alternately, maybe you can use String::format.

> Source/WebCore/html/track/TextTrackCue.h:61
> +    void applyCssProperties();

Nit: applyCSSProperties()

> Source/WebCore/html/track/TextTrackCue.h:146
> +    std::pair<double, double> getCssPosition() const;
> +    int getCssSize() const;
> +    int getCssWritingMode() const;

Nit: Normally CSS is always capitalized in WebKit, e.g., getCSSPosition().  The 'get' prefix is also rare, but I guess this file already has some cases with it.

> Source/WebCore/html/track/TextTrackCue.h:222
> +    std::pair<double, double> m_displayPosition;

Nit: Can we use a FloatPoint or does this need to be doubles?

> Source/WebCore/rendering/RenderTextTrackCue.cpp:42
> +    DEFINE_STATIC_LOCAL(const String, Horizontal, (""));
> +    DEFINE_STATIC_LOCAL(const String, VerticalGrowingLeft, ("rl"));

Doing String comparisons seems slow. Can we add helper methods on TextTrackCue that return an enum or a bool?

> Source/WebCore/rendering/RenderTextTrackCue.cpp:62
> +    // 1. Horizontal: Let step be the height of the first line box in boxes.
> +    //    Vertical: Let step be the width of the first line box in boxes.

In the long run, I think these comments just add noise.  There are old comments like this in places like CSSParser.cpp, but they tend to get out of date and mislead.  This code would be easier to read without the comments.

You could break this layout() method into some helper methods. That might help to document what's going on.

> Source/WebCore/rendering/RenderTextTrackCue.cpp:63
> +    double step = cue->vertical() == Horizontal ? firstLineBox->height() : firstLineBox->width();

Why a double? It looks like height() and width() are floats.  I suspect you want to convert these to LayoutUnit since we're now switching to layout positions.

> Source/WebCore/rendering/RenderTextTrackCue.cpp:77
> +    double position = step * linePosition;

This should probably be a float too.

> Source/WebCore/rendering/RenderTextTrackCue.cpp:122
> +        // FIXME(BUG ...): This overlap test is in O(N^2), there might be a much more
> +        // optimized way too do this using some WebKit rendering methods.

This is just the text tracks, right?  Is there an upper bound on how many there can be?

> Source/WebCore/rendering/RenderTextTrackCue.cpp:123
> +        for (RenderObject *box = previousSibling(); box; box = box->previousSibling()) {

* in the wrong place

> Source/WebCore/rendering/RenderTextTrackCue.cpp:145
> +        int bottom = top + firstLineBox->height();
> +        int right = left + firstLineBox->width();

Is it OK to use ints here?  Do you want to truncate or round?

> Source/WebCore/rendering/RenderTextTrackCue.cpp:201
> +    setNeedsLayout(false);

I think needslayout was already set to false by RenderBlock::layout().

> Source/WebCore/rendering/RenderTextTrackCue.h:37
> +    RenderTextTrackCue(Node*);

explicit

[Tony Chang]

Levi or Emil can probably provide feedback as to the right units to use and when to convert from floats to LayoutUnit.

[Levi Weintraub]

Comment on attachment 158099 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158099&action=review

>> Source/WebCore/rendering/RenderTextTrackCue.cpp:62
>> +    //    Vertical: Let step be the width of the first line box in boxes.
> 
> In the long run, I think these comments just add noise.  There are old comments like this in places like CSSParser.cpp, but they tend to get out of date and mislead.  This code would be easier to read without the comments.
> 
> You could break this layout() method into some helper methods. That might help to document what's going on.

Functions would make this a lot clearer. Why are the comments numbered?

>> Source/WebCore/rendering/RenderTextTrackCue.cpp:63
>> +    double step = cue->vertical() == Horizontal ? firstLineBox->height() : firstLineBox->width();
> 
> Why a double? It looks like height() and width() are floats.  I suspect you want to convert these to LayoutUnit since we're now switching to layout positions.

LineBox positions are floats.

> Source/WebCore/rendering/RenderTextTrackCue.cpp:113
> +    // 11. Step loop:
> +    for (;;) {

While not the first boundless for loop in WebCore, they're pretty rare. Is this really necessary? "Step loop" means nothing to me...

>> Source/WebCore/rendering/RenderTextTrackCue.cpp:145
>> +        int top = absoluteBoundingBoxRect().y();
>> +        int left = absoluteBoundingBoxRect().x();
>> +        int bottom = top + firstLineBox->height();
>> +        int right = left + firstLineBox->width();
> 
> Is it OK to use ints here?  Do you want to truncate or round?

You almost assuredly don't want to just assign these to an int but rather enclose the line, no? Why are you using absolute coordinates here instead of local LayoutUnits?

[Eric Carlson]

(In reply to comment #21)
> (From update of attachment 158099 [details])
> > Source/WebCore/rendering/RenderTextTrackCue.cpp:62
> > +    // 1. Horizontal: Let step be the height of the first line box in boxes.
> > +    //    Vertical: Let step be the width of the first line box in boxes.
> 
> In the long run, I think these comments just add noise.  There are old comments like this in places like CSSParser.cpp, but they tend to get out of date and mislead.  This code would be easier to read without the comments.

(In reply to comment #23)
> Functions would make this a lot clearer. Why are the comments numbered?

The comments are text straight from the spec. I think they are extremely useful for two reasons: 1) it makes it much easier to check the logic against what the spec requires, and 2) the comments make it much easier to find parts that need to be changed because of spec updates. The later is a real concern because the spec is not final and will change. We have comments like this in HTMLMediaElement and they have helped in precisely this way.

I agree that they need to be kept in sync with both the code and the spec, but that is exactly the point of including them.

[Tony Chang]

Re: comments

I would do something like:

RenderTextTrackCue::layout()
{
    ...
    computePositionAndStep(&position, &step);
    placesBoxesInDefaultPosition(position);
    fixOverlappingTextBoxes(step);
    ...
}

// This is steps 1-6 from ...
RenderTextTrackCue::computePositionAndStep(&position, &step)
{
    ...
}

// This is steps 7-9 from ...
RenderTextTrackCue::placesBoxesInDefaultPosition(position)
{
}

// This is steps 11-19 from ...
RenderTextTrackCue::fixOverlappingTextBoxes(step)
{
}


That said, I don't feel super strongly about this, but I doubt these comments will be that useful 5 years from now.

[Victor Carbune]

(In reply to comment #25)
> Re: comments
> 
> I would do something like:
> 
> RenderTextTrackCue::layout()
> {
>     ...
>     computePositionAndStep(&position, &step);
>     placesBoxesInDefaultPosition(position);
>     fixOverlappingTextBoxes(step);
>     ...
> }
> 
> // This is steps 1-6 from ...
> RenderTextTrackCue::computePositionAndStep(&position, &step)
> {
>     ...
> }
> 
> // This is steps 7-9 from ...
> RenderTextTrackCue::placesBoxesInDefaultPosition(position)
> {
> }
> 
> // This is steps 11-19 from ...
> RenderTextTrackCue::fixOverlappingTextBoxes(step)
> {
> }
I just did a refactoring following pretty much the same idea, I will upload the patch and if the naming / grouping of steps isn't good enough, I will re-iterate.
 
> That said, I don't feel super strongly about this, but I doubt these comments will be that useful 5 years from now.

I second Eric's suggestion on keeping them, at least until the WebVTT specification can be anywhere near "stable". However, there are important parts that will certainly change and it's just easier to track for us.

[Victor Carbune]

Created attachment 158681 [details]
Updated patch

Significant refactoring

[Victor Carbune]

Thanks for taking the time and reviewing this! I have addressed most of the comments - the mac and qt still fail, but I'll fix them at the next iteration.

(In reply to comment #21)
> (From update of attachment 158099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158099&action=review
> 
> r- since there are some compile errors on qt and mac.
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:165
> > +        String translateX = "-" + String::number(position.first) + "%";
> > +        String translateY = "-" + String::number(position.second) + "%";
> > +        String webkitTransformTranslateValue = "translate(" + translateX + "," + translateY + ")";
> 
> Can we use a StringBuilder here?  It should be a little bit faster.  Alternately, maybe you can use String::format.
Done, used String::format.

> > Source/WebCore/html/track/TextTrackCue.h:61
> > +    void applyCssProperties();
> 
> Nit: applyCSSProperties()
Done.

> > Source/WebCore/html/track/TextTrackCue.h:146
> > +    std::pair<double, double> getCssPosition() const;
> > +    int getCssSize() const;
> > +    int getCssWritingMode() const;
> 
> Nit: Normally CSS is always capitalized in WebKit, e.g., getCSSPosition().  The 'get' prefix is also rare, but I guess this file already has some cases with it.
Capitalized CSS.
 
> > Source/WebCore/html/track/TextTrackCue.h:222
> > +    std::pair<double, double> m_displayPosition;
> 
> Nit: Can we use a FloatPoint or does this need to be doubles?
They don't need to be doubles, I changed them to floats. I would leave the the refactoring to FloatPoint for a different patch, if that's fine.

> 
> > Source/WebCore/rendering/RenderTextTrackCue.cpp:42
> > +    DEFINE_STATIC_LOCAL(const String, Horizontal, (""));
> > +    DEFINE_STATIC_LOCAL(const String, VerticalGrowingLeft, ("rl"));
> 
> Doing String comparisons seems slow. Can we add helper methods on TextTrackCue that return an enum or a bool?
I added getWritingDirection and made the enum public and used it in this file as well.
 
> > Source/WebCore/rendering/RenderTextTrackCue.cpp:63
> > +    double step = cue->vertical() == Horizontal ? firstLineBox->height() : firstLineBox->width();
> 
> Why a double? It looks like height() and width() are floats.  I suspect you want to convert these to LayoutUnit since we're now switching to layout positions.
Switched to LayoutUnit everywhere.

> > Source/WebCore/rendering/RenderTextTrackCue.cpp:77
> > +    double position = step * linePosition;
> 
> This should probably be a float too.
Switched to LayoutUnit.
 
> > Source/WebCore/rendering/RenderTextTrackCue.cpp:122
> > +        // FIXME(BUG ...): This overlap test is in O(N^2), there might be a much more
> > +        // optimized way too do this using some WebKit rendering methods.
> 
> This is just the text tracks, right?  Is there an upper bound on how many there can be?
It's not an upper bound of how many cues at once you may have, but as the screen fills up this probably gets very inefficient.

> > Source/WebCore/rendering/RenderTextTrackCue.cpp:123
> > +        for (RenderObject *box = previousSibling(); box; box = box->previousSibling()) {
> 
> * in the wrong place
Done.

> > Source/WebCore/rendering/RenderTextTrackCue.cpp:145
> > +        int bottom = top + firstLineBox->height();
> > +        int right = left + firstLineBox->width();
> 
> Is it OK to use ints here?  Do you want to truncate or round?
No, mistake. Changed to LayoutUnit.

> > Source/WebCore/rendering/RenderTextTrackCue.cpp:201
> > +    setNeedsLayout(false);
> 
> I think needslayout was already set to false by RenderBlock::layout().
Indeed, removed.

> > Source/WebCore/rendering/RenderTextTrackCue.h:37
> > +    RenderTextTrackCue(Node*);
> 
> explicit
Done.

(In reply to comment #23)
> > Why a double? It looks like height() and width() are floats.  I suspect you want to convert these to LayoutUnit since we're now switching to layout positions.
> 
> LineBox positions are floats.
Changed to LayoutUnit.
 
> > Source/WebCore/rendering/RenderTextTrackCue.cpp:113
> > +    // 11. Step loop:
> > +    for (;;) {
> 
> While not the first boundless for loop in WebCore, they're pretty rare. Is this really necessary? "Step loop" means nothing to me...
Refactored.

> >> Source/WebCore/rendering/RenderTextTrackCue.cpp:145
> >> +        int top = absoluteBoundingBoxRect().y();
> >> +        int left = absoluteBoundingBoxRect().x();
> >> +        int bottom = top + firstLineBox->height();
> >> +        int right = left + firstLineBox->width();
> > 
> > Is it OK to use ints here?  Do you want to truncate or round?
> 
> You almost assuredly don't want to just assign these to an int but rather enclose the line, no? Why are you using absolute coordinates here instead of local LayoutUnits?
Oops, these should have been local LayoutUnits indeed, fixed.

[Build Bot]

Comment on attachment 158681 [details]
Updated patch

Attachment 158681 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13511366

[Early Warning System Bot]

Comment on attachment 158681 [details]
Updated patch

Attachment 158681 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13511369

[Early Warning System Bot]

Comment on attachment 158681 [details]
Updated patch

Attachment 158681 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13511372

[Tony Chang]

Comment on attachment 158681 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158681&action=review

I mainly just looked at the code in rendering, but it looks OK to me.  Looks like you still have compile errors on Qt and Mac.

> Source/WebCore/rendering/RenderTextTrackCue.cpp:45
> +    m_cue->snapToLines() ? repositionCueSnapToLinesSet() : repositionCueSnapToLinesNotSet();

Nit: I would just use a regular if/else here. I always feel weird using a ternary without a return/assignment.

> Source/WebCore/rendering/RenderTextTrackCue.cpp:112
> +inline bool RenderTextTrackCue::isOutside() const

Nit: I normally prefer to let the compiler decide to inline or not unless we know this is going to cause slowness.  I guess it's not a big deal here since we call each of these functions once.

> Source/WebCore/rendering/RenderTextTrackCue.cpp:168
> +    if (m_cue->getWritingDirection() != TextTrackCue::Horizontal)

Nit: Can you use an 'else' here?

> Source/WebCore/rendering/RenderTextTrackCue.cpp:216
> +    // FIXME(BUG 84296): Implement overlapping detection when snap-to-lines is not set.

Nit: I've never seen this FIXME() syntax.  I would just put the URL after the comment. E.g.,
// FIXME: Implement overlapping detection when snap-to-lines is not set. http://wkb.ug/84296

> Source/WebCore/rendering/RenderTextTrackCue.h:60
> +    TextTrackCue* m_cue;
> +
> +    FloatPoint m_fallbackPosition;
> +    LayoutUnit m_position;
> +    LayoutUnit m_step;

Having all these member variables is unfortunate.  They'll use memory after layout is done even though we don't need them.  One way to avoid this would be to make a struct with the params you need and pass it around to each function or just pass a bunch of params around.

[Victor Carbune]

Created attachment 159189 [details]
Updated patch

Addressed comments and solved Mac build issue

[Victor Carbune]

(In reply to comment #32)
> (From update of attachment 158681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158681&action=review
> 
> I mainly just looked at the code in rendering, but it looks OK to me.  Looks like you still have compile errors on Qt and Mac.
I should have fixed the compile issues for Mac and added the test cases there as well. I'm not sure about Qt (maybe the IF ENABLE(VIDEO_TRACK) guard I correct fixes it).

Other changes:
* Changed the TextTrackCueBox to extend HTMLElement, as HTMLDivElement was breaking WebView compiling on Mac.
* Changed the RenderTextTrackCue constructor to accept TextTrackCueBox type node.

> > Source/WebCore/rendering/RenderTextTrackCue.cpp:45
> > +    m_cue->snapToLines() ? repositionCueSnapToLinesSet() : repositionCueSnapToLinesNotSet();
> 
> Nit: I would just use a regular if/else here. I always feel weird using a ternary without a return/assignment.
Done.

> > Source/WebCore/rendering/RenderTextTrackCue.cpp:112
> > +inline bool RenderTextTrackCue::isOutside() const
> 
> Nit: I normally prefer to let the compiler decide to inline or not unless we know this is going to cause slowness.  I guess it's not a big deal here since we call each of these functions once.
Sure, done.
 
> > Source/WebCore/rendering/RenderTextTrackCue.cpp:168
> > +    if (m_cue->getWritingDirection() != TextTrackCue::Horizontal)
> 
> Nit: Can you use an 'else' here?
Done.

> > Source/WebCore/rendering/RenderTextTrackCue.cpp:216
> > +    // FIXME(BUG 84296): Implement overlapping detection when snap-to-lines is not set.
> 
> Nit: I've never seen this FIXME() syntax.  I would just put the URL after the comment. E.g.,
> // FIXME: Implement overlapping detection when snap-to-lines is not set. http://wkb.ug/84296
Oops, not sure why I used it like this.. Fixed.
 
> > Source/WebCore/rendering/RenderTextTrackCue.h:60
> > +    TextTrackCue* m_cue;
> > +
> > +    FloatPoint m_fallbackPosition;
> > +    LayoutUnit m_position;
> > +    LayoutUnit m_step;
> 
> Having all these member variables is unfortunate.  They'll use memory after layout is done even though we don't need them.  One way to avoid this would be to make a struct with the params you need and pass it around to each function or just pass a bunch of params around.
Left the cue and fallback position as local variables.

[Tony Chang]

Comment on attachment 159189 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159189&action=review

> Source/WebCore/html/track/TextTrackCue.h:62
> +    virtual const AtomicString& shadowPseudoId() const;

Nit: OVERRIDE

> Source/WebCore/html/track/TextTrackCue.h:67
> +    virtual RenderObject* createRenderer(RenderArena*, RenderStyle*);

Nit: OVERRIDE

> Source/WebCore/rendering/RenderTextTrackCue.h:45
> +    virtual void layout();

Nit: OVERRIDE

[Victor Carbune]

Created attachment 159736 [details]
Landing patch

Fixed nits

[WebKit Review Bot]

Comment on attachment 159736 [details]
Landing patch

Clearing flags on attachment: 159736

Committed r126233: <http://trac.webkit.org/changeset/126233>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This breaks the build on chromium mac and chromium linux w/ clang:

In file included from ../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.cpp:36:
../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.h:216:9: error: private field 'm_displayHeight' is not used [-Werror,-Wunused-private-field]
    int m_displayHeight;
        ^
../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.h:217:9: error: private field 'm_displayWidth' is not used [-Werror,-Wunused-private-field]
    int m_displayWidth;

[Victor Carbune]

(In reply to comment #39)
> This breaks the build on chromium mac and chromium linux w/ clang:
> 
> In file included from ../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.cpp:36:
> ../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.h:216:9: error: private field 'm_displayHeight' is not used [-Werror,-Wunused-private-field]
>     int m_displayHeight;
>         ^
> ../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.h:217:9: error: private field 'm_displayWidth' is not used [-Werror,-Wunused-private-field]
>     int m_displayWidth;
Sorry, I'm on it right now

[WebKit Review Bot]

Re-opened since this is blocked by 94656

[Victor Carbune]

Created attachment 159840 [details]
Build fix

Removed the two unused vars

[WebKit Review Bot]

Comment on attachment 159840 [details]
Build fix

Rejecting attachment 159840 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rack-cue-rendering-vertical-expected.txt
patching file LayoutTests/platform/chromium/TestExpectations
Hunk #1 succeeded at 3448 (offset -4 lines).
patching file LayoutTests/platform/mac/media/track/track-cue-rendering-horizontal-expected.txt
patching file LayoutTests/platform/mac/media/track/track-cue-rendering-vertical-expected.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Tony Chang']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13570395

[Victor Carbune]

Created attachment 160034 [details]
Rebased

[WebKit Review Bot]

Comment on attachment 160034 [details]
Rebased

Clearing flags on attachment: 160034

Committed r126372: <http://trac.webkit.org/changeset/126372>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.