721496 - Remove code ifdef'd MOZ_WINSDK_TARGETVER for pre-Windows 7 SDKs

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Rain [:rain1] (inactive)]

Everything ifdef'd as MOZ_WINSDK_TARGETVER < MOZ_NTDDI_WIN7 should go.

Comment 1

[Serge Gautherie (:sgautherie)]

"Found 142 matching lines in 41 files"

Feel free to do this in multiple parts, maybe in blocking bugs too.

Comment 3

[Mark Capella [:capella]]

So looking at this request and without discussing it with anyone yet...

With CONFIGURE.IN having:

   AC_DEFINE_UNQUOTED(MOZ_NTDDI_WS03, 0x05020000)
   AC_DEFINE_UNQUOTED(MOZ_NTDDI_LONGHORN, 0x06000000)
   AC_DEFINE_UNQUOTED(MOZ_NTDDI_WIN7, 0x06010000)

Case #1 ===> (2) unused references to MOZ_NTDDI_WS03 seems safe to remove:

   /configure.in
       line 977 -- AC_DEFINE_UNQUOTED(MOZ_NTDDI_WS03, 0x05020000)

   /js/src/configure.in (View Hg log or Hg annotations)
       line 933 -- AC_DEFINE_UNQUOTED(MOZ_NTDDI_WS03, 0x05020000)


Is Edge Case #2 ===> (1) reference to < MOZ_NTDDI_LONGHORN safe to change / consistent with this request?:

/dom/plugins/ipc/PluginModuleChild.cpp 
    line 692 -- #if !defined XP_WIN || MOZ_WINSDK_TARGETVER < MOZ_NTDDI_LONGHORN

This code:
----------------------------
   bool
   PluginModuleChild::RecvSetAudioSessionData(const nsID& aId,
                                              const nsString& aDisplayName,
                                              const nsString& aIconPath)
   {
   #if !defined XP_WIN || MOZ_WINSDK_TARGETVER < MOZ_NTDDI_LONGHORN
       NS_RUNTIMEABORT("Not Reached!");
       return false;
   #else
       nsresult rv = mozilla::widget::RecvAudioSessionData(aId, aDisplayName, aIconPath);
       NS_ENSURE_SUCCESS(rv, true); // Bail early if this fails
   	
       // Ignore failures here; we can't really do anything about them
       mozilla::widget::StartAudioSession();
       return true;
   #endif
   }

Would become This code:	
----------------------------
bool
PluginModuleChild::RecvSetAudioSessionData(const nsID& aId,
                                           const nsString& aDisplayName,
                                           const nsString& aIconPath)
   {
   #if defined XP_WIN && MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
       nsresult rv = mozilla::widget::RecvAudioSessionData(aId, aDisplayName, aIconPath);
       NS_ENSURE_SUCCESS(rv, true); // Bail early if this fails

       // Ignore failures here; we can't really do anything about them
       mozilla::widget::StartAudioSession();
       return true;
   #else
       NS_RUNTIMEABORT("Not Reached!");
       return false;
   #endif
   }

Comment 4

[Rain [:rain1] (inactive)]

