WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(26.85 KB, patch)
2011-01-28 15:19 PST
,
Jeff Miller
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 80493
[details]
Patch
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
Created
attachment 80506
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug