Display the proxy PAC javascript errors in the NetLog....

Display the proxy PAC javascript errors in the NetLog.

BUG=47226
TEST=Configure chrome with a PAC script that throws errors. Load a URL and should see the javascript error displayed on about:net-internals.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52253

[eroman, 2010-07-12 00:59:47]

Here is what it looks like:

http://i.imgur.com/MDHw0.png
http://i.imgur.com/RjVAv.png

The first image shows how the error is inlined into a per-request log.

The second image is the same example, but shows how the same error is also
duplicated as a global event (this is for consistency, since not all PAC errors
are "per-request").

[wtc, 2010-07-12 22:20:58]

eroman: I have a big backlog of code reviews, so I'd
appreciate it if you could find someone else to review this
CL, or I can review this next week.

My only comment is that "PAC" should be part of the
event name (which is "PROXY_JAVASCRIPT_ERROR" right now).

[eroman, 2010-07-13 00:44:50]

Chris, could I get you to review this in place of wtc?
Thanks!

[cbentzel, 2010-07-13 02:22:25]

Yes - I owe Wan-Teh and you some reviews for all the ones you've done for
me!

On Mon, Jul 12, 2010 at 8:44 PM, <eroman@chromium.org> wrote:

> Chris, could I get you to review this in place of wtc?
> Thanks!
>
>
> http://codereview.chromium.org/2978001/show
>

[cbentzel, 2010-07-13 20:24:27]

LGTM

Mostly some nits.

http://codereview.chromium.org/2978001/diff/39001/33002
File net/base/forwarding_net_log.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33002#newcode44
net/base/forwarding_net_log.cc:44: scoped_refptr<EventParameters>(params)));
Should AddEntry interface change to take a const scoped_refptr<EventParameters>
instead of passing in a raw EventParameters pointer? There's no guarantee that
the params being passed in is actually being managed via refcounting (even if it
is convention).

http://codereview.chromium.org/2978001/diff/39001/33002#newcode90
net/base/forwarding_net_log.cc:90: CHECK(false) << "Not supported";
Should you remove NextID/HasListener from NetLog interface and make a SyncNetLog
: public NetLog interface which adds those?

http://codereview.chromium.org/2978001/diff/39001/33003
File net/base/forwarding_net_log.h (right):

http://codereview.chromium.org/2978001/diff/39001/33003#newcode22
net/base/forwarding_net_log.h:22: ForwardingNetLog(NetLog* impl, MessageLoop*
loop);
This should document lifetime expectations of impl+loop (and maybe that
ForwardingNetLog doesn't take ownership of |impl|

http://codereview.chromium.org/2978001/diff/39001/33003#newcode35
net/base/forwarding_net_log.h:35: class Core;
I haven't seen too many uses of Pimpl idiom in chrome code - is it discouraged,
or have I just not been exposed to enough code?

http://codereview.chromium.org/2978001/diff/39001/33003#newcode36
net/base/forwarding_net_log.h:36: scoped_refptr<Core> core_;
DISALLOW_COPY_AND_ASSIGN -> with Orphan behavior this is fairly critical.

http://codereview.chromium.org/2978001/diff/39001/33004
File net/base/forwarding_net_log_unittest.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33004#newcode15
net/base/forwarding_net_log_unittest.cc:15: // Test forwarding a call to
AddEntry() to another implementation, operating
Nice tests!

http://codereview.chromium.org/2978001/diff/39001/33007
File net/proxy/multi_threaded_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33007#newcode1
net/proxy/multi_threaded_proxy_resolver_unittest.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2978001/diff/39001/33009
File net/proxy/proxy_resolver_js_bindings.h (right):

http://codereview.chromium.org/2978001/diff/39001/33009#newcode1
net/proxy/proxy_resolver_js_bindings.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2978001/diff/39001/33009#newcode62
net/proxy/proxy_resolver_js_bindings.h:62: //   - Send script error messages to
both LOG(INFO), and the NetLog.
|*net_log|?

http://codereview.chromium.org/2978001/diff/39001/33010
File net/proxy/proxy_resolver_js_bindings_unittest.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33010#newcode294
net/proxy/proxy_resolver_js_bindings_unittest.cc:294:
ProxyResolverJSBindings::CreateDefault(host_resolver, NULL));
You should add a CapturingNetLog for the global NetLog, and make sure errors
show up appropriately.

http://codereview.chromium.org/2978001/diff/39001/33011
File net/proxy/proxy_resolver_perftest.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33011#newcode1
net/proxy/proxy_resolver_perftest.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2978001/diff/39001/33011#newcode202
net/proxy/proxy_resolver_perftest.cc:202: 
Nit: extra newline

