|
|
Created:
7 years, 2 months ago by vcarbune.chromium Modified:
7 years, 2 months ago CC:
blink-reviews, dglazkov+blink, adamk+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionFix cue rendering test and include support for left/right alignment.
The test has been broken for a while somehow, but only because at some
point the code for computing the width got close to the current version
of the WebVTT specification and the alignment has an impact on the width
of the text (so not specifying the alignment determines the default
align:middle and in some cases the width gets zero, e.g. with position:100%)
The specification also got changed in the sense that it also supports
left and right values for cue alignment, which are now included in the test
case and supported in the code.
TEST=Update existing test case.
BUG=299752
R=acolwell,scherkus
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159228
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressed comments #
Total comments: 8
Patch Set 3 : Static const variables #Patch Set 4 : Addressed comments #Patch Set 5 : Remove commented code #
Total comments: 4
Patch Set 6 : Added COMPILE_ASSERT #
Messages
Total messages: 22 (0 generated)
Test fix and several code updates to better match the spec.
On 2013/09/28 12:07:02, vcarbune.chromium wrote: > Test fix and several code updates to better match the spec. Looks good generally. I assume this is to follow the current WebVTT spec at http://dev.w3.org/html5/webvtt/ rather than the newly proposed way at https://dvcs.w3.org/hg/text-tracks/raw-file/default/webvtt/webvtt.html ? nit: I think it might make sense to separate the tests for "line" settings from the "position" settings instead of mixing them in one WebVTT file / one test.
On 2013/09/29 05:25:22, nessy wrote: > https://dvcs.w3.org/hg/text-tracks/raw-file/default/webvtt/webvtt.html respec is broken - look at this instead: https://dvcs.w3.org/hg/text-tracks/raw-file/default/webvtt/Overview.html
On 2013/09/29 08:06:40, nessy wrote: > On 2013/09/29 05:25:22, nessy wrote: > > https://dvcs.w3.org/hg/text-tracks/raw-file/default/webvtt/webvtt.html > > respec is broken - look at this instead: > https://dvcs.w3.org/hg/text-tracks/raw-file/default/webvtt/Overview.html Thanks! Looks like it changed significantly. Are we okay keeping this patch as it is, as it doesn't contain any significant changes (other than left/right)? It also shows that implementation follows that version of the spec, while the test was broken. I also filed this bug to associate patches related to implementation update. http://crbug.com/301580
philipj: Could you take a look at this as well. I'd like to get your perspective since you are much more familiar with the VTT spec than I am.
https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:679: m_computedLinePosition = calculateComputedLinePosition(); Does this really need to be moved to the top of the function or is it sufficient to just move it before the places it is used below. If it doesn't need to be up here then I'd prefer it to be closer to where it is used so that people don't think it is accessed in one of the method calls between here and there. https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:709: } nit: Should we have an } else { ASSERT_NOT_REACHED(); } here to make sure we have all the combinations covered? https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:745: // Cases for m_writingDirection being VerticalGrowing{Left|Right} nit: Please use switch(m_cueAlignment) here and above so the compiler can help us ensure we handle all possible values.
On 2013/10/02 15:42:57, acolwell wrote: > philipj: Could you take a look at this as well. I'd like to get your perspective > since you are much more familiar with the VTT spec than I am. Sure thing. In the archaeology department, I found that the left/right align values were added in September 2012: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17973 As Silvia has already pointed out, rather big changes are happening to the WebVTT rendering spec, see in particular the discussion in https://www.w3.org/Bugs/Public/show_bug.cgi?id=20037 and comment 56 about possible changes to alignment. That being said, I see no reason to not go ahead with this change, it's just that a lot of this code will need to change soon again.
https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:168: else if (m_cue->align() == middleKeyword()) The implementation of TextTrackCue::align() already asserts if the align keywork wasn't one of the known ones, so dropping the default-to-middle here should be safe, but it's not really obvious when looking at this code. The spec expresses this bit of the algorithm as a mapping of the alignment concepts and not the strings themselves, so perhaps this could be converted to a switch over m_cue->getAlignment() to get rid of the unnecessary string conversion/comparison, and also make it clear to the compiler and humans that all cases are handled? One could even go one step further and just convert this to an mapping array from CueAlignment to CSSPropertyTextAlign values, but that might require some static_assert to make the compile fail if things get out of sync. https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:679: m_computedLinePosition = calculateComputedLinePosition(); On 2013/10/02 16:19:37, acolwell wrote: > Does this really need to be moved to the top of the function or is it sufficient > to just move it before the places it is used below. If it doesn't need to be up > here then I'd prefer it to be closer to where it is used so that people don't > think it is accessed in one of the method calls between here and there. In <http://dev.w3.org/html5/webvtt/> it's used in step 10.8, which is step 10.9 in this source file, putting it there would be nice. Or, to never compute it when not needed, one could add a updateComputedLinePosition which both sets m_computedLinePosition and returns it. Victor, was it just a bug in the original implementation that the line position was updated *after* being used in this algorithm, or is it in some way related to the align change? https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:754: // 10.9 Determine the value of whichever of x-position or y-position is not This is step 10.8 in <http://dev.w3.org/html5/webvtt/>, so something is out of sync. Can you add a link somewhere to the exact spec commit that this is now supposed to be in sync with? That's really helpful when checking if there are new spec changes to implement.
https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:168: else if (m_cue->align() == middleKeyword()) On 2013/10/03 09:04:43, philipj wrote: > One could even go one step further and just convert this to an mapping array > from CueAlignment to CSSPropertyTextAlign values, but that might require some > static_assert to make the compile fail if things get out of sync. I added the map and a getCSSAlignment method on TextTrackCue. https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:679: m_computedLinePosition = calculateComputedLinePosition(); On 2013/10/03 09:04:43, philipj wrote: > On 2013/10/02 16:19:37, acolwell wrote: > > Does this really need to be moved to the top of the function or is it > sufficient > > to just move it before the places it is used below. If it doesn't need to be > up > > here then I'd prefer it to be closer to where it is used so that people don't > > think it is accessed in one of the method calls between here and there. > > In <http://dev.w3.org/html5/webvtt/> it's used in step 10.8, which is step 10.9 > in this source file, putting it there would be nice. Or, to never compute it > when not needed, one could add a updateComputedLinePosition which both sets > m_computedLinePosition and returns it. I moved it right before the point where it's used. > Victor, was it just a bug in the original implementation that the line position > was updated *after* being used in this algorithm, or is it in some way related > to the align change? This is probably a bug in the original implementation, but it's use case is not covered by tests (since moving it around doesn't break anything). However, it needs to be computed here as other code paths depend on it (and removing it breaks the test, but doesn't have anything to do with align). Probably I should file a separate bug for proper test coverage of this case and we can leave it before where it's used, or as in the original case. https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:709: } On 2013/10/02 16:19:37, acolwell wrote: > nit: Should we have an } else { ASSERT_NOT_REACHED(); } here to make sure we > have all the combinations covered? Done. https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:745: // Cases for m_writingDirection being VerticalGrowing{Left|Right} On 2013/10/02 16:19:37, acolwell wrote: > nit: Please use switch(m_cueAlignment) here and above so the compiler can help > us ensure we handle all possible values. Done. I had to add a default case though because of the map I added; I understand the idea would be to not have it so the compiler shouts if not covered; I can revert this or remove the map, if you prefer, otherwise we have this debug runtime assert. https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:754: // 10.9 Determine the value of whichever of x-position or y-position is not On 2013/10/03 09:04:43, philipj wrote: > This is step 10.8 in <http://dev.w3.org/html5/webvtt/>, so something is out of > sync. Can you add a link somewhere to the exact spec commit that this is now > supposed to be in sync with? That's really helpful when checking if there are > new spec changes to implement. Looking at the dates when this was submitted, looks like r7059. Is there an easy way to see the whole spec at a given revision number? I'm tempted to say it might be a manual numbering error, step 10.7 is missing since the moment these lines were submitted.
lgtm % nits https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.cpp:744: default: nit: You only need default here because of NumberOfAlignments right? If so just s/default/NumberOfAlignments/ here and below. That way if a new value is added we get a compile error instead of a runtime error. https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... File Source/core/html/track/TextTrackCue.h (right): https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.h:276: CSSValueID m_displayAlignmentMap[NumberOfAlignments]; nit: You should be able to make this static const since CSSValueID is a simple type. I believe this should work for m_displayWritingModeMap as well.
https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:754: // 10.9 Determine the value of whichever of x-position or y-position is not On 2013/10/04 13:30:26, vcarbune.chromium wrote: > On 2013/10/03 09:04:43, philipj wrote: > > This is step 10.8 in <http://dev.w3.org/html5/webvtt/>, so something is out of > > sync. Can you add a link somewhere to the exact spec commit that this is now > > supposed to be in sync with? That's really helpful when checking if there are > > new spec changes to implement. > > Looking at the dates when this was submitted, looks like r7059. Is there an easy > way to see the whole spec at a given revision number? > > I'm tempted to say it might be a manual numbering error, step 10.7 is missing > since the moment these lines were submitted. For HTML there's <https://github.com/whatwg/html-mirror>, which you can check out and bisect locally to find stuff. For WebVTT you can browse <https://dvcs.w3.org/hg/text-tracks/> and then take the hash of a commit to get to a URL like <https://dvcs.w3.org/hg/text-tracks/raw-file/6803fbcb723d/webvtt/webvtt.html>. However, it would be a lot of work to verify that the implementation is in sync with any particular version, so if you're not already sure then it can wait until someone can do a full spec-syncing pass. https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.cpp:744: default: On 2013/10/04 17:26:41, acolwell wrote: > nit: You only need default here because of NumberOfAlignments right? If so just > s/default/NumberOfAlignments/ here and below. That way if a new value is added > we get a compile error instead of a runtime error. Isn't it a good idea to catch the case where someone has made a funny cast and ends up with some value that isn't even one of the enum values?
Something seems a bit strange with patch set 4, if I download the raw diff and apply it I see this in TextTrackCue.cpp: +/* +const CSSValueID TextTrackCue::displayWritingModeMap[NumberOfWritingDirections] = { + CSSValueHorizontalTb, CSSValueVerticalRl, CSSValueVerticalLr +}; + +const CSSValueID TextTrackCue::displayAlignmentMap[NumberOfAlignments] = { + CSSValueStart, CSSValueCenter, CSSValueEnd, CSSValueLeft, CSSValueRight +}; +*/ Is that going to be committed, or am I just unfamiliar with Rietveld?
The commented code left there is my fault, sorry. I removed it now, PTAL. https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTr... Source/core/html/track/TextTrackCue.cpp:754: // 10.9 Determine the value of whichever of x-position or y-position is not On 2013/10/07 08:52:31, philipj wrote: > On 2013/10/04 13:30:26, vcarbune.chromium wrote: > > On 2013/10/03 09:04:43, philipj wrote: > > > This is step 10.8 in <http://dev.w3.org/html5/webvtt/>, so something is out > of > > > sync. Can you add a link somewhere to the exact spec commit that this is now > > > supposed to be in sync with? That's really helpful when checking if there > are > > > new spec changes to implement. > > > > Looking at the dates when this was submitted, looks like r7059. Is there an > easy > > way to see the whole spec at a given revision number? > > > > I'm tempted to say it might be a manual numbering error, step 10.7 is missing > > since the moment these lines were submitted. > > For HTML there's <https://github.com/whatwg/html-mirror>, which you can check > out and bisect locally to find stuff. > > For WebVTT you can browse <https://dvcs.w3.org/hg/text-tracks/> and then take > the hash of a commit to get to a URL like > <https://dvcs.w3.org/hg/text-tracks/raw-file/6803fbcb723d/webvtt/webvtt.html>. > > However, it would be a lot of work to verify that the implementation is in sync > with any particular version, so if you're not already sure then it can wait > until someone can do a full spec-syncing pass. Thanks for the explanation, I think this can be easily done in a different (dedicated) pass, so I just added a FIXME in the code for now. https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.cpp:744: default: On 2013/10/04 17:26:41, acolwell wrote: > nit: You only need default here because of NumberOfAlignments right? If so just > s/default/NumberOfAlignments/ here and below. That way if a new value is added > we get a compile error instead of a runtime error. Done. https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.cpp:744: default: On 2013/10/07 08:52:32, philipj wrote: > On 2013/10/04 17:26:41, acolwell wrote: > > nit: You only need default here because of NumberOfAlignments right? If so > just > > s/default/NumberOfAlignments/ here and below. That way if a new value is added > > we get a compile error instead of a runtime error. > > Isn't it a good idea to catch the case where someone has made a funny cast and > ends up with some value that isn't even one of the enum values? I feel that the current version of the code guards this quite well, since the cue alignment should only be set in setAlign() https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... File Source/core/html/track/TextTrackCue.h (right): https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.h:276: CSSValueID m_displayAlignmentMap[NumberOfAlignments]; On 2013/10/04 17:26:41, acolwell wrote: > nit: You should be able to make this static const since CSSValueID is a simple > type. I believe this should work for m_displayWritingModeMap as well. Nice catch. Done.
On 2013/10/07 09:37:27, vcarbune.chromium wrote: > The commented code left there is my fault, sorry. > I removed it now, PTAL. Thanks, it's gone from the raw diff of patch set 5.
https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.cpp:744: default: On 2013/10/07 08:52:32, philipj wrote: > On 2013/10/04 17:26:41, acolwell wrote: > > nit: You only need default here because of NumberOfAlignments right? If so > just > > s/default/NumberOfAlignments/ here and below. That way if a new value is added > > we get a compile error instead of a runtime error. > > Isn't it a good idea to catch the case where someone has made a funny cast and > ends up with some value that isn't even one of the enum values? We typically guard against this at the assignment points instead of where it is evaluated to cut down on the "double check that it is still valid" code that gets sprinkled around everywhere. In general we try to minimize casting as much as possible so that we can let the compiler help us detect bugs when new enum values are added.
https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.cpp:744: default: On 2013/10/07 15:32:19, acolwell wrote: > On 2013/10/07 08:52:32, philipj wrote: > > On 2013/10/04 17:26:41, acolwell wrote: > > > nit: You only need default here because of NumberOfAlignments right? If so > > just > > > s/default/NumberOfAlignments/ here and below. That way if a new value is > added > > > we get a compile error instead of a runtime error. > > > > Isn't it a good idea to catch the case where someone has made a funny cast and > > ends up with some value that isn't even one of the enum values? > > We typically guard against this at the assignment points instead of where it is > evaluated to cut down on the "double check that it is still valid" code that > gets sprinkled around everywhere. In general we try to minimize casting as much > as possible so that we can let the compiler help us detect bugs when new enum > values are added. Sounds reasonable, I'll keep that in mind when writing patches myself.
Ping for an owner review (and maybe lgtm) from (auto) CCed owners.
https://codereview.chromium.org/25155003/diff/33001/Source/core/html/track/Te... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/33001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.cpp:58: static const CSSValueID displayWritingModeMap[TextTrackCue::NumberOfWritingDirections] = { Putting NumberOfWritingDirections inside the brackets isn't doing much for you here. Better would be to use empty brackets ('[]') and then add a COMPILE_ASSERT that WTF_ARRAY_LENGTH(displayWritingModeMap) is equal to NumberOfWritingDirections. https://codereview.chromium.org/25155003/diff/33001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.cpp:62: static const CSSValueID displayAlignmentMap[TextTrackCue::NumberOfAlignments] = { Same here as above.
https://codereview.chromium.org/25155003/diff/33001/Source/core/html/track/Te... File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/33001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.cpp:58: static const CSSValueID displayWritingModeMap[TextTrackCue::NumberOfWritingDirections] = { On 2013/10/08 19:46:10, adamk wrote: > Putting NumberOfWritingDirections inside the brackets isn't doing much for you > here. Better would be to use empty brackets ('[]') and then add a COMPILE_ASSERT > that WTF_ARRAY_LENGTH(displayWritingModeMap) is equal to > NumberOfWritingDirections. Nice, I didn't use it before. Thanks. https://codereview.chromium.org/25155003/diff/33001/Source/core/html/track/Te... Source/core/html/track/TextTrackCue.cpp:62: static const CSSValueID displayAlignmentMap[TextTrackCue::NumberOfAlignments] = { On 2013/10/08 19:46:10, adamk wrote: > Same here as above. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vcarbune@chromium.org/25155003/43001
Message was sent while issue was closed.
Change committed as 159228 |