Closed Bug 1368150 Opened 3 years ago Closed 3 years ago

Add functions to WindowsVersion for Windows 10 updates

Categories

(Core :: MFBT, enhancement)

Unspecified
Windows
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

Details

Attachments

(2 files, 1 obsolete file)

Sadly I need to distinguish between Windows 10 Creators Update and previous releases.
Blocks: 1354077
Attachment #8871925 - Flags: review?(nfroyd)
Comment on attachment 8871925 [details] [diff] [review]
Add IsWindows10BuildOrNewer to MFBT

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

I was going to rs+ this, but I looked at the code and am confused.  I am willing to believe that everything works as expected, but let's have a discussion about this first.

::: mfbt/WindowsVersion.h
@@ +89,5 @@
> +{
> +  static uint32_t minBuild = 0;
> +  static uint32_t maxBuild = UINT32_MAX;
> +
> +  if (minBuild >= aBuild) {

The >= is a little weird, because you can't have > in this case, but OK.

@@ +93,5 @@
> +  if (minBuild >= aBuild) {
> +    return true;
> +  }
> +
> +  if (aBuild >= maxBuild) {

Likewise here.  What if aBuild is maxBuild, though?  Wouldn't that satisfy the "Is" of the function name?

@@ +97,5 @@
> +  if (aBuild >= maxBuild) {
> +    return false;
> +  }
> +
> +  OSVERSIONINFOEX info = {sizeof(OSVERSIONINFOEX), 10, 0, aBuild};

IsWindowsVersionOrLater explicitly zeros this structure and assigns the relevant members explicitly.  Why not do that here as well?

@@ +109,5 @@
> +
> +  if (VerifyVersionInfo(&info, VER_MAJORVERSION | VER_MINORVERSION |
> +                        VER_BUILDNUMBER | VER_SERVICEPACKMAJOR |
> +                        VER_SERVICEPACKMINOR, conditionMask)) {
> +    minBuild = aBuild;

This assignment is dead code.

@@ +113,5 @@
> +    minBuild = aBuild;
> +    return true;
> +  }
> +
> +  maxBuild = aBuild;

This assignment is also dead code.  In fact, I'm kind of confused about what minBuild and maxBuild are trying to do...
Attachment #8871925 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8871925 [details] [diff] [review]
> Add IsWindows10BuildOrNewer to MFBT
> 
> Review of attachment 8871925 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I was going to rs+ this, but I looked at the code and am confused.  I am
> willing to believe that everything works as expected, but let's have a
> discussion about this first.

So the minBuild and maxBuild stuff is identical to how it is done in IsWindowsBuildOrLater(). I didn't originally write this code, but the idea here is that, rather than calling VerifyVersionInfo() over and over again to obtain the same results for the same input, we accumulate the results of previous calls by storing them as thresholds in minBuild and maxBuild.

So if I call IsWindows10BuildOrLater(15000) and the result is false, we know that any future calls whose build number is >= 15000 are also false. Hence 15000 is stored in maxBuild. OTOH, if we call IsWindows10BuildOrLater(10000) and the result is true, we know that any build < 10000 is also true.

> 
> ::: mfbt/WindowsVersion.h
> @@ +89,5 @@
> > +{
> > +  static uint32_t minBuild = 0;
> > +  static uint32_t maxBuild = UINT32_MAX;
> > +
> > +  if (minBuild >= aBuild) {
> 
> The >= is a little weird, because you can't have > in this case, but OK.

Well, since minBuild is static, you might eventually have >.

> 
> @@ +93,5 @@
> > +  if (minBuild >= aBuild) {
> > +    return true;
> > +  }
> > +
> > +  if (aBuild >= maxBuild) {
> 
> Likewise here.  What if aBuild is maxBuild, though?  Wouldn't that satisfy
> the "Is" of the function name?

maxBuild is being used as an upper threshold, so when maxBuild < aBuild, we know that no build number >= maxBuild will succeed.

> 
> @@ +97,5 @@
> > +  if (aBuild >= maxBuild) {
> > +    return false;
> > +  }
> > +
> > +  OSVERSIONINFOEX info = {sizeof(OSVERSIONINFOEX), 10, 0, aBuild};
> 
> IsWindowsVersionOrLater explicitly zeros this structure and assigns the
> relevant members explicitly.  Why not do that here as well?

Sure, fixed.

> 
> @@ +109,5 @@
> > +
> > +  if (VerifyVersionInfo(&info, VER_MAJORVERSION | VER_MINORVERSION |
> > +                        VER_BUILDNUMBER | VER_SERVICEPACKMAJOR |
> > +                        VER_SERVICEPACKMINOR, conditionMask)) {
> > +    minBuild = aBuild;
> 
> This assignment is dead code.

I don't follow. It looks reachable to me.

> 
> @@ +113,5 @@
> > +    minBuild = aBuild;
> > +    return true;
> > +  }
> > +
> > +  maxBuild = aBuild;
> 
> This assignment is also dead code.

Ditto.
Attachment #8871925 - Attachment is obsolete: true
Attachment #8874915 - Flags: review?(nfroyd)
Comment on attachment 8874915 [details] [diff] [review]
Add IsWindows10BuildOrNewer to MFBT (r2)

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

OK.  I missed the staticness before, or failed to fully consider the implications thereof.  Thank you for patiently explaining things to me!
Attachment #8874915 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/80924717bcc6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attached patch Patch for ESR52Splinter Review
aklotz:

How do you think the risk of taking this into ESR52? (Looks like that the patch works fine even on Vista and XP, though, but I'm not sure.)

FYI: If we need to fix bug 1361132, the patch requires your patch.
Flags: needinfo?(aklotz)
Low risk. Sure.
Flags: needinfo?(aklotz)
(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #8)
> Low risk. Sure.

Thank you!
Depends on: 1406793
Out of curiosity, is there a reason this couldn't be achieved by just calling IsWindowsBuildOrLater(15063)? The "Windows 10" part ought to be implied by the build number, IIUC.
Flags: needinfo?(aklotz)
I guess so. Chalk it up to OCD on my part.
Flags: needinfo?(aklotz)
You need to log in before you can comment on or make changes to this bug.