Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
898883003:
Fixes play seek when user sets loop after ended. (Closed)
|
Created:
5 years, 10 months ago by chcunningham Modified:
5 years, 10 months ago CC:
blink-reviews, nessy, gasubic, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews-html_chromium.org, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionCalling/clicking play on an ended media element should always cause a seek back to the start. This fixes a edge case where the seek-to-start did not occur if the loop attribute had been set after playback had ended.
BUG=364442
TEST=media/video-loop.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190181
Patch Set 1 : Initial commit #Patch Set 2 : Minor cleanup #
Total comments: 31
Patch Set 3 : Responding to feedback #
Total comments: 4
Patch Set 4 : Responding to feedback #2 #
Total comments: 16
Patch Set 5 : Responding to feedback #3 #
Messages
Total messages: 17 (4 generated)
chcunningham@chromium.org changed reviewers: + philipj@opera.com, wolenetz@chromium.org
Hey Guys, Simple change to fix bug where user sets loop after playback ends and then clicks play. Added layout test coverage. Thanks! Chris
Thanks for working on this, Chris. Other than a bunch of minor nits, and a couple style questions I'm unsure about and deferring to PhilipJ, I'm most curious about endedPlayback() in the context of couldPlayIfEnoughData() (see comment). Also nit: Insert T=media/video-loop.html next to the B= line in CL description. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... File LayoutTests/media/video-loop.html (right): https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:57: consoleWrite("<br><em>++ third seek completed, unset loop and let play to the end.</em>"); nit: s/loop/'loop' https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:70: // Pause now that test is over to prevent re-firing ended and annoying debugging experience. nit: change comment to a consoleWrite to make expectation even clearer (perhaps like ..."pausing playback to prevent another 'ended' while ending test") https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:89: // Testing edge case where we set loop after playback ends. Verifies fix for crbug.com/364442 nit: usually we don't spend code to reference a fixed bug in the [test] code (except in chromium for cases like test media files specific to particular regressions). Curious folks can always see the commit history to figure out which bug these lines were added to verify. (also s/loop/'loop'/ if you keep some reference to the attribute in comment here) https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:90: consoleWrite("<br><em>++ with ended == true, set loop and play again.</em>"); nit: s/loop/'loop' https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:94: // Playback cannot be "ended" when loop attribute is set; It is simply paused. nit: move this part of this comment to the consoleWrite to make expectation even more clear. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:95: // See http://dev.w3.org/html5/spec-preview/media-elements.html#ended-playback nit: No strong feeling here, but I suspect we don't really need to clarify with link to spec definition of HTMLMediaElement.ended here. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:96: testExpected("video.ended", false); nit: there is no explicit check that the video is indeed paused, though comment, above, asserts this. add check please. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:134: <li>When 'seeked' event fires, verify that time has jumped back and movie is playing.</li> nit: seeked() case 2 doesn't explicitly check that time has jumped back relative to the last known currentTime from app's perspective. (e.g. instead maybe check currentTime < duration - 0.4). Strengthen case 2's verification? https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:2305: if (endedPlayback(/* ignore_loop_attr */ true)) nit: I don't see this style of commenting params much in Blink at all. philipj@ should this inline comment simply be dropped? https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3254: bool HTMLMediaElement::couldPlayIfEnoughData() const Should this method also ignore loopAttr w.r.t. endedPlayback calculation? https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3264: return endedPlayback(/* ignore_loop_attr */ false); nit: ditto: drop the inline comment? https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:414: bool endedPlayback() const; There are only a few calls to this method. I'm not convinced the overload is justified versus explicitly passing the ignoreLoopAttr. philipj@: I defer to you on this though.
On 2015/02/12 22:12:27, wolenetz wrote: > Also nit: Insert T=media/video-loop.html next to the B= line in CL description. s/T=/TEST=/ and s/B=/BUG=/...
Thanks Matt https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... File LayoutTests/media/video-loop.html (right): https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:57: consoleWrite("<br><em>++ third seek completed, unset loop and let play to the end.</em>"); On 2015/02/12 22:12:26, wolenetz wrote: > nit: s/loop/'loop' Done. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:70: // Pause now that test is over to prevent re-firing ended and annoying debugging experience. On 2015/02/12 22:12:27, wolenetz wrote: > nit: change comment to a consoleWrite to make expectation even clearer (perhaps > like ..."pausing playback to prevent another 'ended' while ending test") Done. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:89: // Testing edge case where we set loop after playback ends. Verifies fix for crbug.com/364442 On 2015/02/12 22:12:26, wolenetz wrote: > nit: usually we don't spend code to reference a fixed bug in the [test] code > (except in chromium for cases like test media files specific to particular > regressions). Curious folks can always see the commit history to figure out > which bug these lines were added to verify. (also s/loop/'loop'/ if you keep > some reference to the attribute in comment here) Know what you mean, but this comment is pretty tiny and the significance of this test is not obvious. I'd like to keep it so no one comes along and deletes what might initially appear to be an over-engineered test case. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:90: consoleWrite("<br><em>++ with ended == true, set loop and play again.</em>"); On 2015/02/12 22:12:26, wolenetz wrote: > nit: s/loop/'loop' Done. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:94: // Playback cannot be "ended" when loop attribute is set; It is simply paused. On 2015/02/12 22:12:26, wolenetz wrote: > nit: move this part of this comment to the consoleWrite to make expectation even > more clear. Done. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:95: // See http://dev.w3.org/html5/spec-preview/media-elements.html#ended-playback On 2015/02/12 22:12:26, wolenetz wrote: > nit: No strong feeling here, but I suspect we don't really need to clarify with > link to spec definition of HTMLMediaElement.ended here. Done. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:96: testExpected("video.ended", false); On 2015/02/12 22:12:26, wolenetz wrote: > nit: there is no explicit check that the video is indeed paused, though comment, > above, asserts this. add check please. Done. https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:134: <li>When 'seeked' event fires, verify that time has jumped back and movie is playing.</li> On 2015/02/12 22:12:27, wolenetz wrote: > nit: seeked() case 2 doesn't explicitly check that time has jumped back relative > to the last known currentTime from app's perspective. (e.g. instead maybe check > currentTime < duration - 0.4). Strengthen case 2's verification? Done. https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:2305: if (endedPlayback(/* ignore_loop_attr */ true)) On 2015/02/12 22:12:27, wolenetz wrote: > nit: I don't see this style of commenting params much in Blink at all. philipj@ > should this inline comment simply be dropped? Perhaps this never should have been a bool. I'm not a fan of simply dropping the comment... it looks like a setter and is generally much harder to read. I've replaced the bool with an enum to make its purpose/value self documenting. https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3254: bool HTMLMediaElement::couldPlayIfEnoughData() const On 2015/02/12 22:12:27, wolenetz wrote: > Should this method also ignore loopAttr w.r.t. endedPlayback calculation? I don't think so. I've checked usage in 3 different places, and the usage of ended here really means "stopped", like "played the whole video and I'm not looping back to play it again". In this case, I think the default consideration of 'loop' (where loop = true -> not ended, still playing) is correct. https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3264: return endedPlayback(/* ignore_loop_attr */ false); On 2015/02/12 22:12:27, wolenetz wrote: > nit: ditto: drop the inline comment? Now an enum. https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:414: bool endedPlayback() const; On 2015/02/12 22:12:27, wolenetz wrote: > There are only a few calls to this method. I'm not convinced the overload is > justified versus explicitly passing the ignoreLoopAttr. > > philipj@: I defer to you on this though. For me its less about the convenience, more about the self documenting nature of the overload. This neatly expresses a default (considering loop) vs special case (not considering the loop).
fs@opera.com changed reviewers: + fs@opera.com
LGTM w/ a few nits (Philip is OOO for the [extended] weekend just so you know.) https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... File LayoutTests/media/video-loop.html (right): https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:89: // Testing edge case where we set loop after playback ends. Verifies fix for crbug.com/364442 On 2015/02/13 03:37:29, chcunningham wrote: > On 2015/02/12 22:12:26, wolenetz wrote: > > nit: usually we don't spend code to reference a fixed bug in the [test] code > > (except in chromium for cases like test media files specific to particular > > regressions). Curious folks can always see the commit history to figure out > > which bug these lines were added to verify. (also s/loop/'loop'/ if you keep > > some reference to the attribute in comment here) > > Know what you mean, but this comment is pretty tiny and the significance of this > test is not obvious. I'd like to keep it so no one comes along and deletes what > might initially appear to be an over-engineered test case. This whole interchange seems to suggest that you really just should make a separate TC for the bug you're fixing. That ought to address both of your concerns (and it's quite possible that this TC shouldn't have been allowed to balloon like this in the first place.) https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:414: bool endedPlayback() const; On 2015/02/13 03:37:29, chcunningham wrote: > On 2015/02/12 22:12:27, wolenetz wrote: > > There are only a few calls to this method. I'm not convinced the overload is > > justified versus explicitly passing the ignoreLoopAttr. > > > > philipj@: I defer to you on this though. > > For me its less about the convenience, more about the self documenting nature of > the overload. This neatly expresses a default (considering loop) vs special case > (not considering the loop). > It's fairly common in Blink to use default arguments for these cases (unlike in Chromium in general where you wouldn't). Given the documentation/comment in the zero-arity overload this split might be useful though (even if that seems to somewhat contradict your statement about "self documenting" =)) https://codereview.chromium.org/898883003/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/898883003/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3263: // http://dev.w3.org/html5/spec-preview/media-elements.html#ended-playback Make this link https://html.spec.whatwg.org/multipage/embedded-content.html#ended-playback https://codereview.chromium.org/898883003/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/898883003/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:417: enum class LoopFactor { Included, Ignored }; The "Factor" suffix made me go "huh?". I think using non-class enums and named the values "IncludeLoop" and "IgnoreLoop" or some such might've been less noisy. Any other way to eliminate "Factor" would be fine by me though - LoopCondition?
On 2015/02/13 11:49:23, fs wrote: > ... ...and I forgot: A slightly longer commit message wouldn't hurt. (What's there ATM is too terse IMHO - and you only need to be terse on the subject-line ;-))
lgtm % fs's comments and below. I agree overloading of video-loop.html for this additional edge case makes that test case yet more complex, and splitting out this test into a separate case sgtm. https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3254: bool HTMLMediaElement::couldPlayIfEnoughData() const On 2015/02/13 03:37:29, chcunningham wrote: > On 2015/02/12 22:12:27, wolenetz wrote: > > Should this method also ignore loopAttr w.r.t. endedPlayback calculation? > > I don't think so. I've checked usage in 3 different places, and the usage of > ended here really means "stopped", like "played the whole video and I'm not > looping back to play it again". In this case, I think the default consideration > of 'loop' (where loop = true -> not ended, still playing) is correct. Acknowledged. https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:414: bool endedPlayback() const; On 2015/02/13 11:49:22, fs wrote: > On 2015/02/13 03:37:29, chcunningham wrote: > > On 2015/02/12 22:12:27, wolenetz wrote: > > > There are only a few calls to this method. I'm not convinced the overload is > > > justified versus explicitly passing the ignoreLoopAttr. > > > > > > philipj@: I defer to you on this though. > > > > For me its less about the convenience, more about the self documenting nature > of > > the overload. This neatly expresses a default (considering loop) vs special > case > > (not considering the loop). > > > > It's fairly common in Blink to use default arguments for these cases (unlike in > Chromium in general where you wouldn't). Given the documentation/comment in the > zero-arity overload this split might be useful though (even if that seems to > somewhat contradict your statement about "self documenting" =)) Good points. I would prefer default argument, with more documentation at endedPlayback() declaration here about what the argument means.
New patchsets have been uploaded after l-g-t-m from fs@opera.com,wolenetz@chromium.org
Thanks both! https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... File LayoutTests/media/video-loop.html (right): https://codereview.chromium.org/898883003/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-loop.html:89: // Testing edge case where we set loop after playback ends. Verifies fix for crbug.com/364442 On 2015/02/13 11:49:22, fs wrote: > On 2015/02/13 03:37:29, chcunningham wrote: > > On 2015/02/12 22:12:26, wolenetz wrote: > > > nit: usually we don't spend code to reference a fixed bug in the [test] code > > > (except in chromium for cases like test media files specific to particular > > > regressions). Curious folks can always see the commit history to figure out > > > which bug these lines were added to verify. (also s/loop/'loop'/ if you keep > > > some reference to the attribute in comment here) > > > > Know what you mean, but this comment is pretty tiny and the significance of > this > > test is not obvious. I'd like to keep it so no one comes along and deletes > what > > might initially appear to be an over-engineered test case. > > This whole interchange seems to suggest that you really just should make a > separate TC for the bug you're fixing. That ought to address both of your > concerns (and it's quite possible that this TC shouldn't have been allowed to > balloon like this in the first place.) Done. https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:414: bool endedPlayback() const; On 2015/02/13 22:24:23, wolenetz wrote: > On 2015/02/13 11:49:22, fs wrote: > > On 2015/02/13 03:37:29, chcunningham wrote: > > > On 2015/02/12 22:12:27, wolenetz wrote: > > > > There are only a few calls to this method. I'm not convinced the overload > is > > > > justified versus explicitly passing the ignoreLoopAttr. > > > > > > > > philipj@: I defer to you on this though. > > > > > > For me its less about the convenience, more about the self documenting > nature > > of > > > the overload. This neatly expresses a default (considering loop) vs special > > case > > > (not considering the loop). > > > > > > > It's fairly common in Blink to use default arguments for these cases (unlike > in > > Chromium in general where you wouldn't). Given the documentation/comment in > the > > zero-arity overload this split might be useful though (even if that seems to > > somewhat contradict your statement about "self documenting" =)) > > Good points. I would prefer default argument, with more documentation at > endedPlayback() declaration here about what the argument means. Done. https://codereview.chromium.org/898883003/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:414: bool endedPlayback() const; On 2015/02/13 11:49:22, fs wrote: > On 2015/02/13 03:37:29, chcunningham wrote: > > On 2015/02/12 22:12:27, wolenetz wrote: > > > There are only a few calls to this method. I'm not convinced the overload is > > > justified versus explicitly passing the ignoreLoopAttr. > > > > > > philipj@: I defer to you on this though. > > > > For me its less about the convenience, more about the self documenting nature > of > > the overload. This neatly expresses a default (considering loop) vs special > case > > (not considering the loop). > > > > It's fairly common in Blink to use default arguments for these cases (unlike in > Chromium in general where you wouldn't). Given the documentation/comment in the > zero-arity overload this split might be useful though (even if that seems to > somewhat contradict your statement about "self documenting" =)) Acknowledged. https://codereview.chromium.org/898883003/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/898883003/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3263: // http://dev.w3.org/html5/spec-preview/media-elements.html#ended-playback On 2015/02/13 11:49:22, fs wrote: > Make this link > > https://html.spec.whatwg.org/multipage/embedded-content.html#ended-playback Done. Now in the .h since its an explanation of the default value. https://codereview.chromium.org/898883003/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/898883003/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:417: enum class LoopFactor { Included, Ignored }; On 2015/02/13 11:49:22, fs wrote: > The "Factor" suffix made me go "huh?". I think using non-class enums and named > the values "IncludeLoop" and "IgnoreLoop" or some such might've been less noisy. > Any other way to eliminate "Factor" would be fine by me though - LoopCondition? Its like X-Factor :P But sure, LoopCondition it is.
Thanks! lgtm2 % nits: https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... File LayoutTests/media/video-loop-from-ended-expected.txt (right): https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended-expected.txt:1: Test looping edge case to verifies crbug.com/364442. Steps: nit: s/"verifies "/verify http://"/ https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... File LayoutTests/media/video-loop-from-ended.html (right): https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:1: nit: drop blank line https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:14: nit: drop the trailing spaces on this line fyi http://www.chromium.org/developers/sublime-text has some ide configs that could help prevent these in future; vim configs or syntax highlighting can do similar. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:15: consoleWrite("<br><em>++ Video is initially paused and 'loop' unset.</em>"); nit ditto (trailing spaces) https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:19: playWhenReady(); nit: s/play/seekThenPlay/ ? no strong feeling though about this nit. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:29: run("video.currentTime = video.duration - .5"); nit: why .5 versus .4? No strong feeling.. just curious. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:36: function ended() nit ditto (trailing spaces) https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:44: nit: remove extra blank line
Thanks Matt. Enabled the trim spaces setting going fwd. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... File LayoutTests/media/video-loop-from-ended-expected.txt (right): https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended-expected.txt:1: Test looping edge case to verifies crbug.com/364442. Steps: On 2015/02/13 23:47:45, wolenetz wrote: > nit: s/"verifies "/verify http://"/ Done. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... File LayoutTests/media/video-loop-from-ended.html (right): https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:1: On 2015/02/13 23:47:46, wolenetz wrote: > nit: drop blank line Done. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:14: On 2015/02/13 23:47:45, wolenetz wrote: > nit: drop the trailing spaces on this line > fyi http://www.chromium.org/developers/sublime-text has some ide configs that > could help prevent these in future; vim configs or syntax highlighting can do > similar. Done. And added the trim on save to my config. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:15: consoleWrite("<br><em>++ Video is initially paused and 'loop' unset.</em>"); On 2015/02/13 23:47:45, wolenetz wrote: > nit ditto (trailing spaces) Done. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:19: playWhenReady(); On 2015/02/13 23:47:46, wolenetz wrote: > nit: s/play/seekThenPlay/ ? no strong feeling though about this nit. Done. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:29: run("video.currentTime = video.duration - .5"); On 2015/02/13 23:47:45, wolenetz wrote: > nit: why .5 versus .4? No strong feeling.. just curious. This file doesn't reference .4 at all. .4 seemed arbitrary to me. .5 is half a second... just felt less random. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:36: function ended() On 2015/02/13 23:47:45, wolenetz wrote: > nit ditto (trailing spaces) Done. https://codereview.chromium.org/898883003/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-loop-from-ended.html:44: On 2015/02/13 23:47:45, wolenetz wrote: > nit: remove extra blank line Done.
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/898883003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190181
Message was sent while issue was closed.
The fix looks good, but AFAICT this was implemented per spec, so I've filed a spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28032 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
