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 User image Serge Gautherie (:sgautherie) 2012-01-30 09:59:32 PST
3 mail/ files and 1 suite/ one.
Comment 1 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Murali 2012-04-03 05:04:20 PDT
Created attachment 611780 [details] [diff] [review]
New Patch

Thanks a lot!
Comment 12 User image 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 User image 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 User image 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 User image 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 User image Ryan VanderMeulen [:RyanVM] 2012-04-03 16:35:23 PDT
Thanks for the patch!

http://hg.mozilla.org/comm-central/rev/31da82521973
Comment 17 User image 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.