The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Thunderbird 14.0

Status

MailNews Core
Build Config
--
minor
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: Murali)

Tracking

Trunk
Thunderbird 14.0
All
Windows 7
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

8.41 KB, patch
Usul
: review+
Usul
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
3 mail/ files and 1 suite/ one.
(Reporter)

Updated

5 years ago
Depends on: 721447
No longer depends on: 699385
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
(Reporter)

Updated

5 years ago
Blocks: 721496
No longer blocks: 722366
(Assignee)

Comment 1

5 years ago
Hi, I'm interested in working on this bug. Could you please tell me more about what I'm supposed to do?

Comment 2

5 years ago
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
(Reporter)

Updated

5 years ago
Whiteboard: [good first bug] → [good first bug][mentor=dbienvenu][lang=c++]
(Assignee)

Comment 3

5 years ago
Created attachment 611266 [details] [diff] [review]
Patch 1

Could you please check whether this patch is somewhere in the region of okay? Thank you.
Attachment #611266 - Flags: feedback?(dbienvenu)

Comment 4

5 years ago
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-
(Assignee)

Comment 5

5 years ago
Created attachment 611276 [details] [diff] [review]
Updated Patch

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)
(Reporter)

Comment 6

5 years ago
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+
(Assignee)

Comment 7

5 years ago
Created attachment 611313 [details] [diff] [review]
Updated Patch 2

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)
(Reporter)

Updated

5 years ago
Attachment #611313 - Flags: review?(dbienvenu)
Attachment #611313 - Flags: feedback?(sgautherie.bz)
Attachment #611313 - Flags: feedback+
(Reporter)

Comment 8

5 years ago
See
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 9

5 years ago
Comment on attachment 611313 [details] [diff] [review]
Updated Patch 2

thx for the patch!
Attachment #611313 - Flags: review?(dbienvenu) → review+

Comment 10

5 years ago
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!
(Assignee)

Comment 11

5 years ago
Created attachment 611780 [details] [diff] [review]
New Patch

Thanks a lot!
Attachment #611313 - Attachment is obsolete: true
Attachment #611780 - Flags: review?
(Reporter)

Comment 12

5 years ago
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?
(Assignee)

Comment 13

5 years ago
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
(Reporter)

Comment 15

5 years ago
(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
(Reporter)

Updated

5 years ago
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
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Reporter)

Comment 17

5 years ago
V.Fixed, per MXR search.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.