Last Comment Bug 722370 - Remove obsolete MOZ_WINSDK_TARGETVER checks in comm-central, after bug 721447
: Remove obsolete MOZ_WINSDK_TARGETVER checks in comm-central, after bug 721447
Status: VERIFIED FIXED
:
Product: MailNews Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All Windows 7
: -- minor (vote)
: Thunderbird 14.0
Assigned To: Murali
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 721447
Blocks: 721496
  Show dependency treegraph
 
Reported: 2012-01-30 09:59 PST by Serge Gautherie (:sgautherie)
Modified: 2012-04-04 04:04 PDT (History)
4 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch 1 (9.26 KB, patch)
2012-04-01 05:17 PDT, Murali
mozilla: feedback-
Details | Diff | Splinter Review
Updated Patch (8.36 KB, patch)
2012-04-01 07:52 PDT, Murali
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
Updated Patch 2 (8.39 KB, patch)
2012-04-01 13:39 PDT, Murali
mozilla: review+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
New Patch (8.41 KB, patch)
2012-04-03 05:04 PDT, Murali
ludovic: review+
ludovic: feedback+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-01-30 09:59:32 PST
3 mail/ files and 1 suite/ one.
Comment 1 Murali 2012-03-30 06:33:51 PDT
Hi, I'm interested in working on this bug. Could you please tell me more about what I'm supposed to do?
Comment 2 David :Bienvenu 2012-03-30 10:38:09 PDT
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
Comment 3 Murali 2012-04-01 05:17:00 PDT
Created attachment 611266 [details] [diff] [review]
Patch 1

Could you please check whether this patch is somewhere in the region of okay? Thank you.
Comment 4 David :Bienvenu 2012-04-01 07:17:47 PDT
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.
Comment 5 Murali 2012-04-01 07:52:34 PDT
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.
Comment 6 Serge Gautherie (:sgautherie) 2012-04-01 12:53:59 PDT
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.
Comment 7 Murali 2012-04-01 13:39:20 PDT
Created attachment 611313 [details] [diff] [review]
Updated Patch 2

Updated the patch, implementing the change you mentioned. Thanks a lot!
Comment 9 David :Bienvenu 2012-04-02 10:06:53 PDT
Comment on attachment 611313 [details] [diff] [review]
Updated Patch 2

thx for the patch!
Comment 10 David :Bienvenu 2012-04-02 10:08:15 PDT
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!
Comment 11 Murali 2012-04-03 05:04:20 PDT
Created attachment 611780 [details] [diff] [review]
New Patch

Thanks a lot!
Comment 12 Serge Gautherie (:sgautherie) 2012-04-03 05:32:17 PDT
Comment on attachment 611780 [details] [diff] [review]
New Patch

No more review is needed.
Include f= and r=, then add 'checkin-needed' keyword.
Comment 13 Murali 2012-04-03 05:46:40 PDT
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 14 Ludovic Hirlimann [:Usul] 2012-04-03 05:56:00 PDT
Comment on attachment 611780 [details] [diff] [review]
New Patch

Carrying feedback and review flags.
Comment 15 Serge Gautherie (:sgautherie) 2012-04-03 06:20:59 PDT
(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
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-04-03 16:35:23 PDT
Thanks for the patch!

http://hg.mozilla.org/comm-central/rev/31da82521973
Comment 17 Serge Gautherie (:sgautherie) 2012-04-04 04:04:17 PDT
V.Fixed, per MXR search.

Note You need to log in before you can comment on or make changes to this bug.