Remove Windows Vista/XP specific code from net/base

Remove Windows Vista/XP specific code from net/base.
Files affected: net_interfaces_win + unittests.
+ Fixed build failure after code dynamic linking
of functions

BUG=580691
Committed: https://crrev.com/c69619d715a66dd0fc0c5d795e93236c6ef66577
Cr-Commit-Position: refs/heads/master@{#395134}

[maksims (do not use this acc), 2016-05-17 10:42:36]

Description was changed from

==========
remove windows xp support

BUG=
==========

to

==========
Remove Windows Vista/XP specific code from net/base.

BUG=580691
==========

[maksims (do not use this acc), 2016-05-17 10:42:36]

maksim.sisov@intel.com changed reviewers:
+ mef@chromium.org

[maksims (do not use this acc), 2016-05-17 10:42:45]

ptal

[maksims (do not use this acc), 2016-05-17 10:52:45]

maksim.sisov@intel.com changed reviewers:
+ mmenke@chromium.org

[mmenke, 2016-05-17 11:25:23]

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
File net/base/network_interfaces_unittest.cc (right):

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
net/base/network_interfaces_unittest.cc:39: #include
"base/win/windows_version.h"
No longer needed.

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
net/base/network_interfaces_unittest.cc:156: if (interface_to_luid &&
luid_to_guid) {
I think these should both be EXPECT_TRUE, now?

Actually, can we just include <Iphlpapi.h>, and statically link them?  Net's
already linked against iphlpapi.lib.

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
File net/base/network_interfaces_win.cc (right):

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
net/base/network_interfaces_win.cc:18: #include "base/win/windows_version.h"
This header is no longer needed.

[maksims (do not use this acc), 2016-05-17 11:47:48]

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
File net/base/network_interfaces_unittest.cc (right):

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
net/base/network_interfaces_unittest.cc:156: if (interface_to_luid &&
luid_to_guid) {
On 2016/05/17 11:25:23, mmenke wrote:
> I think these should both be EXPECT_TRUE, now?
> 
> Actually, can we just include <Iphlpapi.h>, and statically link them?  Net's
> already linked against iphlpapi.lib.

Did not get the part about static linking. What do you exactly mean?

[mattmenke_gmail.com, 2016-05-17 12:34:38]

mattmenke@gmail.com changed reviewers:
+ mattmenke@gmail.com

[mattmenke_gmail.com, 2016-05-17 12:34:39]

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
File net/base/network_interfaces_unittest.cc (right):

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
net/base/network_interfaces_unittest.cc:154:
phlpapi_lib.GetFunctionPointer("ConvertInterfaceLuidToGuid"));
If you look at these, right now, we're trying to load these methods from a dll
at runtime, rather than when the test binary is loaded.  That's because these
methods don't exist on XP.  Now that we're not using them on XP, we no longer
need to do this.  So can remove this code, and use the methods directly. 
THey're declared in iphlpapi.h.  We're already linking to the requisite lib
file, so nothing else should be needed, I believe.

[mmenke, 2016-05-17 18:57:49]

On 2016/05/17 12:34:39, mattmenke wrote:
>
https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
> File net/base/network_interfaces_unittest.cc (right):
> 
>
https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
> net/base/network_interfaces_unittest.cc:154:
> phlpapi_lib.GetFunctionPointer("ConvertInterfaceLuidToGuid"));
> If you look at these, right now, we're trying to load these methods from a dll
> at runtime, rather than when the test binary is loaded.  That's because these
> methods don't exist on XP.  Now that we're not using them on XP, we no longer
> need to do this.  So can remove this code, and use the methods directly. 
> THey're declared in iphlpapi.h.  We're already linking to the requisite lib
> file, so nothing else should be needed, I believe.

Sorry, that should be "Now that we're not running the code under XP, we no
longer need to do this."

[mmenke, 2016-05-18 04:29:14]

mmenke@chromium.org changed reviewers:
- mattmenke@gmail.com

[maksims (do not use this acc), 2016-05-18 09:12:06]

Is it correct now? I do not have windows environment at the moment and cannot
test if it runs.

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
File net/base/network_interfaces_unittest.cc (right):

https://codereview.chromium.org/1984133003/diff/1/net/base/network_interfaces...
net/base/network_interfaces_unittest.cc:39: #include
"base/win/windows_version.h"
On 2016/05/17 11:25:23, mmenke wrote:
> No longer needed.

Done.

[mmenke, 2016-05-18 17:54:03]

Just two more things.  I've verified that the test compiles and runs with the
suggested changes.

https://codereview.chromium.org/1984133003/diff/20001/net/base/network_interf...
File net/base/network_interfaces_unittest.cc (right):

https://codereview.chromium.org/1984133003/diff/20001/net/base/network_interf...
net/base/network_interfaces_unittest.cc:143: GUID*);
Actually, you need to remove these two typedefs as well.

