Support find bouncy in WebKit2 on Windows
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
OpenSource side of <rdar://problem/8565843> WebKit2: getting the find bouncy to work on Windows
Created attachment 80493 [details] Patch
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?
>> 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.
Created attachment 80506 [details] Patch
Comment on attachment 80506 [details] Patch Clearing flags on attachment: 80506 Committed r77054: <http://trac.webkit.org/changeset/77054>
All reviewed patches have been landed. Closing bug.
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.)