Last Comment Bug 741979 - downloaded blocklist doesn't support unspecified version comparator
: downloaded blocklist doesn't support unspecified version comparator
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Joe Drew (not getting mail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-03 11:28 PDT by Joe Drew (not getting mail)
Modified: 2012-05-05 20:26 PDT (History)
2 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add support for no version comparison + tests (6.37 KB, patch)
2012-04-03 11:28 PDT, Joe Drew (not getting mail)
jacob.benoit.1: review+
Details | Diff | Review

Description Joe Drew (not getting mail) 2012-04-03 11:28:19 PDT
Created attachment 611904 [details] [diff] [review]
add support for no version comparison + tests

This caused the problems we had in 722538.
Comment 1 Mozilla RelEng Bot 2012-04-03 18:47:06 PDT
Try run for 0955a02c7bd8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0955a02c7bd8
Results (out of 29 total builds):
    success: 27
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdrew@mozilla.com-0955a02c7bd8
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-04-04 10:39:29 PDT
Comment on attachment 611904 [details] [diff] [review]
add support for no version comparison + tests

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

::: toolkit/mozapps/extensions/test/xpcshell/data/test_gfxBlacklist.xml
@@ +119,5 @@
> +      <vendor>0xabcd</vendor>
> +      <devices>
> +        <device>0x6666</device>
> +      </devices>
> +      <feature> DIRECT2D </feature>

Do the spaces around DIRECT2D matter here, and below?

::: widget/xpwidgets/GfxInfoBase.cpp
@@ +691,5 @@
>        break;
>      case DRIVER_BETWEEN_INCLUSIVE_START:
>        match = driverVersion >= info[i].mDriverVersion && driverVersion < info[i].mDriverVersionMax;
>        break;
> +    case DRIVER_UNKNOWN_COMPARISON:

Why can't this be more explicitly names DRIVER_ALL_VERSIONS since that's what it does?
Comment 3 Joe Drew (not getting mail) 2012-04-04 10:46:27 PDT
(In reply to Benoit Jacob [:bjacob] from comment #2)
> Comment on attachment 611904 [details] [diff] [review]
> add support for no version comparison + tests
> 
> Review of attachment 611904 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/extensions/test/xpcshell/data/test_gfxBlacklist.xml
> @@ +119,5 @@
> > +      <vendor>0xabcd</vendor>
> > +      <devices>
> > +        <device>0x6666</device>
> > +      </devices>
> > +      <feature> DIRECT2D </feature>
> 
> Do the spaces around DIRECT2D matter here, and below?

No. We explicitly trim all spaces before doing comparisons.

> ::: widget/xpwidgets/GfxInfoBase.cpp
> @@ +691,5 @@
> >        break;
> >      case DRIVER_BETWEEN_INCLUSIVE_START:
> >        match = driverVersion >= info[i].mDriverVersion && driverVersion < info[i].mDriverVersionMax;
> >        break;
> > +    case DRIVER_UNKNOWN_COMPARISON:
> 
> Why can't this be more explicitly names DRIVER_ALL_VERSIONS since that's
> what it does?

How about DRIVER_COMPARISON_IGNORED?
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-04-04 11:20:42 PDT
Works for me.
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-04-04 11:22:14 PDT
I still feel very incompetent to review the test. Is it a mochitest? it doesn't use mochitest stuff like ok() or todo().
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-04-04 11:22:53 PDT
ah ok, xpcshell test. can you find someone who knows that stuff? or teach me what i need to know,
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-04-07 08:10:29 PDT
Comment on attachment 611904 [details] [diff] [review]
add support for no version comparison + tests

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

r=me with constant name change discussed above, but consider the xpcshell test part as not very closely reviewed.
Comment 8 Joe Drew (not getting mail) 2012-05-04 20:16:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7cb2d403f33
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-05-05 20:26:00 PDT
https://hg.mozilla.org/mozilla-central/rev/f7cb2d403f33

Note You need to log in before you can comment on or make changes to this bug.