Fix memory leak in ThreadWatcher.

Fix memory leak in ThreadWatcher.

Also simplify initialization path.

BUG=none
TEST=unit tests

Committed: https://crrev.com/bf03b6136f729384110523754d4ee57ed8ba9356
Cr-Commit-Position: refs/heads/master@{#416286}

[Joshua LeVasseur, 2016-09-01 17:46:08]

Description was changed from

==========
Fix memory leak in ThreadWatcher.

Also simplify initialization path.

BUG=none
TEST=unit tests
==========

to

==========
Fix memory leak in ThreadWatcher.

Also simplify initialization path.

BUG=none
TEST=unit tests
==========

[Joshua LeVasseur, 2016-09-01 17:46:09]

jlevasseur@chromium.org changed reviewers:
+ alokp@chromium.org, isherman@chromium.org, wzhong@chromium.org

[Joshua LeVasseur, 2016-09-01 17:49:14]

This is a revisit of https://codereview.chromium.org/2295983002/ which was
reverted due to misuse of unique_ptr.  Here is another try, which still
simplifies the initialization path, but without using unique_ptr to document
ownership --- if you'd prefer using unique_ptr, let me know.

[Ilya Sherman, 2016-09-01 18:18:11]

I'd still prefer that you use a unique_ptr -- please see my comment on the
original CL.  Also, when relanding a CL, the best practice is to upload the
originally-landed CL as Patch Set 1, and then upload any changes as Patch Set
2+.  That way, it's easy for reviewers to see what's changed since they last
reviewed the code =)  Thanks!

[Joshua LeVasseur, 2016-09-01 20:28:02]

On 2016/09/01 18:18:11, Ilya Sherman wrote:
> I'd still prefer that you use a unique_ptr -- please see my comment on the
> original CL.  Also, when relanding a CL, the best practice is to upload the
> originally-landed CL as Patch Set 1, and then upload any changes as Patch Set
> 2+.  That way, it's easy for reviewers to see what's changed since they last
> reviewed the code =)  Thanks!


I've updated to use the unique_ptr.  I chose to return a pointer from
Register(), as that supports the unit tests as they are currently structured.

Thanks for pointing out the idea for first uploading the originally-landed CL as
the first patch set --- that is a good idea.

[Ilya Sherman, 2016-09-01 20:30:25]

LGTM % nits, thanks.

https://codereview.chromium.org/2300133002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/thread_watcher.cc (right):

https://codereview.chromium.org/2300133002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/thread_watcher.cc:90: }
nit: No need for curlies here.

https://codereview.chromium.org/2300133002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/thread_watcher.h (right):

https://codereview.chromium.org/2300133002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/thread_watcher.h:397: // Returns the pointer if it was
successfully registered.
nit: Please add ", null otherwise" to the end of this sentence.

[Joshua LeVasseur, 2016-09-01 21:28:22]

https://codereview.chromium.org/2300133002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/thread_watcher.cc (right):

https://codereview.chromium.org/2300133002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/thread_watcher.cc:90: }
On 2016/09/01 20:30:25, Ilya Sherman wrote:
> nit: No need for curlies here.

Done.

https://codereview.chromium.org/2300133002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/thread_watcher.h (right):

https://codereview.chromium.org/2300133002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/thread_watcher.h:397: // Returns the pointer if it was
successfully registered.
On 2016/09/01 20:30:25, Ilya Sherman wrote:
> nit: Please add ", null otherwise" to the end of this sentence.

Done.

[Joshua LeVasseur, 2016-09-01 21:29:19]

The CQ bit was checked by jlevasseur@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 21:30:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alokp, 2016-09-01 21:52:06]

lgtm

[commit-bot: I haz the power, 2016-09-01 22:18:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 22:18:51]

Dry run: This issue passed the CQ dry run.

[wzhong, 2016-09-02 16:58:41]

lgtm

[Joshua LeVasseur, 2016-09-02 17:00:13]

The CQ bit was checked by jlevasseur@chromium.org

[Joshua LeVasseur, 2016-09-02 17:00:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2300133002/#ps40001
(title: "Implement isherman's feedback (from comment #6)")

[commit-bot: I haz the power, 2016-09-02 17:00:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 17:04:17]

Description was changed from

==========
Fix memory leak in ThreadWatcher.

Also simplify initialization path.

BUG=none
TEST=unit tests
==========

to

==========
Fix memory leak in ThreadWatcher.

Also simplify initialization path.

BUG=none
TEST=unit tests
==========

[commit-bot: I haz the power, 2016-09-02 17:04:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-02 17:05:51]

Description was changed from

==========
Fix memory leak in ThreadWatcher.

Also simplify initialization path.

BUG=none
TEST=unit tests
==========

to

==========
Fix memory leak in ThreadWatcher.

Also simplify initialization path.

BUG=none
TEST=unit tests

Committed: https://crrev.com/bf03b6136f729384110523754d4ee57ed8ba9356
Cr-Commit-Position: refs/heads/master@{#416286}
==========

[commit-bot: I haz the power, 2016-09-02 17:05:55]

Patchset 3 (id:??) landed as
https://crrev.com/bf03b6136f729384110523754d4ee57ed8ba9356
Cr-Commit-Position: refs/heads/master@{#416286}