Bug 38253 - REGRESSION(r58313): Regression evident in pixel tests: the search icon is always clipped at the bottom. (Requested by jorlow on #webkit).
Summary: REGRESSION(r58313): Regression evident in pixel tests: the search icon is alw...
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: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks: 38160
  Show dependency treegraph
 
Reported: 2010-04-28 03:47 PDT by WebKit Review Bot
Modified: 2010-04-30 19:57 PDT (History)
5 users (show)

See Also:


Attachments
ROLLOUT of r58313 (27.31 KB, patch)
2010-04-28 03:47 PDT, WebKit Review Bot
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (73.51 KB, patch)
2010-04-28 13:31 PDT, Joseph Pecoraro
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2010-04-28 03:47:07 PDT
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
Comment 1 WebKit Review Bot 2010-04-28 03:47:15 PDT
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.
Comment 2 Jeremy Orlow 2010-04-28 03:57:35 PDT
Comment on attachment 54546 [details]
ROLLOUT of r58313

Clearing flags on attachment: 54546

Committed r58400: <http://trac.webkit.org/changeset/58400>
Comment 3 Jeremy Orlow 2010-04-28 03:57:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Jeremy Orlow 2010-04-28 06:06:35 PDT
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.
Comment 5 Joseph Pecoraro 2010-04-28 13:31:02 PDT
(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?
Comment 6 Joseph Pecoraro 2010-04-28 13:31:41 PDT
Created attachment 54613 [details]
[PATCH] Proposed Fix
Comment 7 Joseph Pecoraro 2010-04-28 13:32:58 PDT
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.
Comment 8 Jeremy Orlow 2010-04-28 13:56:48 PDT
(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.
Comment 9 Jeremy Orlow 2010-04-28 14:02:45 PDT
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.  :-)
Comment 10 Joseph Pecoraro 2010-04-28 14:53:37 PDT
> > 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.
Comment 11 Oliver Hunt 2010-04-30 15:15:08 PDT
Comment on attachment 54613 [details]
[PATCH] Proposed Fix

r=me
Comment 12 Joseph Pecoraro 2010-04-30 19:57:49 PDT
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 =).