Basically what was happening is that Windows Vista and later have
ConvertInterfaceIndexToLuid and ConvertInterfaceLuidToGuid methods.  Windows XP
does not.  So to use the methods in tests, we had to create typedefs for
function pointers to methods with their prototyes, and create pointers of those
types, and try to load those functions (And their pointers) from a DLL while the
test was running.  If they failed to load, we verified we were on XP.

Now that we're not supporting XP, we can get rid of the typedefs and loading the
functions.

One thing that made this confusing is that "ConvertInterfaceIndexToLuid" is the
name of the function, but here we're using the same string for the typedef of a
pointer to the real function, so the typedef overrides the global
"ConvertInterfaceIndexToLuid" function, which is what we really want to use.

Hope you can follow this explanation - DLL loading and function pointers are a
bit weird.

https://codereview.chromium.org/1984133003/diff/20001/net/base/network_interf...
net/base/network_interfaces_unittest.cc:477: &results,
INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES, true, &adapter_address));
All 7 of the "internal::GetNetworkListImpl" calls in this test should have their
3rd parameter (the "true") removed, since you're removing the "is_xp" argument. 
No other test needs to be updated (Other than GetNetworkList, of course)

[maksims (do not use this acc), 2016-05-19 05:58:31]

https://codereview.chromium.org/1984133003/diff/20001/net/base/network_interf...
File net/base/network_interfaces_unittest.cc (right):

https://codereview.chromium.org/1984133003/diff/20001/net/base/network_interf...
net/base/network_interfaces_unittest.cc:143: GUID*);
On 2016/05/18 17:54:03, mmenke wrote:
> Actually, you need to remove these two typedefs as well.
> 
> Basically what was happening is that Windows Vista and later have
> ConvertInterfaceIndexToLuid and ConvertInterfaceLuidToGuid methods.  Windows
XP
> does not.  So to use the methods in tests, we had to create typedefs for
> function pointers to methods with their prototyes, and create pointers of
those
> types, and try to load those functions (And their pointers) from a DLL while
the
> test was running.  If they failed to load, we verified we were on XP.
> 
> Now that we're not supporting XP, we can get rid of the typedefs and loading
the
> functions.
> 
> One thing that made this confusing is that "ConvertInterfaceIndexToLuid" is
the
> name of the function, but here we're using the same string for the typedef of
a
> pointer to the real function, so the typedef overrides the global
> "ConvertInterfaceIndexToLuid" function, which is what we really want to use.
> 
> Hope you can follow this explanation - DLL loading and function pointers are a
> bit weird.

Done. Thank you for explanations!

https://codereview.chromium.org/1984133003/diff/20001/net/base/network_interf...
net/base/network_interfaces_unittest.cc:477: &results,
INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES, true, &adapter_address));
On 2016/05/18 17:54:03, mmenke wrote:
> All 7 of the "internal::GetNetworkListImpl" calls in this test should have
their
> 3rd parameter (the "true") removed, since you're removing the "is_xp"
argument. 
> No other test needs to be updated (Other than GetNetworkList, of course)

Done.

[mmenke, 2016-05-19 15:02:37]

LGTM!

https://codereview.chromium.org/1984133003/diff/40001/net/base/network_interf...
File net/base/network_interfaces_unittest.cc (right):

