Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2886203002:
tracing: the recorder implementation (Closed)
|
Created:
3 years, 7 months ago by chiniforooshan Modified:
3 years, 7 months ago Reviewers:
kenrb, oystein (OOO til 10th of July), Primiano Tucci (use gerrit), Ken Rockot(use gerrit already) CC:
Aaron Boodman, abarth-chromium, chrome-grc-reviews_chromium.org, chromium-reviews, darin (slow to review), Primiano Tucci (use gerrit), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptiontracing: the recorder implementation
Recorders are created by the tracing::Coordinator and sent to the
agents at StartTracing. Agents should flush their trace events to
the recorder when then receive the StopTracing message.
A prototype of the final product (a little bit stale and incomplete):
https://codereview.chromium.org/2833873003/
Design doc (use @chromium.org account):
https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM
BUG=640235
Review-Url: https://codereview.chromium.org/2886203002
Cr-Commit-Position: refs/heads/master@{#474886}
Committed: https://chromium.googlesource.com/chromium/src/+/2e2b178d99399c687bc60adea7e6b2460c9cfca6
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 14
Patch Set 3 : review #
Total comments: 13
Patch Set 4 : review #Patch Set 5 : rebase #
Messages
Total messages: 31 (15 generated)
chiniforooshan@chromium.org changed reviewers: + oysteine@chromium.org
ptal
The CQ bit was checked by chiniforooshan@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...
chiniforooshan@chromium.org changed reviewers: + kenrb@chromium.org
Rebased after https://codereview.chromium.org/2878533003. Also, +Ken as discussed in https://codereview.chromium.org/2723773002.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/05/19 02:57:43, chiniforooshan wrote: > Rebased after https://codereview.chromium.org/2878533003. > > Also, +Ken as discussed in https://codereview.chromium.org/2723773002. pinging Oystein :)
primiano@chromium.org changed reviewers: + primiano@chromium.org
minor comments. looks good overall with minor comments but let's wait for oysteine to check if it fits properly into the GRC architecture? https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... File services/resource_coordinator/tracing/recorder.cc (right): https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.cc:25: &Recorder::OnConnectionError, base::Unretained(this))); Is this class going to be long lived? If yes, please add a documentational NOTREACHED() in the Recorder dtor (or at least a comment) If no, base::Unretained is unsafe here and you should use weakptr probably https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.cc:31: if (chunk.size() == 0) chunk.empty() https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... File services/resource_coordinator/tracing/recorder.h (right): https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:22: Recorder( can you add some comments to explain what these arguments are for? https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:24: bool is_array, data_is_array ? I understood what this was only at the end of the cl https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:25: const base::Closure& recorder_change_callback, Isn't this clearer if you call it on_data_change_callback? https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:47: // mojom::Recorder small¬-strong style thing: not sure if there is a guideline for this, I tend to prefer the mojo implementation methods to be in the public: section. I know they don't have to, but makes easier to read the class, as they are de-facto the main methods that the class is exposing. But honestly don't know what happens in other mojo implementaitons and I might be the outlier here? https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:48: void AddChunk(const std::string& chunk) override; Can you please add some comments to specify who adds chunk into what? Services are great for their loose code coupling, but at the same time that makes a bit harder to understand the end-to-end call chain. Maybe something like // These methods are called by ??? to ???
https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... File services/resource_coordinator/tracing/recorder.cc (right): https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.cc:25: &Recorder::OnConnectionError, base::Unretained(this))); On 2017/05/25 13:15:01, Primiano Tucci wrote: > Is this class going to be long lived? > If yes, please add a documentational NOTREACHED() in the Recorder dtor (or at > least a comment) > If no, base::Unretained is unsafe here and you should use weakptr probably It's not long lived, but it's deleted after the connection is closed (i.e. after base::Unretained is used). I made it a weak pointer. https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.cc:31: if (chunk.size() == 0) On 2017/05/25 13:15:01, Primiano Tucci wrote: > chunk.empty() Done. https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... File services/resource_coordinator/tracing/recorder.h (right): https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:22: Recorder( On 2017/05/25 13:15:01, Primiano Tucci wrote: > can you add some comments to explain what these arguments are for? Done. https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:24: bool is_array, On 2017/05/25 13:15:01, Primiano Tucci wrote: > data_is_array ? I understood what this was only at the end of the cl Done. https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:25: const base::Closure& recorder_change_callback, On 2017/05/25 13:15:01, Primiano Tucci wrote: > Isn't this clearer if you call it on_data_change_callback? Yes, although the callback is also run when the connection is closed, to signal the end of receiving data; but, that's kind of a data change, too :) Done. https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:47: // mojom::Recorder On 2017/05/25 13:15:01, Primiano Tucci wrote: > small¬-strong style thing: not sure if there is a guideline for this, I tend > to prefer the mojo implementation methods to be in the public: section. I know > they don't have to, but makes easier to read the class, as they are de-facto the > main methods that the class is exposing. > But honestly don't know what happens in other mojo implementaitons and I might > be the outlier here? There are examples of both in the code base. I cannot say which one is clearly more popular. My preference is private by default unless we need them to be public mainly for thread safety. When they are private, it's easier to see that they are called by Mojo internals only, and Mojo promises to call them from one thread only. If they are public, it's harder to prevent future random calls from random threads; we have to add a thread_checker or something. https://codereview.chromium.org/2886203002/diff/20001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:48: void AddChunk(const std::string& chunk) override; On 2017/05/25 13:15:01, Primiano Tucci wrote: > Can you please add some comments to specify who adds chunk into what? Services > are great for their loose code coupling, but at the same time that makes a bit > harder to understand the end-to-end call chain. > > Maybe something like > // These methods are called by ??? to ??? Done.
Eep, sorry for the delay. lgtm w/ comments https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/recorder.cc (right): https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.cc:29: Recorder::~Recorder() {} nit: = default; https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.cc:40: if (data_is_array_ && data_.size() > 0) nit: !data_.empty() https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/recorder.h (right): https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:16: class DictionaryValue; What's this? If was originally a forward declaration of base::DictionaryValue, I assume that's no longer needed as there s a full base::DictionaryValue member now (and "base/values.h" should probably be explicitly included) https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:38: Recorder( The convention is usually that the Mojo interface is mojom::Recorder and the actual implementation class would be "RecorderImpl", I think? https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:47: DCHECK(background_task_runner_->RunsTasksOnCurrentThread()); "base/logging.h" for these https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:82: DISALLOW_COPY_AND_ASSIGN(Recorder); nit: base/macros.h
LGMT % remaining oystein comments
Ken, could you please take a look? This implements one of the interfaces defined in services/resource_coordinator/public/interfaces/tracing/tracing.mojom. Thanks! https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/recorder.cc (right): https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.cc:29: Recorder::~Recorder() {} On 2017/05/25 16:55:26, oystein wrote: > nit: = default; Done. https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.cc:40: if (data_is_array_ && data_.size() > 0) On 2017/05/25 16:55:26, oystein wrote: > nit: !data_.empty() Done. https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/recorder.h (right): https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:16: class DictionaryValue; On 2017/05/25 16:55:26, oystein wrote: > What's this? If was originally a forward declaration of base::DictionaryValue, I > assume that's no longer needed as there > s a full base::DictionaryValue member now (and "base/values.h" should probably > be explicitly included) Done. https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:38: Recorder( On 2017/05/25 16:55:26, oystein wrote: > The convention is usually that the Mojo interface is mojom::Recorder and the > actual implementation class would be "RecorderImpl", I think? It looks like the convention is to name the implementation the same as the interface. For example, ui.ws.Gpu, ui.InputDeviceServer, file.FileSystem, bluetooth.Device, identity.IdentityManager, ... https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:47: DCHECK(background_task_runner_->RunsTasksOnCurrentThread()); On 2017/05/25 16:55:27, oystein wrote: > "base/logging.h" for these Done. https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:82: DISALLOW_COPY_AND_ASSIGN(Recorder); On 2017/05/25 16:55:26, oystein wrote: > nit: base/macros.h Done.
https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/recorder.h (right): https://codereview.chromium.org/2886203002/diff/40001/services/resource_coord... services/resource_coordinator/tracing/recorder.h:38: Recorder( On 2017/05/25 at 19:45:32, chiniforooshan wrote: > On 2017/05/25 16:55:26, oystein wrote: > > The convention is usually that the Mojo interface is mojom::Recorder and the > > actual implementation class would be "RecorderImpl", I think? > > It looks like the convention is to name the implementation the same as the interface. For example, ui.ws.Gpu, ui.InputDeviceServer, file.FileSystem, bluetooth.Device, identity.IdentityManager, ... Also ImageDecoderImpl, BatteryMonitorImpl, ClipboardImpl, DeviceFactoryProviderImpl, etc. Maybe the convention flipped from one to the other at some point, or it's arbitrarily chosen in the different services; hopefully rockot@ has some guidance there :)
chiniforooshan@chromium.org changed reviewers: + rockot@chromium.org
rockot@, do you know what the class name convention is, if there is any :), for
the implementation of an interface? Should we use the same name as the interface
or should add an Impl suffix? It looks like both styles are used a lot in the
code base. i.e.
class Recorder : public mojom::Recorder {
vs
class RecorderImpl : public mojom::Recorder {
I see no problem with Recorder. One of the main motivations for establishing the convention of inner mojom namespaces was to avoid having a million redundant Impl suffixes everywhere On Thu, May 25, 2017 at 1:23 PM, <chiniforooshan@chromium.org> wrote: > rockot@, do you know what the class name convention is, if there is any > :), for > the implementation of an interface? Should we use the same name as the > interface > or should add an Impl suffix? It looks like both styles are used a lot in > the > code base. i.e. > > class Recorder : public mojom::Recorder { > vs > class RecorderImpl : public mojom::Recorder { > > https://codereview.chromium.org/2886203002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
mojom lgtm
The CQ bit was checked by chiniforooshan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/2886203002/#ps60001 (title: "review")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chiniforooshan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/2886203002/#ps80001 (title: "rebase")
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": 80001, "attempt_start_ts": 1495749691176220,
"parent_rev": "5f7fd11473c746726e2e1917680a34acde058f19", "commit_rev":
"2e2b178d99399c687bc60adea7e6b2460c9cfca6"}
Message was sent while issue was closed.
Description was changed from ========== tracing: the recorder implementation Recorders are created by the tracing::Coordinator and sent to the agents at StartTracing. Agents should flush their trace events to the recorder when then receive the StopTracing message. A prototype of the final product (a little bit stale and incomplete): https://codereview.chromium.org/2833873003/ Design doc (use @chromium.org account): https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM BUG=640235 ========== to ========== tracing: the recorder implementation Recorders are created by the tracing::Coordinator and sent to the agents at StartTracing. Agents should flush their trace events to the recorder when then receive the StopTracing message. A prototype of the final product (a little bit stale and incomplete): https://codereview.chromium.org/2833873003/ Design doc (use @chromium.org account): https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM BUG=640235 Review-Url: https://codereview.chromium.org/2886203002 Cr-Commit-Position: refs/heads/master@{#474886} Committed: https://chromium.googlesource.com/chromium/src/+/2e2b178d99399c687bc60adea7e6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2e2b178d99399c687bc60adea7e6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
