Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
845003002:
[chromecast] ExternalMetrics has to destroy itself in file thread. (Closed)
Description(1) ExternalMetrics has to destroy itself in file thread due to weakptr.
(2) MetricsService has to stop before destruction of CastBrowserProcess, which is referenced inside MetricsService's stopping code.
Committed: https://crrev.com/827a380cfdb31aa54c8d56e63ce2c3fd8c3ba4d4
Cr-Commit-Position: refs/heads/master@{#310958}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : #
Total comments: 8
Patch Set 5 : ' #Patch Set 6 : #Patch Set 7 : #
Messages
Total messages: 26 (5 generated)
gfhuang@chromium.org changed reviewers: + byungchul@chromium.org, gunsch@chromium.org
https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/c... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/c... chromecast/browser/metrics/cast_metrics_service_client.cc:258: metrics_service_.reset(); what's the purpose of this call? won't this happen automatically shortly afterwards? https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/e... File chromecast/browser/metrics/external_metrics.cc (right): https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/e... chromecast/browser/metrics/external_metrics.cc:65: if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)) { Why does it need to be destroyed on the file thread? Is it because of the weak pointer factory? https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/e... File chromecast/browser/metrics/external_metrics.h (right): https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/e... chromecast/browser/metrics/external_metrics.h:33: // Destroy itself in appropriate thread. nit: "Destroy" --> "Destroys"
https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/c... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/c... chromecast/browser/metrics/cast_metrics_service_client.cc:258: metrics_service_.reset(); Changed to Stop() to be more clear. and edited CL description. https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/e... File chromecast/browser/metrics/external_metrics.cc (right): https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/e... chromecast/browser/metrics/external_metrics.cc:65: if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)) { On 2015/01/09 18:56:19, gunsch wrote: > Why does it need to be destroyed on the file thread? Is it because of the weak > pointer factory? Yes https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/e... File chromecast/browser/metrics/external_metrics.h (right): https://codereview.chromium.org/845003002/diff/1/chromecast/browser/metrics/e... chromecast/browser/metrics/external_metrics.h:33: // Destroy itself in appropriate thread. On 2015/01/09 18:56:19, gunsch wrote: > nit: "Destroy" --> "Destroys" Done.
https://codereview.chromium.org/845003002/diff/20001/chromecast/browser/metri... File chromecast/browser/metrics/external_metrics.cc (right): https://codereview.chromium.org/845003002/diff/20001/chromecast/browser/metri... chromecast/browser/metrics/external_metrics.cc:72: delete this; why not just: content::BrowserThread::DeleteSoon(content::BrowserThread::FILE, FROM_HERE, this); ? https://codereview.chromium.org/845003002/diff/20001/chromecast/browser/metri... File chromecast/browser/metrics/external_metrics.h (right): https://codereview.chromium.org/845003002/diff/20001/chromecast/browser/metri... chromecast/browser/metrics/external_metrics.h:34: void DestroySelf(); StopAndDestory?
https://codereview.chromium.org/845003002/diff/20001/chromecast/browser/metri... File chromecast/browser/metrics/external_metrics.cc (right): https://codereview.chromium.org/845003002/diff/20001/chromecast/browser/metri... chromecast/browser/metrics/external_metrics.cc:72: delete this; On 2015/01/09 21:23:24, byungchul wrote: > why not just: > > content::BrowserThread::DeleteSoon(content::BrowserThread::FILE, FROM_HERE, > this); > > ? Done. https://codereview.chromium.org/845003002/diff/20001/chromecast/browser/metri... File chromecast/browser/metrics/external_metrics.h (right): https://codereview.chromium.org/845003002/diff/20001/chromecast/browser/metri... chromecast/browser/metrics/external_metrics.h:34: void DestroySelf(); On 2015/01/09 21:23:24, byungchul wrote: > StopAndDestory? Done.
lgtm
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/845003002/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.
https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... chromecast/browser/metrics/cast_metrics_service_client.cc:256: external_metrics_.release()->StopAndSelfDestroy(); Thought: this mechanic is sort of strange. I think it would make more sense to do the following: 1) Make ~ExternalMetrics private (since no one should be calling it) 2) Make CastMetricsServiceClient have a ExternalMetrics* instead of a scoped_ptr<ExternalMetrics> what do you think? https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... File chromecast/browser/metrics/external_metrics.h (right): https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... chromecast/browser/metrics/external_metrics.h:34: void StopAndSelfDestroy(); "SelfDestroy" is sort of strange, why not just "StopAndDestroy" or "StopAndDestroySelf"
lcwu@chromium.org changed reviewers: + lcwu@chromium.org
https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... chromecast/browser/metrics/cast_metrics_service_client.cc:255: #if defined(OS_LINUX) In our case, !OS_ANDROID is equal to OS_LINUX, so you should be able to move the following code to the block above (i.e. below line 251).
https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... chromecast/browser/metrics/cast_metrics_service_client.cc:255: #if defined(OS_LINUX) I'd prefer not---RecordCompletedSessionEnd is actually declared with !OS_ANDROID, and ExternalMetrics is guarded with OS_LINUX due to Linux-specific dependencies. I agree that for us those two are equivalent, but it seems like an unnecessary assumption to make.
https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... chromecast/browser/metrics/cast_metrics_service_client.cc:255: #if defined(OS_LINUX) On 2015/01/10 00:42:23, gunsch wrote: > I'd prefer not---RecordCompletedSessionEnd is actually declared with > !OS_ANDROID, and ExternalMetrics is guarded with OS_LINUX due to Linux-specific > dependencies. > > I agree that for us those two are equivalent, but it seems like an unnecessary > assumption to make. Acknowledged. https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... chromecast/browser/metrics/cast_metrics_service_client.cc:256: external_metrics_.release()->StopAndSelfDestroy(); On 2015/01/10 00:31:46, gunsch wrote: > Thought: this mechanic is sort of strange. I think it would make more sense to > do the following: > > 1) Make ~ExternalMetrics private (since no one should be calling it) > 2) Make CastMetricsServiceClient have a ExternalMetrics* instead of a > scoped_ptr<ExternalMetrics> > > what do you think? Done. https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... File chromecast/browser/metrics/external_metrics.h (right): https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... chromecast/browser/metrics/external_metrics.h:34: void StopAndSelfDestroy(); On 2015/01/10 00:31:46, gunsch wrote: > "SelfDestroy" is sort of strange, why not just "StopAndDestroy" or > "StopAndDestroySelf" Done.
https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... chromecast/browser/metrics/cast_metrics_service_client.cc:256: external_metrics_.release()->StopAndSelfDestroy(); On 2015/01/10 00:58:30, gfhuang wrote: > On 2015/01/10 00:31:46, gunsch wrote: > > Thought: this mechanic is sort of strange. I think it would make more sense to > > do the following: > > > > 1) Make ~ExternalMetrics private (since no one should be calling it) > > 2) Make CastMetricsServiceClient have a ExternalMetrics* instead of a > > scoped_ptr<ExternalMetrics> > > > > what do you think? > > Done. Note that I have to revert back to use PostTask instead of DeleteSoon though, since dtor is private now.
On 2015/01/10 01:17:30, gfhuang wrote: > https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... > File chromecast/browser/metrics/cast_metrics_service_client.cc (right): > > https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... > chromecast/browser/metrics/cast_metrics_service_client.cc:256: > external_metrics_.release()->StopAndSelfDestroy(); > On 2015/01/10 00:58:30, gfhuang wrote: > > On 2015/01/10 00:31:46, gunsch wrote: > > > Thought: this mechanic is sort of strange. I think it would make more sense > to > > > do the following: > > > > > > 1) Make ~ExternalMetrics private (since no one should be calling it) > > > 2) Make CastMetricsServiceClient have a ExternalMetrics* instead of a > > > scoped_ptr<ExternalMetrics> > > > > > > what do you think? > > > > Done. > > Note that I have to revert back to use PostTask instead of DeleteSoon though, > since dtor is private now. Why not add "friend class base::DeleteHelper<ExternalMetrics>;" ? That seems to be a common pattern: https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...
On 2015/01/10 01:23:27, gunsch wrote: > On 2015/01/10 01:17:30, gfhuang wrote: > > > https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... > > File chromecast/browser/metrics/cast_metrics_service_client.cc (right): > > > > > https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... > > chromecast/browser/metrics/cast_metrics_service_client.cc:256: > > external_metrics_.release()->StopAndSelfDestroy(); > > On 2015/01/10 00:58:30, gfhuang wrote: > > > On 2015/01/10 00:31:46, gunsch wrote: > > > > Thought: this mechanic is sort of strange. I think it would make more > sense > > to > > > > do the following: > > > > > > > > 1) Make ~ExternalMetrics private (since no one should be calling it) > > > > 2) Make CastMetricsServiceClient have a ExternalMetrics* instead of a > > > > scoped_ptr<ExternalMetrics> > > > > > > > > what do you think? > > > > > > Done. > > > > Note that I have to revert back to use PostTask instead of DeleteSoon though, > > since dtor is private now. > > Why not add "friend class base::DeleteHelper<ExternalMetrics>;" ? That seems to > be a common pattern: > https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ... Would that defeat the purpose of private dtor lockdown? other object could DeleteSoon it outside.
On 2015/01/10 01:26:56, gfhuang wrote: > On 2015/01/10 01:23:27, gunsch wrote: > > On 2015/01/10 01:17:30, gfhuang wrote: > > > > > > https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... > > > File chromecast/browser/metrics/cast_metrics_service_client.cc (right): > > > > > > > > > https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... > > > chromecast/browser/metrics/cast_metrics_service_client.cc:256: > > > external_metrics_.release()->StopAndSelfDestroy(); > > > On 2015/01/10 00:58:30, gfhuang wrote: > > > > On 2015/01/10 00:31:46, gunsch wrote: > > > > > Thought: this mechanic is sort of strange. I think it would make more > > sense > > > to > > > > > do the following: > > > > > > > > > > 1) Make ~ExternalMetrics private (since no one should be calling it) > > > > > 2) Make CastMetricsServiceClient have a ExternalMetrics* instead of a > > > > > scoped_ptr<ExternalMetrics> > > > > > > > > > > what do you think? > > > > > > > > Done. > > > > > > Note that I have to revert back to use PostTask instead of DeleteSoon > though, > > > since dtor is private now. > > > > Why not add "friend class base::DeleteHelper<ExternalMetrics>;" ? That seems > to > > be a common pattern: > > > https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ... > > Would that defeat the purpose of private dtor lockdown? other object could > DeleteSoon it outside. Well, it's just a pointer, someone could also get a handle to the pointer and call free() on it :) Private dtor combined with "StopAndDestroy" method is more to prevent another user from accidentally trying to take ownership of this object in, say, a scoped_ptr and not using StopAndDestroy.
On 2015/01/10 01:28:48, gunsch wrote: > On 2015/01/10 01:26:56, gfhuang wrote: > > On 2015/01/10 01:23:27, gunsch wrote: > > > On 2015/01/10 01:17:30, gfhuang wrote: > > > > > > > > > > https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... > > > > File chromecast/browser/metrics/cast_metrics_service_client.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/845003002/diff/60001/chromecast/browser/metri... > > > > chromecast/browser/metrics/cast_metrics_service_client.cc:256: > > > > external_metrics_.release()->StopAndSelfDestroy(); > > > > On 2015/01/10 00:58:30, gfhuang wrote: > > > > > On 2015/01/10 00:31:46, gunsch wrote: > > > > > > Thought: this mechanic is sort of strange. I think it would make more > > > sense > > > > to > > > > > > do the following: > > > > > > > > > > > > 1) Make ~ExternalMetrics private (since no one should be calling it) > > > > > > 2) Make CastMetricsServiceClient have a ExternalMetrics* instead of a > > > > > > scoped_ptr<ExternalMetrics> > > > > > > > > > > > > what do you think? > > > > > > > > > > Done. > > > > > > > > Note that I have to revert back to use PostTask instead of DeleteSoon > > though, > > > > since dtor is private now. > > > > > > Why not add "friend class base::DeleteHelper<ExternalMetrics>;" ? That seems > > to > > > be a common pattern: > > > > > > https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ... > > > > Would that defeat the purpose of private dtor lockdown? other object could > > DeleteSoon it outside. > > Well, it's just a pointer, someone could also get a handle to the pointer and > call free() on it :) > > Private dtor combined with "StopAndDestroy" method is more to prevent another > user from accidentally trying to take ownership of this object in, say, a > scoped_ptr and not using StopAndDestroy. done.
lgtm Quick comment: the subject line is seen Chromium-wide, so it usually helps to start with "Chromecast:" or "[Chromecast]" to help indicate for people watching the builders or scanning emails.
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/845003002/110001
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/827a380cfdb31aa54c8d56e63ce2c3fd8c3ba4d4 Cr-Commit-Position: refs/heads/master@{#310958} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