(In reply to Mark Capella [:capella] from comment #3)
> So looking at this request and without discussing it with anyone yet...
> 
> With CONFIGURE.IN having:
> 
>    AC_DEFINE_UNQUOTED(MOZ_NTDDI_WS03, 0x05020000)
>    AC_DEFINE_UNQUOTED(MOZ_NTDDI_LONGHORN, 0x06000000)
>    AC_DEFINE_UNQUOTED(MOZ_NTDDI_WIN7, 0x06010000)
> 
> Case #1 ===> (2) unused references to MOZ_NTDDI_WS03 seems safe to remove:

If MOZ_NTDDI_WS03 isn't used anywhere, sure, it's safe to remove now.

> 
> Is Edge Case #2 ===> (1) reference to < MOZ_NTDDI_LONGHORN safe to change /
> consistent with this request?:
> 
> /dom/plugins/ipc/PluginModuleChild.cpp 
>     line 692 -- #if !defined XP_WIN || MOZ_WINSDK_TARGETVER <
> MOZ_NTDDI_LONGHORN

This should become #ifndef XP_WIN.

> Would become This code:	
> ----------------------------
> bool
> PluginModuleChild::RecvSetAudioSessionData(const nsID& aId,
>                                            const nsString& aDisplayName,
>                                            const nsString& aIconPath)
>    {
>    #if defined XP_WIN && MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN

no, just #ifdef XP_WIN. We can now assume that MOZ_WINSDK_TARGETVER is always greater than MOZ_NTDDI_LONGHORN (and WIN7 for that matter).

Comment 5

[Mark Capella [:capella]]

Attachment: First Cut at changes

I should have asked you to assign the bug to me, but I went ahead and started on it...

   This attached DIFF is code changes where I removed references to MOZ_NTDDI_WS03 and MOZ_NTDDI_LONGHORN ... (25) files .IN / .H / .CPP changed ... (I didn't remove references to MOZ_NTDDI_WIN7 yet...)

   I can go ahead and make the changes to remove MOZ_NTDDI_WIN7 references and re-build and retest if you want. I wasn't sure how a change of this size would be tested / moved forward (pieces or all at once)

   I ran a build and tested the browser, then ran the PYTEST suite and got 140,682 passes, 9 fails, and 3953 ToDo's. I have the .LOG file if you need to see it.

   Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120214 Firefox/13.0a1

--mark

Comment 6

[[e-mail]]

Comment on attachment 596985 [details] [diff] [review]
First Cut at changes

> 
>-#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
> 
In cases where there is blank space both before and after the removed line, you should reduce the blank space too.

>-#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
> // For Vista IFileDialog interfaces
> #if _WIN32_WINNT < _WIN32_WINNT_LONGHORN
> #define _WIN32_WINNT_bak _WIN32_WINNT
> #undef _WIN32_WINNT
> #define _WIN32_WINNT _WIN32_WINNT_LONGHORN
> #define _WIN32_IE_bak _WIN32_IE
> #undef _WIN32_IE
> #define _WIN32_IE _WIN32_IE_IE70
> #endif
>-#endif
I'm not sure offhand but I think the Windows 7 SDK might default to newer values here too.

>-#if defined(XP_WIN) && MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>     REGISTER(Private);
>-#endif
This change looks wrong.

Comment 7

[Mark Capella [:capella]]

Attachment: Second Cut at changes

>>> In cases where there is blank space both before and after the removed line, you should reduce the blank space too.

    Ok, I Fixed up (5) files:

        windowsbattery.cpp    removed (1) empty line
        nsdownloadserver.cpp  removed (2) empty lines
        audiosession.cpp      removed (2) empty lines
        audiosession.h        removed (2) empty lines
        nsfilepicker.cpp      removed (9) empty lines


>>> I'm not sure offhand but I think the Windows 7 SDK might default to newer values here too.

    Fixed up file nsfilepicker.h by removing two blocks:

    - #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
    - // For Vista IFileDialog interfaces
    - #if _WIN32_WINNT < _WIN32_WINNT_LONGHORN
    - #define _WIN32_WINNT_bak _WIN32_WINNT
    - #undef _WIN32_WINNT
    - #define _WIN32_WINNT _WIN32_WINNT_LONGHORN
    - #define _WIN32_IE_bak _WIN32_IE
    - #undef _WIN32_IE
    - #define _WIN32_IE _WIN32_IE_IE70
    - #endif
    - #endif
    -

    -#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
    -#if defined(_WIN32_WINNT_bak)
    -#undef _WIN32_WINNT
    -#define _WIN32_WINNT _WIN32_WINNT_bak
    -#undef _WIN32_IE
    -#define _WIN32_IE _WIN32_IE_bak
    -#endif
    -#endif
    -

>>> This change looks wrong.

    Agrees  :-) .......... fixed nsmemoryreportmanager.cpp as:

    #if defined(XP_WIN)
         REGISTER(Private);
    #endif

Comment 8

[Jim Mathies [:jimm]]

> >>> I'm not sure offhand but I think the Windows 7 SDK might default to newer values here too.
> 
>     Fixed up file nsfilepicker.h by removing two blocks:
> 
>     - #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>     - // For Vista IFileDialog interfaces
>     - #if _WIN32_WINNT < _WIN32_WINNT_LONGHORN
>     - #define _WIN32_WINNT_bak _WIN32_WINNT
>     - #undef _WIN32_WINNT
>     - #define _WIN32_WINNT _WIN32_WINNT_LONGHORN
>     - #define _WIN32_IE_bak _WIN32_IE
>     - #undef _WIN32_IE
>     - #define _WIN32_IE _WIN32_IE_IE70
>     - #endif
>     - #endif
>     -
> 
>     -#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>     -#if defined(_WIN32_WINNT_bak)
>     -#undef _WIN32_WINNT
>     -#define _WIN32_WINNT _WIN32_WINNT_bak
>     -#undef _WIN32_IE
>     -#define _WIN32_IE _WIN32_IE_bak
>     -#endif
>     -#endif
>     -

Try run that change - I don't think it will build. But it might!

Comment 9

[Jim Mathies [:jimm]]

Note, my mc mozilla-config.h has

#define _WIN32_WINNT 0x502

http://mxr.mozilla.org/mozilla-central/source/configure.in#968

Comment 10

[Mark Capella [:capella]]

   Built / running with it right now ....

Comment 11

[[e-mail]]

(In reply to Jim Mathies from comment #9)
> Note, my mc mozilla-config.h has
> 
> #define _WIN32_WINNT 0x502
> 
> http://mxr.mozilla.org/mozilla-central/source/configure.in#968

Hmm, so should we be bumping WINVER too?

Comment 12

[Jim Mathies [:jimm]]

(In reply to [e-mail] from comment #11)
> (In reply to Jim Mathies from comment #9)
> > Note, my mc mozilla-config.h has
> > 
> > #define _WIN32_WINNT 0x502
> > 
> > http://mxr.mozilla.org/mozilla-central/source/configure.in#968
> 
> Hmm, so should we be bumping WINVER too?

Don't think so, this current targets server 2003 - 

This targets the Windows 2000 operating system. Other valid values include 0x0501 for Windows XP, 0x0502 for Windows Server 2003, 0x0600 for Windows Vista, and 0x0601 for Windows 7.

http://msdn.microsoft.com/en-us/library/6sehtctf.aspx

Comment 13

[Rain [:rain1] (inactive)]

(In reply to [e-mail] from comment #11)
> Hmm, so should we be bumping WINVER too?

No, WINVER is the oldest version of Windows code's guaranteed to run on. Seems like it was last bumped up (to Win2k3/XP SP2) by http://hg.mozilla.org/mozilla-central/rev/11c34494c1f3.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Mark Capella [:capella] from comment #10)
>    Built / running with it right now ....

Doesn't build locally for me - 

nsFilePicker.cpp
f:\mozilla\firefox\mc\widget\windows\nsFilePicker.h(67) : error C2504: 'IFileDialogEvents' : base class undefined

IFileDialogEvents are only defined if _WIN32_IE >= _WIN32_IE_IE60, and our current config has this set to 0x0500.  _WIN32_IE_IE60 is defined as 0x0600 in sdkddkvers.h.

Maybe we should stop defining this, as the sdkddkvers file sets if appropriately if it's not defined - 

#ifndef _WIN32_IE
#ifdef _WIN32_WINNT
// set _WIN32_IE based on _WIN32_WINNT
#if (_WIN32_WINNT <= _WIN32_WINNT_NT4)
#define _WIN32_IE       _WIN32_IE_IE50
#elif (_WIN32_WINNT <= _WIN32_WINNT_WIN2K)
#define _WIN32_IE       _WIN32_IE_IE501
#elif (_WIN32_WINNT <= _WIN32_WINNT_WINXP)
#define _WIN32_IE       _WIN32_IE_IE60
#elif (_WIN32_WINNT <= _WIN32_WINNT_WS03)
#define _WIN32_IE       0x0602
#else
#define _WIN32_IE       0x0800
#endif
#else
#define _WIN32_IE       0x0800
#endif
#endif

Comment 15

[Rain [:rain1] (inactive)]

Yeah, IE6 or above seems safe to me!

Comment 16

[Jim Mathies [:jimm]]

doing a full clobber build now with these two lines removed - 

http://mxr.mozilla.org/mozilla-central/source/configure.in#969

Comment 17

[Jim Mathies [:jimm]]

So this works, although we will still have to up _WIN32_IE in places like nsFilePicker.h - 

w/o configure setting _WIN32_IE:

_WIN32_IE=0x0602 NTDDI_VERSION=0x5020000 _WIN32_WINNT=0x502

after upping in nsFilePicker.h:

_WIN32_IE=0x0700 NTDDI_VERSION=0x06000000 _WIN32_WINNT=0x0600

We need _WIN32_IE=_WIN32_IE_IE70 to get at those interfaces.

Doesn't look like there's much else to change, we don't do this very often - 

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

Although there are some cases where we can get rid of _WIN32_WINNT upping code - 

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

Comment 18

[[e-mail]]

(In reply to Siddharth Agarwal from comment #13)
> (In reply to comment #11)
> > Hmm, so should we be bumping WINVER too?
> 
> No, WINVER is the oldest version of Windows code's guaranteed to run on.
> Seems like it was last bumped up (to Win2k3/XP SP2) by
> http://hg.mozilla.org/mozilla-central/rev/11c34494c1f3.

Odd, since that was 2009, and we only just dropped W2K...

Comment 19

[Mark Capella [:capella]]

   Ok, dropping back and re-doing a clobber build did fail for me also this time. (Not sure what I missed).

So then, are we asking me to correct by:

    Removing (2) lines from CONFIGURE.IN:

    -    # Require OS features provided by IE 5.0
    -    AC_DEFINE_UNQUOTED(_WIN32_IE,0x0500)

    Allowing nsfilepicker to bump / drop VARS as in my first DIFF:

    -#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
     // For Vista IFileDialog interfaces
     #if _WIN32_WINNT < _WIN32_WINNT_LONGHORN
     #define _WIN32_WINNT_bak _WIN32_WINNT
     #undef _WIN32_WINNT
     #define _WIN32_WINNT _WIN32_WINNT_LONGHORN
     #define _WIN32_IE_bak _WIN32_IE
     #undef _WIN32_IE
     #define _WIN32_IE _WIN32_IE_IE70
     #endif
    -#endif

    -#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
     #if defined(_WIN32_WINNT_bak)
     #undef _WIN32_WINNT
     #define _WIN32_WINNT _WIN32_WINNT_bak
     #undef _WIN32_IE
     #define _WIN32_IE _WIN32_IE_bak
     #endif
    -#endif


Or did I miss something yet? I've got my clobber build running again, so I guess I'll find out ...

Comment 20

[Jim Mathies [:jimm]]

(In reply to Mark Capella [:capella] from comment #19)
> Ok, dropping back and re-doing a clobber build did fail for me also this
> time. (Not sure what I missed).
> 
> So then, are we asking me to correct by:
> 
>     Removing (2) lines from CONFIGURE.IN:
> 
>     -    # Require OS features provided by IE 5.0
>     -    AC_DEFINE_UNQUOTED(_WIN32_IE,0x0500)

Hmm, since this is unrelated, I'm tempted to say lets file this in a separate bug and post this patch there. kheuy or ted should probably get an r? for this.

  
> 
>     Allowing nsfilepicker to bump / drop VARS as in my first DIFF:
> 
>     -#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>      // For Vista IFileDialog interfaces
>      #if _WIN32_WINNT < _WIN32_WINNT_LONGHORN
>      #define _WIN32_WINNT_bak _WIN32_WINNT
>      #undef _WIN32_WINNT
>      #define _WIN32_WINNT _WIN32_WINNT_LONGHORN
>      #define _WIN32_IE_bak _WIN32_IE
>      #undef _WIN32_IE
>      #define _WIN32_IE _WIN32_IE_IE70
>      #endif
>     -#endif
> 
>     -#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>      #if defined(_WIN32_WINNT_bak)
>      #undef _WIN32_WINNT
>      #define _WIN32_WINNT _WIN32_WINNT_bak
>      #undef _WIN32_IE
>      #define _WIN32_IE _WIN32_IE_bak
>      #endif
>     -#endif

yep - that looks right.

Comment 21

[Mark Capella [:capella]]

Attachment: Third Try a charm...

Changes as up through comment #20 ...

   Left configure.in alone as in original DIFF
   Left nsfilepicker.h alone as in original DIFF

Asking :khuey for review, as I thought a saw a comment from :ted in IRQ about soon to be away for a while.

Comment 22

[Jim Mathies [:jimm]]

Note, you can scrap all changes in nsFilePicker from the patches here, I've landed those changes with bug 718374.

Comment 23

[Mark Capella [:capella]]

Attachment: Fourth Try

Removed changes to nsfilepicker ...

Comment 24

[[e-mail]]

Whichever patch it was I was requested on way back when, I did some testing with it, and I concluded that fiddling with _WINNT_WIN32 and/or _WINNT_IE wasn't going to allow the filepicker to compile, but the rest of the changes were OK.

Comment 25

[Mark Capella [:capella]]

   Ok, I guess I'm waiting for a review+ or other help to do a push to the try server.

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 598533 [details] [diff] [review]
Fourth Try

(In reply to [e-mail] from comment #24)
> Whichever patch it was I was requested on way back when, I did some testing
> with it, and I concluded that fiddling with _WINNT_WIN32 and/or _WINNT_IE
> wasn't going to allow the filepicker to compile, but the rest of the changes
> were OK.

So basically, r+ on the patch from you Neil?

Comment 27

[Jim Mathies [:jimm]]

https://hg.mozilla.org/try/rev/4570b9554147

Comment 28

[[e-mail]]

Comment on attachment 598533 [details] [diff] [review]
Fourth Try

>-    AC_DEFINE_UNQUOTED(MOZ_NTDDI_WS03, 0x05020000)
>     AC_DEFINE_UNQUOTED(MOZ_NTDDI_LONGHORN, 0x06000000)
Well, the other patch removed this define too, but r=me either way.

Comment 29

[Mark Capella [:capella]]

hmmm ... thought I saw nsFilePicker::AppendFilter still references MOZ_NTDDI_LONGHORN after :jimm's patch and before mine/this one ...

Comment 30

[Jim Mathies [:jimm]]

(In reply to Mark Capella [:capella] from comment #29)
> hmmm ... thought I saw nsFilePicker::AppendFilter still references
> MOZ_NTDDI_LONGHORN after :jimm's patch and before mine/this one ...

Ugh, you're right, I missed one.

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsFilePicker.cpp#1246

Comment 31

[Mark Capella [:capella]]

   I can fix the nsFilePicker.cpp and re-post a new DIFF ... (yes/do it?)

   I assume comment #27 was a push to try whose results I haven't seen yet ... (yes?)

   Should I be leaving MOZ_NTDDI_LONGHORN defined anyhow, as code search finds other: 73 matching lines in 24 files (yes?)

Comment 32

[Jim Mathies [:jimm]]

(In reply to Mark Capella [:capella] from comment #31)
>    I can fix the nsFilePicker.cpp and re-post a new DIFF ... (yes/do it?)

yes please, sorry I missed that one.

>    I assume comment #27 was a push to try whose results I haven't seen yet
> ... (yes?)

yes - it'll post results here or you can view the results page. Builds look ok thus far.

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=4570b9554147


>    Should I be leaving MOZ_NTDDI_LONGHORN defined anyhow, as code search
> finds other: 73 matching lines in 24 files (yes?)

won't your patch remove all remaining references?

Comment 33

[Mozilla RelEng Bot]

Try run for 4570b9554147 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4570b9554147
Results (out of 47 total builds):
    exception: 1
    success: 39
    warnings: 3
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4570b9554147

Comment 34

[Mark Capella [:capella]]

Attachment: Fifth Patch

Ok, I removed MOZ_NTDDI_LONGHORN from two configure.in files, and updated one missed item in nsFilePicker.cpp. I successfully ran a local build afterwards but am asking for another review? to be safe ... I can do an AUTOLAND to TRY after that to be thorough ...

   Dis-regard my comments about MOZ_NTDDI_LONGHORN. I was looking ahead to outstanding changes that might be left here to remove MOZ_NTDDI_WIN7 as per original comment, and comment # 4, and my brain got full -- mark

Comment 35

[Jim Mathies [:jimm]]

pushed to try:

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=260457f8a8be

Comment 36

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c31d6eac1dc6

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c31d6eac1dc6