RESOLVED FIXED 53329
Support find bouncy in WebKit2 on Windows
https://bugs.webkit.org/show_bug.cgi?id=53329
Summary Support find bouncy in WebKit2 on Windows
Jeff Miller
Reported 2011-01-28 13:38:12 PST
Support find bouncy in WebKit2 on Windows
Attachments
Patch (27.00 KB, patch)
2011-01-28 14:01 PST, Jeff Miller
no flags
Patch (26.85 KB, patch)
2011-01-28 15:19 PST, Jeff Miller
no flags
Jeff Miller
Comment 1 2011-01-28 13:41:17 PST
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
Comment 2 2011-01-28 13:51:14 PST
OpenSource side of <rdar://problem/8565843> WebKit2: getting the find bouncy to work on Windows
Jeff Miller
Comment 3 2011-01-28 14:01:16 PST
Anders Carlsson
Comment 4 2011-01-28 14:51:58 PST
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
Comment 5 2011-01-28 15:17:00 PST
>> 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
Comment 6 2011-01-28 15:19:17 PST
WebKit Commit Bot
Comment 7 2011-01-29 00:27:40 PST
Comment on attachment 80506 [details] Patch Clearing flags on attachment: 80506 Committed r77054: <http://trac.webkit.org/changeset/77054>
WebKit Commit Bot
Comment 8 2011-01-29 00:27:43 PST
All reviewed patches have been landed. Closing bug.
Adam Roben (:aroben)
Comment 9 2011-01-31 07:38:13 PST
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.)
Note You need to log in before you can comment on or make changes to this bug.