Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699833003:
arc: enable Android tracing from chrome://tracing in dev mode (Closed)
|
Created:
3 years, 10 months ago by shunhsingou Modified:
3 years, 9 months ago Reviewers:
Primiano Tucci (use gerrit), reveman, tzik, dsinclair, Luis Héctor Chávez, dcheng, Earl Ou, oystein (OOO til 10th of July), bccheng, Daniel Erat, Yusuke Sato, alexmos CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, wfh+watch_chromium.org, hidehiko+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, oshima+watch_chromium.org, tracing+reviews_chromium.org, darin-cc_chromium.org, darin (slow to review), davemoore+watch_chromium.org, qsr+mojo_chromium.org, chiniforooshan Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: enable Android tracing from chrome://tracing in dev mode
To enable tracing from chrome://tracing, we create a Mojo interface
for host side to trigger tracing in Android.
The trace is written into debugfs/tracefs in dev-mode, and is read back
by debugd.
Related changes:
- http://crrev.com/2699833003
- http://ag/1915548
BUG=653795
TEST=Run chrome://tracing and see events from Android.
Review-Url: https://codereview.chromium.org/2699833003
Cr-Commit-Position: refs/heads/master@{#456985}
Committed: https://chromium.googlesource.com/chromium/src/+/0b110db84a13d9087ddf55b5e99d961e54f55e16
Patch Set 1 #
Total comments: 16
Patch Set 2 : Fix according to the reviewer comments #Patch Set 3 : Fix nits #
Total comments: 6
Patch Set 4 : Fix according to the comments #
Total comments: 10
Patch Set 5 : Fix according to comments #Patch Set 6 : fix according to the comments #
Total comments: 4
Patch Set 7 : Fix according to comments #
Total comments: 2
Patch Set 8 : Fix according to the comments #
Total comments: 34
Patch Set 9 : Fix according to the comments #Patch Set 10 : Fix according to the comments #Patch Set 11 : Fix according to comments #
Total comments: 16
Patch Set 12 : Fix according to comments #
Total comments: 6
Patch Set 13 : Fix according to comments #Patch Set 14 : Move //chromeos/tracing to //content/browser/tracing #
Total comments: 2
Patch Set 15 : Fix according to comments #Patch Set 16 : Fix compiling error #Patch Set 17 : Compiling error workaround #
Total comments: 1
Messages
Total messages: 81 (29 generated)
Description was changed from ========== arc: enable Android tracing from chrome://tracing in dev mode To enable tracing from chrome://tracing, we create a Mojo interface for host side to trigger tracing in Android. The trace is written into debugfs/tracefs in dev-mode, and is read back by debugd. BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android tracing from chrome://tracing in dev mode To enable tracing from chrome://tracing, we create a Mojo interface for host side to trigger tracing in Android. The trace is written into debugfs/tracefs in dev-mode, and is read back by debugd. Related changes: - http://crrev.com/2699833003 - http://ag/1915548 BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
shunhsingou@google.com changed reviewers: + bccheng@chromium.org, lhchavez@chromium.org, reveman@chromium.org, yusukes@chromium.org
shunhsingou@google.com changed reviewers: + shunhsingou@google.com
Split from crrev.com/2400163003
https://codereview.chromium.org/2699833003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:7: #include <utility> unused? https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.cc:77: ArcTraceAgentImpl() : delegate_(nullptr), weak_ptr_factory_(this) {} prefer initializing |delegate_| in L81. https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.h:8: #include <string> unused? https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.h:10: #include "base/callback.h" Maybe "base/callback_forward.h"? https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.h:11: #include "base/files/scoped_file.h" unused https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.h:19: class CHROMEOS_EXPORT ArcTraceAgent : public base::trace_event::TracingAgent { Maybe add a comment about how this class should only be interacted with in the UI thread. https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.h:26: virtual ~Delegate() = default; nit: please move the declarations to the .cc file (this and L35). https://codereview.chromium.org/2699833003/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:308: if (arc_trace_agent) { Why would |arc_trace_agent| be nullptr? Maybe you should follow the pattern that the EtwTracingAgent uses. Same in L584.
https://codereview.chromium.org/2699833003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:7: #include <utility> On 2017/02/16 16:45:10, Luis Héctor Chávez wrote: > unused? Removed https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.cc:77: ArcTraceAgentImpl() : delegate_(nullptr), weak_ptr_factory_(this) {} On 2017/02/16 16:45:10, Luis Héctor Chávez wrote: > prefer initializing |delegate_| in L81. Done. https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.h:8: #include <string> On 2017/02/16 16:45:10, Luis Héctor Chávez wrote: > unused? Removed https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.h:10: #include "base/callback.h" On 2017/02/16 16:45:11, Luis Héctor Chávez wrote: > Maybe "base/callback_forward.h"? Done. https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.h:11: #include "base/files/scoped_file.h" On 2017/02/16 16:45:11, Luis Héctor Chávez wrote: > unused Done. https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.h:19: class CHROMEOS_EXPORT ArcTraceAgent : public base::trace_event::TracingAgent { On 2017/02/16 16:45:11, Luis Héctor Chávez wrote: > Maybe add a comment about how this class should only be interacted with in the > UI thread. Done. https://codereview.chromium.org/2699833003/diff/1/chromeos/trace/arc_trace_ag... chromeos/trace/arc_trace_agent.h:26: virtual ~Delegate() = default; On 2017/02/16 16:45:11, Luis Héctor Chávez wrote: > nit: please move the declarations to the .cc file (this and L35). Done. https://codereview.chromium.org/2699833003/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:308: if (arc_trace_agent) { On 2017/02/16 16:45:11, Luis Héctor Chávez wrote: > Why would |arc_trace_agent| be nullptr? Maybe you should follow the pattern that > the EtwTracingAgent uses. Same in L584. Done.
https://codereview.chromium.org/2699833003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:52: categories_.clear(); Should you remove any elements in |categories_| from the selection UI? https://codereview.chromium.org/2699833003/diff/40001/chromeos/trace/arc_trac... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2699833003/diff/40001/chromeos/trace/arc_trac... chromeos/trace/arc_trace_agent.cc:25: LOG(WARNING) << "Failed to stop ARC tracing."; nit: LOG_IF(WARNING, !success) https://codereview.chromium.org/2699833003/diff/40001/chromeos/trace/arc_trac... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2699833003/diff/40001/chromeos/trace/arc_trac... chromeos/trace/arc_trace_agent.h:16: // The overrided Stop/StartAgentTracing functions is implemented by the nit: s/overrided/overridden/, s/is implemented/are implemented/
https://codereview.chromium.org/2699833003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:52: categories_.clear(); On 2017/02/27 20:47:18, Luis Héctor Chávez wrote: > Should you remove any elements in |categories_| from the selection UI? I failed to find an API for this, but this only happens when people trying to modify code on Android side and only reboot Android. I added a comment here. https://codereview.chromium.org/2699833003/diff/40001/chromeos/trace/arc_trac... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2699833003/diff/40001/chromeos/trace/arc_trac... chromeos/trace/arc_trace_agent.cc:25: LOG(WARNING) << "Failed to stop ARC tracing."; On 2017/02/27 20:47:18, Luis Héctor Chávez wrote: > nit: LOG_IF(WARNING, !success) Done. https://codereview.chromium.org/2699833003/diff/40001/chromeos/trace/arc_trac... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2699833003/diff/40001/chromeos/trace/arc_trac... chromeos/trace/arc_trace_agent.h:16: // The overrided Stop/StartAgentTracing functions is implemented by the On 2017/02/27 20:47:18, Luis Héctor Chávez wrote: > nit: s/overrided/overridden/, s/is implemented/are implemented/ Done.
https://codereview.chromium.org/2699833003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:35: QueryAvailableCategories(); This looks unnecessary: how about moving the implementation of QueryAvailableCategories() to here? https://codereview.chromium.org/2699833003/diff/60001/chromeos/trace/arc_trac... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2699833003/diff/60001/chromeos/trace/arc_trac... chromeos/trace/arc_trace_agent.cc:78: ArcTraceAgentImpl() : weak_ptr_factory_(this) {} this can now be |= default;|. Also, you won't need the empty line in L79. https://codereview.chromium.org/2699833003/diff/60001/chromeos/trace/arc_trac... chromeos/trace/arc_trace_agent.cc:84: base::WeakPtrFactory<ArcTraceAgentImpl> weak_ptr_factory_; unused. https://codereview.chromium.org/2699833003/diff/60001/content/browser/tracing... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/60001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:307: chromeos::ArcTraceAgent::GetInstance()->StartAgentTracing( Maybe surround this with if (trace_config.IsSystraceEnabled()) ? Or move to L296. https://codereview.chromium.org/2699833003/diff/60001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:698: if (agent_name != kArcTracingAgentName) { What happens in L694 in the ARC case? Should we skip it in the if/else chain too?
https://codereview.chromium.org/2699833003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:35: QueryAvailableCategories(); On 2017/03/02 18:45:50, Luis Héctor Chávez wrote: > This looks unnecessary: how about moving the implementation of > QueryAvailableCategories() to here? Done. https://codereview.chromium.org/2699833003/diff/60001/chromeos/trace/arc_trac... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2699833003/diff/60001/chromeos/trace/arc_trac... chromeos/trace/arc_trace_agent.cc:78: ArcTraceAgentImpl() : weak_ptr_factory_(this) {} On 2017/03/02 18:45:50, Luis Héctor Chávez wrote: > this can now be |= default;|. Also, you won't need the empty line in L79. Done. https://codereview.chromium.org/2699833003/diff/60001/chromeos/trace/arc_trac... chromeos/trace/arc_trace_agent.cc:84: base::WeakPtrFactory<ArcTraceAgentImpl> weak_ptr_factory_; On 2017/03/02 18:45:50, Luis Héctor Chávez wrote: > unused. Done. https://codereview.chromium.org/2699833003/diff/60001/content/browser/tracing... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/60001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:307: chromeos::ArcTraceAgent::GetInstance()->StartAgentTracing( On 2017/03/02 18:45:50, Luis Héctor Chávez wrote: > Maybe surround this with if (trace_config.IsSystraceEnabled()) ? Or move to > L296. Done. https://codereview.chromium.org/2699833003/diff/60001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:698: if (agent_name != kArcTracingAgentName) { On 2017/03/02 18:45:50, Luis Héctor Chávez wrote: > What happens in L694 in the ARC case? Should we skip it in the if/else chain > too? Done.
style nits: https://codereview.chromium.org/2699833003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:54: Category{category, std::string(kCategoryPrefix) + category}); nit: std::string() seems redundant as |category| is already a std::string object. https://codereview.chromium.org/2699833003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2699833003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:51: base::WeakPtrFactory<ArcTraceBridge> weak_ptr_factory_; nit: can you add a comment like this? // NOTE: Weak pointers must be invalidated before all other member variables.
https://codereview.chromium.org/2699833003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:54: Category{category, std::string(kCategoryPrefix) + category}); On 2017/03/06 02:09:13, Yusuke Sato wrote: > nit: std::string() seems redundant as |category| is already a std::string > object. Done. https://codereview.chromium.org/2699833003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2699833003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:51: base::WeakPtrFactory<ArcTraceBridge> weak_ptr_factory_; On 2017/03/06 02:09:13, Yusuke Sato wrote: > nit: can you add a comment like this? > > // NOTE: Weak pointers must be invalidated before all other member variables. Done.
shunhsingou@google.com changed reviewers: + dcheng@chromium.org, dsinclair@chromium.org
Add reviewer OWNERS dcheng@ for changes in Mojo IPC, and dsinclair@ for changes in content/browser/trace. Hi dcheng@, dsinclair@, This change is for enabling ARC++ tracing from chrome://tracing. Could you help to review the related files? Thanks!
https://codereview.chromium.org/2699833003/diff/120001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/120001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:306: #if defined(OS_CHROMEOS) Why not put this with the if defined(OS_CHROMEOS) block at line 296? That also contains the if (tracing_config.IsSYsTraceEnabled()) check as well.
lgtm
https://codereview.chromium.org/2699833003/diff/120001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/120001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:306: #if defined(OS_CHROMEOS) On 2017/03/06 14:25:59, dsinclair wrote: > Why not put this with the if defined(OS_CHROMEOS) block at line 296? That also > contains the if (tracing_config.IsSYsTraceEnabled()) check as well. Done.
shunhsingou@google.com changed reviewers: + derat@chromium.org
Add derat@ as OWNERS for chromeos/ Hi derat@, Could you help to review the related files? Thanks!
https://codereview.chromium.org/2699833003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:90: callback.Run(false); same comment as in later file: this callback should probably be run asynchronously too https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:411: 'trace/arc_trace_agent.cc', why does this live under //chromeos instead of in //components/arc? creating a new trace/ subdirectory also seems strange unless you're planning to put more things here in the future. https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.cc:58: callback.Run(GetTracingAgentName(), GetTraceEventLabel(), this interface seems surprising. why is the callback being invoked synchronously here? StartAgentTracing runs its callback asynchronously. https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:23: using StartTracingCallback = base::Callback<void(bool)>; document parameters https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:28: virtual void StartTracing( please add method-level comments documenting what these methods do https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:35: virtual ~ArcTraceAgent(); use 'override' instead of 'virtual' here, i think https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:37: static ArcTraceAgent* GetInstance(); does this actually need to be a singleton? if not, clear ownership is better. https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:42: virtual void SetDelegate(Delegate* delegate) = 0; if ownership of |delegate| is transferred, it should be a std::unique_ptr. if it isn't, please document that https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:43: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:694: // For other trace data, quoted as JSON string and merge them into nit: "Quote other trace data as JSON strings and merge them into ..." https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:695: // |trace_data_sink|. nit: |trace_data_sink_| (with trailing underscore)
content/browser/tracing LGTM with derat@'s comments.
https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:411: 'trace/arc_trace_agent.cc', On 2017/03/07 15:53:21, Daniel Erat wrote: > why does this live under //chromeos instead of in //components/arc? creating a > new trace/ subdirectory also seems strange unless you're planning to put more > things here in the future. unless i'm misreading the code, this new class doesn't have dependencies on anything besides //base, and it's only used by //content and //chrome (i.e. not //components/arc). if that's correct, would it be possible to just put it somewhere within //content? or do you anticipate adding more dependencies later? yusuke, i think that the location in //chromeos came from your suggestion at https://codereview.chromium.org/2400163003/ -- can you comment here? https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:19: class CHROMEOS_EXPORT ArcTraceAgent : public base::trace_event::TracingAgent { this should probably be named ArcTracingAgent instead to match the interface name and other implementations. https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:579: auto* arc_trace_agent = chromeos::ArcTraceAgent::GetInstance(); this pattern feels a bit weird to me. it looks like we always instantiate this singleton, even if arc isn't enabled. the tracing agent name is static, and i think it's also duplicated in the kArcTracingAgentName constant declared above. why does GetTracingAgentName() need to be called here?
https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:411: 'trace/arc_trace_agent.cc', On 2017/03/08 02:58:40, Daniel Erat wrote: > On 2017/03/07 15:53:21, Daniel Erat wrote: > > why does this live under //chromeos instead of in //components/arc? creating a > > new trace/ subdirectory also seems strange unless you're planning to put more > > things here in the future. > > unless i'm misreading the code, this new class doesn't have dependencies on > anything besides //base, and it's only used by //content and //chrome (i.e. not > //components/arc). if that's correct, would it be possible to just put it > somewhere within //content? or do you anticipate adding more dependencies later? > > yusuke, i think that the location in //chromeos came from your suggestion at > https://codereview.chromium.org/2400163003/ -- can you comment here? Looks OK for me. 1. There is /content/browser/android, and the controller for Chrome tracing on Android is there. Does it make sense to create /content/browser/arc and put these two files there? 2. Or maybe we can just put arc_trace_agent.h in /content/browser/tracing, similar to the other tracing agents, e.g., power_tracing_agent, etw_tracing_agent. The solution 2 seems better for me, but would like to hear the comments from reviewers here.
https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:411: 'trace/arc_trace_agent.cc', On 2017/03/08 03:18:08, Earl Ou wrote: > On 2017/03/08 02:58:40, Daniel Erat wrote: > > On 2017/03/07 15:53:21, Daniel Erat wrote: > > > why does this live under //chromeos instead of in //components/arc? creating > a > > > new trace/ subdirectory also seems strange unless you're planning to put > more > > > things here in the future. > > > > unless i'm misreading the code, this new class doesn't have dependencies on > > anything besides //base, and it's only used by //content and //chrome (i.e. > not > > //components/arc). if that's correct, would it be possible to just put it > > somewhere within //content? or do you anticipate adding more dependencies > later? > > > > yusuke, i think that the location in //chromeos came from your suggestion at > > https://codereview.chromium.org/2400163003/ -- can you comment here? > > Looks OK for me. > > 1. There is /content/browser/android, and the controller for Chrome tracing on > Android is there. Does it make sense to create /content/browser/arc and put > these two files there? or maybe /content/browser/chromeos? > 2. Or maybe we can just put arc_trace_agent.h in /content/browser/tracing, > similar to the other tracing agents, e.g., power_tracing_agent, > etw_tracing_agent. yeah, i think that this sounds slightly better to me too if there are already other agents there. > The solution 2 seems better for me, but would like to hear the comments from > reviewers here.
https://codereview.chromium.org/2699833003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:90: callback.Run(false); On 2017/03/07 15:53:21, Daniel Erat wrote: > same comment as in later file: this callback should probably be run > asynchronously too Done. https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.cc:58: callback.Run(GetTracingAgentName(), GetTraceEventLabel(), On 2017/03/07 15:53:21, Daniel Erat wrote: > this interface seems surprising. why is the callback being invoked synchronously > here? StartAgentTracing runs its callback asynchronously. Changed to PostTask now. https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:19: class CHROMEOS_EXPORT ArcTraceAgent : public base::trace_event::TracingAgent { On 2017/03/08 02:58:40, Daniel Erat wrote: > this should probably be named ArcTracingAgent instead to match the interface > name and other implementations. Done. https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:23: using StartTracingCallback = base::Callback<void(bool)>; On 2017/03/07 15:53:22, Daniel Erat wrote: > document parameters Done. https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:28: virtual void StartTracing( On 2017/03/07 15:53:21, Daniel Erat wrote: > please add method-level comments documenting what these methods do Done. https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:35: virtual ~ArcTraceAgent(); On 2017/03/07 15:53:22, Daniel Erat wrote: > use 'override' instead of 'virtual' here, i think We actually implement this class in a subclass, and use pointer ArcTraceAgent* to own the subclass. So we should need 'virtual' here? https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:37: static ArcTraceAgent* GetInstance(); On 2017/03/07 15:53:21, Daniel Erat wrote: > does this actually need to be a singleton? if not, clear ownership is better. This is related to the dependency issue. The instance should be owned by ArcService, but the dependency between content/ and arc is not allowed. That's why we're using singleton here. https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:42: virtual void SetDelegate(Delegate* delegate) = 0; On 2017/03/07 15:53:21, Daniel Erat wrote: > if ownership of |delegate| is transferred, it should be a std::unique_ptr. if > it isn't, please document that It's owned by ArcServiceLauncher, I added comments here. https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:43: }; On 2017/03/07 15:53:22, Daniel Erat wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:579: auto* arc_trace_agent = chromeos::ArcTraceAgent::GetInstance(); On 2017/03/08 02:58:40, Daniel Erat wrote: > this pattern feels a bit weird to me. it looks like we always instantiate this > singleton, even if arc isn't enabled. the tracing agent name is static, and i > think it's also duplicated in the kArcTracingAgentName constant declared above. > why does GetTracingAgentName() need to be called here? We always instantiate this as we don't know if there is arc or not (otherwise we need dependency on /components/arc or /chrome/browser/chromeos/arc). For calling GetTracingAgentName(), true that it's weird, but all other tracing agent applies the same pattern here, e.g., debug_deamon, etw_agent, power_agent, so I just keep it consistent. https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:694: // For other trace data, quoted as JSON string and merge them into On 2017/03/07 15:53:22, Daniel Erat wrote: > nit: "Quote other trace data as JSON strings and merge them into ..." Done. https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:695: // |trace_data_sink|. On 2017/03/07 15:53:22, Daniel Erat wrote: > nit: |trace_data_sink_| (with trailing underscore) Done.
https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:35: virtual ~ArcTraceAgent(); On 2017/03/08 05:03:04, Earl Ou wrote: > On 2017/03/07 15:53:22, Daniel Erat wrote: > > use 'override' instead of 'virtual' here, i think > > We actually implement this class in a subclass, and use pointer ArcTraceAgent* > to own the subclass. So we should need 'virtual' here? i think it should have 'override' since it's overriding TracingAgent's d'tor. https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:579: auto* arc_trace_agent = chromeos::ArcTraceAgent::GetInstance(); On 2017/03/08 05:03:04, Earl Ou wrote: > On 2017/03/08 02:58:40, Daniel Erat wrote: > > this pattern feels a bit weird to me. it looks like we always instantiate this > > singleton, even if arc isn't enabled. the tracing agent name is static, and i > > think it's also duplicated in the kArcTracingAgentName constant declared > above. > > why does GetTracingAgentName() need to be called here? > > We always instantiate this as we don't know if there is arc or not (otherwise we > need dependency on /components/arc or /chrome/browser/chromeos/arc). > > For calling GetTracingAgentName(), true that it's weird, but all other tracing > agent applies the same pattern here, e.g., debug_deamon, etw_agent, power_agent, > so I just keep it consistent. hmm. i'm not an expert on tracing, but does this method only get called with an agent_name of "arc" if ARC tracing is explicitly requested? i'm just wondering why this pattern exists, and why this method doesn't instead use the string constants defined within this file to avoid instantiating agents unnecessarily.
https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:579: auto* arc_trace_agent = chromeos::ArcTraceAgent::GetInstance(); On 2017/03/08 05:21:07, Daniel Erat wrote: > On 2017/03/08 05:03:04, Earl Ou wrote: > > On 2017/03/08 02:58:40, Daniel Erat wrote: > > > this pattern feels a bit weird to me. it looks like we always instantiate > this > > > singleton, even if arc isn't enabled. the tracing agent name is static, and > i > > > think it's also duplicated in the kArcTracingAgentName constant declared > > above. > > > why does GetTracingAgentName() need to be called here? > > > > We always instantiate this as we don't know if there is arc or not (otherwise > we > > need dependency on /components/arc or /chrome/browser/chromeos/arc). > > > > For calling GetTracingAgentName(), true that it's weird, but all other tracing > > agent applies the same pattern here, e.g., debug_deamon, etw_agent, > power_agent, > > so I just keep it consistent. > > hmm. i'm not an expert on tracing, but does this method only get called with an > agent_name of "arc" if ARC tracing is explicitly requested? i'm just wondering > why this pattern exists, and why this method doesn't instead use the string > constants defined within this file to avoid instantiating agents unnecessarily. This function is called in the callback of StartTracingAgent, so indeed it's called with 'arc' if we previously call ArcTracingAgent::StartTracingAgent(...). I think we can just use the string const to avoid instantiating here. The reason is just for consistency, as other trace agents might not have the similar constant in this file.
https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:411: 'trace/arc_trace_agent.cc', On 2017/03/08 04:58:06, Daniel Erat wrote: > On 2017/03/08 03:18:08, Earl Ou wrote: > > On 2017/03/08 02:58:40, Daniel Erat wrote: > > > On 2017/03/07 15:53:21, Daniel Erat wrote: > > > > why does this live under //chromeos instead of in //components/arc? > creating > > a > > > > new trace/ subdirectory also seems strange unless you're planning to put > > more > > > > things here in the future. > > > > > > unless i'm misreading the code, this new class doesn't have dependencies on > > > anything besides //base, and it's only used by //content and //chrome (i.e. > > not > > > //components/arc). if that's correct, would it be possible to just put it > > > somewhere within //content? or do you anticipate adding more dependencies > > later? > > > > > > yusuke, i think that the location in //chromeos came from your suggestion at > > > https://codereview.chromium.org/2400163003/ -- can you comment here? > > > > Looks OK for me. > > > > 1. There is /content/browser/android, and the controller for Chrome tracing on > > Android is there. Does it make sense to create /content/browser/arc and put > > these two files there? > > or maybe /content/browser/chromeos? > > > 2. Or maybe we can just put arc_trace_agent.h in /content/browser/tracing, > > similar to the other tracing agents, e.g., power_tracing_agent, > > etw_tracing_agent. > > yeah, i think that this sounds slightly better to me too if there are already > other agents there. > > > The solution 2 seems better for me, but would like to hear the comments from > > reviewers here. > I made the suggestion just because I wasn't really sure if content/ OWNERS were okay with having ARC specific files there (currently there's zero arc_*.{cc,h} files in content/.) If they're okay, I have no objection to the plan to move the files to content/.
On 2017/03/08 07:17:45, Yusuke Sato wrote: > https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp > File chromeos/chromeos.gyp (right): > > https://codereview.chromium.org/2699833003/diff/140001/chromeos/chromeos.gyp#... > chromeos/chromeos.gyp:411: 'trace/arc_trace_agent.cc', > On 2017/03/08 04:58:06, Daniel Erat wrote: > > On 2017/03/08 03:18:08, Earl Ou wrote: > > > On 2017/03/08 02:58:40, Daniel Erat wrote: > > > > On 2017/03/07 15:53:21, Daniel Erat wrote: > > > > > why does this live under //chromeos instead of in //components/arc? > > creating > > > a > > > > > new trace/ subdirectory also seems strange unless you're planning to put > > > more > > > > > things here in the future. > > > > > > > > unless i'm misreading the code, this new class doesn't have dependencies > on > > > > anything besides //base, and it's only used by //content and //chrome > (i.e. > > > not > > > > //components/arc). if that's correct, would it be possible to just put it > > > > somewhere within //content? or do you anticipate adding more dependencies > > > later? > > > > > > > > yusuke, i think that the location in //chromeos came from your suggestion > at > > > > https://codereview.chromium.org/2400163003/ -- can you comment here? > > > > > > Looks OK for me. > > > > > > 1. There is /content/browser/android, and the controller for Chrome tracing > on > > > Android is there. Does it make sense to create /content/browser/arc and put > > > these two files there? > > > > or maybe /content/browser/chromeos? > > > > > 2. Or maybe we can just put arc_trace_agent.h in /content/browser/tracing, > > > similar to the other tracing agents, e.g., power_tracing_agent, > > > etw_tracing_agent. > > > > yeah, i think that this sounds slightly better to me too if there are already > > other agents there. > > > > > The solution 2 seems better for me, but would like to hear the comments from > > > reviewers here. > > > > I made the suggestion just because I wasn't really sure if content/ OWNERS were > okay with having ARC specific files there (currently there's zero arc_*.{cc,h} > files in content/.) If they're okay, I have no objection to the plan to move the > files to content/. Hi dsinclair, Do you think it OK to put those files in /content/browser/tracing as there are already other tracing agents there?
https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2699833003/diff/140001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:35: virtual ~ArcTraceAgent(); On 2017/03/08 05:21:07, Daniel Erat wrote: > On 2017/03/08 05:03:04, Earl Ou wrote: > > On 2017/03/07 15:53:22, Daniel Erat wrote: > > > use 'override' instead of 'virtual' here, i think > > > > We actually implement this class in a subclass, and use pointer ArcTraceAgent* > > to own the subclass. So we should need 'virtual' here? > > i think it should have 'override' since it's overriding TracingAgent's d'tor. You're right. I need to take some c++ course :) https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2699833003/diff/140001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:579: auto* arc_trace_agent = chromeos::ArcTraceAgent::GetInstance(); On 2017/03/08 07:11:10, Earl Ou wrote: > On 2017/03/08 05:21:07, Daniel Erat wrote: > > On 2017/03/08 05:03:04, Earl Ou wrote: > > > On 2017/03/08 02:58:40, Daniel Erat wrote: > > > > this pattern feels a bit weird to me. it looks like we always instantiate > > this > > > > singleton, even if arc isn't enabled. the tracing agent name is static, > and > > i > > > > think it's also duplicated in the kArcTracingAgentName constant declared > > > above. > > > > why does GetTracingAgentName() need to be called here? > > > > > > We always instantiate this as we don't know if there is arc or not > (otherwise > > we > > > need dependency on /components/arc or /chrome/browser/chromeos/arc). > > > > > > For calling GetTracingAgentName(), true that it's weird, but all other > tracing > > > agent applies the same pattern here, e.g., debug_deamon, etw_agent, > > power_agent, > > > so I just keep it consistent. > > > > hmm. i'm not an expert on tracing, but does this method only get called with > an > > agent_name of "arc" if ARC tracing is explicitly requested? i'm just wondering > > why this pattern exists, and why this method doesn't instead use the string > > constants defined within this file to avoid instantiating agents > unnecessarily. > > This function is called in the callback of StartTracingAgent, so indeed it's > called with 'arc' if we previously call ArcTracingAgent::StartTracingAgent(...). > > I think we can just use the string const to avoid instantiating here. The reason > is just for consistency, as other trace agents might not have the similar > constant in this file. I changed this to use the constant instead of instantiating instance.
primiano@chromium.org changed reviewers: + primiano@chromium.org
+chiniforooshan to keep him in the loop, as he is mojoifying and servicifying the tracing controller.
https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:316: "arc/trace/arc_trace_bridge.cc", rename these to arc/tracing/arc_tracing_bridge? a "trace" is the thing that gets generated, but "tracing" is what these classes do. "tracing" seems more consistent with existing naming (see chrome/browser/tracing/, components/tracing/, services/tracing/, etc.). https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:19: constexpr char kCategoryPrefix[] = TRACE_DISABLED_BY_DEFAULT("android "); is the space at the end of this string intentional? https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:36: auto* trace_instance = ARC_GET_INSTANCE_FOR_METHOD( you probably should use an explicit type here instead of 'auto', since the type isn't obvious from reading the statement or surrounding code. https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:65: auto* trace_instance = same comment here about not using 'auto' https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:87: auto* trace_instance = and here https://codereview.chromium.org/2699833003/diff/190015/chromeos/trace/arc_tra... File chromeos/trace/arc_tracing_agent.h (right): https://codereview.chromium.org/2699833003/diff/190015/chromeos/trace/arc_tra... chromeos/trace/arc_tracing_agent.h:25: // success or fail. nit: s/fail/failure/ https://codereview.chromium.org/2699833003/diff/190015/chromeos/trace/arc_tra... chromeos/trace/arc_tracing_agent.h:28: // success or fail. nit: s/fail/failure/ https://codereview.chromium.org/2699833003/diff/190015/chromeos/trace/arc_tra... chromeos/trace/arc_tracing_agent.h:50: // |delegate| as it should be managed by |ArcServiceLauncher| nit: i'd just say "Ownership of |delegate| remains with the caller." so this comment doesn't get out of date if/when the delegate is moved in the future. (also, referring to this outside class here is a bit of a layering violation.)
https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:316: "arc/trace/arc_trace_bridge.cc", On 2017/03/08 15:41:13, Daniel Erat wrote: > rename these to arc/tracing/arc_tracing_bridge? > > a "trace" is the thing that gets generated, but "tracing" is what these classes > do. "tracing" seems more consistent with existing naming (see > chrome/browser/tracing/, components/tracing/, services/tracing/, etc.). Done. I also change mojom interface to tracing.mojo, TracingInstance etc. On Android side, it will still be arctraceservice instead of arctracingservice as we have 16 character limit for service name. https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:19: constexpr char kCategoryPrefix[] = TRACE_DISABLED_BY_DEFAULT("android "); On 2017/03/08 15:41:13, Daniel Erat wrote: > is the space at the end of this string intentional? Yes. I leave a comment for this now. https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:36: auto* trace_instance = ARC_GET_INSTANCE_FOR_METHOD( On 2017/03/08 15:41:13, Daniel Erat wrote: > you probably should use an explicit type here instead of 'auto', since the type > isn't obvious from reading the statement or surrounding code. Looks like it's a convention in arc/ for using auto* here, but I'm OK to use explicit type here. https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:65: auto* trace_instance = On 2017/03/08 15:41:13, Daniel Erat wrote: > same comment here about not using 'auto' Done https://codereview.chromium.org/2699833003/diff/190015/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:87: auto* trace_instance = On 2017/03/08 15:41:13, Daniel Erat wrote: > and here Done. https://codereview.chromium.org/2699833003/diff/190015/chromeos/trace/arc_tra... File chromeos/trace/arc_tracing_agent.h (right): https://codereview.chromium.org/2699833003/diff/190015/chromeos/trace/arc_tra... chromeos/trace/arc_tracing_agent.h:25: // success or fail. On 2017/03/08 15:41:14, Daniel Erat wrote: > nit: s/fail/failure/ Done. https://codereview.chromium.org/2699833003/diff/190015/chromeos/trace/arc_tra... chromeos/trace/arc_tracing_agent.h:28: // success or fail. On 2017/03/08 15:41:14, Daniel Erat wrote: > nit: s/fail/failure/ Done. https://codereview.chromium.org/2699833003/diff/190015/chromeos/trace/arc_tra... chromeos/trace/arc_tracing_agent.h:50: // |delegate| as it should be managed by |ArcServiceLauncher| On 2017/03/08 15:41:14, Daniel Erat wrote: > nit: i'd just say "Ownership of |delegate| remains with the caller." so this > comment doesn't get out of date if/when the delegate is moved in the future. > (also, referring to this outside class here is a bit of a layering violation.) Done.
dsinclair@chromium.org changed reviewers: + oysteine@chromium.org
Keeping the tracing agents together makes sense to me. Adding oysteine@ in case there is something I'm missing.
On 2017/03/09 at 14:42:54, dsinclair wrote: > Keeping the tracing agents together makes sense to me. Adding oysteine@ in case there is something I'm missing. Yeah this seems consistent with the current ETW agent, though it's getting to the point where some cleanup of all the platform-specific stuff would be good (not related to this CL however). lgtm.
mojo lgtm https://codereview.chromium.org/2699833003/diff/210001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/210001/chrome/browser/chromeo... chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:55: categories_.push_back(Category{category, kCategoryPrefix + category}); Nit: Omit Category (that way it's clear this isn't a ctor) https://codereview.chromium.org/2699833003/diff/210001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h (right): https://codereview.chromium.org/2699833003/diff/210001/chrome/browser/chromeo... chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h:54: // so it must be the last member. Nit: I don't think this comment is too useful (as the build will break if this isn't true) https://codereview.chromium.org/2699833003/diff/210001/chromeos/tracing/arc_t... File chromeos/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2699833003/diff/210001/chromeos/tracing/arc_t... chromeos/tracing/arc_tracing_agent.cc:23: LOG_IF(WARNING, !success) << "Failed to stop ARC tracing."; DLOG?
https://codereview.chromium.org/2699833003/diff/210001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc (right): https://codereview.chromium.org/2699833003/diff/210001/chrome/browser/chromeo... chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:55: categories_.push_back(Category{category, kCategoryPrefix + category}); On 2017/03/10 22:09:24, dcheng wrote: > Nit: Omit Category (that way it's clear this isn't a ctor) Done. https://codereview.chromium.org/2699833003/diff/210001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h (right): https://codereview.chromium.org/2699833003/diff/210001/chrome/browser/chromeo... chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h:54: // so it must be the last member. On 2017/03/10 22:09:24, dcheng wrote: > Nit: I don't think this comment is too useful (as the build will break if this > isn't true) In my local machine, the build will still success if the order is not correct, and also many files in this codebase have this type of comments to avoid mistake in the future modification. I think it's still worth to keep this. https://codereview.chromium.org/2699833003/diff/210001/chromeos/tracing/arc_t... File chromeos/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2699833003/diff/210001/chromeos/tracing/arc_t... chromeos/tracing/arc_tracing_agent.cc:23: LOG_IF(WARNING, !success) << "Failed to stop ARC tracing."; On 2017/03/10 22:09:24, dcheng wrote: > DLOG? Done.
The CQ bit was checked by shunhsingou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, dsinclair@chromium.org, dcheng@chromium.org, oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/2699833003/#ps230001 (title: "Fix according to comments")
The CQ bit was unchecked by shunhsingou@chromium.org
The CQ bit was checked by shunhsingou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by shunhsingou@chromium.org
you're still planning to move this from //chromeos/tracing to somewhere under //content, right?
On 2017/03/13 02:12:58, Daniel Erat wrote: > you're still planning to move this from //chromeos/tracing to somewhere under > //content, right? Yes
On 2017/03/13 02:15:19, Earl Ou wrote: > On 2017/03/13 02:12:58, Daniel Erat wrote: > > you're still planning to move this from //chromeos/tracing to somewhere under > > //content, right? > > Yes Done moving this. This also introduces a dependency in //chrome/browser/chromeos/arc/tracing/DEPS and changes in //content/browser/BUILD.gn
shunhsingou@google.com changed reviewers: + alexmos@chromium.org
Add alexmos@ for changes in //content/browser/BUILD.gn
thanks! lgtm with a suggestion https://codereview.chromium.org/2699833003/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h (right): https://codereview.chromium.org/2699833003/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h:41: struct Category { can you just forward-declare this struct here and move the definition to the .cc file?
content/browser/BUILD.gn LGTM
https://codereview.chromium.org/2699833003/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h (right): https://codereview.chromium.org/2699833003/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h:41: struct Category { On 2017/03/13 14:46:48, Daniel Erat wrote: > can you just forward-declare this struct here and move the definition to the .cc > file? Done.
The CQ bit was checked by shunhsingou@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Not sure why, but ChromeOS bot is not happy about the following code snippet:
struct Category {
const std::string name;
const std::string full_name:
}
...
std::vector<Category>.push_back({...,...})
../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:62:17: note: in
instantiation of member function 'std::vector<arc::ArcTracingBridge::Category,
std::allocator<arc::ArcTracingBridge::Category> >::push_back' requested here
categories_.push_back({category, kCategoryPrefix + category});
^
../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:27:21: note:
copy assignment operator of 'Category' is implicitly deleted because field
'name' has no copy assignment operator
const std::string name;
This looks pretty weird. Seems like our toolchain on trybots require the item in
container to be assignable, which is not really required in c++11?
The CQ bit was checked by shunhsingou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by shunhsingou@google.com
On 2017/03/14 09:05:24, Earl Ou wrote:
> Not sure why, but ChromeOS bot is not happy about the following code snippet:
>
> struct Category {
> const std::string name;
> const std::string full_name:
> }
> ...
>
> std::vector<Category>.push_back({...,...})
>
> ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:62:17: note:
in
> instantiation of member function 'std::vector<arc::ArcTracingBridge::Category,
> std::allocator<arc::ArcTracingBridge::Category> >::push_back' requested here
> categories_.push_back({category, kCategoryPrefix + category});
> ^
> ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:27:21: note:
> copy assignment operator of 'Category' is implicitly deleted because field
> 'name' has no copy assignment operator
> const std::string name;
>
> This looks pretty weird. Seems like our toolchain on trybots require the item
in
> container to be assignable, which is not really required in c++11?
does categories_.emplace_back(category, kCategoryPrefix + category); work?
On 2017/03/14 14:41:55, Daniel Erat wrote:
> On 2017/03/14 09:05:24, Earl Ou wrote:
> > Not sure why, but ChromeOS bot is not happy about the following code
snippet:
> >
> > struct Category {
> > const std::string name;
> > const std::string full_name:
> > }
> > ...
> >
> > std::vector<Category>.push_back({...,...})
> >
> > ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:62:17: note:
> in
> > instantiation of member function
'std::vector<arc::ArcTracingBridge::Category,
> > std::allocator<arc::ArcTracingBridge::Category> >::push_back' requested here
> > categories_.push_back({category, kCategoryPrefix + category});
> > ^
> > ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:27:21: note:
> > copy assignment operator of 'Category' is implicitly deleted because field
> > 'name' has no copy assignment operator
> > const std::string name;
> >
> > This looks pretty weird. Seems like our toolchain on trybots require the
item
> in
> > container to be assignable, which is not really required in c++11?
>
> does categories_.emplace_back(category, kCategoryPrefix + category); work?
No, it doesn't work...
On 2017/03/14 14:46:58, Earl Ou wrote:
> On 2017/03/14 14:41:55, Daniel Erat wrote:
> > On 2017/03/14 09:05:24, Earl Ou wrote:
> > > Not sure why, but ChromeOS bot is not happy about the following code
> snippet:
> > >
> > > struct Category {
> > > const std::string name;
> > > const std::string full_name:
> > > }
> > > ...
> > >
> > > std::vector<Category>.push_back({...,...})
> > >
> > > ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:62:17:
note:
> > in
> > > instantiation of member function
> 'std::vector<arc::ArcTracingBridge::Category,
> > > std::allocator<arc::ArcTracingBridge::Category> >::push_back' requested
here
> > > categories_.push_back({category, kCategoryPrefix + category});
> > > ^
> > > ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:27:21:
note:
> > > copy assignment operator of 'Category' is implicitly deleted because field
> > > 'name' has no copy assignment operator
> > > const std::string name;
> > >
> > > This looks pretty weird. Seems like our toolchain on trybots require the
> item
> > in
> > > container to be assignable, which is not really required in c++11?
> >
> > does categories_.emplace_back(category, kCategoryPrefix + category); work?
>
> No, it doesn't work...
hmm. well, presumably you can just make the members non-const as a workaround.
maybe others will have more suggestions.
On 2017/03/14 14:48:33, Daniel Erat wrote:
> On 2017/03/14 14:46:58, Earl Ou wrote:
> > On 2017/03/14 14:41:55, Daniel Erat wrote:
> > > On 2017/03/14 09:05:24, Earl Ou wrote:
> > > > Not sure why, but ChromeOS bot is not happy about the following code
> > snippet:
> > > >
> > > > struct Category {
> > > > const std::string name;
> > > > const std::string full_name:
> > > > }
> > > > ...
> > > >
> > > > std::vector<Category>.push_back({...,...})
> > > >
> > > > ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:62:17:
> note:
> > > in
> > > > instantiation of member function
> > 'std::vector<arc::ArcTracingBridge::Category,
> > > > std::allocator<arc::ArcTracingBridge::Category> >::push_back' requested
> here
> > > > categories_.push_back({category, kCategoryPrefix + category});
> > > > ^
> > > > ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:27:21:
> note:
> > > > copy assignment operator of 'Category' is implicitly deleted because
field
> > > > 'name' has no copy assignment operator
> > > > const std::string name;
> > > >
> > > > This looks pretty weird. Seems like our toolchain on trybots require the
> > item
> > > in
> > > > container to be assignable, which is not really required in c++11?
> > >
> > > does categories_.emplace_back(category, kCategoryPrefix + category); work?
> >
> > No, it doesn't work...
>
> hmm. well, presumably you can just make the members non-const as a workaround.
> maybe others will have more suggestions.
Yes. I'm trying to get some suggestion from toolchain team. Will probably wait
one more day to see if there is better solution. If not, will remove "const" as
a simple workaround.
On 2017/03/14 14:53:21, Earl Ou wrote:
> On 2017/03/14 14:48:33, Daniel Erat wrote:
> > On 2017/03/14 14:46:58, Earl Ou wrote:
> > > On 2017/03/14 14:41:55, Daniel Erat wrote:
> > > > On 2017/03/14 09:05:24, Earl Ou wrote:
> > > > > Not sure why, but ChromeOS bot is not happy about the following code
> > > snippet:
> > > > >
> > > > > struct Category {
> > > > > const std::string name;
> > > > > const std::string full_name:
> > > > > }
> > > > > ...
> > > > >
> > > > > std::vector<Category>.push_back({...,...})
> > > > >
> > > > > ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:62:17:
> > note:
> > > > in
> > > > > instantiation of member function
> > > 'std::vector<arc::ArcTracingBridge::Category,
> > > > > std::allocator<arc::ArcTracingBridge::Category> >::push_back'
requested
> > here
> > > > > categories_.push_back({category, kCategoryPrefix + category});
> > > > > ^
> > > > > ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:27:21:
> > note:
> > > > > copy assignment operator of 'Category' is implicitly deleted because
> field
> > > > > 'name' has no copy assignment operator
> > > > > const std::string name;
> > > > >
> > > > > This looks pretty weird. Seems like our toolchain on trybots require
the
> > > item
> > > > in
> > > > > container to be assignable, which is not really required in c++11?
> > > >
> > > > does categories_.emplace_back(category, kCategoryPrefix + category);
work?
> > >
> > > No, it doesn't work...
> >
> > hmm. well, presumably you can just make the members non-const as a
workaround.
> > maybe others will have more suggestions.
>
> Yes. I'm trying to get some suggestion from toolchain team. Will probably wait
> one more day to see if there is better solution. If not, will remove "const"
as
> a simple workaround.
According to the build logs, the builder used libstdc++-4.6, which doesn't
support full c++11:
...
In file included from
../../build/linux/ubuntu_precise_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/vector:70:
...
The problem can be reproduced with gcc-4.6, although the builder used clang
bundled in chromium:
$ g++-4.6 -c test.cc
...
/usr/include/c++/4.6/bits/vector.tcc:319:4: error: use of deleted function
‘Category& Category::operator=(const Category&)’
...
where test.cc:
...
std::vector<Category>().push_back({"x", "y"});
...
I'm not sure why the builder targets at ubuntu-precise (12.04). It seems to me
that it could be updated somehow.
Typo: $ g++-4.6 -c test.cc should be $ g++-4.6 -std=c++0x -c test.cc
The CQ bit was checked by shunhsingou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shunhsingou@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org, lhchavez@chromium.org, dcheng@chromium.org, oysteine@chromium.org, derat@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2699833003/#ps310001 (title: "Compiling error workaround")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 310001, "attempt_start_ts": 1489550033345890,
"parent_rev": "d9ae71b54435ce738e3e7d3717a42183bbea811c", "commit_rev":
"0b110db84a13d9087ddf55b5e99d961e54f55e16"}
Message was sent while issue was closed.
Description was changed from ========== arc: enable Android tracing from chrome://tracing in dev mode To enable tracing from chrome://tracing, we create a Mojo interface for host side to trigger tracing in Android. The trace is written into debugfs/tracefs in dev-mode, and is read back by debugd. Related changes: - http://crrev.com/2699833003 - http://ag/1915548 BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android tracing from chrome://tracing in dev mode To enable tracing from chrome://tracing, we create a Mojo interface for host side to trigger tracing in Android. The trace is written into debugfs/tracefs in dev-mode, and is read back by debugd. Related changes: - http://crrev.com/2699833003 - http://ag/1915548 BUG=653795 TEST=Run chrome://tracing and see events from Android. Review-Url: https://codereview.chromium.org/2699833003 Cr-Commit-Position: refs/heads/master@{#456985} Committed: https://chromium.googlesource.com/chromium/src/+/0b110db84a13d9087ddf55b5e99d... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:310001) as https://chromium.googlesource.com/chromium/src/+/0b110db84a13d9087ddf55b5e99d...
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:310001) has been created in https://codereview.chromium.org/2747363002/ by tzik@chromium.org. The reason for reverting is: This CL seems to cause a compile failure on the CI. The error log is: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FLi... [10030/10041] LINK ./chrome_app_unittests FAILED: chrome_app_unittests (snip) ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:35: error: undefined reference to 'content::ArcTracingAgent::GetInstance()' ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:39: error: undefined reference to 'content::ArcTracingAgent::GetInstance()' ../../chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:41: error: undefined reference to 'content::ArcTracingAgent::Delegate::~Delegate()' ../../content/browser/tracing/arc_tracing_agent.h:21: error: undefined reference to 'vtable for content::ArcTracingAgent::Delegate' ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: the vtable symbol may be undefined because the class is missing its key function clang: error: linker command failed with exit code 1 (use -v to see invocation).
Message was sent while issue was closed.
tzik@chromium.org changed reviewers: + tzik@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2699833003/diff/310001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.h (right): https://codereview.chromium.org/2699833003/diff/310001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.h:19: class ArcTracingAgent : public base::trace_event::TracingAgent { I think CONTENT_EXPORT is needed here.
Message was sent while issue was closed.
On 2017/03/15 05:16:32, tzik wrote: > https://codereview.chromium.org/2699833003/diff/310001/content/browser/tracin... > File content/browser/tracing/arc_tracing_agent.h (right): > > https://codereview.chromium.org/2699833003/diff/310001/content/browser/tracin... > content/browser/tracing/arc_tracing_agent.h:19: class ArcTracingAgent : public > base::trace_event::TracingAgent { > I think CONTENT_EXPORT is needed here. Oops. Thanks for catching this. I've uploaded a new CL with the fix: https://codereview.chromium.org/2749283003 BTW, is there any try bot I can use to avoid such error? |
