741979 - downloaded blocklist doesn't support unspecified version comparator

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Joe Drew (not getting mail)]

Attachment: add support for no version comparison + tests

This caused the problems we had in 722538.

Comment 1

[Mozilla RelEng Bot]

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/[e-mail]-0955a02c7bd8

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

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)]

(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)]

Works for me.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

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)]

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)]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7cb2d403f33

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7cb2d403f33