[Webkit-unassigned] [Bug 150436] Enable MediaSource::isTypeSupported() to handle the upper-cased MIME type & Codec

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=150436

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #264027|review?                     |review+
              Flags|                            |

--- Comment #14 from Darin Adler <darin at apple.com> ---
Comment on attachment 264027
  --> https://bugs.webkit.org/attachment.cgi?id=264027
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264027&action=review

> Source/WebCore/Modules/mediasource/MediaSource.cpp:732
>      if (type.isNull() || type.isEmpty())

Not part of your patch, but this line of code is slightly wasteful. The isNull check isn’t needed because all null strings are empty strings too.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:735
> +    ContentType contentType(type.lower());

There are two issues here.

When we want to fold case for something like this, we want to fold only ASCII case, not full Unicode case. That means we should use functions like convertToASCIILowercase() rather than lower().

I am not sure that this call site is the right level to fold case. We should study the other clients of ContentType and see why they don’t need lowercasing. If they do, then convertToASCIILowercase should be done in the ContentType constructor, not here. There’s even another use of ContentType in the same file that might need the case folding. The problem with that is that changing ContentType will affect a lot more code paths and we’ll have to do more testing, and possibly remove some now-unneeded code that does additional lowercasing in other cases that use ContentType.

I am going to say review+ but we should do better on both of those issues if we can.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151031/5532afe6/attachment.html>