Closed Bug 942402 Opened 6 years ago Closed 6 years ago

Port Bug 925599 to MailNews (GetVersionEx is deprecated in VS2013, breaks dirs that use FAIL_ON_WARNINGS)

Categories

(MailNews Core :: Import, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 30.0

People

(Reporter: philip.chee, Assigned: emk)

References

Details

Attachments

(2 files, 1 obsolete file)

m-c part
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8339223 - Flags: review?(jmathies)
Attached patch Stop using GetVersionEx in c-c (obsolete) — Splinter Review
Attachment #8339224 - Flags: review?(mbanner)
Attachment #8339223 - Flags: review?(jmathies) → review+
Comment on attachment 8339224 [details] [diff] [review]
Stop using GetVersionEx in c-c

This doesn't seem to read nicely:

+  return mozilla::IsVistaSP1OrLater() ||
+         !mozilla::IsWin2003OrLater() && mozilla::IsXPSP3OrLater() ||
+         !mozilla::IsVistaOrLater() && mozilla::IsWin2003SP2OrLater();

I don't see why parts of this are split, e.g. I'd have thought that IsWin2003OrLater would be alongside IsWin2003SP2OrLater or even not at all.

From the original comment, it feels like this is the simplest way:

return mozilla::IsVistaSP1OrLater() || mozilla::IsXPSP3OrLater() || mozilla::IsWin2003SP2OrLater()

Why the need for the nots?
Attachment #8339224 - Flags: review?(mbanner) → review-
Comment on attachment 8339224 [details] [diff] [review]
Stop using GetVersionEx in c-c

(In reply to Mark Banner (:standard8) from comment #4)
> +  return mozilla::IsVistaSP1OrLater() ||
> +         !mozilla::IsWin2003OrLater() && mozilla::IsXPSP3OrLater() ||
> +         !mozilla::IsVistaOrLater() && mozilla::IsWin2003SP2OrLater();
> 
> I don't see why parts of this are split, e.g. I'd have thought that
> IsWin2003OrLater would be alongside IsWin2003SP2OrLater or even not at all.
> 
> From the original comment, it feels like this is the simplest way:
> 
> return mozilla::IsVistaSP1OrLater() || mozilla::IsXPSP3OrLater() ||
> mozilla::IsWin2003SP2OrLater()
> 
> Why the need for the nots?

Because "XPSP3OrLater" includes Win2003 SP0/SP1 and "Win2003SP2OrLater" includes VistaSP0.
Win2003 SP0/SP1 and VistaSP0 doesn't contain KB933612. Although the original comment doesn't mention about Win2003, MSKB does. We are supposed to support Win2003:
http://www.mozilla.org/en-US/thunderbird/system-requirements/
Even if we ignore the Win2003, we still need to consider about Vista SP0.
Attachment #8339224 - Flags: review- → review?(mbanner)
Comment on attachment 8339224 [details] [diff] [review]
Stop using GetVersionEx in c-c

Ok, so I think the changes are fine, but please expand the comments to include the 2003 notes and be clearer to explain what is being checked here. Thanks.
Attachment #8339224 - Flags: review?(mbanner) → review-
Attachment #8339224 - Attachment is obsolete: true
Attachment #8369126 - Flags: review?(mbanner)
Ping?
Comment on attachment 8369126 [details] [diff] [review]
Stop using GetVersionEx in c-c

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

That's better, thanks.
Attachment #8369126 - Flags: review?(mbanner) → review+
Keywords: checkin-needed
Whiteboard: [leave open]
https://hg.mozilla.org/comm-central/rev/bcb19733ea2e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 30.0
You need to log in before you can comment on or make changes to this bug.