Bug 209094 – TextTrackBase should validate language before setting m_validBCP47Language

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

TextTrackBase should validate language before setting m_validBCP47Language

[Radar WebKit Bug Importer]

<rdar://problem/60439603>

[Eric Carlson]

Created attachment 393694 [details]
Patch

[Eric Carlson]

Created attachment 393706 [details]
Patch

[Eric Carlson]

Created attachment 393762 [details]
Patch

[Jer Noble]

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();

[WebKit Commit Bot]

Comment on attachment 393762 [details]
Patch

Clearing flags on attachment: 393762

Committed r258587: <https://trac.webkit.org/changeset/258587>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Eric Carlson]

(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.

[Eric Carlson]

Reopening to attach new patch.

[Eric Carlson]

Created attachment 393790 [details]
Address post-review comments

[Darin Adler]

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!

[Eric Carlson]

(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!

[WebKit Commit Bot]

Comment on attachment 393790 [details]
Address post-review comments

Clearing flags on attachment: 393790

Committed r258606: <https://trac.webkit.org/changeset/258606>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.