Closed Bug 791742 Opened 12 years ago Closed 12 years ago

Alter blacklisting logic to consider substrings 'decimals'

Categories

(Core :: Graphics, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox16 - ---
firefox17 + fixed
firefox18 --- fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Consider driver version substrings decimals (obsolete) (deleted) — Splinter Review
So we've found out ATI treats the substring as decimals. I've created a patch which deals with this and seems to work well for all other vendor's driver strings. We should keep a close eye on if this stays valid in the future, but for now everything looks good. This patch is pretty terrible, but it does the trick.
Attachment #661840 - Flags: review?(joe)
Comment on attachment 661840 [details] [diff] [review] Consider driver version substrings decimals Review of attachment 661840 [details] [diff] [review]: ----------------------------------------------------------------- *barf* ::: widget/xpwidgets/GfxDriverInfo.h @@ +131,5 @@ > + int destIdx = 0; > + int destPos = 0; > + > + for (int i = 0; i < len; i++) { > + if (destIdx > 3) { ArrayLength(dest) instead of hard-coding. @@ +139,5 @@ > + > + if (aSource[i] == '.') { > + dest[destIdx][destPos] = 0; > + destIdx++; > + destPos = 0; uggghhhhhhh @@ +142,5 @@ > + destIdx++; > + destPos = 0; > + continue; > + } > + kill the spaces on this line @@ +152,5 @@ > + dest[destIdx][destPos++] = aSource[i]; > + } > + > + // Add last terminator. > + dest[destIdx][destPos] = 0; Can you please refactor this loop so it doesn't need this special case? @@ +154,5 @@ > + > + // Add last terminator. > + dest[destIdx][destPos] = 0; > + > + if (destIdx != 3) { ArrayLength(dest) instead of hard-coding.
Attachment #661840 - Flags: review?(joe) → review+
Updated to properly interpret the hardcoded blacklist.
Attachment #661840 - Attachment is obsolete: true
Attachment #662244 - Flags: review?(joe)
Comment on attachment 662244 [details] [diff] [review] Consider driver version substrings decimals v2 Review of attachment 662244 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/xpwidgets/GfxDriverInfo.h @@ +124,5 @@ > > +static uint64_t > +V(uint32_t a, uint32_t b, uint32_t c, uint32_t d) > +{ > + while (b > 0 && b < 1000) { You should add a comment as to why we do this
Attachment #662244 - Flags: review?(joe) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 662244 [details] [diff] [review] Consider driver version substrings decimals v2 Review of attachment 662244 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/xpwidgets/GfxDriverInfo.h @@ +210,5 @@ > + > + a = atoi(aStr); > + b = atoi(bStr); > + c = atoi(cStr); > + d = atoi(dStr); We want the locale-dependent behaviour of atoi here?
Copying the tracking flag from the dupe.
We think we've got Win8 driver blocklisting in an OK state for FF16, but given https://bugzilla.mozilla.org/show_bug.cgi?id=744672#c14, we may still want to uplift for FF17 rather than waiting another 6 weeks.
Nomination for uplift would be great so we can assess the risk to uplifting now while we're on Aurora.
(In reply to :Ms2ger from comment #6) > Comment on attachment 662244 [details] [diff] [review] > Consider driver version substrings decimals v2 > > Review of attachment 662244 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/xpwidgets/GfxDriverInfo.h > @@ +210,5 @@ > > + > > + a = atoi(aStr); > > + b = atoi(bStr); > > + c = atoi(cStr); > > + d = atoi(dStr); > > We want the locale-dependent behaviour of atoi here? Probably not. Fed plain numerics will this ever matter though?
Bas - what's the status of this right now? We've only got two more Betas to consider taking more speculative fixes on if this is going to ship in FF 17. Please nominate for uplift if that's an option here with a risk assessment.
Comment on attachment 662244 [details] [diff] [review] Consider driver version substrings decimals v2 [Approval Request Comment] User impact if declined: inability to correctly block AMD driver versions Testing completed (on m-c, etc.): on m-c and aurora Risk to taking this patch (and alternatives if risky): could potentially break driver version comparison, but likely not String or UUID changes made by this patch: none
Attachment #662244 - Flags: approval-mozilla-beta?
Attachment #662244 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 822804
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: