Bug 79750

Summary: Display a TextTrackCue when snap-to-lines flag is not set
Product: WebKit Reporter: Victor Carbune <vcarbune>
Component: MediaAssignee: Victor Carbune <vcarbune>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric.carlson, feature-media-reviews, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 79747    
Bug Blocks: 79347    
Attachments:
Description Flags
Preliminary patch
none
Archive of layout-test-results from ec2-cr-linux-04
none
Updated patch
none
Patch none

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
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
Updated patch (27.10 KB, patch)
2012-04-16 14:58 PDT, Victor Carbune
no flags
Patch (32.14 KB, patch)
2012-04-18 16:25 PDT, Victor Carbune
no flags
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.