Closed Bug 722370 Opened 8 years ago Closed 8 years ago

Remove obsolete MOZ_WINSDK_TARGETVER checks in comm-central, after bug 721447

Categories

(MailNews Core :: Build Config, defect, minor)

All
Windows 7
defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 14.0

People

(Reporter: sgautherie, Assigned: murali.sr92)

References

()

Details

Attachments

(1 file, 3 obsolete files)

3 mail/ files and 1 suite/ one.
Depends on: 721447
No longer depends on: RequireWin7SDK
Summary: Remove obsolete MOZ_WINSDK_TARGETVER checks in comm-central, after bug 699385 → Remove obsolete MOZ_WINSDK_TARGETVER checks in comm-central, after bug 721447
Blocks: 721496
No longer blocks: 722366
Hi, I'm interested in working on this bug. Could you please tell me more about what I'm supposed to do?
first thing is to use MXR to find them:

http://mxr.mozilla.org/comm-central/search?string=MOZ_WINSDK_TARGETVER

and then figure out which ones we don't need because we no longer support the versions of Windows they're checking for. (I don't think we need any of the checks, fwiw). Then, propose a patch that removes them and attach it here, and ask for a code review.

I'm not sure how much you've done with Thunderbird, but here are build instructions - https://developer.mozilla.org/en/Simple_Thunderbird_build
Whiteboard: [good first bug] → [good first bug][mentor=dbienvenu][lang=c++]
Attached patch Patch 1 (obsolete) — Splinter Review
Could you please check whether this patch is somewhere in the region of okay? Thank you.
Attachment #611266 - Flags: feedback?(dbienvenu)
Comment on attachment 611266 [details] [diff] [review]
Patch 1

thx for the patch. You should only remove the checks against MOZ_NTDDI_LONGHORN, not all the checks. So the changes that remove that check are good, but I think the more generic removals are wrong.
Attachment #611266 - Flags: feedback?(dbienvenu) → feedback-
Attached patch Updated Patch (obsolete) — Splinter Review
As per your comment, I've modified the patch. It now removes all checks against MOZ_NTDDI_LONGHORN. Please suggest necessary changes. Thank you.
Attachment #611266 - Attachment is obsolete: true
Attachment #611276 - Flags: review?(dbienvenu)
Comment on attachment 611276 [details] [diff] [review]
Updated Patch

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

::: configure.in
@@ +788,5 @@
>      fi
>  
>      AC_DEFINE_UNQUOTED(MOZ_WINSDK_TARGETVER,0x$MOZ_WINSDK_TARGETVER)
>      # Definitions matching sdkddkver.h
>      AC_DEFINE_UNQUOTED(MOZ_NTDDI_WS03, 0x05020000)

Remove the MOZ_NTDDI_WS03 line too.
Attachment #611276 - Flags: feedback+
Attached patch Updated Patch 2 (obsolete) — Splinter Review
Updated the patch, implementing the change you mentioned. Thanks a lot!
Attachment #611276 - Attachment is obsolete: true
Attachment #611313 - Flags: feedback?(sgautherie.bz)
Attachment #611276 - Flags: review?(dbienvenu)
Attachment #611313 - Flags: review?(dbienvenu)
Attachment #611313 - Flags: feedback?(sgautherie.bz)
Attachment #611313 - Flags: feedback+
Comment on attachment 611313 [details] [diff] [review]
Updated Patch 2

thx for the patch!
Attachment #611313 - Flags: review?(dbienvenu) → review+
Murali, if you can look at the link Serge helpfully posted about how to generate a patch for somebody else to checkin, follow those instructions, attach a new patch, and add the checkin-needed keyword, we can land your patch. Thx again!
Attached patch New PatchSplinter Review
Thanks a lot!
Attachment #611313 - Attachment is obsolete: true
Attachment #611780 - Flags: review?
Comment on attachment 611780 [details] [diff] [review]
New Patch

No more review is needed.
Include f= and r=, then add 'checkin-needed' keyword.
Attachment #611780 - Flags: review?
Hi,

I am unable to edit the keywords (I think I need to be assigned the bug first?). Also, are there any parameters for f and r?

Thanks!
Comment on attachment 611780 [details] [diff] [review]
New Patch

Carrying feedback and review flags.
Attachment #611780 - Flags: review+
Attachment #611780 - Flags: feedback+
Assignee: nobody → murali.sr92
Status: NEW → ASSIGNED
Keywords: checkin-needed
(In reply to Murali from comment #13)

> I am unable to edit the keywords (I think I need to be assigned the bug
> first?).

Oh. Ludovic did it...

> Also, are there any parameters for f and r?

f=sgautherie r=dbienvenu
Flags: in-testsuite-
Hardware: x86 → All
Whiteboard: [good first bug][mentor=dbienvenu][lang=c++]
Target Milestone: --- → Thunderbird 14.0
Thanks for the patch!

http://hg.mozilla.org/comm-central/rev/31da82521973
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
V.Fixed, per MXR search.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.