Bug 47631 - Draw the find overlay holes
Summary: Draw the find overlay holes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-13 15:55 PDT by Anders Carlsson
Modified: 2010-10-13 16:12 PDT (History)
0 users

See Also:

Attachments
Patch (4.43 KB, patch)
2010-10-13 16:02 PDT, Anders Carlsson 		sullivan: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2010-10-13 15:55:53 PDT 
Draw the find overlay holes
Comment 1 Anders Carlsson 2010-10-13 16:02:47 PDT 
Created attachment 70677 [details]
Patch
Comment 2 John Sullivan 2010-10-13 16:07:38 PDT 
Comment on attachment 70677 [details]
Patch

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

See my comment above about where to put the constant definitions. I don't have a horse in that race, though.

> WebKit2/ChangeLog:10
> +        Move the color component constants into overlayBackgroundColor.

I'm not sure what the style guidelines say about this. I'm used to putting all the static consts at the top of the file. It seems odd to put the color ones inside overlayBackgroundColor, but not put the shadow ones inside drawRect.
Comment 3 mitz 2010-10-13 16:10:11 PDT 
Comment on attachment 70677 [details]
Patch

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

> WebKit2/WebProcess/WebPage/FindPageOverlay.cpp:92
> +static const float shadowOffsetX = 0.0;
> +static const float shadowOffsetY = 1.0;
> +static const float shadowBlurRadius = 2.0;
> +static const float whiteFrameThickness = 1.0;
> +

Please remove the “.0”s.

> WebKit2/WebProcess/WebPage/FindPageOverlay.cpp:115
>      graphicsContext.fillRect(paintRect, overlayBackgroundColor(), sRGBColorSpace);

I wonder if this shouldn’t have been in device space.

> WebKit2/WebProcess/WebPage/FindPageOverlay.cpp:125
> +        whiteFrameRect.inflate(1);

Did you mean to use whiteFrameThickness?
Comment 4 Anders Carlsson 2010-10-13 16:12:39 PDT 
Committed r69711: <http://trac.webkit.org/changeset/69711>