Bug 39128 – fast/canvas/webgl/texture-npot.html failed on leopard bot

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Zhenyao Mo]

See https://bugs.webkit.org/show_bug.cgi?id=33805

[Zhenyao Mo]

Created attachment 56098 [details]
patch: an attempted fix

Skip pixel (0, 0) in the test; also print out actual pixel color when failing.

[Kenneth Russell]

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.

[Darin Adler]

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.

[Zhenyao Mo]

(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.

[Eric Seidel (no email)]

(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.

[Zhenyao Mo]

Created attachment 56279 [details]
revised patch: add a link to the bug in test comments.

[Darin Adler]

Comment on attachment 56279 [details]
revised patch: add a link to the bug in test comments.

OK, rs=me

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59758 might have broken Leopard Intel Debug (Tests)

[Adam Barth]

I'm confused.  This patch appears to cause the bug it claims to fix...  Rolling out.

[Adam Barth]

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r59762%20(14638)/fast/canvas/webgl/texture-npot-pretty-diff.html

[Zhenyao Mo]

(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.

[Zhenyao Mo]

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.

[Kenneth Russell]

Comment on attachment 56504 [details]
patch

Looks good to me.

[Dimitri Glazkov (Google)]

Comment on attachment 56504 [details]
patch

ok.

[WebKit Commit Bot]

Comment on attachment 56504 [details]
patch

Clearing flags on attachment: 56504

Committed r59899: <http://trac.webkit.org/changeset/59899>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.