Bug 209094 - TextTrackBase should validate language before setting m_validBCP47Language
Summary: TextTrackBase should validate language before setting m_validBCP47Language
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-13 16:42 PDT by Eric Carlson
Modified: 2020-03-17 15:45 PDT (History)
13 users (show)

See Also:


Attachments
Patch (11.52 KB, patch)
2020-03-16 15:38 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (11.85 KB, patch)
2020-03-16 16:37 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (11.89 KB, patch)
2020-03-17 09:54 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Address post-review comments (1.80 KB, patch)
2020-03-17 14:36 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2020-03-13 16:42:28 PDT
TextTrackBase should validate language before setting m_validBCP47Language
Comment 1 Radar WebKit Bug Importer 2020-03-13 16:42:39 PDT
<rdar://problem/60439603>
Comment 2 Eric Carlson 2020-03-16 15:38:40 PDT
Created attachment 393694 [details]
Patch
Comment 3 Eric Carlson 2020-03-16 16:37:59 PDT
Created attachment 393706 [details]
Patch
Comment 4 Eric Carlson 2020-03-17 09:54:24 PDT
Created attachment 393762 [details]
Patch
Comment 5 Jer Noble 2020-03-17 11:59:58 PDT
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 6 WebKit Commit Bot 2020-03-17 13:52:05 PDT
Comment on attachment 393762 [details]
Patch

Clearing flags on attachment: 393762

Committed r258587: <https://trac.webkit.org/changeset/258587>
Comment 7 WebKit Commit Bot 2020-03-17 13:52:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2020-03-17 13:53:22 PDT
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.
Comment 9 Eric Carlson 2020-03-17 14:14:13 PDT
(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.
Comment 10 Eric Carlson 2020-03-17 14:36:06 PDT
Reopening to attach new patch.
Comment 11 Eric Carlson 2020-03-17 14:36:07 PDT
Created attachment 393790 [details]
Address post-review comments
Comment 12 Darin Adler 2020-03-17 14:57:47 PDT
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!
Comment 13 Eric Carlson 2020-03-17 14:59:57 PDT
(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 14 WebKit Commit Bot 2020-03-17 15:45:22 PDT
Comment on attachment 393790 [details]
Address post-review comments

Clearing flags on attachment: 393790

Committed r258606: <https://trac.webkit.org/changeset/258606>
Comment 15 WebKit Commit Bot 2020-03-17 15:45:24 PDT
All reviewed patches have been landed.  Closing bug.