Bug 79368

<rdar://problem/10918891>

And the possible values for direction changed as well: "horizontal" -> "","vertical" - > "rl", "vertical-lr" -> "lr"

Created attachment 128526 [details] Proposed patch

Comment on attachment 128526 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=128526&action=review > Source/WebCore/html/track/TextTrackCue.cpp:70 > - DEFINE_STATIC_LOCAL(const AtomicString, horizontal, ("horizontal")); > + DEFINE_STATIC_LOCAL(const AtomicString, horizontal, ("")); > return horizontal; Why not just use emptyString() ? There's already a global static for the empty string.

(In reply to comment #4) > (From update of attachment 128526 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128526&action=review > > > Source/WebCore/html/track/TextTrackCue.cpp:70 > > - DEFINE_STATIC_LOCAL(const AtomicString, horizontal, ("horizontal")); > > + DEFINE_STATIC_LOCAL(const AtomicString, horizontal, ("")); > > return horizontal; > > Why not just use emptyString() ? There's already a global static for the empty string. I initially tried to just "return emptyString()", but that doesn't work: error: returning reference to local temporary object [-Werror,-Wreturn-stack-address] return emptyString(); ^~~~~~~~~~~~~ 1 error generated. If you mean I should initialize my static with emptyString(), I can do that.

From WTFString.cpp: const String& emptyString() { DEFINE_STATIC_LOCAL(String, emptyString, (StringImpl::empty())); return emptyString; }

Comment on attachment 128526 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=128526&action=review > Source/WebCore/html/track/TextTrackCue.cpp:67 > static const AtomicString& horizontalKeyword() Ah, the problem is that you're returning an AtomicString rather than a String. Do we have an emptyAtomicString() ?

Created attachment 128777 [details] Updated patch.

Comment on attachment 128526 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=128526&action=review > Source/WebCore/html/track/TextTrackCue.cpp:67 > static const AtomicString& horizontalKeyword() The two uses in this patch use this as a String& instead of an AtomicString&. Doesn't really matter, but it's possible that those uses would want to be AtomicStrings themselves to get the benifit of AtomicString. > Source/WebCore/html/track/TextTrackCue.cpp:210 > if (value == horizontalKeyword()) > direction = Horizontal; So now the value must be empty to mean horizongal? Does it ignore leading/trailing spaces?

Created attachment 128800 [details] Updated patch.

(In reply to comment #9) > (From update of attachment 128526 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128526&action=review > > > Source/WebCore/html/track/TextTrackCue.cpp:67 > > static const AtomicString& horizontalKeyword() > > The two uses in this patch use this as a String& instead of an AtomicString&. Doesn't really matter, but it's possible that those uses would want to be AtomicStrings themselves to get the benifit of AtomicString. > Good point, I updated the static functions to all return Strings. > > Source/WebCore/html/track/TextTrackCue.cpp:210 > > if (value == horizontalKeyword()) > > direction = Horizontal; > > So now the value must be empty to mean horizongal? Does it ignore leading/trailing spaces? The spec says [1]: On setting, the text track cue writing direction must be set to the value given in the first cell of the row in the table above whose second cell is a case-sensitive match for the new value, if any. If none of the values match, then the user agent must instead throw a SyntaxError exception. The second cell in the table is: "" (the empty string) [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-texttrackcue-vertical

Comment on attachment 128800 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=128800&action=review I feel a bit under-informed on this effort, so I fear I'm just picking at nits. I'm comfortable rubber-stamping this change though, I trust your awareness of the spec. > Source/WebCore/html/track/TextTrackCue.cpp:423 > + if (writingDirection == verticalKeywordOLD()) We've historically used deprecated() for this sort of thing, but it's not clear (to me at least) what the proper replacement function is. Will we always have these OLD() functions around?

(In reply to comment #12) > (From update of attachment 128800 [details]) > > Source/WebCore/html/track/TextTrackCue.cpp:423 > > + if (writingDirection == verticalKeywordOLD()) > > We've historically used deprecated() for this sort of thing, but it's not clear (to me at least) what the proper replacement function is. Will we always have these OLD() functions around? No, the implementation of those functions both have this comment: // FIXME: remove this once https://bugs.webkit.org/show_bug.cgi?id=78706 has been fixed. Thanks for the review!

Comment on attachment 128800 [details] Updated patch. Clearing flags on attachment: 128800 Committed r108872: <http://trac.webkit.org/changeset/108872>

All reviewed patches have been landed. Closing bug.