| Summary: | Support find bouncy in WebKit2 on Windows | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jeff Miller <jeffm> | ||||||
| Component: | WebKit2 | Assignee: | Jeff Miller <jeffm> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andersca, aroben, commit-queue, mjs | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Other | ||||||||
| OS: | Windows 7 | ||||||||
| Attachments: |
|
||||||||
|
Description
Jeff Miller
2011-01-28 13:38:12 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 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.) |