[webkit-reviews] review denied: [Bug 53329] Support find bouncy in WebKit2 on Windows : [Attachment 80493] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Jeff Miller <jeffm at apple.com>'s
request for review:
Bug 53329: Support find bouncy in WebKit2 on Windows
https://bugs.webkit.org/show_bug.cgi?id=53329

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

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80493&action=review

> Source/WebKit2/UIProcess/win/WebView.cpp:1025
> +    if (m_findIndicatorCallback) {

You can use an early return here instead.

> Source/WebKit2/UIProcess/win/WebView.cpp:1032
> +	       HDC hdc = CreateCompatibleDC(0);

In WebKit2 we usually prefix Win32 functions with ::

> Source/WebKit2/UIProcess/win/WebView.cpp:1033
> +	       int w = contentImage->bounds().width();

I think 'width' would be a better name than 'w'.

> Source/WebKit2/UIProcess/win/WebView.cpp:1034
> +	       int h = contentImage->bounds().height();

'h' -> 'height'

> Source/WebKit2/UIProcess/win/WebView.cpp:1035
> +	       BitmapInfo bmp = BitmapInfo::create(contentImage->size());

I think 'bitmap' or 'bitmapInfo' would be a better name for this variable.

> Source/WebKit2/UIProcess/win/WebView.cpp:1037
> +	       hbmp = CreateDIBSection(0, &bmp, DIB_RGB_COLORS,
static_cast<void**>(&bits), 0, 0);

Is this static_cast really needed?

> Source/WebKit2/UIProcess/win/WebView.cpp:1039
> +	       CGContextRef context =
CGBitmapContextCreate(static_cast<void*>(bits), w, h,

You should use a RetainPtr here, then you don't need the release:

RetainPtr context<CGContextRef>(AdoptCF, CGBitmapContextCreate(...

> Source/WebKit2/UIProcess/win/WebView.cpp:1041
> +	       CGContextSaveGState(context);

I don't think you need to save the state here.

> Source/WebKit2/UIProcess/win/WebView.cpp:1043
> +	       GraphicsContext gc(context);

'gc' -> 'graphicsContext'

> Source/WebKit2/UIProcess/win/WebView.cpp:1045
> +	       contentImage->paint(gc, IntPoint(0, 0), contentImage->bounds());


You can just use the IntPoint() constructor here.

> Source/WebKit2/UIProcess/win/WebView.cpp:1048
> +	       SelectObject(hdc, hbmpOld);

::SelectObject.

> Source/WebKit2/UIProcess/win/WebView.cpp:1049
> +	       DeleteDC(hdc);

::DeleteDC.

> Source/WebKit2/UIProcess/win/WebView.cpp:1053
> +	   RECT selectionRect = {selectionIntRect.x(), selectionIntRect.y(),
selectionIntRect.right(), selectionIntRect.bottom()};

I think IntRect has a RECT conversion operator, so you can just pass
selectionIntRect to the callback.

> Source/WebKit2/UIProcess/win/WebView.cpp:1055
> +    }

Does this assume ownership of the HBITMAP? If so, that should probably be
documented. If not, you're leaking it :)

> Source/WebKit2/UIProcess/win/WebView.cpp:1064
> +WKViewFindIndicatorCallback WebView::getFindIndicatorCallback(void**
context)

Do we really need a getter for the callback?