Closed Bug 1325368 Opened 3 years ago Closed 3 years ago

Assume Win7 or later when testing Windows version

Categories

(Core :: Widget: Win32, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: emk, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: tpi:-)

Attachments

(1 file)

For x86, this is almost dead code removal except for a bug fix in nsFilePicker.
For x64, this is almost no-op except for the bug fix.
I wouldn't have done the whole codebase in a single patch. And my upcoming patch for bug 1325503 removes more stuff than yours does, because I grepped for comments and the like as well as the function IsWin7OrLater() functions.
I had to make a tree-wide change because my patch removes IsWin7OrLater() and friends. And this patch tries to minimize behavior changes as written in comment #0.
(In reply to Masatoshi Kimura [:emk] from comment #3)
> I had to make a tree-wide change because my patch removes IsWin7OrLater()
> and friends.

You didn't have to do it that way. The proper way to do this would be to split it into multiple patches, one per module, possibly across multiple bugs, get the appropriate people to review, and only remove those functions at the end. jimm isn't a peer for all the involved modules and it's unfair to ask him to review all the changes.
I have some local patches to IsXPSP3OrLater, IsWin2003OrLater, IsWind2003SP2OrLater, IsVistaOrLater, IsVistaSP1OrLater, and IsWin7OrLater in separate patches, further subdivided by module. That's the nice incremental way to remove these checks, though it will be more overhead. I will be on PTO for a week so I won't be able to submit them any time soon.
Comment on attachment 8821147 [details]
Bug 1325368 - Assume Win7 or later when testing Windows version.

https://reviewboard.mozilla.org/r/100514/#review101692
Attachment #8821147 - Flags: review?(jmathies) → review-
I'm sorry but I don't have permissions to blanket approve changes across the entire code base like this. Lets break this up and seek review from appropriate peers. Especially the media areas like webrtc.
Whiteboard: tpi:-
No longer blocks: xp-eol
Depends on: xp-eol
Depends on: 1334870
Blocks: xp-eol
No longer depends on: xp-eol
I think Jim can review this now.
Some of the changes in this large changeset have already landed in other bugs, such as the xpcom changes. You should rebase your changeset on a more recent mozilla-central or break it into separate, smaller patches.
Yes, the latest patch is minimal, I believe.
Comment on attachment 8821147 [details]
Bug 1325368 - Assume Win7 or later when testing Windows version.

https://reviewboard.mozilla.org/r/100514/#review112868
Attachment #8821147 - Flags: review?(jmathies) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/45e8e39788c8
Assume Win7 or later when testing Windows version. r=jimm
https://hg.mozilla.org/mozilla-central/rev/45e8e39788c8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.