[webkit-reviews] review denied: [Bug 102689] [WinCairo] Crash when requesting favicon. : [Attachment 176017] 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 176017: Patch
https://bugs.webkit.org/attachment.cgi?id=176017&action=review

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


Looks very good.  I think you need one more export to satisfy the EWS, and
we're ready to go!

> Source/WebCore/ChangeLog:1
> +2012-11-23  peavo at outlook.com  <peavo at outlook.com>

Can you put a full name here instead of just the e-mail twice?

> Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:98
> +	   if (!frameAtIndex(i))

I would prefer something like:

NativeImageCairo* nativeImage = frameAtIndex(i);
if (!nativeImage)
    continue;
cairo_surface_t* image = nativeImage->surface();

> Source/WebKit2/ChangeLog:1
> +2012-11-23  peavo at outlook.com  <peavo at outlook.com>

Can you put a full name here instead of just the e-mail twice?

> Source/WebKit2/win/WebKit2.def:347
> +	   ?nativeImageForCurrentFrame at BitmapImage@WebCore@@UAEPAUCGImage@@XZ

Looks like we need to add:
 ?notSolidColor at BitmapImage@WebCore@@UAE_NXZ

> Source/WebKit2/win/WebKit2CFLite.def:338
> +	  
?nativeImageForCurrentFrame at BitmapImage@WebCore@@UAEPAVNativeImageCairo at 2@XZ

Looks like we need to add:
 ?notSolidColor at BitmapImage@WebCore@@UAE_NXZ

> Tools/ChangeLog:1
> +2012-11-23  peavo at outlook.com  <peavo at outlook.com>

Can you put a full name here instead of just the e-mail twice?

> Tools/ChangeLog:10
> +	   (TestWebKitAPI):

Delete this row.

> Tools/ChangeLog:11
> +	   (BitmapImageTest):

Delete this row.

> Tools/TestWebKitAPI/Tests/WebCore/win/BitmapImage.cpp:2
> + * Copyright (C) 2011, 2012 Apple Inc. All rights reserved.

Copyright should be you, or at least add yourself to the next line.