https://codereview.chromium.org/1984133003/diff/40001/net/base/network_interf...
net/base/network_interfaces_unittest.cc:153: }
Hrm...This code wasn't being run on non-XP systems before, because of the
continue line that you removed.  Since this code wasn't in the XP branch, I
strongly suspect that was a bug, and removing the continue line fixes it.  Just
adding this comment to point out the change in behavior (I do think the CL,
as-is, is exactly what we should land).

[mmenke, 2016-05-19 15:02:42]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 15:03:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984133003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984133003/40001

[commit-bot: I haz the power, 2016-05-19 16:43:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-19 16:43:20]

Dry run: Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[mmenke, 2016-05-19 18:41:01]

On 2016/05/19 16:43:20, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   win_clang on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

I haven't had a chance to figure out how to replicate this build locally, but if
you look at net/BUILD.gn, you'll see a section for the net component:

  if (is_win) {
    libs = [
      "crypt32.lib",
      "dhcpcsvc.lib",
      "iphlpapi.lib",
      "rpcrt4.lib",
      "secur32.lib",
      "urlmon.lib",
      "winhttp.lib",
    ]
  }

There's no matching section for net_unittests, though.  I suspect you need to
add:

  if (is_win) {
    libs = [
      "iphlpapi.lib",
    ]
  }

to the net_unittests project.  That's where the methods that we're now linking
at load time instead of run time should be.

[maksims (do not use this acc), 2016-05-20 06:26:18]

Description was changed from

==========
Remove Windows Vista/XP specific code from net/base.

BUG=580691
==========

to

==========
Remove Windows Vista/XP specific code from net/base.
Files affected: net_interfaces_win + unittests.
+ Fixed build failure after code dynamic linking
of functions

BUG=580691
==========

[maksims (do not use this acc), 2016-05-20 07:30:06]

It's working now? Can I land the patch?

[maksims (do not use this acc), 2016-05-20 07:30:39]

On 2016/05/20 07:30:06, maksims wrote:
> It's working now? Can I land the patch?

Typo. . (dot) instead of ? in the first sentence

[mmenke, 2016-05-20 15:34:41]

On 2016/05/20 07:30:39, maksims wrote:
> On 2016/05/20 07:30:06, maksims wrote:
> > It's working now? Can I land the patch?
> 
> Typo. . (dot) instead of ? in the first sentence

Yes, this LGTM.  Feel free to land whenever you want.

[maksims (do not use this acc), 2016-05-20 19:19:56]

The CQ bit was checked by maksim.sisov@intel.com

[commit-bot: I haz the power, 2016-05-20 19:20:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984133003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984133003/60001

[commit-bot: I haz the power, 2016-05-20 19:24:05]

Description was changed from

==========
Remove Windows Vista/XP specific code from net/base.
Files affected: net_interfaces_win + unittests.
+ Fixed build failure after code dynamic linking
of functions

BUG=580691
==========

to

==========
Remove Windows Vista/XP specific code from net/base.
Files affected: net_interfaces_win + unittests.
+ Fixed build failure after code dynamic linking
of functions

BUG=580691
==========

[commit-bot: I haz the power, 2016-05-20 19:24:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-20 19:25:42]

Description was changed from

==========
Remove Windows Vista/XP specific code from net/base.
Files affected: net_interfaces_win + unittests.
+ Fixed build failure after code dynamic linking
of functions

BUG=580691
==========

to

==========
Remove Windows Vista/XP specific code from net/base.
Files affected: net_interfaces_win + unittests.
+ Fixed build failure after code dynamic linking
of functions

BUG=580691

Committed: https://crrev.com/c69619d715a66dd0fc0c5d795e93236c6ef66577
Cr-Commit-Position: refs/heads/master@{#395134}
==========

[commit-bot: I haz the power, 2016-05-20 19:25:43]

Patchset 4 (id:??) landed as
https://crrev.com/c69619d715a66dd0fc0c5d795e93236c6ef66577
Cr-Commit-Position: refs/heads/master@{#395134}