Bug 23335 – Update <input type="search"> for RenderThemeWin

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Adele Peterson]

Update <input type="search"> for RenderThemeWin

[Adele Peterson]

Created attachment 26739 [details]
patch

[Darin Adler]

Comment on attachment 26739 [details]
patch

> +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.

[Adele Peterson]

Comment on attachment 26739 [details]
patch

Darin came by and reviewed a revised version in person

[Adele Peterson]

Committed revision 39924.