net: Add a new target 'net_with_v8'.

net: Add a new target 'net_with_v8'.

This target depends on v8 as the name implies, and thus we can remove the v8
dependency from net library itself.

This will benefit views package, because views does not need anything from v8,
but since it depends on ui_base it needs things from net too.

BUG=137800
R=eroman@chromium.org
TBR=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=154434

[tfarina, 2012-08-26 14:30:27]

Hi Will, could you review this to me?

I'll fix any other deps failures as soon as they arise after trybots report them
to me.

[tfarina, 2012-08-26 21:03:48]

https://chromiumcodereview.appspot.com/10874077/diff/9003/net/net.gyp
File net/net.gyp (left):

https://chromiumcodereview.appspot.com/10874077/diff/9003/net/net.gyp#oldcode605
net/net.gyp:605: 'proxy/proxy_service.cc',
Looks like I'll need to extract the v8 part from this file, and move the v8
implementation into a new file.

[tfarina, 2012-08-26 22:12:48]

This now passed in mac_rel, linux_rel and android.

Looking into fix linux_clang now.

[willchan no longer on Chromium, 2012-08-27 07:40:29]

Redirecting to eroman, since he's the proxy master.

[eroman, 2012-08-27 19:20:33]

overall looks good, but I would like you to remove the helper from ProxyService
and make it a standalone function.

http://codereview.chromium.org/10874077/diff/4012/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/10874077/diff/4012/net/proxy/proxy_service.h#n...
net/proxy/proxy_service.h:42: extern const size_t kDefaultNumPacThreads;
Please make this constant a member of ProxyService.

