Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
224833002:
Implement DataCue interface. (Closed)
|
Created:
6 years, 8 months ago by Brendan Long Modified:
6 years, 4 months ago CC:
blink-reviews, nessy, arv+blink, jsbell+bindings_chromium.org, marja+watch_chromium.org, kouhei+bindings_chromium.org, gasubic, eric.carlson_apple.com, abarth-chromium, feature-media-reviews_chromium.org, dglazkov+blink, adamk+blink_chromium.org, kojih, haraken, watchdog-blink-watchlist_google.com, Nate Chapin, vcarbune.chromium, Inactive Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionImplement DataCue interface.
BUG=359746
Patch Set 1 #
Total comments: 32
Patch Set 2 : Updated based on reviews so we'll be ready if/when this gets approved by WHATWG #Patch Set 3 : Use TypeChecking IDL extended attribute #Patch Set 4 : Remove text attribute #
Messages
Total messages: 19 (0 generated)
Can you review this? The layout test is copied directly from WebKit (with one change, using testDOMException instead of testException): https://svn.webkit.org/repository/webkit/trunk/LayoutTests/media/track/track-... Let me know if I need to send an "intent to implement".
LGTM for bindings/.
I'm not aware of a use case for DataCue for normal desktop or mobile browsers, so if this is accepted I think this should be runtime-enabled until the time that it makes sense to enable it on all platforms.
https://codereview.chromium.org/224833002/diff/1/LayoutTests/media/video-test.js File LayoutTests/media/video-test.js (right): https://codereview.chromium.org/224833002/diff/1/LayoutTests/media/video-test... LayoutTests/media/video-test.js:220: "TEST(" + testString + ") THROWS(" + exceptionString + ": " + (exception ? exception.message : undefined) + ")"); Unrelated change? https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:63: exceptionState.throwDOMException(InvalidNodeTypeError, "DataCue.data must be a non-null ArrayBuffer"); [StrictTypeChecking] in the IDL should take care of this. https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:71: return String(); "The text attribute, on getting, must return UTF-16 text converted from data of the text track cue that the TextTrackCue object represents. If a conversion of the content in data is not possible, e.g. because the UA is unable to identify the encoding, it must return null." I think it doesn't make sense to expose the property at all if it always returns null. (I argued when Silvia was spec'ing this that the property itself doesn't make sense and is just a convenience we could do without.) https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... File Source/core/html/track/TextTrack.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... Source/core/html/track/TextTrack.cpp:241: exceptionState.throwDOMException(InvalidNodeTypeError, "DataCues can only be added to tracks with kind='metadata'"); The spec does say this, but it doesn't make sense. TextTrack.kind is readonly, but it can change if the TextTrack is backed by a <track> element, and acolwell wants to make the attribute readwrite for MSE. Also, is there any code that is made simpler if DataCue implies kind=metadata? If not it seems the spec should change. https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... File Source/core/html/track/TextTrackCue.h (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... Source/core/html/track/TextTrackCue.h:89: virtual CueType cueType() const = 0; isVTTCue would be more in line with how the Node hierachy type testing/casting works.
On 2014/04/04 14:35:29, philipj wrote: > I'm not aware of a use case for DataCue for normal desktop or mobile browsers It's not currently used in your media player, but DataCues can also be used in JavaScript. It would be useful in any case where you want JavaScript to create cues with binary data. Polyfills are a good example. Another would be when you have binary data and you don't want to waste time decoding it until it's time to use it.
https://codereview.chromium.org/224833002/diff/1/LayoutTests/media/video-test.js File LayoutTests/media/video-test.js (right): https://codereview.chromium.org/224833002/diff/1/LayoutTests/media/video-test... LayoutTests/media/video-test.js:220: "TEST(" + testString + ") THROWS(" + exceptionString + ": " + (exception ? exception.message : undefined) + ")"); On 2014/04/04 14:54:33, philipj wrote: > Unrelated change? This was necessary to make the test not crash if it fails (previously, if no exception was through, 'exception' would be undefined and 'exception.message' would cause a crash). I changed this to make it easier to write the tests, but I think it's reasonable to leave it in place. I can remove it if you don't think it should be here though. https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:63: exceptionState.throwDOMException(InvalidNodeTypeError, "DataCue.data must be a non-null ArrayBuffer"); On 2014/04/04 14:54:33, philipj wrote: > [StrictTypeChecking] in the IDL should take care of this. I tried that and it still accepted null and strings (but correctly refused to use numbers). https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:71: return String(); On 2014/04/04 14:54:33, philipj wrote: > "The text attribute, on getting, must return UTF-16 text converted from data of > the text track cue that the TextTrackCue object represents. If a conversion of > the content in data is not possible, e.g. because the UA is unable to identify > the encoding, it must return null." > > I think it doesn't make sense to expose the property at all if it always returns > null. (I argued when Silvia was spec'ing this that the property itself doesn't > make sense and is just a convenience we could do without.) My problem is, how do we identify the encoding? As far as I can tell, the String constructors expect us to already know the encoding before we use them. I guess there's probably a function somewhere that does that for HTML documents, but I suspect cues are short enough that we'd get a lot of false positives. It seems like the .text attribute could be useful in cases where the UA is creating DataCues, and knows that the content is text, but Chromium doesn't (won't?) do that. https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... File Source/core/html/track/TextTrack.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... Source/core/html/track/TextTrack.cpp:241: exceptionState.throwDOMException(InvalidNodeTypeError, "DataCues can only be added to tracks with kind='metadata'"); On 2014/04/04 14:54:33, philipj wrote: > The spec does say this, but it doesn't make sense. TextTrack.kind is readonly, > but it can change if the TextTrack is backed by a <track> element, and acolwell > wants to make the attribute readwrite for MSE. Also, is there any code that is > made simpler if DataCue implies kind=metadata? If not it seems the spec should > change. That makes sense. I'll try bring it up on the list again. https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... File Source/core/html/track/TextTrackCue.h (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... Source/core/html/track/TextTrackCue.h:89: virtual CueType cueType() const = 0; On 2014/04/04 14:54:33, philipj wrote: > isVTTCue would be more in line with how the Node hierachy type testing/casting > works. So, remove CueType, and add a 'bool isVTTCue()'? Do we also want a 'bool isDataCue()', or should we just assume that anything that isn't a VTTCue is a DataCue?
https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... File Source/core/html/track/TextTrack.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... Source/core/html/track/TextTrack.cpp:241: exceptionState.throwDOMException(InvalidNodeTypeError, "DataCues can only be added to tracks with kind='metadata'"); On 2014/04/04 14:54:33, philipj wrote: > The spec does say this, but it doesn't make sense. TextTrack.kind is readonly, > but it can change if the TextTrack is backed by a <track> element, and acolwell > wants to make the attribute readwrite for MSE. Also, is there any code that is > made simpler if DataCue implies kind=metadata? If not it seems the spec should > change. New bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25261
https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:63: exceptionState.throwDOMException(InvalidNodeTypeError, "DataCue.data must be a non-null ArrayBuffer"); On 2014/04/04 15:07:22, Brendan Long wrote: > On 2014/04/04 14:54:33, philipj wrote: > > [StrictTypeChecking] in the IDL should take care of this. > > I tried that and it still accepted null and strings (but correctly refused to > use numbers). Sounds like an issue with the bindings generator - I uploaded https://codereview.chromium.org/226073003 which might help (the case with string still sounds odd though.)
https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:63: exceptionState.throwDOMException(InvalidNodeTypeError, "DataCue.data must be a non-null ArrayBuffer"); On 2014/04/04 17:10:35, fs wrote: > Sounds like an issue with the bindings generator - I uploaded > https://codereview.chromium.org/226073003 which might help (the case with string > still sounds odd though.) That looks like it would fix the null case. For strings, I think the issue is that the parameter version of [StrictTypeChecking] is different: > [StrictTypeChecking] can also be applied to a DOMString parameter in an overloaded method to make the overload resolution only match for ECMAScript types null, undefined, string or object, but not number or boolean. http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Strict... If I use this WebIDL: [ Constructor(double startTime, double endTime, [StrictTypeChecking] ArrayBuffer data), ConstructorCallWith=ExecutionContext, ] interface DataCue : TextTrackCue { [StrictTypeChecking] attribute ArrayBuffer data; readonly attribute DOMString? text; }; Then I get exceptions if I try to set .data to anything except null or an ArrayBuffer, but pretty much anything works for for the constructor (my layout test tries passing null, a string, and an array, and all three pass; I also tried a number and that also works without an exception).
On 2014/04/04 17:43:05, Brendan Long wrote: > https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... > File Source/core/html/track/DataCue.cpp (right): > > https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... > Source/core/html/track/DataCue.cpp:63: > exceptionState.throwDOMException(InvalidNodeTypeError, "DataCue.data must be a > non-null ArrayBuffer"); > On 2014/04/04 17:10:35, fs wrote: > > Sounds like an issue with the bindings generator - I uploaded > > https://codereview.chromium.org/226073003 which might help (the case with > string > > still sounds odd though.) > > That looks like it would fix the null case. For strings, I think the issue is > that the parameter version of [StrictTypeChecking] is different: > > > [StrictTypeChecking] can also be applied to a DOMString parameter in an > overloaded method to make the overload resolution only match for ECMAScript > types null, undefined, string or object, but not number or boolean. > > http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Strict... Not familiar with the overload resolution handling, but I suspect that's not the issue here. See below. > If I use this WebIDL: > > [ > Constructor(double startTime, double endTime, [StrictTypeChecking] > ArrayBuffer data), > ConstructorCallWith=ExecutionContext, > ] interface DataCue : TextTrackCue { > [StrictTypeChecking] attribute ArrayBuffer data; > readonly attribute DOMString? text; > }; I think you could put StrictTypeChecking on the interface-level here (and only there). Quite possible that StrictTypeChecking isn't transferred to the constructor description though even in that case (ought to be easy to fix though if not.) > Then I get exceptions if I try to set .data to anything except null or an > ArrayBuffer, but pretty much anything works for for the constructor (my layout > test tries passing null, a string, and an array, and all three pass; I also > tried a number and that also works without an exception). Right, because the attribute doesn't have any effect (on type-checking) in that context.
On 2014/04/03 23:37:56, Brendan Long wrote: > Can you review this? > > The layout test is copied directly from WebKit (with one change, using > testDOMException instead of testException): > > https://svn.webkit.org/repository/webkit/trunk/LayoutTests/media/track/track-... > > Let me know if I need to send an "intent to implement". I haven't had a chance to look at this yet, but I plan to do so today. I think you should send out an Intent to Implement so that the wider community becomes aware of this feature being added. From the comments so far, it sounds like the spec may not be stable yet so I'd prefer to have this behind a runtime flag initially. While I haven't looked at the code yet, I have concerns about the text attribute mentioned in the spec. It seems like the DataCue() constructor should have some sort of encoding parameter so that it would be clear whether it should be possible to convert data to text or not. I think this would unify behavior between objects created by the UA and ones created by JS. It seems like doing that or removing the text attribute all together would be better than what is there now.
https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.h (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.h:45: return adoptRef(new DataCue(context, start, end, data, exceptionState)); Argument validation could with benefit be put here -- if the conversion into an ArrayBuffer fails, 'data' will be 0, which you need to check for (and throw TypeError) before constructing the instance. But not inline here in the class declaration.
https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.h (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.h:45: return adoptRef(new DataCue(context, start, end, data, exceptionState)); On 2014/04/04 19:11:31, sof wrote: > Argument validation could with benefit be put here -- if the conversion into an > ArrayBuffer fails, 'data' will be 0, which you need to check for (and throw > TypeError) before constructing the instance. But not inline here in the class > declaration. The check is already done in setData() (which is used by the constructor), but I could put an additional one here if you think that's better.
On 2014/04/04 19:37:50, Brendan Long wrote: > https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... > File Source/core/html/track/DataCue.h (right): > > https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... > Source/core/html/track/DataCue.h:45: return adoptRef(new DataCue(context, start, > end, data, exceptionState)); > On 2014/04/04 19:11:31, sof wrote: > > Argument validation could with benefit be put here -- if the conversion into > an > > ArrayBuffer fails, 'data' will be 0, which you need to check for (and throw > > TypeError) before constructing the instance. But not inline here in the class > > declaration. > > The check is already done in setData() (which is used by the constructor), but I > could put an additional one here if you think that's better. I think that would be better (and throw TypeError); the instance should only be constructed if the constructor arguments are valid. You also need to check that the 'double' arguments follow http://heycam.github.io/webidl/#es-double
On 2014/04/04 14:55:01, Brendan Long wrote: > On 2014/04/04 14:35:29, philipj wrote: > > I'm not aware of a use case for DataCue for normal desktop or mobile browsers > > It's not currently used in your media player, but DataCues can also be used in > JavaScript. It would be useful in any case where you want JavaScript to create > cues with binary data. Polyfills are a good example. Another would be when you > have binary data and you don't want to waste time decoding it until it's time to > use it. You can trivially store binary data in VTTCue by either packing two bytes per code point or by using some encoding scheme. Perhaps this kind of discussion is best had in an Intent to Implement thread, however.
https://codereview.chromium.org/224833002/diff/1/LayoutTests/media/track/trac... File LayoutTests/media/track/track-datacue.html (right): https://codereview.chromium.org/224833002/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-datacue.html:46: testDOMException("cue.data = null", "DOMException.INVALID_NODE_TYPE_ERR"); Shouldn't all these invalid assignments be TypeErrors according to WebIDL rules? https://codereview.chromium.org/224833002/diff/1/Source/bindings/v8/custom/V8... File Source/bindings/v8/custom/V8TextTrackCueCustom.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/bindings/v8/custom/V8... Source/bindings/v8/custom/V8TextTrackCueCustom.cpp:45: default: nit: Please remove default: code so a compiler error will occur if a new cue type is added w/o this code getting updated. https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:2: * Copyright (c) 2014, CableLabs Inc. All rights reserved. nit: This should use the Chromium header in the Blink style guide. https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:36: : TextTrackCue(startTime, endTime) I'm surprised that there doesn't appear to be any logic to validate that startTime <= endTime. Are these really supposed to be allowed by the spec? https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:37: , m_executionContext(context) DataCue should probably from ContextLifecycleObserver if it wants to safely hold onto tihs context pointer. https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:63: exceptionState.throwDOMException(InvalidNodeTypeError, "DataCue.data must be a non-null ArrayBuffer"); Should this be exceptionState.throwTypeError() instead? https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:71: return String(); On 2014/04/04 15:07:22, Brendan Long wrote: > On 2014/04/04 14:54:33, philipj wrote: > > "The text attribute, on getting, must return UTF-16 text converted from data > of > > the text track cue that the TextTrackCue object represents. If a conversion of > > the content in data is not possible, e.g. because the UA is unable to identify > > the encoding, it must return null." > > > > I think it doesn't make sense to expose the property at all if it always > returns > > null. (I argued when Silvia was spec'ing this that the property itself doesn't > > make sense and is just a convenience we could do without.) > > My problem is, how do we identify the encoding? As far as I can tell, the String > constructors expect us to already know the encoding before we use them. I guess > there's probably a function somewhere that does that for HTML documents, but I > suspect cues are short enough that we'd get a lot of false positives. > > It seems like the .text attribute could be useful in cases where the UA is > creating DataCues, and knows that the content is text, but Chromium doesn't > (won't?) do that. I agree with Philip here. It seems odd that we would have to always return null. At a minimum, I'd expect some sort of encoding string to signal whether it was valid to convert data to text or not. As is this attribute doesn't seem very useful. https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.h (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.h:2: * Copyright (c) 2014, CableLabs Inc. All rights reserved. nit: New files should have the Chromium header mentioned in the style guide (http://dev.chromium.org/blink/coding-style#TOC-License) https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.h:70: // DataCue is currently the only TextTrackCue subclass. nit: Remove comment since it is a lie. :) https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.idl (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.idl:2: * Copyright (c) 2014, CableLabs Inc. All rights reserved. nit: This should use the Chromium header in the Blink style guide. https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... File Source/core/html/track/TextTrack.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/TextT... Source/core/html/track/TextTrack.cpp:238: // track kind set to metadata, throw a InvalidNodeTypeError exception and don't add the cue to the TextTrackList nit: I think there is a typo in the spec. s/TextTrackList/TextTrackCueList/ https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/vtt/V... File Source/core/html/track/vtt/VTTCue.h (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/vtt/V... Source/core/html/track/vtt/VTTCue.h:192: // VTTCue is currently the only TextTrackCue subclass. nit: Please remove this comment now that it is no longer true.
Here's the intent to implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/U06zrT2N-Xk
https://codereview.chromium.org/224833002/diff/1/Source/bindings/v8/custom/V8... File Source/bindings/v8/custom/V8TextTrackCueCustom.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/bindings/v8/custom/V8... Source/bindings/v8/custom/V8TextTrackCueCustom.cpp:45: default: On 2014/04/04 23:25:27, acolwell_OOO_5-24_to_5-30 wrote: > nit: Please remove default: code so a compiler error will occur if a new cue > type is added w/o this code getting updated. I get compile warnings if I remove it: ./../third_party/WebKit/Source/bindings/v8/custom/V8TextTrackCueCustom.cpp: In function ‘v8::Handle<v8::Value> WebCore::toV8(WebCore::TextTrackCue*, v8::Handle<v8::Object>, v8::Isolate*)’: ../../third_party/WebKit/Source/bindings/v8/custom/V8TextTrackCueCustom.cpp:46:1: warning: control reaches end of non-void function [-Wreturn-type] } ^ https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:36: : TextTrackCue(startTime, endTime) On 2014/04/04 23:25:27, acolwell_OOO_5-24_to_5-30 wrote: > I'm surprised that there doesn't appear to be any logic to validate that > startTime <= endTime. Are these really supposed to be allowed by the spec? I don't see anything about that: http://www.w3.org/html/wg/drafts/html/CR/embedded-content-0.html#dom-texttrac... https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:71: return String(); On 2014/04/04 23:25:27, acolwell_OOO_5-24_to_5-30 wrote: > On 2014/04/04 15:07:22, Brendan Long wrote: > > On 2014/04/04 14:54:33, philipj wrote: > > > "The text attribute, on getting, must return UTF-16 text converted from data > > of > > > the text track cue that the TextTrackCue object represents. If a conversion > of > > > the content in data is not possible, e.g. because the UA is unable to > identify > > > the encoding, it must return null." > > > > > > I think it doesn't make sense to expose the property at all if it always > > returns > > > null. (I argued when Silvia was spec'ing this that the property itself > doesn't > > > make sense and is just a convenience we could do without.) > > > > My problem is, how do we identify the encoding? As far as I can tell, the > String > > constructors expect us to already know the encoding before we use them. I > guess > > there's probably a function somewhere that does that for HTML documents, but I > > suspect cues are short enough that we'd get a lot of false positives. > > > > It seems like the .text attribute could be useful in cases where the UA is > > creating DataCues, and knows that the content is text, but Chromium doesn't > > (won't?) do that. > > I agree with Philip here. It seems odd that we would have to always return null. > At a minimum, I'd expect some sort of encoding string to signal whether it was > valid to convert data to text or not. As is this attribute doesn't seem very > useful. There was some discussion about removing .text, but I can't find a W3C bug for it :\
https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... File Source/core/html/track/DataCue.cpp (right): https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:36: : TextTrackCue(startTime, endTime) On 2014/05/28 16:31:48, Brendan Long wrote: > On 2014/04/04 23:25:27, acolwell_OOO_5-24_to_5-30 wrote: > > I'm surprised that there doesn't appear to be any logic to validate that > > startTime <= endTime. Are these really supposed to be allowed by the spec? > > I don't see anything about that: > > http://www.w3.org/html/wg/drafts/html/CR/embedded-content-0.html#dom-texttrac... Right, there is no such restriction. Negative times are also allowed, although Blink doesn't support it yet ("TODO(93143): Add spec-compliant behavior for negative time values.") https://codereview.chromium.org/224833002/diff/1/Source/core/html/track/DataC... Source/core/html/track/DataCue.cpp:71: return String(); On 2014/05/28 16:31:48, Brendan Long wrote: > On 2014/04/04 23:25:27, acolwell_OOO_5-24_to_5-30 wrote: > > On 2014/04/04 15:07:22, Brendan Long wrote: > > > On 2014/04/04 14:54:33, philipj wrote: > > > > "The text attribute, on getting, must return UTF-16 text converted from > data > > > of > > > > the text track cue that the TextTrackCue object represents. If a > conversion > > of > > > > the content in data is not possible, e.g. because the UA is unable to > > identify > > > > the encoding, it must return null." > > > > > > > > I think it doesn't make sense to expose the property at all if it always > > > returns > > > > null. (I argued when Silvia was spec'ing this that the property itself > > doesn't > > > > make sense and is just a convenience we could do without.) > > > > > > My problem is, how do we identify the encoding? As far as I can tell, the > > String > > > constructors expect us to already know the encoding before we use them. I > > guess > > > there's probably a function somewhere that does that for HTML documents, but > I > > > suspect cues are short enough that we'd get a lot of false positives. > > > > > > It seems like the .text attribute could be useful in cases where the UA is > > > creating DataCues, and knows that the content is text, but Chromium doesn't > > > (won't?) do that. > > > > I agree with Philip here. It seems odd that we would have to always return > null. > > At a minimum, I'd expect some sort of encoding string to signal whether it was > > valid to convert data to text or not. As is this attribute doesn't seem very > > useful. > > There was some discussion about removing .text, but I can't find a W3C bug for > it :\ It's been removed now: https://github.com/w3c/html/commit/d5ec35198fea01e56b0ec38eedce58a57a013138 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
