[Webkit-unassigned] [Bug 152890] [WinCairo] Support more video formats.

[bugzilla-daemon at webkit.org]

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

Alex Christensen <achristensen at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #268541|review?                     |review-
              Flags|                            |

--- Comment #2 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 268541
  --> https://bugs.webkit.org/attachment.cgi?id=268541
Patch

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

Functionality looks good, there are just a few optimizations and reorganizations we need to do.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:124
> +    static HashSet<String> cachedTypes;

Use NeverDestroyed instead of static.  Also, you should probably make this in a separate function like gstreamer's mimeTypeCache(), except have it just return a reference, not a whole HashSet.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:134
> +    HRESULT hr = MFGetSupportedMimeTypesPtr()(&propVarMimeTypeArray);

This will crash on WindowsXP.  I don't think that's a problem, because we don't support XP any more.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:156
> +    HashSet<String> types;
> +    getSupportedTypes(types);

We should't copy the whole HashSet every time we want to check if one type is supported.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:158
> +    for (HashSet<String>::iterator it = types.begin(); it != types.end(); ++it) {

This is what HashSet.contains is for.  No need to iterate.

-- 
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/20160108/bac087ac/attachment-0001.html>