[webkit-reviews] review granted: [Bug 46111] video-served-as-text.html failing on Windows : [Attachment 70019] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 46111: video-served-as-text.html failing on Windows
https://bugs.webkit.org/show_bug.cgi?id=46111

Attachment 70019: Patch
https://bugs.webkit.org/attachment.cgi?id=70019&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=70019&action=review

> WebCore/platform/graphics/win/QTMovie.cpp:65
> +static bool sDisabledComponents = 0;

The “s” prefix here is not WebKit style. I think we were talking about a style
like s_disabledComponents for static data members, but I don’t think we planned
to use a prefix at all for file-scoped globals.

But further, this global does not have to exist outside the
disableUnsupportedComponents function. You can just put the static bool in
there.

> WebCore/platform/graphics/win/QTMovie.cpp:279
> +    ComponentDescription components[] = {

Code needs a why comment.

> WebCore/platform/graphics/win/QTMovie.cpp:287
> +    ComponentDescription nullDesc = {'null', 'base', kAppleManufacturer, 0,
0};
> +    Component nullComp = FindNextComponent(0, &nullDesc);

I would like the code better if there was less Desc and Comp and more
Description and Component.

> WebCore/platform/graphics/win/QTMovie.cpp:294
> +	   Component disabledComp = 0;
> +	   while (disabledComp = FindNextComponent(disabledComp,
&components[i]))

Ditto.