WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
79750
Display a TextTrackCue when snap-to-lines flag is not set
https://bugs.webkit.org/show_bug.cgi?id=79750
Summary
Display a TextTrackCue when snap-to-lines flag is not set
Victor Carbune
Reported
2012-02-27 23:29:14 PST
Substeps 10.10 - 10.16, from section 3.5 of the WebVTT rendering rules:
http://dev.w3.org/html5/webvtt/#webvtt-cue-text-rendering-rules
"10.10 Let left be 'x-position vw' and top be 'y-position vh'. (These again are CSS values used by the next section to set CSS properties for the rendering; 'vw' and 'vh' are CSS units.) [CSSVALUES] [...] 10.13 If cue's text track cue snap-to-lines flag is not set [...] 10.16 Add the CSS boxes in boxes to output."
Attachments
Preliminary patch
(26.41 KB, patch)
2012-04-16 07:07 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-04
(6.33 MB, application/zip)
2012-04-16 08:05 PDT
,
WebKit Review Bot
no flags
Details
Updated patch
(27.10 KB, patch)
2012-04-16 14:58 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Patch
(32.14 KB, patch)
2012-04-18 16:25 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Victor Carbune
Comment 1
2012-04-16 07:07:29 PDT
Created
attachment 137340
[details]
Preliminary patch
WebKit Review Bot
Comment 2
2012-04-16 08:05:45 PDT
Comment on
attachment 137340
[details]
Preliminary patch
Attachment 137340
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/12416120
New failing tests: fast/canvas/webgl/shader-precision-format.html
WebKit Review Bot
Comment 3
2012-04-16 08:05:58 PDT
Created
attachment 137349
[details]
Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Eric Carlson
Comment 4
2012-04-16 08:49:02 PDT
Comment on
attachment 137340
[details]
Preliminary patch View in context:
https://bugs.webkit.org/attachment.cgi?id=137340&action=review
Nice test! Marking r- for the relatively minor cleanup notes.
> Source/WebCore/html/track/TextTrackCue.cpp:414 > +int TextTrackCue::determineComputedLinePosition()
Nit: this isn't a great name, maybe something like "calculateComputedLinePosition"?
> Source/WebCore/html/track/TextTrackCue.cpp:423 > + // If the text track cue snap-to-lines flag of the text track cue is not set
It looks like you left out part of this comment?
> Source/WebCore/html/track/TextTrackCue.cpp:435 > void TextTrackCue::determineDisplayParameters()
Nit: maybe "calculateDisplayParameters"?
> Source/WebCore/html/track/TextTrackCue.cpp:469 > + m_displaySize = std::min(m_cueSize, maximumSize);
Nit: Adding "using namespace std" at the beginning of the file would let you omit the "std::" here and elsewhere in the file.
> Source/WebCore/html/track/TextTrackCue.cpp:476 > + // 10.7 If the text track cue writing direction is horizontal, then let > + // width be 'size vw' and height be 'auto'. Otherwise, let width be 'auto' > + // and height be 'size vh'. (These are CSS values used by the next section > + // to set CSS properties for the rendering; 'vw' and 'vh' are CSS units.) > + m_displayWidth = m_writingDirection == Horizontal ? m_displaySize : 0; > + m_displayHeight = m_writingDirection == Horizontal ? 0 : m_displaySize;
Does "0" mean "size vw" and "auto"? If so, is there any reason to not have named constants instead?
> Source/WebCore/html/track/TextTrackCue.cpp:546 > - m_displayTree->setShadowPseudoId(AtomicString("-webkit-media-text-track-display"), ASSERT_NO_EXCEPTION); > + m_displayTree->setShadowPseudoId("-webkit-media-text-track-display", ASSERT_NO_EXCEPTION);
setShadowPseudoId takes a "const AtomicString&". If this is called frequently it might be worth defining and using a static const.
> Source/WebCore/html/track/TextTrackCue.cpp:568 > + m_displayTree->setInlineStyleProperty( > + CSSPropertyLeft, > + getPositionCoordinates().first, > + CSSPrimitiveValue::CSS_PERCENTAGE);
I don't think breaking this into multiple lines makes it more readable.
> Source/WebCore/html/track/TextTrackCue.cpp:573 > + m_displayTree->setInlineStyleProperty( > + CSSPropertyTop, > + getPositionCoordinates().second, > + CSSPrimitiveValue::CSS_PERCENTAGE);
Ditto.
> Source/WebCore/html/track/TextTrackCue.cpp:583 > + String translateX = "-" + String::number(getPositionCoordinates().first) + "%"; > + String translateY = "-" + String::number(getPositionCoordinates().second) + "%";
Is it necessary call getPositionCoordinates() twice?
> Source/WebCore/html/track/TextTrackCue.cpp:587 > + m_displayTree->setInlineStyleProperty(CSSPropertyWebkitTransform, > + webkitTransformTranslateValue);
Ditto.
> Source/WebCore/html/track/TextTrackCue.cpp:619 > + if (m_writingDirection == Horizontal && m_displayDirection == CSSValueLtr) { > + coordinates.first = m_textPosition; > + coordinates.second = m_computedLinePosition; > + } > + > + if (m_writingDirection == Horizontal && m_displayDirection == CSSValueRtl) { > + coordinates.first = 100 - m_textPosition; > + coordinates.second = m_computedLinePosition; > + } > + > + if (m_writingDirection == VerticalGrowingLeft) { > + coordinates.first = 100 - m_computedLinePosition; > + coordinates.second = m_textPosition; > + } > + > + if (m_writingDirection == VerticalGrowingRight) { > + coordinates.first = m_computedLinePosition; > + coordinates.second = m_textPosition; > + }
These are all mutually exclusive so you should us an early returns and add an ASSERT_NOT_REACHED at the end.
> Source/WebCore/html/track/TextTrackCue.h:152 > + TotalWritingDirections
Nit: maybe WritingDirectionCount or NumberOfWritingDirections?
> LayoutTests/ChangeLog:14 > + * media/track/captions-webvtt/captions-snap-to-lines-unset.vtt: Added. > + * media/track/track-cue-rendering-snap-to-lines-unset-expected.txt: Added. > + * media/track/track-cue-rendering-snap-to-lines-unset.html: Added.
Nit: I think "track-cue-rendering-snap-to-lines-not-set" would be a better name.
> LayoutTests/media/media-controls.js:57 > + throw "There are not " + i + " text track cues visible";
Do you mean: "There are not " + cueNumber + " text track cues visible"?
Victor Carbune
Comment 5
2012-04-16 14:58:13 PDT
Created
attachment 137406
[details]
Updated patch
Victor Carbune
Comment 6
2012-04-16 15:04:38 PDT
(In reply to
comment #4
) Thank you for the fast review!
> (From update of
attachment 137340
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=137340&action=review
> > Nice test! > > Marking r- for the relatively minor cleanup notes. > > > Source/WebCore/html/track/TextTrackCue.cpp:414 > > +int TextTrackCue::determineComputedLinePosition() > > Nit: this isn't a great name, maybe something like "calculateComputedLinePosition"?
Renamed. I can't think of anything better.
> > Source/WebCore/html/track/TextTrackCue.cpp:423 > > + // If the text track cue snap-to-lines flag of the text track cue is not set > > It looks like you left out part of this comment?
Added the whole comment.
> > Source/WebCore/html/track/TextTrackCue.cpp:435 > > void TextTrackCue::determineDisplayParameters() > > Nit: maybe "calculateDisplayParameters"?
Renamed.
> > Source/WebCore/html/track/TextTrackCue.cpp:469 > > + m_displaySize = std::min(m_cueSize, maximumSize); > > Nit: Adding "using namespace std" at the beginning of the file would let you omit the "std::" here and elsewhere in the file.
I know, but I would prefer to keep it prefixed, unless you believe it breaks readability.
> > Source/WebCore/html/track/TextTrackCue.cpp:476 > > + // 10.7 If the text track cue writing direction is horizontal, then let > > + // width be 'size vw' and height be 'auto'. Otherwise, let width be 'auto' > > + // and height be 'size vh'. (These are CSS values used by the next section > > + // to set CSS properties for the rendering; 'vw' and 'vh' are CSS units.) > > + m_displayWidth = m_writingDirection == Horizontal ? m_displaySize : 0; > > + m_displayHeight = m_writingDirection == Horizontal ? 0 : m_displaySize; > > Does "0" mean "size vw" and "auto"? If so, is there any reason to not have named constants instead?
I added a constant named autoSize, which has the value 0. "size vw" means m_displaySize in vw css units.
> > Source/WebCore/html/track/TextTrackCue.cpp:546 > > - m_displayTree->setShadowPseudoId(AtomicString("-webkit-media-text-track-display"), ASSERT_NO_EXCEPTION); > > + m_displayTree->setShadowPseudoId("-webkit-media-text-track-display", ASSERT_NO_EXCEPTION); > > setShadowPseudoId takes a "const AtomicString&". If this is called frequently it might be worth defining and using a static const.
Done.
> > Source/WebCore/html/track/TextTrackCue.cpp:568 > > + m_displayTree->setInlineStyleProperty( > > + CSSPropertyLeft, > > + getPositionCoordinates().first, > > + CSSPrimitiveValue::CSS_PERCENTAGE); > > I don't think breaking this into multiple lines makes it more readable.
Left it as a single line.
> > Source/WebCore/html/track/TextTrackCue.cpp:573 > > + m_displayTree->setInlineStyleProperty( > > + CSSPropertyTop, > > + getPositionCoordinates().second, > > + CSSPrimitiveValue::CSS_PERCENTAGE); > > Ditto.
Same.
> > Source/WebCore/html/track/TextTrackCue.cpp:583 > > + String translateX = "-" + String::number(getPositionCoordinates().first) + "%"; > > + String translateY = "-" + String::number(getPositionCoordinates().second) + "%"; > > Is it necessary call getPositionCoordinates() twice?
I stored the position coordinates in a local variable.
> > Source/WebCore/html/track/TextTrackCue.cpp:587 > > + m_displayTree->setInlineStyleProperty(CSSPropertyWebkitTransform, > > + webkitTransformTranslateValue); > > Ditto.
Same.
> > Source/WebCore/html/track/TextTrackCue.cpp:619 > > + if (m_writingDirection == Horizontal && m_displayDirection == CSSValueLtr) { > > + coordinates.first = m_textPosition; > > + coordinates.second = m_computedLinePosition; > > + } > > + > > + if (m_writingDirection == Horizontal && m_displayDirection == CSSValueRtl) { > > + coordinates.first = 100 - m_textPosition; > > + coordinates.second = m_computedLinePosition; > > + } > > + > > + if (m_writingDirection == VerticalGrowingLeft) { > > + coordinates.first = 100 - m_computedLinePosition; > > + coordinates.second = m_textPosition; > > + } > > + > > + if (m_writingDirection == VerticalGrowingRight) { > > + coordinates.first = m_computedLinePosition; > > + coordinates.second = m_textPosition; > > + } > > These are all mutually exclusive so you should us an early returns and add an ASSERT_NOT_REACHED at the end.
Added.
> > Source/WebCore/html/track/TextTrackCue.h:152 > > + TotalWritingDirections > > Nit: maybe WritingDirectionCount or NumberOfWritingDirections?
Renamed to NumberOfWritingDirections.
> > LayoutTests/ChangeLog:14 > > + * media/track/captions-webvtt/captions-snap-to-lines-unset.vtt: Added. > > + * media/track/track-cue-rendering-snap-to-lines-unset-expected.txt: Added. > > + * media/track/track-cue-rendering-snap-to-lines-unset.html: Added. > > Nit: I think "track-cue-rendering-snap-to-lines-not-set" would be a better name.
I agree, renamed.
> > LayoutTests/media/media-controls.js:57 > > + throw "There are not " + i + " text track cues visible"; > > Do you mean: "There are not " + cueNumber + " text track cues visible"?
Fixed.
Build Bot
Comment 7
2012-04-16 15:27:34 PDT
Comment on
attachment 137406
[details]
Updated patch
Attachment 137406
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12410879
Eric Carlson
Comment 8
2012-04-17 09:48:53 PDT
Comment on
attachment 137406
[details]
Updated patch View in context:
https://bugs.webkit.org/attachment.cgi?id=137406&action=review
> Source/WebCore/html/track/TextTrackCue.cpp:54 > +static const AtomicString trackDisplayShadowPseudoId = "-webkit-media-text-track-display";
This can't be in the global scope, or it creates an initializer that slows down loading. It is only used in one method so you can define it there, you can use something like: DEFINE_STATIC_LOCAL(const AtomicString, trackDisplayShadowPseudoId, ("-webkit-media-text-track-display"));
Eric Carlson
Comment 9
2012-04-17 10:07:45 PDT
Comment on
attachment 137406
[details]
Updated patch View in context:
https://bugs.webkit.org/attachment.cgi?id=137406&action=review
This looks good modulo the static initializer mentioned previously.
> Source/WebCore/html/track/TextTrackCue.cpp:443 > + m_displayDirection = CSSValueLtr;
Nit: because this part of the algorithm comes from the WebVTT spec, it would be nice to have a URL here.
Victor Carbune
Comment 10
2012-04-18 16:25:47 PDT
Created
attachment 137800
[details]
Patch Updated patch
WebKit Review Bot
Comment 11
2012-04-19 09:27:42 PDT
Comment on
attachment 137800
[details]
Patch Clearing flags on attachment: 137800 Committed
r114640
: <
http://trac.webkit.org/changeset/114640
>
WebKit Review Bot
Comment 12
2012-04-19 09:27:56 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug