|
Description
Jer Noble
2013-11-06 14:14:10 PST
Created attachment 297227 [details]
Patch
Comment on attachment 297227 [details]
Patch
Nice! r=me
Comment on attachment 297227 [details] Patch Attachment 297227 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2731653 New failing tests: media/track/w3c/interfaces/TextTrack/language.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/language.html Created attachment 297240 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 297227 [details] Patch Attachment 297227 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2731662 New failing tests: media/track/w3c/interfaces/TextTrack/language.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/language.html Created attachment 297243 [details]
Archive of layout-test-results from ews117 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 297227 [details] Patch Attachment 297227 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2731722 New failing tests: media/track/w3c/interfaces/TextTrack/language.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/language.html Created attachment 297246 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 297227 [details] Patch Attachment 297227 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2731793 New failing tests: media/only-bcp47-language-tags-accepted.html Created attachment 297255 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
I believe the W3C platform test is wrong (html/semantics/embedded-content/media-elements/interfaces/TextTrack/language.html). Pull request: https://github.com/w3c/web-platform-tests/pull/4359 Comment on attachment 297227 [details]
Patch
Based on discussion on the spec, I do not think we should land as is. The W3C test seems correct and should not fail.
After a discussion with Jer and Chris I will change this patch to only validate for internal purposes. This will allow the existing Web Platform test to continue working. Created attachment 297503 [details]
Patch
Need a new review, please. Comment on attachment 297503 [details] Patch Attachment 297503 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2757285 New failing tests: imported/w3c/web-platform-tests/html/dom/reflection-embedded.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/HTMLElement/HTMLTrackElement/srclang.html Created attachment 297505 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 297503 [details] Patch Attachment 297503 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2757288 New failing tests: imported/w3c/web-platform-tests/html/dom/reflection-embedded.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/HTMLElement/HTMLTrackElement/srclang.html Created attachment 297506 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 297503 [details] Patch Attachment 297503 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2757284 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/HTMLElement/HTMLTrackElement/srclang.html Created attachment 297508 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 297503 [details] Patch Attachment 297503 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2757291 New failing tests: imported/w3c/web-platform-tests/html/dom/reflection-embedded.html media/only-bcp47-language-tags-accepted-as-valid.html Created attachment 297509 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 297503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297503&action=review > Source/WebCore/html/HTMLMediaElement.cpp:3777 > + LOG(Media, "HTMLMediaElement::configureTextTrackGroup(%p) - '%s' track with language '%s' and BCP 47 language '%s' has score %i", this, textTrack->kindKeyword().string().utf8().data(), textTrack->language().string().utf8().data(), textTrack->validBCP47Language().string().utf8().data(), trackScore); Nit: Although the format specifier %i is equivalent to %d for formatting a signed decimal number, a quick grep over the WebCore source code shows that we prefer %d. > Source/WebCore/html/track/TrackBase.cpp:63 > + // If the value being assigned to this attribute is not an empty string > + // or a BCP 47 language tag[BCP47], then abort these steps. This comment states what the code does. A good comment explains "why" the code does is operations. Either explain why we need to do this (if it is not self-explanatory) or please remove this comment. > Source/WebCore/html/track/TrackBase.cpp:75 > + if (!language.isEmpty() && !isValidBCP47LanguageTag(language)) { > + String message; > + if (language.contains('\0')) > + message = WTF::ASCIILiteral("The language contains a null character and is not a valid BCP 47 language tag."); > + else { > + StringBuilder stringBuilder; > + stringBuilder.appendLiteral("The language '"); > + stringBuilder.append(language); > + stringBuilder.appendLiteral("' is not a valid BCP 47 language tag."); > + message = stringBuilder.toString(); > + } > + element()->document().addConsoleMessage(MessageSource::Rendering, MessageLevel::Warning, message); Do other browsers emit a warning message for an invalid language? Neither <https://html.spec.whatwg.org/multipage/embedded-content.html#attr-track-srclang> nor <http://www.w3.org/TR/2016/PR-media-source-20161004/#sourcebuffer-init-segment-received> state that we need to emit a warning for an invalid language. > Source/WebCore/html/track/TrackBase.cpp:79 > + m_language = language; <http://www.w3.org/TR/2016/PR-media-source-20161004/#sourcebuffer-init-segment-received> seems to imply that we should take language to be the empty string when the language string is not a valid BCP 47 identifier. > LayoutTests/media/only-bcp47-language-tags-accepted-as-valid-expected.txt:155 > +EXPECTED (videoTrack.language == 'invalid-char-') OK Is this what other browser do? According to <https://html.spec.whatwg.org/multipage/embedded-content.html#dom-videotrack-language> we should return an empty string for the track language: [[ The AudioTrack.language and VideoTrack.language attributes must return the BCP 47 language tag of the language of the track, if it has one, or the empty string otherwise. If the user agent is not able to express that language as a BCP 47 language tag (for example because the language information in the media resource's format is a free-form string without a defined interpretation), then the method must return the empty string, as if the track had no language. [INBAND] ]] > LayoutTests/media/only-bcp47-language-tags-accepted-as-valid.html:55 > + videoTrack.language = notOverwritten; Would it make sense to test AudioTracks? Comment on attachment 297503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297503&action=review >> Source/WebCore/html/track/TrackBase.cpp:63 >> + // or a BCP 47 language tag[BCP47], then abort these steps. > > This comment states what the code does. A good comment explains "why" the code does is operations. Either explain why we need to do this (if it is not self-explanatory) or please remove this comment. I know that you did not write this comment and though this comment did not reference a spec. the formatting, context lines, and the presence of a step numbers served as visual cues that this comment likely came from a spec. This comment comes from the an early revision of the Media Source Extensions spec. When you moved this comment you removed the leading sectional information and the step number, "10.1 language, on setting:\\1.", that served as visual cues. If we feel that there is value in annotating our code with the relevant spec legalese then we should reproduce such legalese verbatim. Even better we should update the comment to reference a permalink or a URL to the spec and mention the date so that a reader will know where the legalese came from and the version of the spec we implemented. > Source/WebCore/html/track/VideoTrack.cpp:-205 > - // 10.1 language, on setting: Please do not remove this. > Source/WebCore/html/track/VideoTrack.cpp:-208 > - // 1. If the value being assigned to this attribute is not an empty string or a BCP 47 language > - // tag[BCP47], then abort these steps. > - // FIXME(123926): Validate the BCP47-ness of langague. It seems weird that we are moving the validation logic out of this function that implements the spec. steps for processing a language. If the correct thing to do is to move the validation outside of this function then we should add a comment here that explains where the validation logic is located and even better explain why it could not be done here. > Source/WebCore/html/track/VideoTrack.cpp:207 > - // 3. If the sourceBuffer attribute on this track is not null, then queue a task to fire a simple > + // If the sourceBuffer attribute on this track is not null, then queue a task to fire a simple Please do not remove the leading number. It serves as a visual cue that this line came from a spec. We update the top-most comment in this function to reference the spec. > Source/WebCore/html/track/VideoTrack.cpp:212 > + // Queue a task to fire a simple event named change at the VideoTrackList object referenced by Ditto. > Source/WebCore/platform/Language.cpp:204 > +bool isValidBCP47LanguageTag(const String& languageTag) > +{ > + if (languageTag.isEmpty()) > + return false; > + > + // https://tools.ietf.org/html/bcp47#section-2.1 > + for (unsigned i = 0; i < languageTag.length(); ++i) { > + UChar c = languageTag[i]; > + if (isASCIIAlphanumeric(c) || c == '-') > + continue; > + return false; > + } > + return true; > +} There is exactly one caller of this function in TrackBase::setLanguage(). Unless you plan to make use of this function in a subsequent bug/patch then I suggest we make this a static non-member, non-friend file function in file Source/WebCore/html/track/TrackBase.cpp. (In reply to comment #24) > Comment on attachment 297503 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297503&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:3777 > > + LOG(Media, "HTMLMediaElement::configureTextTrackGroup(%p) - '%s' track with language '%s' and BCP 47 language '%s' has score %i", this, textTrack->kindKeyword().string().utf8().data(), textTrack->language().string().utf8().data(), textTrack->validBCP47Language().string().utf8().data(), trackScore); > > Nit: Although the format specifier %i is equivalent to %d for formatting a > signed decimal number, a quick grep over the WebCore source code shows that > we prefer %d. Thanks. I'll change it as part of my patch. > > Source/WebCore/html/track/TrackBase.cpp:63 > > + // If the value being assigned to this attribute is not an empty string > > + // or a BCP 47 language tag[BCP47], then abort these steps. > > This comment states what the code does. A good comment explains "why" the > code does is operations. Either explain why we need to do this (if it is not > self-explanatory) or please remove this comment. Will comment on this in my reply to your follow-up. > > Source/WebCore/html/track/TrackBase.cpp:75 > > + if (!language.isEmpty() && !isValidBCP47LanguageTag(language)) { > > + String message; > > + if (language.contains('\0')) > > + message = WTF::ASCIILiteral("The language contains a null character and is not a valid BCP 47 language tag."); > > + else { > > + StringBuilder stringBuilder; > > + stringBuilder.appendLiteral("The language '"); > > + stringBuilder.append(language); > > + stringBuilder.appendLiteral("' is not a valid BCP 47 language tag."); > > + message = stringBuilder.toString(); > > + } > > + element()->document().addConsoleMessage(MessageSource::Rendering, MessageLevel::Warning, message); > > Do other browsers emit a warning message for an invalid language? Neither > <https://html.spec.whatwg.org/multipage/embedded-content.html#attr-track- > srclang> nor > <http://www.w3.org/TR/2016/PR-media-source-20161004/#sourcebuffer-init- > segment-received> state that we need to emit a warning for an invalid > language. No, not that we know of we decided to do this because of two things: 1. We should help developers debug their code since with this change we will no longer accept non-compliant language tags internally. Without a warning there is no way for developers to understand what went wrong when their language setting is not picked up. 2. It allows for layout testing. > > Source/WebCore/html/track/TrackBase.cpp:79 > > + m_language = language; > > <http://www.w3.org/TR/2016/PR-media-source-20161004/#sourcebuffer-init- > segment-received> seems to imply that we should take language to be the > empty string when the language string is not a valid BCP 47 identifier. Are you saying we should drop the existing language setting (for internal use) if the page sets an invalid language? I actually implemented it that way first with just a boolean for whether m_language was valid or not. If you mean language setting exposed to the page, see below. > > LayoutTests/media/only-bcp47-language-tags-accepted-as-valid-expected.txt:155 > > +EXPECTED (videoTrack.language == 'invalid-char-') OK > > Is this what other browser do? According to > <https://html.spec.whatwg.org/multipage/embedded-content.html#dom-videotrack- > language> we should return an empty string for the track language: > [[ > The AudioTrack.language and VideoTrack.language attributes must return the > BCP 47 language tag of the language of the track, if it has one, or the > empty string otherwise. If the user agent is not able to express that > language as a BCP 47 language tag (for example because the language > information in the media resource's format is a free-form string without a > defined interpretation), then the method must return the empty string, as if > the track had no language. [INBAND] > ]] The discussion in my platform test pull request <https://github.com/w3c/web-platform-tests/pull/4359> ended with an acknowledgment that the spec is confusing but that BCP 47 compliance "is an authoring requirement, it does not affect user agents." We should just reflect to the page whatever the web developer decided to set. Thus the change only validates for internal purposes. > > LayoutTests/media/only-bcp47-language-tags-accepted-as-valid.html:55 > > + videoTrack.language = notOverwritten; > > Would it make sense to test AudioTracks? We deliberately left AudioTrack out since it does not override TrackBase::setLanguage() and that was to be implemented in a follow-up patch by Jer. But since we moved validation to TrackBase::setLanguage() the validation will work for AudioTracks too and we should test it. Thanks! (In reply to comment #25) > Comment on attachment 297503 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297503&action=review > > >> Source/WebCore/html/track/TrackBase.cpp:63 > >> + // or a BCP 47 language tag[BCP47], then abort these steps. > > > > This comment states what the code does. A good comment explains "why" the code does is operations. Either explain why we need to do this (if it is not self-explanatory) or please remove this comment. > > I know that you did not write this comment and though this comment did not > reference a spec. the formatting, context lines, and the presence of a step > numbers served as visual cues that this comment likely came from a spec. > This comment comes from the an early revision of the Media Source Extensions > spec. When you moved this comment you removed the leading sectional > information and the step number, "10.1 language, on setting:\\1.", that > served as visual cues. If we feel that there is value in annotating our code > with the relevant spec legalese then we should reproduce such legalese > verbatim. Even better we should update the comment to reference a permalink > or a URL to the spec and mention the date so that a reader will know where > the legalese came from and the version of the spec we implemented. Got it. I'll restructure the comments and restore the proper spec quote. > > Source/WebCore/html/track/VideoTrack.cpp:-205 > > - // 10.1 language, on setting: > > Please do not remove this. Will fix. > > Source/WebCore/html/track/VideoTrack.cpp:-208 > > - // 1. If the value being assigned to this attribute is not an empty string or a BCP 47 language > > - // tag[BCP47], then abort these steps. > > - // FIXME(123926): Validate the BCP47-ness of langague. > > It seems weird that we are moving the validation logic out of this function > that implements the spec. steps for processing a language. If the correct > thing to do is to move the validation outside of this function then we > should add a comment here that explains where the validation logic is > located and even better explain why it could not be done here. I will add a comment. > > Source/WebCore/html/track/VideoTrack.cpp:207 > > - // 3. If the sourceBuffer attribute on this track is not null, then queue a task to fire a simple > > + // If the sourceBuffer attribute on this track is not null, then queue a task to fire a simple > > Please do not remove the leading number. It serves as a visual cue that this > line came from a spec. We update the top-most comment in this function to > reference the spec. Will fix. > > Source/WebCore/html/track/VideoTrack.cpp:212 > > + // Queue a task to fire a simple event named change at the VideoTrackList object referenced by > > Ditto. Will fix. > > Source/WebCore/platform/Language.cpp:204 > > +bool isValidBCP47LanguageTag(const String& languageTag) > > +{ > > + if (languageTag.isEmpty()) > > + return false; > > + > > + // https://tools.ietf.org/html/bcp47#section-2.1 > > + for (unsigned i = 0; i < languageTag.length(); ++i) { > > + UChar c = languageTag[i]; > > + if (isASCIIAlphanumeric(c) || c == '-') > > + continue; > > + return false; > > + } > > + return true; > > +} > > There is exactly one caller of this function in TrackBase::setLanguage(). > Unless you plan to make use of this function in a subsequent bug/patch then > I suggest we make this a static non-member, non-friend file function in file > Source/WebCore/html/track/TrackBase.cpp. The plan is to reuse this in HTTP header validation of Accept-Language and Content-language. But I don't know when that will happen. Would you prefer this to be a static non-member for now and then open it up when needed? (In reply to comment #26) > > > Source/WebCore/html/track/TrackBase.cpp:75 > > > + if (!language.isEmpty() && !isValidBCP47LanguageTag(language)) { > > > + String message; > > > + if (language.contains('\0')) > > > + message = WTF::ASCIILiteral("The language contains a null character and is not a valid BCP 47 language tag."); > > > + else { > > > + StringBuilder stringBuilder; > > > + stringBuilder.appendLiteral("The language '"); > > > + stringBuilder.append(language); > > > + stringBuilder.appendLiteral("' is not a valid BCP 47 language tag."); > > > + message = stringBuilder.toString(); > > > + } > > > + element()->document().addConsoleMessage(MessageSource::Rendering, MessageLevel::Warning, message); > > > > Do other browsers emit a warning message for an invalid language? Neither > > <https://html.spec.whatwg.org/multipage/embedded-content.html#attr-track- > > srclang> nor > > <http://www.w3.org/TR/2016/PR-media-source-20161004/#sourcebuffer-init- > > segment-received> state that we need to emit a warning for an invalid > > language. > > No, not that we know of we decided to do this because of two things: > 1. We should help developers debug their code since with this change we will > no longer accept non-compliant language tags internally. Without a warning > there is no way for developers to understand what went wrong when their > language setting is not picked up. > 2. It allows for layout testing. > OK. Is it necessary to include the language or whether it includes a null character in the error message? Can we take advantage of the facts that the track.srclang/texttrack.language attribute represents a single BCP 47 identifier (is this true?) and that a console message has line information to emit a generic error message? I also suggest that the error message explain how WebKit is handling this malformed language. (Are we ignoring the value of track.srclang/texttrack.language? What does it mean to ignore the text track language?) (In reply to comment #26) > > > Source/WebCore/html/track/TrackBase.cpp:79 > > > + m_language = language; > > > > <http://www.w3.org/TR/2016/PR-media-source-20161004/#sourcebuffer-init- > > segment-received> seems to imply that we should take language to be the > > empty string when the language string is not a valid BCP 47 identifier. > > Are you saying we should drop the existing language setting (for internal > use) if the page sets an invalid language? Disregard my comment as it does not seem applicable to this patch. (In reply to comment #27) > > > Source/WebCore/platform/Language.cpp:204 > > > +bool isValidBCP47LanguageTag(const String& languageTag) > > > +{ > > > + if (languageTag.isEmpty()) > > > + return false; > > > + > > > + // https://tools.ietf.org/html/bcp47#section-2.1 > > > + for (unsigned i = 0; i < languageTag.length(); ++i) { > > > + UChar c = languageTag[i]; > > > + if (isASCIIAlphanumeric(c) || c == '-') > > > + continue; > > > + return false; > > > + } > > > + return true; > > > +} > > > > There is exactly one caller of this function in TrackBase::setLanguage(). > > Unless you plan to make use of this function in a subsequent bug/patch then > > I suggest we make this a static non-member, non-friend file function in file > > Source/WebCore/html/track/TrackBase.cpp. > > The plan is to reuse this in HTTP header validation of Accept-Language and > Content-language. But I don't know when that will happen. Would you prefer > this to be a static non-member for now and then open it up when needed? Are you planing to reuse this the HTTP header validation logic in a patch in the near future (as in the next day or two)? If not, then I suggest that we make this a static non-member, non-friend file function in file Source/WebCore/html/track/TrackBase.cpp. When we come around to update the HTTP header validation logic then we can move this function to an appropriate place (maybe this file) so that it can be shared between the text track machinery and the HTTP header validation logic. Created attachment 297693 [details]
Patch
Created attachment 297695 [details]
Patch
The GTK build was complaining about ambiguity in AtomicString::contains() for the call language.contains('\0').
Comment on attachment 297695 [details] Patch Attachment 297695 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2772708 New failing tests: imported/w3c/web-platform-tests/html/dom/reflection-embedded.html media/only-bcp47-language-tags-accepted-as-valid.html Created attachment 297701 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 297978 [details]
Patch
Comment on attachment 297978 [details]
Patch
r=me. Nice job!
Thanks! Committing. I'll update it if Alex has further comments (we all discussed the rather complex validation logic). Comment on attachment 297978 [details] Patch Clearing flags on attachment: 297978 Committed r210280: <http://trac.webkit.org/changeset/210280> All reviewed patches have been landed. Closing bug. |