TextTrackBase should validate language before setting m_validBCP47Language
<rdar://problem/60439603>
Created attachment 393694 [details] Patch
Created attachment 393706 [details] Patch
Created attachment 393762 [details] Patch
Comment on attachment 393762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393762&action=review > Source/WebCore/testing/Internals.cpp:3776 > + return String { track.validBCP47Language() }; nit: this could be: return track.validBCP47Language().string();
Comment on attachment 393762 [details] Patch Clearing flags on attachment: 393762 Committed r258587: <https://trac.webkit.org/changeset/258587>
All reviewed patches have been landed. Closing bug.
Comment on attachment 393762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393762&action=review > Source/WebCore/html/track/TrackBase.cpp:156 > + StringBuilder stringBuilder; > + stringBuilder.appendLiteral("The language '"); > + stringBuilder.append(language); > + stringBuilder.appendLiteral("' is not a valid BCP 47 language tag."); > + message = stringBuilder.toString(); This should be makeString. No reason to use StringBuilder here.
(In reply to Darin Adler from comment #8) > Comment on attachment 393762 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393762&action=review > > > Source/WebCore/html/track/TrackBase.cpp:156 > > + StringBuilder stringBuilder; > > + stringBuilder.appendLiteral("The language '"); > > + stringBuilder.append(language); > > + stringBuilder.appendLiteral("' is not a valid BCP 47 language tag."); > > + message = stringBuilder.toString(); > > This should be makeString. No reason to use StringBuilder here. You're right, I should have thought of that when I refactored the code. I'll do that in a follow up.
Reopening to attach new patch.
Created attachment 393790 [details] Address post-review comments
Comment on attachment 393762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393762&action=review >>> Source/WebCore/html/track/TrackBase.cpp:156 >>> + message = stringBuilder.toString(); >> >> This should be makeString. No reason to use StringBuilder here. > > You're right, I should have thought of that when I refactored the code. I'll do that in a follow up. I know you were just moving code around, and I didn’t mean to come on too strong. Thanks for tweaking it!
(In reply to Darin Adler from comment #12) > Comment on attachment 393762 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393762&action=review > > >>> Source/WebCore/html/track/TrackBase.cpp:156 > >>> + message = stringBuilder.toString(); > >> > >> This should be makeString. No reason to use StringBuilder here. > > > > You're right, I should have thought of that when I refactored the code. I'll do that in a follow up. > > I know you were just moving code around, and I didn’t mean to come on too > strong. Thanks for tweaking it! No offense taken, it is a great suggestion!
Comment on attachment 393790 [details] Address post-review comments Clearing flags on attachment: 393790 Committed r258606: <https://trac.webkit.org/changeset/258606>