http://codereview.chromium.org/2978001/diff/39001/33013
File net/proxy/proxy_resolver_v8_unittest.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33013#newcode1
net/proxy/proxy_resolver_v8_unittest.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2978001/diff/39001/33014
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33014#newcode93
net/proxy/proxy_service.cc:93: thread_safe_net_log_(
Change name to forwarding_net_log_ to be consistent?

[eroman, 2010-07-13 21:23:04]

Thanks for the review Chris!

@wtc:

> My only comment is that "PAC" should be part of the
> event name (which is "PROXY_JAVASCRIPT_ERROR" right now).

Done. Renamed these events to PAC_JAVASCRIPT_*

http://codereview.chromium.org/2978001/diff/39001/33002
File net/base/forwarding_net_log.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33002#newcode44
net/base/forwarding_net_log.cc:44: scoped_refptr<EventParameters>(params)));
On 2010/07/13 20:24:27, cbentzel wrote:
> Should AddEntry interface change to take a const
scoped_refptr<EventParameters>
> instead of passing in a raw EventParameters pointer? There's no guarantee that
> the params being passed in is actually being managed via refcounting (even if
it
> is convention). 


Added a TODO to net_log.h (best not to do it as part of this CL).

http://codereview.chromium.org/2978001/diff/39001/33002#newcode90
net/base/forwarding_net_log.cc:90: CHECK(false) << "Not supported";
On 2010/07/13 20:24:27, cbentzel wrote:
> Should you remove NextID/HasListener from NetLog interface and make a
SyncNetLog
> : public NetLog interface which adds those?

That is a good question.

Ideally AddEntry() would be the only method on this interface, but there are
usages for wanting a HasListener() method. (It is needed so clients can select
what level of log information to emit, and hence avoid doing something expensive
when logging isn't enabled).

If it turns out we need that it in the future, I could do some sort of lazy
cross-thread poll, so HasListener() would just report the last known value (in
other words, it would lag by a bit). I reckon that would be good enough for the
way HasListener() is used.

However the simplest solution would be define NetLog as threadsafe. This would
obviate the need for a ForwardingNetLog altogether. The downside though is it
will require locking (but from practice we know that almost every NetLog happens
from the IO thread anyway). I am very interested in exploring the threadsafety
issue further, but will first need to measure the impact of locking, since I
don't want this feature to slow down normal chrome usage in any measurable way.

For now leaving as is, but added a TODO about exploring thread safety.

http://codereview.chromium.org/2978001/diff/39001/33003
File net/base/forwarding_net_log.h (right):

http://codereview.chromium.org/2978001/diff/39001/33003#newcode22
net/base/forwarding_net_log.h:22: ForwardingNetLog(NetLog* impl, MessageLoop*
loop);
On 2010/07/13 20:24:27, cbentzel wrote:
> This should document lifetime expectations of impl+loop (and maybe that
> ForwardingNetLog doesn't take ownership of |impl|

Done.

Also documented the destructor as cancelling outstanding posttasks.

http://codereview.chromium.org/2978001/diff/39001/33003#newcode35
net/base/forwarding_net_log.h:35: class Core;
On 2010/07/13 20:24:27, cbentzel wrote:
> I haven't seen too many uses of Pimpl idiom in chrome code - is it
discouraged,
> or have I just not been exposed to enough code? 

We use this style in other parts of chrome. The best analogy is URLFetcher,
which similarly uses a reference-counted "Core" object.

Note that I don't do this for just stylistic purposes -- it is actually
necessary since tasks we post to the target thread may end up outliving the
ForwardingNetLog, so it is necessary to have a reference-counted class.

http://codereview.chromium.org/2978001/diff/39001/33003#newcode36
net/base/forwarding_net_log.h:36: scoped_refptr<Core> core_;
On 2010/07/13 20:24:27, cbentzel wrote:
> DISALLOW_COPY_AND_ASSIGN -> with Orphan behavior this is fairly critical.

Done.

http://codereview.chromium.org/2978001/diff/39001/33007
File net/proxy/multi_threaded_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33007#newcode1
net/proxy/multi_threaded_proxy_resolver_unittest.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/07/13 20:24:27, cbentzel wrote:
> Nit: 2010

Done.
(however legal tells me this isnt necessary. i agree it is nicer tho).

http://codereview.chromium.org/2978001/diff/39001/33010
File net/proxy/proxy_resolver_js_bindings_unittest.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33010#newcode294
net/proxy/proxy_resolver_js_bindings_unittest.cc:294:
ProxyResolverJSBindings::CreateDefault(host_resolver, NULL));
On 2010/07/13 20:24:27, cbentzel wrote:
> You should add a CapturingNetLog for the global NetLog, and make sure errors
> show up appropriately.

Good point, Done.

http://codereview.chromium.org/2978001/diff/39001/33011
File net/proxy/proxy_resolver_perftest.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33011#newcode202
net/proxy/proxy_resolver_perftest.cc:202: 
On 2010/07/13 20:24:27, cbentzel wrote:
> Nit: extra newline

lint was complaining this file didn't end in a newline, so i added one here.

http://codereview.chromium.org/2978001/diff/39001/33014
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/2978001/diff/39001/33014#newcode93
net/proxy/proxy_service.cc:93: thread_safe_net_log_(
On 2010/07/13 20:24:27, cbentzel wrote:
> Change name to forwarding_net_log_ to be consistent?

Done.

[cbentzel, 2010-07-14 02:01:04]

LGTM!