Closed Bug 1213339 Opened 4 years ago Closed 4 years ago

Faulty logic in WantsSmallTiles()

Categories

(Core :: Graphics: Layers, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: wgianopoulos, Assigned: wgianopoulos)

Details

Attachments

(1 file, 2 obsolete files)

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.
Summary: Faulty logic in WantsSmallTiles() is faulty → Faulty logic in WantsSmallTiles()
Attached patch Fix logic in WantsSmallTiles (obsolete) — Splinter Review
Attachment #8671999 - Flags: feedback?
(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.
Attachment #8671999 - Attachment description: should → Fix logic in WantsSmallTiles
Attached patch Remove test for SGX 540. (obsolete) — Splinter Review
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.
Attachment #8672003 - Flags: review?(matt.woodrow)
Attachment #8671999 - Flags: feedback? → feedback?(matt.woodrow)
BTW this all came about in debugging the issue in bug 1209801.  I should have mentioned that in the original comment.
Assignee: nobody → wgianopoulos
Status: NEW → ASSIGNED
(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.
Attachment #8672003 - Flags: review?(matt.woodrow) → feedback?(matt.woodrow)
Matt

Please pick the approach you like and i will post a checkin ready patch for review for that approach.
Flags: needinfo?(matt.woodrow)
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.
Attachment #8671999 - Flags: feedback?(matt.woodrow) → feedback?(snorp)
Flags: needinfo?(matt.woodrow)
Attachment #8672003 - Flags: feedback?(matt.woodrow) → feedback?(snorp)
Attachment #8671999 - Flags: feedback?(snorp) → feedback+
Attachment #8672003 - Flags: feedback?(snorp) → feedback+
OK so which of the 2 approaches should a submit for review?
Flags: needinfo?(snorp)
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?
I think nical should review.
Flags: needinfo?(snorp)
Attachment #8671999 - Attachment is obsolete: true
Attachment #8674640 - Flags: review?(nical.bugzilla)
Attachment #8674640 - Flags: review?(nical.bugzilla) → review+
Attachment #8672003 - Attachment is obsolete: true
Keywords: checkin-needed
OS: Unspecified → Android
Hardware: Unspecified → ARM
https://hg.mozilla.org/mozilla-central/rev/288eab800d67
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.