Bug 38253 – REGRESSION(r58313): Regression evident in pixel tests: the search icon is always clipped at the bottom. (Requested by jorlow on #webkit).

• Component: New Bugs
• Status: RESOLVED FIXED

[WebKit Review Bot]

http://trac.webkit.org/changeset/58313 broke the build:
Regression evident in pixel tests: the search icon is always clipped at the bottom. (Requested by jorlow on #webkit).

This is an automatic bug report generated by the sheriff-bot. If this bug
report was created because of a flaky test, please file a bug for the flaky
test (if we don't already have one on file) and dup this bug against that bug
so that we can track how often these flaky tests case pain.

"Only you can prevent forest fires." -- Smokey the Bear

[WebKit Review Bot]

Created attachment 54546 [details]
ROLLOUT of r58313

Any committer can land this patch automatically by marking it commit-queue+.  The commit-queue will build and test the patch before landing to ensure that the rollout will be successful.  This process takes approximately 15 minutes.

If you would like to land the rollout faster, you can use the following command:

  webkit-patch land-attachment ATTACHMENT_ID --ignore-builders

where ATTACHMENT_ID is the ID of this attachment.

[Jeremy Orlow]

Comment on attachment 54546 [details]
ROLLOUT of r58313

Clearing flags on attachment: 54546

Committed r58400: <http://trac.webkit.org/changeset/58400>

[Jeremy Orlow]

All reviewed patches have been landed.  Closing bug.

[Jeremy Orlow]

Re-opening to track the fact that this was reverted.  Here's the test results we saw from the Chromium pixel bots.  I verified by hand that this was showing up in mac safari as well.

http://trac.webkit.org/export/58399/trunk/LayoutTests/platform/mac/fast/forms/search-zoomed-expected.png
http://build.chromium.org/buildbot/layout_test_results/webkit-rel-mac-webkit-org/results/layout-test-results/fast/forms/search-zoomed-actual.png
http://build.chromium.org/buildbot/layout_test_results/webkit-rel-mac-webkit-org/results/layout-test-results/fast/forms/search-zoomed-diff.png

If you look closely, the bottom is being clipped.  I'm guessing it's an off by one error or something.

[Joseph Pecoraro]

(In reply to comment #4)
> Re-opening to track the fact that this was reverted.  Here's the test results
> we saw from the Chromium pixel bots.  I verified by hand that this was showing
> up in mac safari as well.
> 
> If you look closely, the bottom is being clipped.  I'm guessing it's an off by
> one error or something.

Thanks for catching and following up on this Jeremy.

In my patch I used the contentHeight() and contentWidth() like it was using before, but I was using a rounded location causing the clip. I think what I should in fact be doing is the entire area of the TextControl.

Also if I am not mistaken this means that even the original image was being slightly clipped because it was also using the old incorrect clip. After this patch you may need to update Chromium's pixel tests. Is there a way I can do this on my mac?

Also, for mac updated results it looks like the shadow for the search input was broken before on my machine. Could this be a change I want to put into leopard or snow-leopard?

[Joseph Pecoraro]

Created attachment 54613 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 54613 [details]
[PATCH] Proposed Fix

> +    // This should only get called for search inputs

Style issue, this should end in a period. I've fixed this locally.

[Jeremy Orlow]

(In reply to comment #5)
> (In reply to comment #4)
> > Re-opening to track the fact that this was reverted.  Here's the test results
> > we saw from the Chromium pixel bots.  I verified by hand that this was showing
> > up in mac safari as well.
> > 
> > If you look closely, the bottom is being clipped.  I'm guessing it's an off by
> > one error or something.
> 
> Thanks for catching and following up on this Jeremy.
> 
> In my patch I used the contentHeight() and contentWidth() like it was using
> before, but I was using a rounded location causing the clip. I think what I
> should in fact be doing is the entire area of the TextControl.

Thanks a lot for looking into this!


> Also if I am not mistaken this means that even the original image was being
> slightly clipped because it was also using the old incorrect clip. After this
> patch you may need to update Chromium's pixel tests. Is there a way I can do
> this on my mac?

There is, but I think you still need a Chromium checkout to do it.  Anyway, it's not your responsibility since we still haven't gotten this stuff upstream.  We'll take care of the re-baselining of the Chromium tests.

If you're interested in seeing the results, you can at this link, however.  It builds the latest Chromium with the latest WebKit.
http://build.chromium.org/buildbot/waterfall.fyi/waterfall?branch=&builder=Webkit+(webkit.org)&builder=Webkit+Linux+(webkit.org)&builder=Webkit+Mac+(webkit.org)&builder=Webkit.org+Builder&builder=XP+Perf+(webkit.org)&builder=XP+Tests+(webkit.org)&builder=Linux+Perf+(webkit.org)&builder=Mac10.5+Perf+(webkit.org)&builder=Webkit+Linux+(valgrind+webkit.org)&committer=&reload=none


> Also, for mac updated results it looks like the shadow for the search input was
> broken before on my machine. Could this be a change I want to put into leopard
> or snow-leopard?

I'm not sure I understand.

[Jeremy Orlow]

Note that the first 3 on that link are the ones running pixel tests for the 3 platforms (Linux, Mac, and Windows..which is the one without an explicit platform).  Also note that it often shows failures which are really just things that need to be rebaselined or which need to be fixed in Chromium...which is why this stuff isn't on build.webkit.org yet.  :-)

[Joseph Pecoraro]

> > Also, for mac updated results it looks like the shadow for the search input was
> > broken before on my machine. Could this be a change I want to put into leopard
> > or snow-leopard?
> 
> I'm not sure I understand.

This can be ignored.

I updated the pixel test results for platform/mac for the test you found was failing:
LayoutTests/platform/mac/fast/forms/search-zoomed.html

The updated results include more than just a change to the bottom of the SearchResults icon (the eyeglass). It also showed me changes to the entire input's outlining shadow. I am new to pixel tests and painting in general. I think updating the results for platform/mac are fine but I was concerned if there were differences between tiger and other mac platforms. I just looked and I don't see common pixel test differences between platform/mac-* so I think I was just being overly paranoid.

[Oliver Hunt]

Comment on attachment 54613 [details]
[PATCH] Proposed Fix

r=me

[Joseph Pecoraro]

Thanks for the review Oliver.

Committed r58627
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderTextControlSingleLine.h
	M	WebCore/rendering/RenderTextControl.cpp
	M	WebCore/rendering/RenderTextControlSingleLine.cpp
	M	WebCore/rendering/RenderTextControl.h
	M	LayoutTests/platform/mac/fast/forms/search-zoomed-expected.checksum
	M	LayoutTests/platform/mac/fast/forms/search-zoomed-expected.png
	A	LayoutTests/platform/mac/fast/css/input-search-padding-expected.checksum
	A	LayoutTests/platform/mac/fast/css/input-search-padding-expected.png
	A	LayoutTests/platform/mac/fast/css/input-search-padding-expected.txt
	A	LayoutTests/fast/css/input-search-padding.html
	M	LayoutTests/ChangeLog
r58627 = e4eb38ab4bd1c131de23ea56dcd6a047f3b7d394 (refs/remotes/trunk)
http://trac.webkit.org/changeset/58627

Chromium may need to update its pixel tests, but I think it will be a progression =).