Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
786233003:
Add UpdateCurrentAppInfo() interface to record info about current app, including (Closed)
DescriptionAdd UpdateCurrentAppInfo() interface to record info about current app, including
app_id, session_id and sdk_version, which are used by MediaPlay/MediaPause
events.
BUG=
Committed: https://crrev.com/5a04a5880f32f1b6b59badb80d7e0f820eddb0b9
Cr-Commit-Position: refs/heads/master@{#308026}
Patch Set 1 #
Total comments: 13
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Messages
Total messages: 26 (6 generated)
gfhuang@chromium.org changed reviewers: + byungchul@chromium.org, gunsch@chromium.org
https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... File chromecast/base/metrics/cast_metrics_helper.cc (right): https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.cc:72: app_id_ = ""; app_id_.clear(); https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.cc:73: session_id_ = ""; ditto https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.cc:74: sdk_version_ = ""; ditto https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.cc:205: return action_name + "#" + app_id_ + "#" + session_id_ + "#" + sdk_version_; Please define a constant for delimiter, '#' for somewhere to parse it.
https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... File chromecast/base/metrics/cast_metrics_helper.cc (right): https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.cc:72: app_id_ = ""; On 2014/12/10 05:39:41, byungchul wrote: > app_id_.clear(); Alternately, you could use UpdateCurrentAppInfo("", "", "") --- this would prevent the two from accidentally getting out of sync in the future, since any API change would be forced to be updated https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.cc:205: return action_name + "#" + app_id_ + "#" + session_id_ + "#" + sdk_version_; On 2014/12/10 05:39:40, byungchul wrote: > Please define a constant for delimiter, '#' for somewhere to parse it. Aside: I saw that you're parsing this in a different location in an internal CL. Why not have the logic for both ways (serializing and deserializing) in this class, and pass in/return a struct with all three args? Then this awkward behavior is at least self-contained. https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... File chromecast/base/metrics/cast_metrics_helper.h (right): https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.h:56: // This function update the info of current active application. Remove "This function" "update" --> "Updates" "of" --> "for" https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.h:90: // This is used by actions, so far MediaPlay/MediaPause only. This comment is bound to become obsolete. Can you remove the reference to MediaPlay/MediaPause? Also, are "used by actions" and "used by histograms" restrictions? If so, can you encode them into the method names? If not, then you should remove the comments.
https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... File chromecast/base/metrics/cast_metrics_helper.cc (right): https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.cc:72: app_id_ = ""; On 2014/12/10 16:54:18, gunsch wrote: > On 2014/12/10 05:39:41, byungchul wrote: > > app_id_.clear(); > > Alternately, you could use UpdateCurrentAppInfo("", "", "") --- this would > prevent the two from accidentally getting out of sync in the future, since any > API change would be forced to be updated Done. https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.cc:205: return action_name + "#" + app_id_ + "#" + session_id_ + "#" + sdk_version_; On 2014/12/10 16:54:18, gunsch wrote: > On 2014/12/10 05:39:40, byungchul wrote: > > Please define a constant for delimiter, '#' for somewhere to parse it. > > Aside: I saw that you're parsing this in a different location in an internal CL. > Why not have the logic for both ways (serializing and deserializing) in this > class, and pass in/return a struct with all three args? Then this awkward > behavior is at least self-contained. Done. https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... File chromecast/base/metrics/cast_metrics_helper.h (right): https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.h:56: // This function update the info of current active application. On 2014/12/10 16:54:18, gunsch wrote: > Remove "This function" > > "update" --> "Updates" > > "of" --> "for" Done. https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.h:90: // This is used by actions, so far MediaPlay/MediaPause only. On 2014/12/10 16:54:19, gunsch wrote: > This comment is bound to become obsolete. Can you remove the reference to > MediaPlay/MediaPause? > > Also, are "used by actions" and "used by histograms" restrictions? If so, can > you encode them into the method names? If not, then you should remove the > comments. Done.
https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... File chromecast/base/metrics/cast_metrics_helper.h (right): https://codereview.chromium.org/786233003/diff/1/chromecast/base/metrics/cast... chromecast/base/metrics/cast_metrics_helper.h:91: virtual std::string GetMetricsNameWithAppInfo( does anyone call this externally? seems to me like it should be a private method https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... File chromecast/base/metrics/cast_metrics_helper.cc (right): https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.cc:49: // TODO(gfhuang): This is a hacky way to encode/decode app infos into a Got a planned approach for the TODO, or is this just an FYI? https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.cc:115: } style nit: blank line between methods https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... File chromecast/base/metrics/cast_metrics_helper.h (right): https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.h:59: static const char kMetricsNameAppInfoDelimiter; will this be referenced outside this file? I would hope not, if MetricsAppInfo is being both serialized/deserialized inside the impementation
https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... File chromecast/base/metrics/cast_metrics_helper.h (right): https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.h:36: ~MetricsAppInfo(); Not necessary https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.h:64: const std::string& metrics_name); Do we prefer structure copy? Why not get out param? and name should be Get(or Extract)MetricsAppInfoFromMetricsName?
https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... File chromecast/base/metrics/cast_metrics_helper.cc (right): https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.cc:49: // TODO(gfhuang): This is a hacky way to encode/decode app infos into a Change to a NOTE. https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.cc:115: } On 2014/12/10 22:43:58, gunsch wrote: > style nit: blank line between methods Done. https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... File chromecast/base/metrics/cast_metrics_helper.h (right): https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.h:36: ~MetricsAppInfo(); On 2014/12/11 00:15:26, byungchul wrote: > Not necessary Done. https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.h:59: static const char kMetricsNameAppInfoDelimiter; On 2014/12/10 22:43:58, gunsch wrote: > will this be referenced outside this file? I would hope not, if MetricsAppInfo > is being both serialized/deserialized inside the impementation Done. https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.h:64: const std::string& metrics_name); On 2014/12/11 00:15:26, byungchul wrote: > Do we prefer structure copy? Why not get out param? > > and name should be Get(or Extract)MetricsAppInfoFromMetricsName? Done.
ping
Nits only. https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... File chromecast/base/metrics/cast_metrics_helper.h (right): https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.h:64: const std::string& metrics_name); On 2014/12/11 02:59:39, gfhuang wrote: > On 2014/12/11 00:15:26, byungchul wrote: > > Do we prefer structure copy? Why not get out param? > > > > and name should be Get(or Extract)MetricsAppInfoFromMetricsName? > > Done. Though I meant passing the pointer of the structure, I am fine with this either. Jesse may have a different opinion. https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... File chromecast/base/metrics/cast_metrics_helper.cc (right): https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.cc:56: std::string* sdk_version) { DCHECK()'s for pointers. https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.cc:59: } remove {}
On 2014/12/11 18:35:49, byungchul wrote: > Nits only. > > https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... > File chromecast/base/metrics/cast_metrics_helper.h (right): > > https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... > chromecast/base/metrics/cast_metrics_helper.h:64: const std::string& > metrics_name); > On 2014/12/11 02:59:39, gfhuang wrote: > > On 2014/12/11 00:15:26, byungchul wrote: > > > Do we prefer structure copy? Why not get out param? > > > > > > and name should be Get(or Extract)MetricsAppInfoFromMetricsName? > > > > Done. > > Though I meant passing the pointer of the structure, I am fine with this either. > Jesse may have a different opinion. > > https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... > File chromecast/base/metrics/cast_metrics_helper.cc (right): > > https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... > chromecast/base/metrics/cast_metrics_helper.cc:56: std::string* sdk_version) { > DCHECK()'s for pointers. > > https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... > chromecast/base/metrics/cast_metrics_helper.cc:59: } > remove {} lgtm
lcwu@chromium.org changed reviewers: + lcwu@chromium.org
https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... File chromecast/base/metrics/cast_metrics_helper.cc (right): https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.cc:62: DCHECK_EQ(tokens.size(), 4u); If the number of the tokens is not 4, which means the metrics_name was probably not encoded by EncodeAppInfoIntoMetricsName, then shouldn't you return false here based on your comment in the header file?
https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... File chromecast/base/metrics/cast_metrics_helper.h (right): https://codereview.chromium.org/786233003/diff/20001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.h:64: const std::string& metrics_name); On 2014/12/11 18:35:49, byungchul wrote: > On 2014/12/11 02:59:39, gfhuang wrote: > > On 2014/12/11 00:15:26, byungchul wrote: > > > Do we prefer structure copy? Why not get out param? > > > > > > and name should be Get(or Extract)MetricsAppInfoFromMetricsName? > > > > Done. > > Though I meant passing the pointer of the structure, I am fine with this either. > Jesse may have a different opinion. Ack. define struct has an extra burden to maintain, i think it's ok since only 4 simple fields. https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... File chromecast/base/metrics/cast_metrics_helper.cc (right): https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.cc:56: std::string* sdk_version) { On 2014/12/11 18:35:49, byungchul wrote: > DCHECK()'s for pointers. Done. https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.cc:59: } On 2014/12/11 18:35:49, byungchul wrote: > remove {} Done. https://codereview.chromium.org/786233003/diff/40001/chromecast/base/metrics/... chromecast/base/metrics/cast_metrics_helper.cc:62: DCHECK_EQ(tokens.size(), 4u); On 2014/12/11 18:41:57, lcwu1 wrote: > If the number of the tokens is not 4, which means the metrics_name was probably > not encoded by EncodeAppInfoIntoMetricsName, then shouldn't you return false > here based on your comment in the header file? If the metrics name has magic char '#', but tokens size if not 4, which means I screw something up. so I put DCHECK here to make sure I don't do something silly in the future, such as use the magic char in normal metric.
The CQ bit was checked by gfhuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/786233003/60001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Lechun/Jesse, still need a LG from one of you.
lgtm from me as you have addressed my question. Please also wait for Jesse's lgtm to make sure that his comments are all addressed.
lgtm l
The CQ bit was checked by gfhuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/786233003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5a04a5880f32f1b6b59badb80d7e0f820eddb0b9 Cr-Commit-Position: refs/heads/master@{#308026}
Message was sent while issue was closed.
Patchset #5 (id:80001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
