| Summary: | Display a TextTrackCue when snap-to-lines flag is not set | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Victor Carbune <vcarbune> | ||||||||||
| Component: | Media | Assignee: | 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
Victor Carbune
2012-02-27 23:29:14 PST
Created attachment 137340 [details]
Preliminary patch
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 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
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"? Created attachment 137406 [details]
Updated patch
(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. Comment on attachment 137406 [details] Updated patch Attachment 137406 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12410879 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")); 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. Created attachment 137800 [details]
Patch
Updated patch
Comment on attachment 137800 [details] Patch Clearing flags on attachment: 137800 Committed r114640: <http://trac.webkit.org/changeset/114640> All reviewed patches have been landed. Closing bug. |