[webkit-reviews] review denied: [Bug 23335] Update <input type="search"> for RenderThemeWin : [Attachment 26739] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Adele Peterson <adele at apple.com>'s
request for review:
Bug 23335: Update <input type="search"> for RenderThemeWin
https://bugs.webkit.org/show_bug.cgi?id=23335

Attachment 26739: patch
https://bugs.webkit.org/attachment.cgi?id=26739&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +static const float defaultCancelButtonSize = 9.0f;
> +static const float minCancelButtonSize = 5.0f;
> +static const float maxCancelButtonSize = 21.0f;
> +static const float defaultSearchFieldResultsDecorationSize = 13.0f;
> +static const float minSearchFieldResultsDecorationSize = 9.0f;
> +static const float maxSearchFieldResultsDecorationSize = 30.0f;
> +static const float defaultSearchFieldResultsButtonWidth = 18.0f;

The .0f suffixes don't add anything. I suggest just leaving them out.

> +using std::min;
> +using std::max;

Normally I use "using namespace std" unless there's some reason I want only
certain symbols.

> +    bounds.setWidth(min(parentBox.width(), bounds.height()));

I think you mean bounds.width, not bounds.height.

> +    // Center the button vertically.  Round up though, so if it has to be
one pixel off-center, it will
> +    // be one pixel closer to the bottom of the field.  This tends to look
better with the text.
> +    bounds.setY(parentBox.y() + ceil(static_cast<float>(parentBox.height())
/ 2.0f - static_cast<float>(bounds.height()) / 2.0f));

To do ceil on a float, you want ceilf, not ceil, which takes a double.

The 2.0f is sufficient to get the math done as float -- you don't have to cast
to float explicitly.

> +    DEFINE_STATIC_LOCAL(RefPtr<Image>, cancelImage,
(Image::loadPlatformResource("searchCancel")));
> +    DEFINE_STATIC_LOCAL(RefPtr<Image>, cancelPressedImage,
(Image::loadPlatformResource("searchCancelPressed")));

Since we just want to leak these images, maybe we should just have the
initialization expression do a releaseRef() and make the globals be Image*.
Then you could use a plain old static instead of DEFINE_STATIC_LOCAL. I'm not
sure what the best style is for these.

> +    int magnifierSize = roundf(min(max(minSearchFieldResultsDecorationSize,
defaultSearchFieldResultsDecorationSize * fontScale), 
> +					maxSearchFieldResultsDecorationSize));

Since you want an integer result, maybe this should be lroundf instead of
roundf, which gives a float result.

> +    bounds.setWidth(min(parentBox.width(), bounds.height()));

Same height/width comment.

> +    bounds.setY(parentBox.y() + ceil(static_cast<float>(parentBox.height())
/ 2.0f - static_cast<float>(bounds.height()) / 2.0f));

Same cast to float comment.

> +    DEFINE_STATIC_LOCAL(RefPtr<Image>, magnifierImage,
(Image::loadPlatformResource("searchMagnifier")));

Same releaseRef comment.

Nothing more except the same comments over again since these issues affect
multiple functions.