Summary: | REGRESSION(r58313): Regression evident in pixel tests: the search icon is always clipped at the bottom. (Requested by jorlow on #webkit). | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | WebKit Review Bot <webkit.review.bot> | ||||||
Component: | New Bugs | Assignee: | Joseph Pecoraro <joepeck> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ddkilzer, joepeck, jorlow, mitz, ukai | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 38160 | ||||||||
Attachments: |
|
Description
WebKit Review Bot
2010-04-28 03:47:07 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 on attachment 54546 [details] ROLLOUT of r58313 Clearing flags on attachment: 54546 Committed r58400: <http://trac.webkit.org/changeset/58400> All reviewed patches have been landed. Closing bug. 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. (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? Created attachment 54613 [details]
[PATCH] Proposed Fix
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. (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. 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. :-) > > 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 on attachment 54613 [details]
[PATCH] Proposed Fix
r=me
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 =). |