Version support in downloadable graphics blocklist

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: milan, Assigned: milan)

Tracking

(Blocks 1 bug)

40 Branch
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 wontfix, firefox38 wontfix, firefox39+ wontfix, firefox38.0.5 wontfix, firefox40+ fixed, firefox41 fixed, relnote-firefox 40+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Right now, once it's in the blocklist, it blocks all versions.  Add the capability to specify minimum, maximum (or both) version(s) in the XML, so that we can blocklist things for a specific version or range.
Assignee: nobody → milan
Blocks: 838845
OS: Unspecified → All
Hardware: Unspecified → All
Other places in the blocklist use <versionRange minVersion="..." maxVersion="..."></versionRange>, we can probably use the same.
[Tracking Requested - why for this release]:

If we fix this for 40, it will let us use downloadable blocklist for release 39 and earlier as well; if we uplift to 39, it means we can handle 38 (and earlier) as well, which bodes well for an ESR release.
This will only work properly for non-e10s until bug 1162700 is fixed.
Attachment #8603742 - Flags: review?(jmuizelaar)
Attachment #8603742 - Flags: feedback?(bas)
Comment on attachment 8603742 [details] [diff] [review]
Allow min and max app versions for the downloadable blocklist. r=jmuizelaar

There's a typo in the patch, let me fix that.
Attachment #8603742 - Flags: review?(jmuizelaar)
Attachment #8603742 - Flags: feedback?(bas)
Use nsVersionComparator for version comparisons.  Add some unit tests for that, as well as the split version from GfxDriverInfo, to better describe what it is that happens in the version comparison (and the split was just a, turns out, unrelated bonus.)
Manual testing was done as described in bug 1164036, which will be the automated test for the same thing, but we have longer dependencies in order to take care of that.
Attachment #8603742 - Attachment is obsolete: true
Attachment #8604715 - Flags: review?(jmuizelaar)
Comment on attachment 8604715 [details] [diff] [review]
Allow min and max app versions for the downloadable blocklist. r=jmuizelaar

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

::: widget/GfxInfoBase.cpp
@@ +1047,1 @@
>          return NS_ERROR_OUT_OF_MEMORY;

It looks like there's a couple of spurious changes here and at the GetFailures() definition.
Attachment #8604715 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Comment on attachment 8604715 [details] [diff] [review]
> Allow min and max app versions for the downloadable blocklist. r=jmuizelaar
> 
> Review of attachment 8604715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/GfxInfoBase.cpp
> @@ +1047,1 @@
> >          return NS_ERROR_OUT_OF_MEMORY;
> 
> It looks like there's a couple of spurious changes here and at the
> GetFailures() definition.

Yeah, I sneaked in a couple of "we're using tabs instead of spaces" changes.
Get rid of the whitespace changes (will add in another changeset.)
Attachment #8604715 - Attachment is obsolete: true
Attachment #8606368 - Flags: review+
Attachment #8606369 - Attachment description: Part 2: Some cleanup in downloadable blocklist - we were not actually checkign for WebRTC, some whitespace changes, watch for unknown OS. r=kats → Part 2: Some cleanup in downloadable blocklist - we were not actually checking for WebRTC, some whitespace changes, watch for unknown OS. r=kats
Comment on attachment 8606369 [details] [diff] [review]
Part 2: Some cleanup in downloadable blocklist - we were not actually checking for WebRTC, some whitespace changes, watch for unknown OS. r=kats

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

Looks ok to me but again I'm hesitant to r+ since I don't know this code much.
Attachment #8606369 - Flags: review?(jmuizelaar)
Attachment #8606369 - Flags: review?(bugmail.mozilla)
Attachment #8606369 - Flags: feedback+
Adding tracking flags for firefox40 and firefox 39 based on comment #2.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> ...
> Looks ok to me but again I'm hesitant to r+ since I don't know this code
> much.

You're becoming an expert just by looking at it :)
Attachment #8606369 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/93fe551e8a55
https://hg.mozilla.org/mozilla-central/rev/edb5b81dd9e7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8606368 [details] [diff] [review]
Part 1: Allow min and max app versions for the downloadable blocklist. Carry r=jmuizelaar

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: This gives us ability to blocklist things we wouldn't before because it would affect too many releases.  Uplifting to 40 means we could do specific blocklisting for release 39 and earlier.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8606368 - Flags: approval-mozilla-aurora?
Comment on attachment 8606369 [details] [diff] [review]
Part 2: Some cleanup in downloadable blocklist - we were not actually checking for WebRTC, some whitespace changes, watch for unknown OS. r=kats

Same as above.
Attachment #8606369 - Flags: approval-mozilla-aurora?
Comment on attachment 8606368 [details] [diff] [review]
Part 1: Allow min and max app versions for the downloadable blocklist. Carry r=jmuizelaar

We do want that in 40!
Attachment #8606368 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8606369 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Added to the release notes with "Graphic blocklist mechanism improved: Firefox version ranges can be specified, limiting the number of devices blocked" as wording.
You need to log in before you can comment on or make changes to this bug.