Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
468083004:
Use NESTABLE_ASYNC APIs to get NetLog data into Tracing (Closed)
|
Created:
6 years, 4 months ago by xunjieli Modified:
6 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, wfh+watch_chromium.org, dsinclair+watch_chromium.org, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionThis CL adds a NetLog observer, and uses the newly added
NESTABLE_ASYNC APIs
(https://codereview.chromium.org/536503002/) to log NetLog
entries in Tracing. This CL is a part of the effort to get
NetLog data into Tracing.
Design Doc:
https://docs.google.com/document/d/1Z2uqj59UEts5IiXX78mkdU4kd6e7kE3JUKPoDK97bVs/edit?usp=sharing
BUG=399701
Committed: https://crrev.com/eb290602c4dbfee90191c3044e68e53a0db4c3e3
Cr-Commit-Position: refs/heads/master@{#295089}
Patch Set 1 #
Total comments: 45
Patch Set 2 : Addressed Matt's Comments #Patch Set 3 : Added Unit Tests #
Total comments: 90
Patch Set 4 : Addressed Comments #Patch Set 5 : Added a missing header #
Total comments: 16
Patch Set 6 : Addressed New Comments #
Total comments: 34
Patch Set 7 : Added comments and cleaned up tests #
Total comments: 4
Patch Set 8 : Separated trace changes #
Total comments: 25
Patch Set 9 : Addressed Comments #Patch Set 10 : Filtered trace events in unittests #
Total comments: 22
Patch Set 11 : Removed SetCurrentThreadBlocksMessageLoop and addressed comments #
Total comments: 6
Patch Set 12 : Addressed comments #Patch Set 13 : Rebased (tracing changes committed in two other CLs) #Patch Set 14 : Removed TraceLog::DeleteForTesting() from unit test #
Messages
Total messages: 48 (6 generated)
Hi Nat and Matt, This is just a CL to get things set-up. Could you take a look? Thanks! Helen https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... File net/base/trace_net_log_observer.cc (right): https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:33: virtual ~TracedValue() {} Not very familiar with ref counted pointers, should I do anything in the destructor?
https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... File net/base/trace_net_log_observer.cc (right): https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:67: "netlog", NetLog::EventTypeToString(entry.type()), entry.source().id, Nat, if I understand correctly, the tracing category here and below will now be "netlog,disabled-by-default-netlog.strip-private-data", right? If so, I will update it in the next patch set.
Just nits, for the most part. I think we're going to want some simple unit tests, too. https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_net_log.cc (right): https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_net_log.cc:75: } nit: Don't use braces on if's when the conditional and the body are both one line, and there's no else clause. https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_net_log.h (right): https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_net_log.h:37: DISALLOW_COPY_AND_ASSIGN(ChromeNetLog); nit: Blank line before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... File net/base/trace_net_log_observer.cc (right): https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:14: nit: Remove extra blank line. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:22: protected: nit: Blank line before protected (And private, same goes for all your files) https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:33: virtual ~TracedValue() {} On 2014/08/15 20:01:18, xunjieli1 wrote: > Not very familiar with ref counted pointers, should I do anything in the > destructor? Don't think so, just make it protected, as you're doing. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:33: virtual ~TracedValue() {} nit: Destructor and constructor should go before other methods, given their scope. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:37: } // namespace nit: Blank line before closing the namespace. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:37: } // namespace nit: Two spaces between comment and code. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:47: DCHECK(!net_log()); This behavior should be mentioned in the header file. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:60: if (log_level_ == NetLog::LOG_NONE) { not needed. The NetLog is responsible for dealing with this. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:83: default: NOTREACHED()? https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:95: // We should only stop if we are currently watching. nit: Don't use we in comment, because it's ambiguous. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:97: } This should just be DCHECK(net_log_) https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:99: net_log()->RemoveThreadSafeObserver(this); net_log() vs net_log_ can be confusing, maybe rename net_log_? (net_log_to_watch_ or something? Open to other ideas) https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:99: net_log()->RemoveThreadSafeObserver(this); This crashes if we're not watching a net_log. Should be: if (net_log()) net_log()->... https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:8: #include <stdio.h> not used. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:11: #include "base/files/scoped_file.h" Not used. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:16: class NET_EXPORT TraceNetLogObserver : public NetLog::ThreadSafeObserver, Should include net/base/net_export.h https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:17: public base::debug::TraceLog::EnabledStateObserver { All parent classes should line up, so this should be: class NET_EXPORT TraceNetLogObserver : public NetLog::ThreadSafeObserver, public base::debug::TraceLog::EnabledStateObserver { https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:22: void set_log_level(NetLog::LogLevel log_level); Only use this naming scheme when functions are inlined in the header. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:36: // base::debug::TraceLog::EnabledStateChangedObserver overrides: nit: overrides -> implementation, to be consistent with the similar line above. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:45: DISALLOW_COPY_AND_ASSIGN(TraceNetLogObserver); Should include base/macros.h for this and OVERRIDE
Also, I'm happy to offer some guidance on unit tests, I'll need to look over the tracing code a bit first, though. On 2014/08/15 20:42:54, mmenke wrote: > Just nits, for the most part. > > I think we're going to want some simple unit tests, too. > > https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... > File chrome/browser/net/chrome_net_log.cc (right): > > https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... > chrome/browser/net/chrome_net_log.cc:75: } > nit: Don't use braces on if's when the conditional and the body are both one > line, and there's no else clause. > > https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... > File chrome/browser/net/chrome_net_log.h (right): > > https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... > chrome/browser/net/chrome_net_log.h:37: DISALLOW_COPY_AND_ASSIGN(ChromeNetLog); > nit: Blank line before DISALLOW_COPY_AND_ASSIGN > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > File net/base/trace_net_log_observer.cc (right): > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:14: > nit: Remove extra blank line. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:22: protected: > nit: Blank line before protected (And private, same goes for all your files) > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:33: virtual ~TracedValue() {} > On 2014/08/15 20:01:18, xunjieli1 wrote: > > Not very familiar with ref counted pointers, should I do anything in the > > destructor? > > Don't think so, just make it protected, as you're doing. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:33: virtual ~TracedValue() {} > nit: Destructor and constructor should go before other methods, given their > scope. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:37: } // namespace > nit: Blank line before closing the namespace. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:37: } // namespace > nit: Two spaces between comment and code. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:47: DCHECK(!net_log()); > This behavior should be mentioned in the header file. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:60: if (log_level_ == NetLog::LOG_NONE) { > not needed. The NetLog is responsible for dealing with this. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:83: default: > NOTREACHED()? > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:95: // We should only stop if we are > currently watching. > nit: Don't use we in comment, because it's ambiguous. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:97: } > This should just be DCHECK(net_log_) > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:99: > net_log()->RemoveThreadSafeObserver(this); > net_log() vs net_log_ can be confusing, maybe rename net_log_? > (net_log_to_watch_ or something? Open to other ideas) > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.cc:99: > net_log()->RemoveThreadSafeObserver(this); > This crashes if we're not watching a net_log. Should be: > if (net_log()) > net_log()->... > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > File net/base/trace_net_log_observer.h (right): > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.h:8: #include <stdio.h> > not used. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.h:11: #include "base/files/scoped_file.h" > Not used. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.h:16: class NET_EXPORT TraceNetLogObserver : > public NetLog::ThreadSafeObserver, > Should include net/base/net_export.h > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.h:17: public > base::debug::TraceLog::EnabledStateObserver { > All parent classes should line up, so this should be: > > class NET_EXPORT TraceNetLogObserver > : public NetLog::ThreadSafeObserver, > public base::debug::TraceLog::EnabledStateObserver { > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.h:22: void set_log_level(NetLog::LogLevel > log_level); > Only use this naming scheme when functions are inlined in the header. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.h:36: // > base::debug::TraceLog::EnabledStateChangedObserver overrides: > nit: overrides -> implementation, to be consistent with the similar line > above. > > https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... > net/base/trace_net_log_observer.h:45: > DISALLOW_COPY_AND_ASSIGN(TraceNetLogObserver); > Should include base/macros.h for this and OVERRIDE
Thanks Matt for the initial review! I incorporated the suggestions. Will work on the unit tests on Monday. https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_net_log.cc (right): https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_net_log.cc:75: } On 2014/08/15 20:42:53, mmenke wrote: > nit: Don't use braces on if's when the conditional and the body are both one > line, and there's no else clause. Done. Thanks! https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_net_log.h (right): https://codereview.chromium.org/468083004/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_net_log.h:37: DISALLOW_COPY_AND_ASSIGN(ChromeNetLog); On 2014/08/15 20:42:53, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... File net/base/trace_net_log_observer.cc (right): https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:14: On 2014/08/15 20:42:54, mmenke wrote: > nit: Remove extra blank line. Done. Oops. Thanks! https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:22: protected: On 2014/08/15 20:42:53, mmenke wrote: > nit: Blank line before protected (And private, same goes for all your files) Done for all my files. Thanks! https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:33: virtual ~TracedValue() {} On 2014/08/15 20:42:53, mmenke wrote: > nit: Destructor and constructor should go before other methods, given their > scope. Done. Thanks! https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:37: } // namespace On 2014/08/15 20:42:53, mmenke wrote: > nit: Two spaces between comment and code. Done. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:47: DCHECK(!net_log()); On 2014/08/15 20:42:53, mmenke wrote: > This behavior should be mentioned in the header file. Acknowledged. I have removed this setter since it is not used. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:60: if (log_level_ == NetLog::LOG_NONE) { On 2014/08/15 20:42:53, mmenke wrote: > not needed. The NetLog is responsible for dealing with this. Done. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:83: default: On 2014/08/15 20:42:53, mmenke wrote: > NOTREACHED()? Done. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:95: // We should only stop if we are currently watching. On 2014/08/15 20:42:53, mmenke wrote: > nit: Don't use we in comment, because it's ambiguous. Done. Thanks! https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:97: } On 2014/08/15 20:42:53, mmenke wrote: > This should just be DCHECK(net_log_) Done. I see. That's convenient. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.cc:99: net_log()->RemoveThreadSafeObserver(this); On 2014/08/15 20:42:53, mmenke wrote: > This crashes if we're not watching a net_log. Should be: > if (net_log()) > net_log()->... Done.Yes, that's indeed very confusing. Thanks for pointing that out! https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:8: #include <stdio.h> On 2014/08/15 20:42:54, mmenke wrote: > not used. Done. Sorry! I copied over another file. Should totally have checked this before I uploaded the CL. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:11: #include "base/files/scoped_file.h" On 2014/08/15 20:42:54, mmenke wrote: > Not used. Done. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:16: class NET_EXPORT TraceNetLogObserver : public NetLog::ThreadSafeObserver, On 2014/08/15 20:42:54, mmenke wrote: > Should include net/base/net_export.h Done. I see! Thanks! https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:17: public base::debug::TraceLog::EnabledStateObserver { On 2014/08/15 20:42:54, mmenke wrote: > All parent classes should line up, so this should be: > > class NET_EXPORT TraceNetLogObserver > : public NetLog::ThreadSafeObserver, > public base::debug::TraceLog::EnabledStateObserver { Done. Thanks! I will keep that in mind next time. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:22: void set_log_level(NetLog::LogLevel log_level); On 2014/08/15 20:42:54, mmenke wrote: > Only use this naming scheme when functions are inlined in the header. Acknowledged. Also, I don't think we need the setter for now, so I will just remove it. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:36: // base::debug::TraceLog::EnabledStateChangedObserver overrides: On 2014/08/15 20:42:54, mmenke wrote: > nit: overrides -> implementation, to be consistent with the similar line > above. Done. https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:45: DISALLOW_COPY_AND_ASSIGN(TraceNetLogObserver); On 2014/08/15 20:42:54, mmenke wrote: > Should include base/macros.h for this and OVERRIDE Partially done. How should I "OVERRIDE" on this?
https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:45: DISALLOW_COPY_AND_ASSIGN(TraceNetLogObserver); On 2014/08/15 21:55:16, xunjieli1 wrote: > On 2014/08/15 20:42:54, mmenke wrote: > > Should include base/macros.h for this and OVERRIDE > > Partially done. How should I "OVERRIDE" on this? Sorry, I meant macros.h makes sure the "OVERRIDE" macro used on lines 30, 37, and 38 is included, in addition to DISALLOW_COPY_AND_ASSIGN. :)
What I'm thinking of for unit tests: Set up a NetLog and a TraceNetLogObserver, and something to watch for tracing events, then check the following: * When not tracing, and you send events to the NetLog directly (No need to make real network requests), no trace events are logged. * When tracing has started, trace events are logged. * A test that tracing can be enabled, disabled, and enabled again, and trace events are logged or not in all 3 states, as appropriate. * The TraceNetLogObserver can be destroyed while tracing, and while not tracing, safely (Also send some events to the NetLog after destruction, to make sure everything's been torn down). * If we choose to support it: The TraceNetLogObserver works when tracing is started before the observer is created. If we don't supported it, we should DCHECK in this case (I'm thinking it's best to handle this case, if it's not too difficult - it may be we already get a trace start event when this happens, and everything just magically works fine, not sure). Ideally we'll send real trace start/trace stop events. We may not be able to monitor for the lack of trace events after a real trace stop event, if the mere process of watching for trace events triggers the OnTraceLogEnabled function. If this is the case, we may want to directly call trace start / trace stop methods for some tests.
Should also test events with and without params, and make sure we produce valid JSON output, with the correct parameters.
On 2014/08/18 14:57:00, mmenke wrote: > Should also test events with and without params, and make sure we produce valid > JSON output, with the correct parameters. And one final spam... Feel free to add things I left out - unit tests should generally be enough to convince you the publicly exposed API works as it should, on all inputs.
Hi Matt, I wrote some simple unit tests (one test for each scenario you mentioned). I have three questions (as comments). Could you take a look? Thanks so much! https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/1/net/base/trace_net_log_obser... net/base/trace_net_log_observer.h:45: DISALLOW_COPY_AND_ASSIGN(TraceNetLogObserver); I see. That makes sense. Thank you! On 2014/08/15 21:59:15, mmenke wrote: > On 2014/08/15 21:55:16, xunjieli1 wrote: > > On 2014/08/15 20:42:54, mmenke wrote: > > > Should include base/macros.h for this and OVERRIDE > > > > Partially done. How should I "OVERRIDE" on this? > > Sorry, I meant macros.h makes sure the "OVERRIDE" macro used on lines 30, 37, > and 38 is included, in addition to DISALLOW_COPY_AND_ASSIGN. :) https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:17: using namespace base; I think this "using namespace" directive is not recommended. How can I avoid doing "base::debug::" all the time? https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:206: trace_net_log_observer_.reset(NULL); Matt, if I just reset the observer scope_ptr, the program will crash, unless I StopWatchForTraceStart() first. Can this be considered "destroy safely"? https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:253: trace_net_log_observer_.reset(new TraceNetLogObserver()); If we start the observer after Tracing is enabled, this does not trigger OnTraceLogEnabled() in the observer. Line 271's EXPECT_TRUE will fail. You mentioned that in this case, we should try to fix the behavior. I looked into this, but I am not sure how to fix it. Do you have any suggestion?
Mostly just a lot of nits, and a couple test suggestions. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... File net/base/trace_net_log_observer.cc (right): https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:9: #include "base/debug/trace_event.h" nit: Probably safe to leave this out, since header includes trace_event_impl. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:22: protected: Suggest either making these private or public - protected means subclasses can't call them, but since they're overriding public functions in the parent class, the can still be called when using an instance as a member of the parent class. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:37: } // namespace nit: Blank line above end of namespace. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:40: log_level_(NetLog::LOG_STRIP_PRIVATE_DATA) { Initialize net_log_to_watch_ to NULL. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:43: TraceNetLogObserver::~TraceNetLogObserver() { Maybe: DCHECK(!net_log_to_watch_); DCHECK(!net_log()); I suggest a change below that makes the first one always the case. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:57: case(NetLog::PHASE_BEGIN): nit: case NetLog::PHASE_BEGIN: (Same goes for the other cases as well) https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:58: TRACE_EVENT_NET_LOG_BEGIN( If this is only for NetLog events, then the macro can include "netlog" and "value" baked into it, unless there's a reason not to do so. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:61: new TracedValue(value.Pass()))); Optional: This is a bit redundant. Value includes phase, event type, and source id. The other things it contains are time (Which tracing already has), source type, and parameters. To reduce redundancy (And size of the JSON data), may make more sense to make source type arg1, and make the entry.ParametersToValue() arg2. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:76: NOTREACHED(); Get rid of default path. Clang is configured to emit a compiler failure when a switch statement uses an enum, there's no default branch, and some values of the enum don't have a case. So without the default, we get a compile failure, instead of a debug-build-only runtime check. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:91: } net_log_to_watch_ = NULL? https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.h:13: nit: Remove above blank line. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.h:18: public base::debug::TraceLog::EnabledStateObserver { nit: Above two lines should be indented two more. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.h:26: void StopObserving(); Neither of these two functions need to be public. Actually, could just get rid of them, since they're only one line. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.h:41: NetLog::LogLevel log_level_; Let's just get rid of this, for now, until we need. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:8: nit: Remove blank line. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:11: #include "testing/gtest/include/gtest/gtest.h" Should include NetLog and TraceLog headers (They're redundant with trace_net_log_observer.h, but we tend to be fairly cautious with unit test files). https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:17: using namespace base; On 2014/08/20 22:00:43, xunjieli1 wrote: > I think this "using namespace" directive is not recommended. How can I avoid > doing "base::debug::" all the time? It's not too common in net/, but you can use individual classes (e.g. "using base::debug::TraceLog"), just can't use entire namespaces. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:23: SetBaseLogLevel(LOG_ALL); This class shouldn't be needed - the NetLog should automatically adjust its level when the observer is attached. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:31: const scoped_refptr<RefCountedString>& events_str, Should include base/memory/ref_counted.h, and ref_counted_memory.h. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:32: bool has_more_events); nit: Should inline all functions, to be consistent. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:33: void Clear() { nit: Blank line before clear. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:56: virtual void SetUp() OVERRIDE { nit: Suggest putting SetUp and TearDown first. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:69: if (debug::TraceLog::GetInstance()) Is this conditional needed? Looks like it always exists on shutdown, currently. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:74: ListValue trace_parsed_; These should be private, with accessors as needed. Google style guide technically does not allow public variables in classes, even in tests (Though a lot of tests violate that rule). https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:78: scoped_ptr<TraceNetLogObserver> trace_net_log_observer_; Should include the scoped_ptr header. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:85: json_output_.json_output.clear(); DCHECK(trace_parsed_.empty());? (DCHECK instead of assert because DCHECKs in tests are sometiems indicate a bug in the test / test fixture, as opposed to a bug in the code being tested) Also should include base/logging.h for DCHECK. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:90: scoped_ptr<Value> root; trace_value, maybe? https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:96: } This can be replaced with: ASSERT_TRUE(root) << json_output_.json_output; (And get rid of the ASSERT_TRUE(root.get()) below) https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:102: while (root_list->GetSize()) { Rather than copying, this can be replaced with: trace_parsed_.swap(root_list); https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:102: while (root_list->GetSize()) { suggest renaming root_list to trace_events and trace_parsed_ to trace_events_ (Or use trace_event_list for both), which I think are a little clearer. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:114: std::string value_str; Should include <string> https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:124: return IsStringInDict(string_to_match, args_dict); This seems much too broad. Surely we know just where the object is we're looking for? If it's a key, a value, or in "args" in some specific place. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:130: const ListValue& trace_parsed, Since this is a member of the class, don't need to take it as an argument. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:131: const char* string_to_match) { Maybe call this event_type, and just take raw NetLog event types, and convert them to strings in this function as well? https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:139: const DictionaryValue* dict = static_cast<const DictionaryValue*>(value); Should get rid of the type check, and use value->GetAsDictionary(&dict) (Which returns false on failure) https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:141: if (IsStringInDict(string_to_match, dict)) Again, it seems like we should know exactly where the thing we're looking for it. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:147: } // namespace Can just extend the anonymous namespace to the end of the file. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:173: EXPECT_TRUE(item); Should we make sure there's only one such event? https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:173: EXPECT_TRUE(item); Also, maybe log a second event, and make sure order is preserved? https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:173: EXPECT_TRUE(item); Also, should check other values as well - make sure that EVENT_PHASE is logged correctly, make sure source IDs end up where they should be. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:206: trace_net_log_observer_.reset(NULL); On 2014/08/20 22:00:43, xunjieli1 wrote: > Matt, if I just reset the observer scope_ptr, the program will crash, unless I > StopWatchForTraceStart() first. Can this be considered "destroy safely"? Yes, this is just what you should be doing. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:253: trace_net_log_observer_.reset(new TraceNetLogObserver()); On 2014/08/20 22:00:43, xunjieli1 wrote: > If we start the observer after Tracing is enabled, this does not trigger > OnTraceLogEnabled() in the observer. Line 271's EXPECT_TRUE will fail. > > You mentioned that in this case, we should try to fix the behavior. I looked > into this, but I am not sure how to fix it. Do you have any suggestion? This is probably expected behavior. I suggest just making sure we don't crash when tracing stops in this case, and not worry about anything else. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:304: EXPECT_EQ("bar", actual_value); Dictionaries support recursion, I believe, so this can just be replaced with: ASSERT_TRUE(item->GetString("args.value.params.foo", &actual_value)); ASSERT_EQ("bar", actual_value); https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:319: } nit: Blank line before ending a namespace.
Matt, thanks for the detailed reviews! Your suggestions are very helpful! I have addressed them. Hope I didn't miss anything. Thanks again! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... File net/base/trace_net_log_observer.cc (right): https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:9: #include "base/debug/trace_event.h" On 2014/08/21 15:35:40, mmenke wrote: > nit: Probably safe to leave this out, since header includes trace_event_impl. Done. Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:22: protected: Done. I see. Thank you! On 2014/08/21 15:35:39, mmenke wrote: > Suggest either making these private or public - protected means subclasses can't > call them, but since they're overriding public functions in the parent class, > the can still be called when using an instance as a member of the parent class. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:37: } // namespace On 2014/08/21 15:35:39, mmenke wrote: > nit: Blank line above end of namespace. Done. Thanks! I will keep this in mind next time! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:40: log_level_(NetLog::LOG_STRIP_PRIVATE_DATA) { On 2014/08/21 15:35:40, mmenke wrote: > Initialize net_log_to_watch_ to NULL. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:43: TraceNetLogObserver::~TraceNetLogObserver() { On 2014/08/21 15:35:39, mmenke wrote: > Maybe: > > DCHECK(!net_log_to_watch_); > DCHECK(!net_log()); > > I suggest a change below that makes the first one always the case. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:57: case(NetLog::PHASE_BEGIN): On 2014/08/21 15:35:40, mmenke wrote: > nit: case NetLog::PHASE_BEGIN: > > (Same goes for the other cases as well) Done.Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:58: TRACE_EVENT_NET_LOG_BEGIN( On 2014/08/21 15:35:39, mmenke wrote: > If this is only for NetLog events, then the macro can include "netlog" and > "value" baked into it, unless there's a reason not to do so. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:61: new TracedValue(value.Pass()))); Done. This is a good idea! Thanks! On 2014/08/21 15:35:39, mmenke wrote: > Optional: This is a bit redundant. Value includes phase, event type, and > source id. > > The other things it contains are time (Which tracing already has), source type, > and parameters. To reduce redundancy (And size of the JSON data), may make more > sense to make source type arg1, and make the entry.ParametersToValue() arg2. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:76: NOTREACHED(); Do you mean not doing anything in the default branch? On 2014/08/21 15:35:39, mmenke wrote: > Get rid of default path. Clang is configured to emit a compiler failure when a > switch statement uses an enum, there's no default branch, and some values of the > enum don't have a case. So without the default, we get a compile failure, > instead of a debug-build-only runtime check. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:91: } On 2014/08/21 15:35:39, mmenke wrote: > net_log_to_watch_ = NULL? Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.h:13: On 2014/08/21 15:35:40, mmenke wrote: > nit: Remove above blank line. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.h:18: public base::debug::TraceLog::EnabledStateObserver { On 2014/08/21 15:35:40, mmenke wrote: > nit: Above two lines should be indented two more. Done. Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.h:26: void StopObserving(); On 2014/08/21 15:35:40, mmenke wrote: > Neither of these two functions need to be public. Actually, could just get rid > of them, since they're only one line. Done. Thanks! Good idea! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer.h:41: NetLog::LogLevel log_level_; On 2014/08/21 15:35:40, mmenke wrote: > Let's just get rid of this, for now, until we need. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:8: On 2014/08/21 15:35:40, mmenke wrote: > nit: Remove blank line. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:11: #include "testing/gtest/include/gtest/gtest.h" On 2014/08/21 15:35:41, mmenke wrote: > Should include NetLog and TraceLog headers (They're redundant with > trace_net_log_observer.h, but we tend to be fairly cautious with unit test > files). Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:17: using namespace base; On 2014/08/21 15:35:40, mmenke wrote: > On 2014/08/20 22:00:43, xunjieli1 wrote: > > I think this "using namespace" directive is not recommended. How can I avoid > > doing "base::debug::" all the time? > > It's not too common in net/, but you can use individual classes (e.g. "using > base::debug::TraceLog"), just can't use entire namespaces. Done. Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:23: SetBaseLogLevel(LOG_ALL); On 2014/08/21 15:35:40, mmenke wrote: > This class shouldn't be needed - the NetLog should automatically adjust its > level when the observer is attached. Done. Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:31: const scoped_refptr<RefCountedString>& events_str, On 2014/08/21 15:35:40, mmenke wrote: > Should include base/memory/ref_counted.h, and ref_counted_memory.h. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:32: bool has_more_events); On 2014/08/21 15:35:41, mmenke wrote: > nit: Should inline all functions, to be consistent. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:33: void Clear() { On 2014/08/21 15:35:41, mmenke wrote: > nit: Blank line before clear. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:56: virtual void SetUp() OVERRIDE { On 2014/08/21 15:35:40, mmenke wrote: > nit: Suggest putting SetUp and TearDown first. Done. Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:69: if (debug::TraceLog::GetInstance()) On 2014/08/21 15:35:41, mmenke wrote: > Is this conditional needed? Looks like it always exists on shutdown, currently. Done. Right, I think it is not needed. Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:74: ListValue trace_parsed_; On 2014/08/21 15:35:41, mmenke wrote: > These should be private, with accessors as needed. Google style guide > technically does not allow public variables in classes, even in tests (Though a > lot of tests violate that rule). Done. Got it. Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:78: scoped_ptr<TraceNetLogObserver> trace_net_log_observer_; On 2014/08/21 15:35:40, mmenke wrote: > Should include the scoped_ptr header. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:85: json_output_.json_output.clear(); On 2014/08/21 15:35:40, mmenke wrote: > DCHECK(trace_parsed_.empty());? > > (DCHECK instead of assert because DCHECKs in tests are sometiems indicate a bug > in the test / test fixture, as opposed to a bug in the code being tested) > > Also should include base/logging.h for DCHECK. Done. Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:90: scoped_ptr<Value> root; On 2014/08/21 15:35:41, mmenke wrote: > trace_value, maybe? Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:96: } On 2014/08/21 15:35:41, mmenke wrote: > This can be replaced with: > > ASSERT_TRUE(root) << json_output_.json_output; > > (And get rid of the ASSERT_TRUE(root.get()) below) Done. Thanks! That's convenient! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:102: while (root_list->GetSize()) { On 2014/08/21 15:35:42, mmenke wrote: > suggest renaming root_list to trace_events and trace_parsed_ to trace_events_ > (Or use trace_event_list for both), which I think are a little clearer. Done. Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:114: std::string value_str; On 2014/08/21 15:35:40, mmenke wrote: > Should include <string> Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:124: return IsStringInDict(string_to_match, args_dict); On 2014/08/21 15:35:41, mmenke wrote: > This seems much too broad. Surely we know just where the object is we're > looking for? If it's a key, a value, or in "args" in some specific place. Done. You are right! Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:130: const ListValue& trace_parsed, On 2014/08/21 15:35:41, mmenke wrote: > Since this is a member of the class, don't need to take it as an argument. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:131: const char* string_to_match) { On 2014/08/21 15:35:41, mmenke wrote: > Maybe call this event_type, and just take raw NetLog event types, and convert > them to strings in this function as well? Done. That's smart. Thank you! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:139: const DictionaryValue* dict = static_cast<const DictionaryValue*>(value); On 2014/08/21 15:35:41, mmenke wrote: > Should get rid of the type check, and use value->GetAsDictionary(&dict) (Which > returns false on failure) Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:141: if (IsStringInDict(string_to_match, dict)) On 2014/08/21 15:35:41, mmenke wrote: > Again, it seems like we should know exactly where the thing we're looking for > it. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:147: } // namespace On 2014/08/21 15:35:42, mmenke wrote: > Can just extend the anonymous namespace to the end of the file. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:173: EXPECT_TRUE(item); On 2014/08/21 15:35:40, mmenke wrote: > Also, maybe log a second event, and make sure order is preserved? Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:173: EXPECT_TRUE(item); On 2014/08/21 15:35:40, mmenke wrote: > Also, should check other values as well - make sure that EVENT_PHASE is logged > correctly, make sure source IDs end up where they should be. Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:173: EXPECT_TRUE(item); On 2014/08/21 15:35:41, mmenke wrote: > Should we make sure there's only one such event? Done. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:206: trace_net_log_observer_.reset(NULL); On 2014/08/21 15:35:41, mmenke wrote: > On 2014/08/20 22:00:43, xunjieli1 wrote: > > Matt, if I just reset the observer scope_ptr, the program will crash, unless I > > StopWatchForTraceStart() first. Can this be considered "destroy safely"? > > Yes, this is just what you should be doing. Done. Thanks! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:253: trace_net_log_observer_.reset(new TraceNetLogObserver()); I see. Thanks! On 2014/08/21 15:35:41, mmenke wrote: > On 2014/08/20 22:00:43, xunjieli1 wrote: > > If we start the observer after Tracing is enabled, this does not trigger > > OnTraceLogEnabled() in the observer. Line 271's EXPECT_TRUE will fail. > > > > You mentioned that in this case, we should try to fix the behavior. I looked > > into this, but I am not sure how to fix it. Do you have any suggestion? > > This is probably expected behavior. I suggest just making sure we don't crash > when tracing stops in this case, and not worry about anything else. https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:304: EXPECT_EQ("bar", actual_value); On 2014/08/21 15:35:41, mmenke wrote: > Dictionaries support recursion, I believe, so this can just be replaced with: > > ASSERT_TRUE(item->GetString("args.value.params.foo", &actual_value)); > ASSERT_EQ("bar", actual_value); Done. Thanks for letting me know! https://codereview.chromium.org/468083004/diff/40001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:319: } On 2014/08/21 15:35:40, mmenke wrote: > nit: Blank line before ending a namespace. Done.
Quick comments. This is looking good, but I want to carefully go over the tests. I hope to get to it on Monday, but I have a lot of commitments for that day (Codereviews, interview writeups), so it's quite possible it'll be Tuesday. https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... File net/base/trace_net_log_observer.cc (right): https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:8: #include <string> nit: blank line between standard C and C++ headers. https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:77: break; No, I meant removing the default case entirely. Without a default case, Clang will fail to build if a new phase type is added. On 2014/08/22 17:20:09, xunjieli1 wrote: > Do you mean not doing anything in the default branch? > > On 2014/08/21 15:35:39, mmenke wrote: > > Get rid of default path. Clang is configured to emit a compiler failure when > a > > switch statement uses an enum, there's no default branch, and some values of > the > > enum don't have a case. So without the default, we get a compile failure, > > instead of a debug-build-only runtime check. > https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer.h:35: NetLog::LogLevel log_level_; No longer used. https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:75: void DisableTraceLog() { Optional: May want to make this and EnableTraceLog static, since they don't affect anything else. https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:148: const size_t npos = size_t(-1); I don't think we need this - if FindTraceEntry returns NULL, then it's not set. Don't need to check the new value. If it shouldn't return NULL, then can just assert it's non-NULL before looking at pos. https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:180: size_t pos1, pos2 = npos; Per Google style guide, use one line per variable. i.e. size_t pos1 = ...; size_t pos2 = ...; ... https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:246: EXPECT_TRUE(pos2 == npos && pos1 < pos3); Should use EXPECT_EQ and EXPECT_LT. They provide more useful output on test failures. In general, should never use a top-level && in EXPECT or DCHECK lines, should split them in two for just that reason. https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:345: EXPECT_TRUE(pos1 < pos2); EXPECT_LE
Thanks Matt! Don't worry about the delay. I still need to ping Nat on the API design. I will have cronet stuff next Monday to work on once Misha is back. Thanks for the detailed code reviews! https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... File net/base/trace_net_log_observer.cc (right): https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:8: #include <string> On 2014/08/22 18:28:14, mmenke wrote: > nit: blank line between standard C and C++ headers. Done. Thanks! https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer.cc:77: break; Done. I see. Thanks! On 2014/08/22 18:28:14, mmenke wrote: > No, I meant removing the default case entirely. Without a default case, Clang > will fail to build if a new phase type is added. > > On 2014/08/22 17:20:09, xunjieli1 wrote: > > Do you mean not doing anything in the default branch? > > > > On 2014/08/21 15:35:39, mmenke wrote: > > > Get rid of default path. Clang is configured to emit a compiler failure > when > > a > > > switch statement uses an enum, there's no default branch, and some values of > > the > > > enum don't have a case. So without the default, we get a compile failure, > > > instead of a debug-build-only runtime check. > > https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer.h:35: NetLog::LogLevel log_level_; Oops. Sorry. Should be gone now. Thanks! On 2014/08/22 18:28:14, mmenke wrote: > No longer used. https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:75: void DisableTraceLog() { On 2014/08/22 18:28:14, mmenke wrote: > Optional: May want to make this and EnableTraceLog static, since they don't > affect anything else. Done. https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:148: const size_t npos = size_t(-1); On 2014/08/22 18:28:14, mmenke wrote: > I don't think we need this - if FindTraceEntry returns NULL, then it's not set. > Don't need to check the new value. If it shouldn't return NULL, then can just > assert it's non-NULL before looking at pos. Done. https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:180: size_t pos1, pos2 = npos; On 2014/08/22 18:28:14, mmenke wrote: > Per Google style guide, use one line per variable. i.e. > > size_t pos1 = ...; > size_t pos2 = ...; > ... Done. https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:246: EXPECT_TRUE(pos2 == npos && pos1 < pos3); On 2014/08/22 18:28:14, mmenke wrote: > Should use EXPECT_EQ and EXPECT_LT. They provide more useful output on test > failures. In general, should never use a top-level && in EXPECT or DCHECK > lines, should split them in two for just that reason. Done. Thanks! https://codereview.chromium.org/468083004/diff/80001/net/base/trace_net_log_o... net/base/trace_net_log_observer_unittest.cc:345: EXPECT_TRUE(pos1 < pos2); On 2014/08/22 18:28:14, mmenke wrote: > EXPECT_LE Done.
I haven't forgotten about this, I'll get to it tomorrow.
Mostly nits. Think we're just about there. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... File net/base/trace_net_log_observer.cc (right): https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer.cc:80: void TraceNetLogObserver::WatchForTraceStart(NetLog* net_log) { DCHECK(!net_log()); https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer.cc:80: void TraceNetLogObserver::WatchForTraceStart(NetLog* net_log) { DCHECK(!net_log_to_watch_);? Otherwise, we'd end up adding ourselves as an EnabledStateObserver twice. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer.h:15: class NET_EXPORT TraceNetLogObserver Should add class level comments, and also document WatchForTraceStart() and StopWatchForTraceStart() (For the functions just a brief description, and also requirements for calling: second must be called before destruction if the first is called, first can't be called if currently watching). https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:31: virtual void SetUp() OVERRIDE { Can we just make these into the constructor and destructor? https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:63: ASSERT_TRUE(trace_value.get()); This is redundant with the one above (minor nit: I suggest not using the get when checking if a scoped_ptr is non-NULL - we support not using it, so seems like that should be the preferred style). https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:70: base::debug::CategoryFilter("*"), Could we just filter on "netlog", and then know the exact positions of items in the log, rather than having to search for them? https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:86: void ResetTraceNetLogObserver(TraceNetLogObserver* trace_net_log_observer) { nit: More common to call methods that do this "set_trace_net_log_observer" (And inline them, though you're already doing that here, anyways) https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:91: size_t& pos) { Passing in non-const references violates the style guide. Should pass it as a size_t*. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:92: // Scan all items nit: Comments should be sentences, and end with periods. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:144: scoped_ptr<CapturingNetLog> net_log_; nit: I don't think this need to be a scoped_ptr. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:161: TEST_F(TraceNetLogObserverTest, TraceEventCaptured) { Should have an event with PHASE_BEGIN and PHASE_END in this test, as well as an instant / PHASE_NONE event. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:164: EXPECT_EQ(0u, entries.size()); Just FYI, entries.empty() is generally preferred for this kind of check, as it may perform better for large vectors. For tests, performance doesn't matter, and the EXPECT_EQ gives us better output on failure, so this is fine as-is. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:185: std::string item1_phase, item2_phase; These should be declared on separate lines. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:187: EXPECT_TRUE(item2 && item2->GetString("ph", &item2_phase)); Should just asserting on item1 / item2 when you try to get them. If they're NULL, we'll crash a couple lines down, anyways, and most of the rest of the test is meaningless, anyways. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:194: item2->GetString("id", &item2_id)); nit: Should have two separate EXPECTs, so if one fails, it's clear which one. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:219: EXPECT_EQ("netlog", item2_category); Suggest making a utility function to clean up this test. Something like: struct TraceEntryInfo { std::string foo; ... }; TraceEntryInfo GetTranceEntryInfoFromValue(Value* blah) { ... } Could handle failure a couple ways - just an EXPECT and early return, or returning a scoped_ptr and just returning an empty one on failure, or could return a bool and take a TraceEntryInfo* as a parameter. Sure there are other options, any of which is fine. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:262: net_log()->AddGlobalEntry(NetLog::TYPE_URL_REQUEST_START_JOB); nit: Don't think we need the final entry.
Thanks Matt for the detailed feedback! By the way, the trace deadlock bug will be fixed once https://codereview.chromium.org/491393002/ is landed. I will fix the TODO in trace_net_log_observer_unittests.cc once it is landed. Thanks! https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... File net/base/trace_net_log_observer.cc (right): https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer.cc:80: void TraceNetLogObserver::WatchForTraceStart(NetLog* net_log) { On 2014/08/27 19:03:14, mmenke wrote: > DCHECK(!net_log()); Done. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer.cc:80: void TraceNetLogObserver::WatchForTraceStart(NetLog* net_log) { On 2014/08/27 19:03:14, mmenke wrote: > DCHECK(!net_log_to_watch_);? > > Otherwise, we'd end up adding ourselves as an EnabledStateObserver twice. Done. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer.h:15: class NET_EXPORT TraceNetLogObserver On 2014/08/27 19:03:14, mmenke wrote: > Should add class level comments, and also document WatchForTraceStart() and > StopWatchForTraceStart() (For the functions just a brief description, and also > requirements for calling: second must be called before destruction if the first > is called, first can't be called if currently watching). Done. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:31: virtual void SetUp() OVERRIDE { On 2014/08/27 19:03:15, mmenke wrote: > Can we just make these into the constructor and destructor? Done. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:63: ASSERT_TRUE(trace_value.get()); On 2014/08/27 19:03:15, mmenke wrote: > This is redundant with the one above (minor nit: I suggest not using the get > when checking if a scoped_ptr is non-NULL - we support not using it, so seems > like that should be the preferred style). Done. I see. Thanks! https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:70: base::debug::CategoryFilter("*"), On 2014/08/27 19:03:15, mmenke wrote: > Could we just filter on "netlog", and then know the exact positions of items in > the log, rather than having to search for them? Done. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:86: void ResetTraceNetLogObserver(TraceNetLogObserver* trace_net_log_observer) { On 2014/08/27 19:03:14, mmenke wrote: > nit: More common to call methods that do this "set_trace_net_log_observer" (And > inline them, though you're already doing that here, anyways) Done. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:91: size_t& pos) { On 2014/08/27 19:03:15, mmenke wrote: > Passing in non-const references violates the style guide. Should pass it as a > size_t*. Done. Got it. Thanks! https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:92: // Scan all items On 2014/08/27 19:03:15, mmenke wrote: > nit: Comments should be sentences, and end with periods. Acknowledged. I changed the category filter, as you suggested. So I removed this loop. Thanks! https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:144: scoped_ptr<CapturingNetLog> net_log_; But there is a DISALLOW_COPY_AND_ASSIGN(CapturingNetLog), so I can't do net_log_ = new CapturingNetLog, right? On 2014/08/27 19:03:15, mmenke wrote: > nit: I don't think this need to be a scoped_ptr. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:161: TEST_F(TraceNetLogObserverTest, TraceEventCaptured) { On 2014/08/27 19:03:15, mmenke wrote: > Should have an event with PHASE_BEGIN and PHASE_END in this test, as well as an > instant / PHASE_NONE event. Done. Thanks! https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:164: EXPECT_EQ(0u, entries.size()); On 2014/08/27 19:03:15, mmenke wrote: > Just FYI, entries.empty() is generally preferred for this kind of check, as it > may perform better for large vectors. For tests, performance doesn't matter, > and the EXPECT_EQ gives us better output on failure, so this is fine as-is. Done. I see. Thanks! https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:185: std::string item1_phase, item2_phase; On 2014/08/27 19:03:15, mmenke wrote: > These should be declared on separate lines. Done. Sorry, I missed this one. Thanks! https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:187: EXPECT_TRUE(item2 && item2->GetString("ph", &item2_phase)); On 2014/08/27 19:03:15, mmenke wrote: > Should just asserting on item1 / item2 when you try to get them. If they're > NULL, we'll crash a couple lines down, anyways, and most of the rest of the test > is meaningless, anyways. Done. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:194: item2->GetString("id", &item2_id)); On 2014/08/27 19:03:14, mmenke wrote: > nit: Should have two separate EXPECTs, so if one fails, it's clear which one. Done. https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:219: EXPECT_EQ("netlog", item2_category); On 2014/08/27 19:03:14, mmenke wrote: > Suggest making a utility function to clean up this test. Something like: > > struct TraceEntryInfo { > std::string foo; > ... > }; > > TraceEntryInfo GetTranceEntryInfoFromValue(Value* blah) { ... } > > Could handle failure a couple ways - just an EXPECT and early return, or > returning a scoped_ptr and just returning an empty one on failure, or could > return a bool and take a TraceEntryInfo* as a parameter. Sure there are other > options, any of which is fine. Done. Thanks! https://codereview.chromium.org/468083004/diff/100001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:262: net_log()->AddGlobalEntry(NetLog::TYPE_URL_REQUEST_START_JOB); On 2014/08/27 19:03:14, mmenke wrote: > nit: Don't think we need the final entry. Done.
can you do the trace_event change separately? Also, your phase letters would seem to collide with the regular phase letters?
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
https://codereview.chromium.org/468083004/diff/110001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/468083004/diff/110001/base/debug/trace_event.... base/debug/trace_event.h:650: #define TRACE_EVENT_NET_LOG_BEGIN(category_group, name, id, arg1_name, \ Each of these macros should have a 2 on the end for consistency with other trace macros. https://codereview.chromium.org/468083004/diff/110001/base/debug/trace_event.... base/debug/trace_event.h:1010: #define TRACE_EVENT_PHASE_NET_LOG_INSTANT ('i') I recommend against using 'i' here. We used to use 'i' for the 'I' event but it got renamed at some point. We've been keeping 'i' free in case there are old .trace files hanging around. The current trace-viewer will take either 'i' or 'I' as input.
Thanks dsinclair and nduca, I have separated trace_event.h into another cl (https://codereview.chromium.org/536503002/) https://codereview.chromium.org/468083004/diff/110001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/468083004/diff/110001/base/debug/trace_event.... base/debug/trace_event.h:650: #define TRACE_EVENT_NET_LOG_BEGIN(category_group, name, id, arg1_name, \ On 2014/09/02 20:11:45, dsinclair wrote: > Each of these macros should have a 2 on the end for consistency with other trace > macros. Done. https://codereview.chromium.org/468083004/diff/110001/base/debug/trace_event.... base/debug/trace_event.h:1010: #define TRACE_EVENT_PHASE_NET_LOG_INSTANT ('i') On 2014/09/02 20:11:45, dsinclair wrote: > I recommend against using 'i' here. We used to use 'i' for the 'I' event but it > got renamed at some point. We've been keeping 'i' free in case there are old > .trace files hanging around. The current trace-viewer will take either 'i' or > 'I' as input. Acknowledged.
https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer.h:28: // This can't be called if TraceNetLogObserver is currently watching. Maybe: Start to watch for TraceLog enable and disable events. This can't be called if already watching for events. Watches NetLog only when tracing is enabled. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer.h:32: // If WatchForTraceStart is called, this should be called before should -> must https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:27: using base::debug::TraceLog; nit: Using directives are usually put outside namespaces. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:36: TraceEntryInfo GetTraceEntryInfoFromValue(const base::DictionaryValue *value) { nit: Asterisk goes before the space. Actually, though, since the input is const and can't be NULL, passing by reference is preferred. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:46: return TraceEntryInfo{category, id, phase, source_type}; nit: TraceEntryInfo info; value->GetString("cat", &info.category); ... return info; Is a little simpler. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:118: size_t pos) { nit: Find -> Get https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:121: trace_events_.Get(pos, &value); Can use GetDictionary to save a step. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:126: if (dict->GetString("name", &actual_type)) { I think we don't expose enough on failure - it could be the entry is missing, or it could be it has the wrong type. Suggest removing the type from this function, and make GetTraceEntryInfoFromValue get it as well. Then this function becomes basically a call to trace_events_.GetDictionary(), so you may as well just get rid of it, and inline the code, adding an accessor for trace_events_. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:136: std::vector<const base::DictionaryValue*> FindTraceEntries( This function adds fuzziness into tests that I don't think we need - we can check the exact values instead. First entry should be TYPE_GOAT_NOT_FOUND event, second should be a TYPE_GOAT_OVERFLOW, etc. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:168: scoped_ptr<CapturingNetLog> net_log_; On 2014/09/02 16:37:33, xunjieli1 wrote: > But there is a DISALLOW_COPY_AND_ASSIGN(CapturingNetLog), so I can't do net_log_ > = new CapturingNetLog, right? > > On 2014/08/27 19:03:15, mmenke wrote: > > nit: I don't think this need to be a scoped_ptr. > That's certainly true, but I don't see any reason you need to explicitly invoke the constructor yourself. It'll be called on construction of the test fixture itself, and it doesn't look like that will cause any issues. Am I missing something? https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:227: GetTraceEntryInfoFromValue(item1))); This will return clearer failures if you just use sets of 4 EXPECT_EQ entries here - will give the line number, so you know which items has the mismatch, and the real/expected values. Not lovely, but can really help diagnose failures on the bots. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:234: EXPECT_EQ(1u, FindTraceEntries(NetLog::TYPE_REQUEST_ALIVE).size()); If you just check the total number of trace entries under the call to StopWatchForTraceStart, these are redundant. Same goes for the other tests
Thanks Matt for the review! there is one issue about the trace_events_ list. I replied in the comments. Could you take a look? Thanks so much! https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer.h:28: // This can't be called if TraceNetLogObserver is currently watching. On 2014/09/02 21:28:59, mmenke wrote: > Maybe: > > Start to watch for TraceLog enable and disable events. > This can't be called if already watching for events. > Watches NetLog only when tracing is enabled. Done. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer.h:32: // If WatchForTraceStart is called, this should be called before On 2014/09/02 21:28:59, mmenke wrote: > should -> must Done. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:27: using base::debug::TraceLog; On 2014/09/02 21:28:59, mmenke wrote: > nit: Using directives are usually put outside namespaces. Done. Thanks! https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:36: TraceEntryInfo GetTraceEntryInfoFromValue(const base::DictionaryValue *value) { On 2014/09/02 21:28:59, mmenke wrote: > nit: Asterisk goes before the space. > > Actually, though, since the input is const and can't be NULL, passing by > reference is preferred. Done. Thanks! I will keep that in mind. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:46: return TraceEntryInfo{category, id, phase, source_type}; On 2014/09/02 21:28:59, mmenke wrote: > nit: > > TraceEntryInfo info; > value->GetString("cat", &info.category); > ... > return info; > > Is a little simpler. Done. I see! Thanks! https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:118: size_t pos) { On 2014/09/02 21:28:59, mmenke wrote: > nit: Find -> Get Done. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:121: trace_events_.Get(pos, &value); On 2014/09/02 21:28:59, mmenke wrote: > Can use GetDictionary to save a step. Done. Thanks! https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:126: if (dict->GetString("name", &actual_type)) { This is a good point. But Trace adds metadata events, no matter what category we are watching. Therefore, the total number of trace entries does not correspond to the number of NetLog calls. trace_events can have a metadata event happen in between two netlog events, so we can't know for sure at which position in the list is a netlog event. Eg. [{"args":{"params":"","source_type":"NONE"},"cat":"netlog","id":"0x2","name":"CANCELLED","ph":"z","pid":15991,"tid":15991,"ts":35531447013.0,"tts":820024},{"args":{"params":"","source_type":"NONE"},"cat":"netlog","id":"0x1","name":"URL_REQUEST_START_JOB","ph":"x","pid":15991,"tid":15991,"ts":35531447071.0,"tts":820078},{"args":{"params":"","source_type":"NONE"},"cat":"netlog","id":"0x1","name":"REQUEST_ALIVE","ph":"y","pid":15991,"tid":15991,"ts":35531447088.0,"tts":820094},{"args":{"number":40},"cat":"__metadata","name":"num_cpus","ph":"M","pid":15991,"tid":0,"ts":0}] On 2014/09/02 21:28:59, mmenke wrote: > I think we don't expose enough on failure - it could be the entry is missing, or > it could be it has the wrong type. Suggest removing the type from this > function, and make GetTraceEntryInfoFromValue get it as well. > > Then this function becomes basically a call to trace_events_.GetDictionary(), so > you may as well just get rid of it, and inline the code, adding an accessor for > trace_events_. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:136: std::vector<const base::DictionaryValue*> FindTraceEntries( On 2014/09/02 21:28:59, mmenke wrote: > This function adds fuzziness into tests that I don't think we need - we can > check the exact values instead. First entry should be TYPE_GOAT_NOT_FOUND > event, second should be a TYPE_GOAT_OVERFLOW, etc. Acknowledged. Ideally, we should be able to do this. But due to this metadata events, we can't know for sure at which position in the list a Netlog entry is. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:168: scoped_ptr<CapturingNetLog> net_log_; On 2014/09/02 21:28:59, mmenke wrote: > On 2014/09/02 16:37:33, xunjieli1 wrote: > > But there is a DISALLOW_COPY_AND_ASSIGN(CapturingNetLog), so I can't do > net_log_ > > = new CapturingNetLog, right? > > > > On 2014/08/27 19:03:15, mmenke wrote: > > > nit: I don't think this need to be a scoped_ptr. > > > > That's certainly true, but I don't see any reason you need to explicitly invoke > the constructor yourself. It'll be called on construction of the test fixture > itself, and it doesn't look like that will cause any issues. Am I missing > something? Done. I see! Didn't know it is automatically constructed for me. That's convenient! thanks! https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:227: GetTraceEntryInfoFromValue(item1))); On 2014/09/02 21:28:59, mmenke wrote: > This will return clearer failures if you just use sets of 4 EXPECT_EQ entries > here - will give the line number, so you know which items has the mismatch, and > the real/expected values. Not lovely, but can really help diagnose failures on > the bots. Done. https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:234: EXPECT_EQ(1u, FindTraceEntries(NetLog::TYPE_REQUEST_ALIVE).size()); On 2014/09/02 21:28:59, mmenke wrote: > If you just check the total number of trace entries under the call to > StopWatchForTraceStart, these are redundant. Same goes for the other tests Acknowledged. Given the metadata events limitation, is there a better way to do this?
Quick response https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:126: if (dict->GetString("name", &actual_type)) { Thanks, I didn't realize that! Weird... I wonder if this is a bug in base::debug::CategoryFilter, since we're only asking for netlog events. My suggestion is to filter out non-"cat: netlog" events in OnTraceDataCollected. Then we should know exactly what the objects look like. Sound reasonable? On 2014/09/02 23:57:17, xunjieli1 wrote: > This is a good point. > But Trace adds metadata events, no matter what category we are watching. > Therefore, the total number of trace entries does not correspond to the number > of NetLog calls. trace_events can have a metadata event happen in between two > netlog events, so we can't know for sure at which position in the list is a > netlog event. > > Eg. > [{"args":{"params":"","source_type":"NONE"},"cat":"netlog","id":"0x2","name":"CANCELLED","ph":"z","pid":15991,"tid":15991,"ts":35531447013.0,"tts":820024},{"args":{"params":"","source_type":"NONE"},"cat":"netlog","id":"0x1","name":"URL_REQUEST_START_JOB","ph":"x","pid":15991,"tid":15991,"ts":35531447071.0,"tts":820078},{"args":{"params":"","source_type":"NONE"},"cat":"netlog","id":"0x1","name":"REQUEST_ALIVE","ph":"y","pid":15991,"tid":15991,"ts":35531447088.0,"tts":820094},{"args":{"number":40},"cat":"__metadata","name":"num_cpus","ph":"M","pid":15991,"tid":0,"ts":0}] > > > On 2014/09/02 21:28:59, mmenke wrote: > > I think we don't expose enough on failure - it could be the entry is missing, > or > > it could be it has the wrong type. Suggest removing the type from this > > function, and make GetTraceEntryInfoFromValue get it as well. > > > > Then this function becomes basically a call to trace_events_.GetDictionary(), > so > > you may as well just get rid of it, and inline the code, adding an accessor > for > > trace_events_. >
rsleevi@chromium.org changed reviewers: - rsleevi@chromium.org
Removing myself, as I won't have the bandwidth to even stay on top of these changes. Go Matt!
On 2014/09/03 00:07:23, mmenke wrote: > Quick response > > https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... > File net/base/trace_net_log_observer_unittest.cc (right): > > https://codereview.chromium.org/468083004/diff/130001/net/base/trace_net_log_... > net/base/trace_net_log_observer_unittest.cc:126: if (dict->GetString("name", > &actual_type)) { > Thanks, I didn't realize that! > > Weird... I wonder if this is a bug in base::debug::CategoryFilter, since we're > only asking for netlog events. My suggestion is to filter out non-"cat: netlog" > events in OnTraceDataCollected. Then we should know exactly what the objects > look like. > > Sound reasonable? > > On 2014/09/02 23:57:17, xunjieli1 wrote: > > This is a good point. > > But Trace adds metadata events, no matter what category we are watching. > > Therefore, the total number of trace entries does not correspond to the number > > of NetLog calls. trace_events can have a metadata event happen in between two > > netlog events, so we can't know for sure at which position in the list is a > > netlog event. > > > > Eg. > > > [{"args":{"params":"","source_type":"NONE"},"cat":"netlog","id":"0x2","name":"CANCELLED","ph":"z","pid":15991,"tid":15991,"ts":35531447013.0,"tts":820024},{"args":{"params":"","source_type":"NONE"},"cat":"netlog","id":"0x1","name":"URL_REQUEST_START_JOB","ph":"x","pid":15991,"tid":15991,"ts":35531447071.0,"tts":820078},{"args":{"params":"","source_type":"NONE"},"cat":"netlog","id":"0x1","name":"REQUEST_ALIVE","ph":"y","pid":15991,"tid":15991,"ts":35531447088.0,"tts":820094},{"args":{"number":40},"cat":"__metadata","name":"num_cpus","ph":"M","pid":15991,"tid":0,"ts":0}] > > > > > > On 2014/09/02 21:28:59, mmenke wrote: > > > I think we don't expose enough on failure - it could be the entry is > missing, > > or > > > it could be it has the wrong type. Suggest removing the type from this > > > function, and make GetTraceEntryInfoFromValue get it as well. > > > > > > Then this function becomes basically a call to > trace_events_.GetDictionary(), > > so > > > you may as well just get rid of it, and inline the code, adding an accessor > > for > > > trace_events_. > > Matt, thanks for the quick response! I did what you suggested. Could you take a look? Thanks!
This looks really good! https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer.h:15: // TraceNetLogObserver watches for Trace enable, and sends NetLog nit: "Trace enable" -> "trace enable" or "TraceLog enable". Trace isn't a class name, so should probably not be capitalized https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer.h:19: public base::debug::TraceLog::EnabledStateObserver { Per email comment, EnabledStateObserver is not declared as BASE_EXPORT. We should resolve that before landing. I'm fine with landing with the call to SetCurrentThreadBlocksMessageLoop, if needed. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:83: base::ListValue* filtered_events = FilterNetLogTraceEvents(trace_events); BUG: You're leaking |filtered_events| here. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:84: trace_events_.Swap(filtered_events); Think it makes sense to just switch trace_events_ to a scoped_ptr now, and set it to filtered_events, instead of swapping it. This didn't work because because of the GetAsList call, but now makes life simpler. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:109: base::ListValue* FilterNetLogTraceEvents(base::ListValue* trace_events) { Should make this: static scoped_ptr<base::ListValue> FilterNetLogTraceEvents(const base::ListValue& trace_events) * static because it doesn't access any member variables. * scoped_ptr to make ownership semantics clear, and to prevent leaks. * const base::ListValue& to make it clear the argument can't be NULL (And is not modified). https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:110: base::ListValue* filtered_trace_events = new base::ListValue(); Should make this a scoped_ptr as well, and return it with .Pass() https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:113: if (!trace_events->GetDictionary(i, &dict)) completely optional: Since this isn't expected, may want to add an ADD_FAILURE() here. Main reason is if we somehow end up adding non-dictionaries, and the test fails as a result, failures here will hint at the problem. Also indicates to people reading the test it shouldn't happen. Failures here would have to be failures of the tracing code, so it's not really necessary. Same goes for the dict->GetString("cat", ...) call. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:116: if (dict->GetString("cat", &category)) { nit: Early abort is preferred, in general, as it reduces nesting, which makes code a little more readable, in general. Some cases it can make code less clear or more regression prone, but I don't think this is one of those cases. So just: if (!dict->GetString("cat", &category))) continue; // Flipping this one isn't really necessary, but is more consistent. if (category != "netlog") continue; filtered_trace_events->Append(dict->DeepCopy()); https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:179: EXPECT_TRUE(trace_events().GetDictionary(2, &item3)); These should all be asserts. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:198: NetLog::SourceTypeToString(entries[2].source.type)}; Don't think these get you a whole lot - suggest just inline these values before. This goes for other tests, too. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:206: EXPECT_EQ(expected_item1.source_type, actual_item1.source_type); Should check the names here, too. This goes for other tests, too.
Patchset #11 (id:190001) has been deleted
Xianzhu fixed the deadlock bug, I have rebased and removed SetCurrentThreadBlocksMessageLoop(). I added BASE_EXPORT to EnabledStateObserver in the CL which has tracing changes (crrev.com/536503002). PTAL. Thanks! https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... File net/base/trace_net_log_observer.h (right): https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer.h:15: // TraceNetLogObserver watches for Trace enable, and sends NetLog On 2014/09/03 19:12:10, mmenke wrote: > nit: "Trace enable" -> "trace enable" or "TraceLog enable". Trace isn't a > class name, so should probably not be capitalized Done. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer.h:19: public base::debug::TraceLog::EnabledStateObserver { On 2014/09/03 19:12:10, mmenke wrote: > Per email comment, EnabledStateObserver is not declared as BASE_EXPORT. We > should resolve that before landing. I'm fine with landing with the call to > SetCurrentThreadBlocksMessageLoop, if needed. Acknowledged. The deadlock problem has been fixed by Xianzhu. I have removed SetCurrentThreadBlocksMessageLoop and rebased. I put BASE_EXPORT in the CL where we added the trace APIs. I will wait for that Cl to land first before commit this one. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:83: base::ListValue* filtered_events = FilterNetLogTraceEvents(trace_events); On 2014/09/03 19:12:11, mmenke wrote: > BUG: You're leaking |filtered_events| here. Done. Thanks! https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:84: trace_events_.Swap(filtered_events); On 2014/09/03 19:12:11, mmenke wrote: > Think it makes sense to just switch trace_events_ to a scoped_ptr now, and set > it to filtered_events, instead of swapping it. This didn't work because because > of the GetAsList call, but now makes life simpler. Done. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:109: base::ListValue* FilterNetLogTraceEvents(base::ListValue* trace_events) { On 2014/09/03 19:12:11, mmenke wrote: > Should make this: > > static scoped_ptr<base::ListValue> FilterNetLogTraceEvents(const > base::ListValue& trace_events) > > * static because it doesn't access any member variables. > * scoped_ptr to make ownership semantics clear, and to prevent leaks. > * const base::ListValue& to make it clear the argument can't be NULL (And is not > modified). Done. Thanks for the detailed explanation! I will keep this in mind. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:110: base::ListValue* filtered_trace_events = new base::ListValue(); On 2014/09/03 19:12:11, mmenke wrote: > Should make this a scoped_ptr as well, and return it with .Pass() Done. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:113: if (!trace_events->GetDictionary(i, &dict)) On 2014/09/03 19:12:11, mmenke wrote: > completely optional: Since this isn't expected, may want to add an > ADD_FAILURE() here. Main reason is if we somehow end up adding > non-dictionaries, and the test fails as a result, failures here will hint at the > problem. Also indicates to people reading the test it shouldn't happen. > > Failures here would have to be failures of the tracing code, so it's not really > necessary. > > Same goes for the dict->GetString("cat", ...) call. Done. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:116: if (dict->GetString("cat", &category)) { On 2014/09/03 19:12:11, mmenke wrote: > nit: Early abort is preferred, in general, as it reduces nesting, which makes > code a little more readable, in general. Some cases it can make code less clear > or more regression prone, but I don't think this is one of those cases. > > So just: > > if (!dict->GetString("cat", &category))) > continue; > // Flipping this one isn't really necessary, but is more consistent. > if (category != "netlog") > continue; > filtered_trace_events->Append(dict->DeepCopy()); Done. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:179: EXPECT_TRUE(trace_events().GetDictionary(2, &item3)); On 2014/09/03 19:12:11, mmenke wrote: > These should all be asserts. Done. https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:198: NetLog::SourceTypeToString(entries[2].source.type)}; On 2014/09/03 19:12:11, mmenke wrote: > Don't think these get you a whole lot - suggest just inline these values before. > This goes for other tests, too. Partially done. I inlined event types, since those are made explicit in these tests. I left source id and source type. Is that okay? https://codereview.chromium.org/468083004/diff/170001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:206: EXPECT_EQ(expected_item1.source_type, actual_item1.source_type); On 2014/09/03 19:12:11, mmenke wrote: > Should check the names here, too. This goes for other tests, too. Done. Sorry!
Fix these, and I'll sign off. https://codereview.chromium.org/468083004/diff/210001/net/base/trace_net_log_... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/210001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:44: value.GetString("args.source_type", &info.source_type); These should all be wrapped in EXPECT_TRUE https://codereview.chromium.org/468083004/diff/210001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:205: NetLog::SourceTypeToString(entries[2].source.type)}; On 2014/09/08 14:08:41, xunjieli1 wrote: > On 2014/09/03 19:12:11, mmenke wrote: > > Don't think these get you a whole lot - suggest just inline these values > before. > > This goes for other tests, too. > > Partially done. I inlined event types, since those are made explicit in these > tests. I left source id and source type. Is that okay? Sorry, I wasn't clear - I mean expected_item1 / expected_item2 / etc aren't needed. Instead, can just check each value directly below. https://codereview.chromium.org/468083004/diff/210001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:226: ASSERT_EQ(expected_item3.source_type, actual_item3.source_type); All of these can be EXPECTs. The size checks could probably be EXPECTs, too. The ones that need to be asserts are just the trace_events()->GetDictionary calls. Generally, should only use ASSERTs if failures will result in a crash. Same goes for other tests.
Thanks Matt for the detailed explanation, which is very helpful! I have uploaded a new patch. PTAL. https://codereview.chromium.org/468083004/diff/210001/net/base/trace_net_log_... File net/base/trace_net_log_observer_unittest.cc (right): https://codereview.chromium.org/468083004/diff/210001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:44: value.GetString("args.source_type", &info.source_type); On 2014/09/08 16:02:04, mmenke wrote: > These should all be wrapped in EXPECT_TRUE Done. https://codereview.chromium.org/468083004/diff/210001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:205: NetLog::SourceTypeToString(entries[2].source.type)}; On 2014/09/08 16:02:04, mmenke wrote: > On 2014/09/08 14:08:41, xunjieli1 wrote: > > On 2014/09/03 19:12:11, mmenke wrote: > > > Don't think these get you a whole lot - suggest just inline these values > > before. > > > This goes for other tests, too. > > > > Partially done. I inlined event types, since those are made explicit in these > > tests. I left source id and source type. Is that okay? > > Sorry, I wasn't clear - I mean expected_item1 / expected_item2 / etc aren't > needed. Instead, can just check each value directly below. Done. https://codereview.chromium.org/468083004/diff/210001/net/base/trace_net_log_... net/base/trace_net_log_observer_unittest.cc:226: ASSERT_EQ(expected_item3.source_type, actual_item3.source_type); On 2014/09/08 16:02:04, mmenke wrote: > All of these can be EXPECTs. The size checks could probably be EXPECTs, too. > The ones that need to be asserts are just the trace_events()->GetDictionary > calls. Generally, should only use ASSERTs if failures will result in a crash. > Same goes for other tests. Done. I see. Thanks for letting me know the distinction! I think I understand it better now :)
Matt, right now only ios_dbg_simulator is failing. I don't it's related to my change. WDYT?
I agree the failures look unrelated. LGTM!
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xunjieli@chromium.org/468083004/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Matt, on a second look, I think the ios_dbg_simulator is related to my change. If you scroll down to the bottom of http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator... , you will see [ RUN ] CookieMonsterTest.GarbageCollectionTriggers [7681:2315:0910/070242:1151150972200:FATAL:lock_impl_posix.cc(46)] Check failed: rv == 0 (22 vs. 0). Invalid argument 8 net_unittests 0x01c42d75 base::debug::TraceLog::OnFlushTimeout(int) + 69 Is there anything that looks like a possible cause of this crash? Seems like I need my mac to debug this, but I left it at home :(
On 2014/09/10 14:24:31, xunjieli1 wrote: > Matt, on a second look, I think the ios_dbg_simulator is related to my change. > If you scroll down to the bottom of > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator... > , you will see > > [ RUN ] CookieMonsterTest.GarbageCollectionTriggers > [7681:2315:0910/070242:1151150972200:FATAL:lock_impl_posix.cc(46)] Check failed: > rv == 0 (22 vs. 0). Invalid argument > 8 net_unittests 0x01c42d75 > base::debug::TraceLog::OnFlushTimeout(int) + 69 > > Is there anything that looks like a possible cause of this crash? > > Seems like I need my mac to debug this, but I left it at home :( Hrm... Your code shouldn't even be running in that test. It's a net_unittest, and you only hooked your code up to the ChromeNetLog, which is not in net. So what does that mean? My guess is your test has some side effects that are causing future uses of the log to act up. Test execution order and when the same/different processes are used may be different on different platforms, so I'd try to run both your test and the CookieMonsterTests locally in sequence, in the same process, to see if you can repro.
On 2014/09/10 14:32:43, mmenke wrote: > On 2014/09/10 14:24:31, xunjieli1 wrote: > > Matt, on a second look, I think the ios_dbg_simulator is related to my change. > > If you scroll down to the bottom of > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator... > > , you will see > > > > [ RUN ] CookieMonsterTest.GarbageCollectionTriggers > > [7681:2315:0910/070242:1151150972200:FATAL:lock_impl_posix.cc(46)] Check > failed: > > rv == 0 (22 vs. 0). Invalid argument > > 8 net_unittests 0x01c42d75 > > base::debug::TraceLog::OnFlushTimeout(int) + 69 > > > > Is there anything that looks like a possible cause of this crash? > > > > Seems like I need my mac to debug this, but I left it at home :( > > Hrm... Your code shouldn't even be running in that test. It's a net_unittest, > and you only hooked your code up to the ChromeNetLog, which is not in net. > > So what does that mean? My guess is your test has some side effects that are > causing future uses of the log to act up. Test execution order and when the > same/different processes are used may be different on different platforms, so > I'd try to run both your test and the CookieMonsterTests locally in sequence, in > the same process, to see if you can repro. Oh, and by "future uses of the log", I mean the TraceLog / tracing infrastructure, not the NetLog.
Per discussion on https://codereview.chromium.org/568073003/ , I have changed the unit tests to not delete TraceLog. Matt, could you take a final look at the CL?
LGTM!
The CQ bit was checked by xunjieli@chromium.org
Thanks Matt! You are an awesome reviewer!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/468083004/270001
Message was sent while issue was closed.
Committed patchset #14 (id:270001) as 7baee9c0a6d1b62f5186f336f26e5c2d830f1c78
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/eb290602c4dbfee90191c3044e68e53a0db4c3e3 Cr-Commit-Position: refs/heads/master@{#295089} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
