Summary: | fast/canvas/webgl/texture-npot.html failed on leopard bot | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zhenyao Mo <zmo> | ||||||||
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, cmarrin, commit-queue, eric, kbr, oliver, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Bug Depends on: | 39361 | ||||||||||
Bug Blocks: | 41545 | ||||||||||
Attachments: |
|
Description
Zhenyao Mo
2010-05-14 11:46:30 PDT
Created attachment 56098 [details]
patch: an attempted fix
Skip pixel (0, 0) in the test; also print out actual pixel color when failing.
Comment on attachment 56098 [details]
patch: an attempted fix
Looks fine to me. Eric suggests to commit (and potentially revise / revert) this during off-hours to minimize the impact of breakage on this bot.
Comment on attachment 56098 [details] patch: an attempted fix > + // We skip the (0, 0) pixel due to a possible bug on the Leopard bot. This comment is too vague. The change otherwise seems fine. I suggest changing it to say "i = 1" instead of adding this if statement. (In reply to comment #3) > (From update of attachment 56098 [details]) > > + // We skip the (0, 0) pixel due to a possible bug on the Leopard bot. > > This comment is too vague. The change otherwise seems fine. I suggest changing it to say "i = 1" instead of adding this if statement. This patch is a temporary attempt to confirm a guess of the cause of this failure. That's why I don't want to add a detailed explanation which might be incorrect. (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 56098 [details] [details]) > > > + // We skip the (0, 0) pixel due to a possible bug on the Leopard bot. > > > > This comment is too vague. The change otherwise seems fine. I suggest changing it to say "i = 1" instead of adding this if statement. > > This patch is a temporary attempt to confirm a guess of the cause of this failure. That's why I don't want to add a detailed explanation which might be incorrect. Temporary things tend to have a nasty habit of becoming permanent. :) IMO, you could just link to a bug as a second line of the comment that that would make it not-vague enough for my tastes. :) At least then someone can look at the bug to know later if/when it's safe to remove that line. Created attachment 56279 [details]
revised patch: add a link to the bug in test comments.
Comment on attachment 56279 [details]
revised patch: add a link to the bug in test comments.
OK, rs=me
Comment on attachment 56279 [details] revised patch: add a link to the bug in test comments. Clearing flags on attachment: 56279 Committed r59758: <http://trac.webkit.org/changeset/59758> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/59758 might have broken Leopard Intel Debug (Tests) I'm confused. This patch appears to cause the bug it claims to fix... Rolling out. (In reply to comment #11) > I'm confused. This patch appears to cause the bug it claims to fix... Rolling out. This patch is testing out a guess of the cause of the failure and at the same time if the guess is wrong, the patch collects some extra information. I already asked kbr and Eric to help rolling this out, but you beat them. Thanks. Created attachment 56504 [details]
patch
Adding test cases back, but skip the test on Leopard for now.
Once we get mesa renderer integrated, we will turn it back on.
Comment on attachment 56504 [details]
patch
Looks good to me.
Comment on attachment 56504 [details]
patch
ok.
Comment on attachment 56504 [details] patch Clearing flags on attachment: 56504 Committed r59899: <http://trac.webkit.org/changeset/59899> All reviewed patches have been landed. Closing bug. |