[webkit-reviews] review denied: [Bug 102689] [WinCairo] Crash when requesting favicon. : [Attachment 175822] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied peavo at outlook.com's request for
review:
Bug 102689: [WinCairo] Crash when requesting favicon.
https://bugs.webkit.org/show_bug.cgi?id=102689

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

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=175822&action=review


I think this looks very good, but is missing coverage for the main Apple
Windows port.  I think the WebKit2.def file needs similar changes, and I'd like
to see the config.h for the test handle the Apple CG build.

The EWS also indicates a failure; We should try to keep this green if at all
possible.

For these reasons, I'm marking it r-, but I think it's just a few small changes
to get this ready to land.

>> Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:98
>> +	    if (!frameAtIndex(i))
> 
> Is it expected that a frame might be null? Maybe we have an underlying
problem in frame handling that is allowing null images to pass through.

I just checked the CG implementation, and they perform similar null checks. So
I think this is the right approach.

> Tools/TestWebKitAPI/config.h:51
> +#undef WTF_USE_CG

How does this test get run under the CG build?	This seems incomplete.