Updated to tip and resolved review comments.
> nit - might as well fix up this indentation.
Done.
> "Windows Vista, Windows XP with SP2" - did we drop SP1 support as well?
Yes. VC10 doesn't support WinXP SP1 as well. See also bug 668574.
> Did we up _WIN32_WINNT in our build? I'm surprised this builds with this removed.
Ah, it is a wreckage of my experiment. Eventually, it was not required. Removed.
> Do we really want to remove this? We might want to leave the ability to detect it in our widget utility code.
It was not used at all from the start.
https://mxr.mozilla.org/mozilla-central/search?string=WIN2K_VERSION
If it's absolutely required to test unsupported versions, we can write the code as follows:
WinUtils::GetWindowsVersion() < WinUtils::WINXP_VERSION

Folded two patches.
> Does anyone still use GetThemeDLL()?
GetThemeDLL() is still used by nsWinGesture.
https://hg.mozilla.org/mozilla-central/file/ccbb41b873cd/widget/windows/nsWinGesture.cpp#l100
I couldn't remove those GetProcAddress() calls because gesture functions are Win7 specific.
> Actually IsDirectory calls ResolveAndStat (there is a patch to improve this
> to avoid calling GetFileAttributesEx) so you don't need to call it yourself.
Good catch. Fixed.

(In reply to Masatoshi Kimura [:emk] from comment #7)
> > Does anyone still use GetThemeDLL()?
> GetThemeDLL() is still used by nsWinGesture.
> https://hg.mozilla.org/mozilla-central/file/ccbb41b873cd/widget/windows/
> nsWinGesture.cpp#l100
> I couldn't remove those GetProcAddress() calls because gesture functions are
> Win7 specific.
Plus, the nsUXThemeData GetProcAddress logic is coming back in bug 373266 due to Vista+ theme calls. Win8 also adds a number of theme calls we might be using.

(In reply to Matheus Kerschbaum from comment #0)
> Once bug 563318 lands Firefox will only run on Windows XP SP2 and higher,
> thus support for older versions of Windows can be removed.
There's a chance that that patch will be backed out.
Why should this one land right now?

(In reply to Jim Mathies [:jimm] from comment #20)
> 'try: -b do -p win32 -u none -t chrome,nochrome,paint'
> runs most of the paint test.
No tests were executed at all with this option.
https://tbpl.mozilla.org/?tree=Try&rev=6d2bd0475526
If I use '-t all', chrome.2 test is executed insted of chrome which contains ts_paint.
I propose just land the patch again because:
- ts_paint does no longer run on m-c and m-i either.
- The "regression" is dubious. ts_paint was around 590 on Win7 even before the patch.

(In reply to Masatoshi Kimura [:emk] from comment #23)
> (In reply to Jim Mathies [:jimm] from comment #20)
> > 'try: -b do -p win32 -u none -t chrome,nochrome,paint'
> > runs most of the paint test.
> No tests were executed at all with this option.
> https://tbpl.mozilla.org/?tree=Try&rev=6d2bd0475526
> If I use '-t all', chrome.2 test is executed insted of chrome which contains
> ts_paint.
>
> I propose just land the patch again because:
> - ts_paint does no longer run on m-c and m-i either.
> - The "regression" is dubious. ts_paint was around 590 on Win7 even before
> the patch.
Did you try '-t all'? that worked for me here -
https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=8426ecbadc6f
(The TS,Paint test that regressed is displayed under the Talos chrome tests in the XP OPT build.)
From that run I see a major perf regression -
http://tinyurl.com/7jsoztv
vs. mozilla-central:
http://tinyurl.com/7qwsx3s
You have approval so if you want to reland on mi to get talos test runs again you can, but if the xp opt regression shows up there's no way this will get merged to mc - here's the the inbound regression from the first landing:
http://tinyurl.com/6qje9lt
That doesn't seem very 'dubious'! :)

Comment on attachment 598463[details][diff][review]
rebased to tip
We can't ignore this big of a regression, even if we're not running the tests anymore. bbondy offered to take a look next week, he's been working on perf lately.

> I've submitted a few more patches with various areas removed. Thus far I can
> rule out the following areas:
> 3) netwerk related code
Really? It looks like WinXP ts_paint went down to 480.32 when netwerk changes are reverted.
https://tbpl.mozilla.org/?tree=Try&rev=fc1d6df25072

I've found this code has rather precarious timing issues on startup -
http://mxr.mozilla.org/mozilla-central/source/netwerk/system/win32/nsNotifyAddrListener.cpp#578
CheckLinkStatus gets call when xpcom is starting up on the main thread.
http://mxr.mozilla.org/mozilla-central/source/netwerk/system/win32/nsNotifyAddrListener.cpp#178
nsNotifyAddrListener::Run() executes simultaneously on a background thread.
Before this patch landed, CheckLinkStatus() gets called first (on Win7) prior to InitIPHelperLibrary() in Run(), so CheckAdaptersAddresses(), CheckAdaptersInfo(), and CheckIPAddrTable() all return early due to uninitialized function pointers. We fall through to the default |mLinkUp = true; // I can't tell, so assume there's a link| and exit.
Once the patch was applied, the GetAdaptersAddresses API call in CheckAdaptersAddresses() succeeded in getting called. According to MSDN:
"GetAdaptersAddresses is implemented only as a synchronous function call. The GetAdaptersAddresses function requires a significant amount of network resources and time to complete since all of the low-level network interface tables must be traversed."
There's also a comment in the file about this -
// XXX this call is very expensive (~650 milliseconds), so we
// don't want to call it synchronously. Instead, we just
// start up assuming we have a network link, but we'll
// report that the status isn't known.
I think what we need to do here is bypass any real checks until after Run() is executed, or maybe just set mLinkUp to true on the first invocation of CheckLinkStatus. The second option might be best since Run() could get executed before CheckLinkStatus() on some systems.
c'ing Makoto who recently worked on this code.
Makoto - side note - if called on the main thread, the CoInitializeEx call you added back in bug 712243 will always fail with error 0x80010106 - "Cannot change thread mode after it is set." I think that's a bug since it prevents CheckAdaptersAddresses() from ever succeeding. Probably best to file that as a follow up though once we sort this out.

Cleaned up and better commented patch. I've removed the mStatusKnown check, there's no need for it.
This should also get some dev doc treatment, MDN doesn't say this should be called on a background thread.