| Summary: | TextTrackBase should validate language before setting m_validBCP47Language | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||
| Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | calvaris, cdumez, commit-queue, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, kondapallykalyan, philipj, sergio, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | Other | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Eric Carlson
2020-03-13 16:42:28 PDT
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> All reviewed patches have been landed. Closing bug. |