Bug 39128

Summary: fast/canvas/webgl/texture-npot.html failed on leopard bot
Product: WebKit Reporter: Zhenyao Mo <zmo>
Component: WebGLAssignee: 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 Flags
patch: an attempted fix
none
revised patch: add a link to the bug in test comments.
none
patch none

Description Zhenyao Mo 2010-05-14 11:46:30 PDT
See https://bugs.webkit.org/show_bug.cgi?id=33805
Comment 1 Zhenyao Mo 2010-05-14 12:57:28 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 2 Kenneth Russell 2010-05-14 12:59:26 PDT
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 3 Darin Adler 2010-05-17 10:54:56 PDT
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.
Comment 4 Zhenyao Mo 2010-05-17 12:04:03 PDT
(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.
Comment 5 Eric Seidel (no email) 2010-05-17 15:12:28 PDT
(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.
Comment 6 Zhenyao Mo 2010-05-17 15:46:12 PDT
Created attachment 56279 [details]
revised patch: add a link to the bug in test comments.
Comment 7 Darin Adler 2010-05-17 15:50:50 PDT
Comment on attachment 56279 [details]
revised patch: add a link to the bug in test comments.

OK, rs=me
Comment 8 WebKit Commit Bot 2010-05-19 03:59:52 PDT
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>
Comment 9 WebKit Commit Bot 2010-05-19 03:59:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 WebKit Review Bot 2010-05-19 04:53:39 PDT
http://trac.webkit.org/changeset/59758 might have broken Leopard Intel Debug (Tests)
Comment 11 Adam Barth 2010-05-19 08:06:18 PDT
I'm confused.  This patch appears to cause the bug it claims to fix...  Rolling out.
Comment 13 Zhenyao Mo 2010-05-19 08:29:48 PDT
(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.
Comment 14 Zhenyao Mo 2010-05-19 11:37:11 PDT
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 15 Kenneth Russell 2010-05-19 12:30:42 PDT
Comment on attachment 56504 [details]
patch

Looks good to me.
Comment 16 Dimitri Glazkov (Google) 2010-05-19 12:56:01 PDT
Comment on attachment 56504 [details]
patch

ok.
Comment 17 WebKit Commit Bot 2010-05-20 19:34:53 PDT
Comment on attachment 56504 [details]
patch

Clearing flags on attachment: 56504

Committed r59899: <http://trac.webkit.org/changeset/59899>
Comment 18 WebKit Commit Bot 2010-05-20 19:34:59 PDT
All reviewed patches have been landed.  Closing bug.