http://codereview.chromium.org/10874077/diff/4012/net/proxy/proxy_service.h#n...
net/proxy/proxy_service.h:237: static ProxyService* CreateUsingV8ProxyResolver(
Please move this out of proxy_service.h alltogether. (Right now it will be an
unresolved symbol by linker if someone in net tries to use it.)

Perhaps name it:
net::CreateProxyServiceUsingV8ProxyResolver()

There will be a few callers that need to be updated.

[tfarina, 2012-08-28 01:03:25]

Eric, please take another look.

Thanks.

https://chromiumcodereview.appspot.com/10874077/diff/4012/net/proxy/proxy_ser...
File net/proxy/proxy_service.h (right):

https://chromiumcodereview.appspot.com/10874077/diff/4012/net/proxy/proxy_ser...
net/proxy/proxy_service.h:42: extern const size_t kDefaultNumPacThreads;
On 2012/08/27 19:20:33, eroman wrote:
> Please make this constant a member of ProxyService.

Done.

https://chromiumcodereview.appspot.com/10874077/diff/4012/net/proxy/proxy_ser...
net/proxy/proxy_service.h:237: static ProxyService* CreateUsingV8ProxyResolver(
On 2012/08/27 19:20:33, eroman wrote:
> Please move this out of proxy_service.h alltogether. (Right now it will be an
> unresolved symbol by linker if someone in net tries to use it.)
> 
> Perhaps name it:
> net::CreateProxyServiceUsingV8ProxyResolver()
> 
> There will be a few callers that need to be updated.

Done.

[eroman, 2012-08-28 01:37:42]

lgtm

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:29: #include "net/proxy/proxy_resolver_js_bindings.h"
you can delete this too.

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service.h#n...
net/proxy/proxy_service.h:203: // Same as CreateUsingV8ProxyResolver, except it
uses system libraries
Please update this. At the very least update the name, even better would be to
inline comments.

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service_v8.cc
File net/proxy/proxy_service_v8.cc (right):

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service_v8....
net/proxy/proxy_service_v8.cc:51: ProxyResolverJSBindings* js_bindings =
I assume this file is a straight up move, and consequently did not review this
in detail!

Please let me know if there were any edits.

[tfarina, 2012-08-30 02:45:52]

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:29: #include "net/proxy/proxy_resolver_js_bindings.h"
On 2012/08/28 01:37:42, eroman wrote:
> you can delete this too.

Done.

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service.h#n...
net/proxy/proxy_service.h:203: // Same as CreateUsingV8ProxyResolver, except it
uses system libraries
On 2012/08/28 01:37:42, eroman wrote:
> Please update this. At the very least update the name, even better would be to
> inline comments.

Done.

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service_v8.cc
File net/proxy/proxy_service_v8.cc (right):

http://codereview.chromium.org/10874077/diff/6014/net/proxy/proxy_service_v8....
net/proxy/proxy_service_v8.cc:51: ProxyResolverJSBindings* js_bindings =
On 2012/08/28 01:37:42, eroman wrote:
> I assume this file is a straight up move
Yup, just cut and paste.

>, and consequently did not review this
> in detail!
> 
> Please let me know if there were any edits.

[tfarina, 2012-08-30 10:51:41]

Eric, why I'm getting this in linux_clang and linux_chromeos?

out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8WithMockBindings::~ProxyResolverV8WithMockBindings():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_131ProxyResolverV8WithMockBindingsD1Ev+0xb):
error: undefined reference to 'net::ProxyResolverV8::~ProxyResolverV8()'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8WithMockBindings::~ProxyResolverV8WithMockBindings():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_131ProxyResolverV8WithMockBindingsD0Ev+0xf):
error: undefined reference to 'net::ProxyResolverV8::~ProxyResolverV8()'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_NoEntryPoint_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_137ProxyResolverV8Test_NoEntryPoint_Test8TestBodyEv+0xfd):
error: undefined reference to
'net::ProxyResolverV8::ProxyResolverV8(net::ProxyResolverJSBindings*)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_NoEntryPoint_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_137ProxyResolverV8Test_NoEntryPoint_Test8TestBodyEv+0x1bd):
error: undefined reference to 'net::ProxyResolverV8::GetProxyForURL(GURL const&,
net::ProxyInfo*, base::Callback<void ()(int)> const&, void**, net::BoundNetLog
const&)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_NoEntryPoint_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_137ProxyResolverV8Test_NoEntryPoint_Test8TestBodyEv+0x226):
error: undefined reference to 'net::ProxyResolverV8::~ProxyResolverV8()'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_ReturnUnicode_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_138ProxyResolverV8Test_ReturnUnicode_Test8TestBodyEv+0xfd):
error: undefined reference to
'net::ProxyResolverV8::ProxyResolverV8(net::ProxyResolverJSBindings*)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_ReturnUnicode_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_138ProxyResolverV8Test_ReturnUnicode_Test8TestBodyEv+0x1bd):
error: undefined reference to 'net::ProxyResolverV8::GetProxyForURL(GURL const&,
net::ProxyInfo*, base::Callback<void ()(int)> const&, void**, net::BoundNetLog
const&)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_ReturnUnicode_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_138ProxyResolverV8Test_ReturnUnicode_Test8TestBodyEv+0x226):
error: undefined reference to 'net::ProxyResolverV8::~ProxyResolverV8()'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_ReturnEmptyString_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_142ProxyResolverV8Test_ReturnEmptyString_Test8TestBodyEv+0xfd):
error: undefined reference to
'net::ProxyResolverV8::ProxyResolverV8(net::ProxyResolverJSBindings*)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_ReturnEmptyString_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_142ProxyResolverV8Test_ReturnEmptyString_Test8TestBodyEv+0x1bd):
error: undefined reference to 'net::ProxyResolverV8::GetProxyForURL(GURL const&,
net::ProxyInfo*, base::Callback<void ()(int)> const&, void**, net::BoundNetLog
const&)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_Direct_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_131ProxyResolverV8Test_Direct_Test8TestBodyEv+0x112):
error: undefined reference to
'net::ProxyResolverV8::ProxyResolverV8(net::ProxyResolverJSBindings*)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_Direct_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_131ProxyResolverV8Test_Direct_Test8TestBodyEv+0x1e0):
error: undefined reference to 'net::ProxyResolverV8::GetProxyForURL(GURL const&,
net::ProxyInfo*, base::Callback<void ()(int)> const&, void**, net::BoundNetLog
const&)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_NoSetPacScript_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_139ProxyResolverV8Test_NoSetPacScript_Test8TestBodyEv+0x30f):
error: undefined reference to
'net::ProxyResolverV8::SetPacScript(scoped_refptr<net::ProxyResolverScriptData>
const&, base::Callback<void ()(int)> const&)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function net::(anonymous
namespace)::ProxyResolverV8Test_Basic_Test::TestBody():proxy_resolver_v8_unittest.cc(.text._ZN3net12_GLOBAL__N_130ProxyResolverV8Test_Basic_Test8TestBodyEv+0x51d):
error: undefined reference to 'net::ProxyResolverV8::PurgeMemory()'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function vtable for net::(anonymous
namespace)::ProxyResolverV8WithMockBindings:proxy_resolver_v8_unittest.cc(.data.rel.ro._ZTVN3net12_GLOBAL__N_131ProxyResolverV8WithMockBindingsE+0x28):
error: undefined reference to 'net::ProxyResolverV8::CancelRequest(void*)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function vtable for net::(anonymous
namespace)::ProxyResolverV8WithMockBindings:proxy_resolver_v8_unittest.cc(.data.rel.ro._ZTVN3net12_GLOBAL__N_131ProxyResolverV8WithMockBindingsE+0x30):
error: undefined reference to 'net::ProxyResolverV8::GetLoadState(void*) const'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function vtable for net::(anonymous
namespace)::ProxyResolverV8WithMockBindings:proxy_resolver_v8_unittest.cc(.data.rel.ro._ZTVN3net12_GLOBAL__N_131ProxyResolverV8WithMockBindingsE+0x38):
error: undefined reference to
'net::ProxyResolverV8::GetLoadStateThreadSafe(void*) const'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function vtable for net::(anonymous
namespace)::ProxyResolverV8WithMockBindings:proxy_resolver_v8_unittest.cc(.data.rel.ro._ZTVN3net12_GLOBAL__N_131ProxyResolverV8WithMockBindingsE+0x40):
error: undefined reference to 'net::ProxyResolverV8::CancelSetPacScript()'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function vtable for net::(anonymous
namespace)::ProxyResolverV8WithMockBindings:proxy_resolver_v8_unittest.cc(.data.rel.ro._ZTVN3net12_GLOBAL__N_131ProxyResolverV8WithMockBindingsE+0x48):
error: undefined reference to 'net::ProxyResolverV8::PurgeMemory()'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function vtable for net::(anonymous
namespace)::ProxyResolverV8WithMockBindings:proxy_resolver_v8_unittest.cc(.data.rel.ro._ZTVN3net12_GLOBAL__N_131ProxyResolverV8WithMockBindingsE+0x50):
error: undefined reference to
'net::ProxyResolverV8::SetPacScript(scoped_refptr<net::ProxyResolverScriptData>
const&, base::Callback<void ()(int)> const&)'
out/Release/../../third_party/gold/gold64:
out/Release/obj.target/net_unittests/net/proxy/proxy_resolver_v8_unittest.o: in
function vtable for net::(anonymous
namespace)::ProxyResolverV8WithMockBindings:proxy_resolver_v8_unittest.cc(.data.rel.ro._ZTVN3net12_GLOBAL__N_131ProxyResolverV8WithMockBindingsE+0x58):
error: undefined reference to 'net::ProxyResolverV8::Shutdown()'

