Bug 53329 – Support find bouncy in WebKit2 on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jeff Miller]

Support find bouncy in WebKit2 on Windows

[Jeff Miller]

The Mac implements the find bouncy inside of WebKit in the UI process for WebKit2, but we need to do this inside Safari for WebKit2 on Windows.

Related bugs:
https://bugs.webkit.org/show_bug.cgi?id=47612
https://bugs.webkit.org/show_bug.cgi?id=47635
https://bugs.webkit.org/show_bug.cgi?id=48490

[Jeff Miller]

OpenSource side of <rdar://problem/8565843> WebKit2: getting the find bouncy to work on Windows

[Jeff Miller]

Created attachment 80493 [details]
Patch

[Anders Carlsson]

Comment on attachment 80493 [details]
Patch

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?

[Jeff Miller]

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

>Do we really need a getter for the callback?

Unfortunately, yes, because WK::View in Safari needs to manage a separate context from the real callback context used by the client, and we can't store state in WK::View.  It doesn't actually need to retrieve the current callback function, just the context, but I figured the call should be symmetrical with setFindIndicatorCallback.

Updated patch with fixes for your other comments is coming soon.

[Jeff Miller]

Created attachment 80506 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 80506 [details]
Patch

Clearing flags on attachment: 80506

Committed r77054: <http://trac.webkit.org/changeset/77054>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

Comment on attachment 80493 [details]
Patch

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

>> 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?

Aren't we leaking this HBITMAP? Or maybe the callback is supposed to take care of deleting it? If the latter is true, I think a comment is warranted. (I think you can also declare it right here instead of declaring it up above and assigning into it here.)

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

Or you could use an OwnPtr<HDC>, and ::DeleteDC will be called automatically for you when it goes out of scope. (OwnPtr<HBITMAP> works, too, if you do in fact need to delete it.)