1213339 - Faulty logic in WantsSmallTiles()

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Bill Gianopoulos [:WG9s]]

In this function, http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLTextureImage.cpp#292 the test for SGX 540 is obviously useless.  If it is a match it returns false otherwise it returns false. The order of the if statements here is backwards from what was intended.

Comment 1

[Bill Gianopoulos [:WG9s]]

Attachment: Fix logic in WantsSmallTiles

Comment 2

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #1)
> Created attachment 8671999 [details] [diff] [review]
> should

This patch corrects the logic in WantsSmallTiles to what was obviously originally intended   otherwise the added code to test for SGX 540 has zero effect.

Comment 3

[Bill Gianopoulos [:WG9s]]

Attachment: Remove test for SGX 540.

However, I recommend this patch because it removes the currently dead patch because it has been this way on the tree for at least 3 years and no one seems to have complained about it being ineffective broke anything.  Plus it was obviously landed with out any proper testing to actually show it fixed the original issue as described.

Comment 4

[Bill Gianopoulos [:WG9s]]

BTW this all came about in debugging the issue in bug 1209801.  I should have mentioned that in the original comment.

Comment 5

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #3)
> Created attachment 8672003 [details] [diff] [review]
> Remove test for SGX 540.

BTW I purposely left in the change from "must" to "should" in the comment to perhaps help prevent future exceptions form being placed at the wrong point in the function.

Comment 6

[Bill Gianopoulos [:WG9s]]

Matt

Please pick the approach you like and i will post a checkin ready patch for review for that approach.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8671999 [details] [diff] [review]
Fix logic in WantsSmallTiles

Review of attachment 8671999 [details] [diff] [review]:
-----------------------------------------------------------------

Switching the request to snorp since this is his domain.

This patch seems just fine to me, but we might want to take the other one if we can.

Comment 8

[Bill Gianopoulos [:WG9s]]

OK so which of the 2 approaches should a submit for review?

Comment 9

[Bill Gianopoulos [:WG9s]]

Assuming the patches for 1209801 stick, I would like to go with the fix the logic rather than the delete the test patch.  The delete the test patch was just a keep current behavior because the code has always been wrong, but fixing it as intended would mask the issue trying to be fixed in bug 1209801.

So I am changing the info needed by the needinfo for who should I ask to review that patch?

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think nical should review.

Comment 11

[Bill Gianopoulos [:WG9s]]

Attachment: Fix logic in WantsSmallTiles

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/288eab800d67

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/288eab800d67

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/288eab800d67