net_unittests has net and net_with_v8 listed in its dependencies. and
proxy_resolver_v8.cc has been compiled as well.

[eroman, 2012-08-30 16:38:17]

I don't know. One thing you could try is rather than _adding_ net_with_v8,
replace the net targets with "net_with_v8". I don't know if that will help
things, but I think it is cleaner anyway (since net_with_v8 already has net as
dependency).

Beyond that, it could be an issue with build system that requires clobber. Are
you able to reproduce the issue locally?

[tfarina, 2012-08-31 14:50:40]

On 2012/08/30 16:38:17, eroman wrote:
> I don't know. One thing you could try is rather than _adding_ net_with_v8,
> replace the net targets with "net_with_v8". I don't know if that will help
> things, but I think it is cleaner anyway (since net_with_v8 already has net as
> dependency).
> 
> Beyond that, it could be an issue with build system that requires clobber. Are
> you able to reproduce the issue locally?

After three fixes:
- Adding NET_IMPLEMENTATION to gyp so NET_EXPORT works
- Adding NET_EXPORT To CreateProxyServiceUsingV8ProxyResolver
- Initializing kDefaultNumPacThreads in the header file (this was to really piss
of the win trybot)

Fixed win, linux_clang and linux_chromeos trybots.

Landing...

[stuartmorgan, 2012-09-03 11:13:40]

Ironically (since the bug mentions this being a win for iOS too), this broke the
iOS build completely because it made a bunch of new targets compile v8 code
(we'll have a CL to fix it shortly).

We're hoping to have the machines provisioned to get iOS into the CQ very soon,
but if there's any other refactoring related to this bug before that happens, it
would be great if you could run them through the iOS try bots (which should be
almost instantaneous).