Break global impact of PersistentHistogramAllocator into a separate class.

Break global impact of PersistentHistogramAllocator into a separate class.

BUG=546019
TBR=grt
grt: setup/installer (no logic changes, just names)

Committed: https://crrev.com/5e748c6dca3b8f585c295956f192ca7c34cbe4b7
Cr-Commit-Position: refs/heads/master@{#385367}

[bcwhite, 2016-03-18 00:54:00]

Patchset #2 (id:20001) has been deleted

[bcwhite, 2016-04-01 17:48:05]

Patchset #2 (id:40001) has been deleted

[bcwhite, 2016-04-01 18:21:23]

Patchset #2 (id:60001) has been deleted

[bcwhite, 2016-04-01 18:23:48]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-04-01 19:52:40]

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:42:
~PersistentHistogramAllocator();
This should be virtual since there is a subclass.

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:111:
scoped_ptr<PersistentMemoryAllocator> memory_allocator_;
Members shouldn't be protected per style guide. ("Note that data members should
be private.").

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:171: class BASE_EXPORT
GlobalHistogramAllocator
Can this be a separate file?

Also, add a comment.

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:174: ~GlobalHistogramAllocator();
Where's the dtor? Did you mean to make it private? If you don't declare one, the
compiler will add an implicit one.

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:211: static
GlobalHistogramAllocator* Get();
Split these functions with a comment for Get() - otherwise it's hard to read -
i.e. from a first glance it may look like a wrapped line.

[bcwhite, 2016-04-01 20:24:16]

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:171: class BASE_EXPORT
GlobalHistogramAllocator
On 2016/04/01 19:52:40, Alexei Svitkine wrote:
> Can this be a separate file?

I could, but I'd rather not.  It's very closely tied; separating them would, I
believe, make it more difficult to understand.

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:174: ~GlobalHistogramAllocator();
On 2016/04/01 19:52:40, Alexei Svitkine wrote:
> Where's the dtor? Did you mean to make it private? If you don't declare one,
the
> compiler will add an implicit one.

Objects of this class are passed by scoped_ptr so the dtor has to be public.

[bcwhite, 2016-04-04 18:42:22]

Patchset #3 (id:100001) has been deleted

[bcwhite, 2016-04-04 18:47:29]

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:42:
~PersistentHistogramAllocator();
On 2016/04/01 19:52:40, Alexei Svitkine wrote:
> This should be virtual since there is a subclass.

Done.

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:111:
scoped_ptr<PersistentMemoryAllocator> memory_allocator_;
On 2016/04/01 19:52:40, Alexei Svitkine wrote:
> Members shouldn't be protected per style guide. ("Note that data members
should
> be private.").

Done.

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:171: class BASE_EXPORT
GlobalHistogramAllocator
On 2016/04/01 19:52:40, Alexei Svitkine wrote:
> Can this be a separate file?
> 
> Also, add a comment.

Done.

https://codereview.chromium.org/1780993002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:211: static
GlobalHistogramAllocator* Get();
On 2016/04/01 19:52:40, Alexei Svitkine wrote:
> Split these functions with a comment for Get() - otherwise it's hard to read -
> i.e. from a first glance it may look like a wrapped line.

Done.

[Alexei Svitkine (slow), 2016-04-05 15:56:00]

lgtm

https://codereview.chromium.org/1780993002/diff/120001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1780993002/diff/120001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:233:
GlobalHistogramAllocator(scoped_ptr<PersistentMemoryAllocator> memory);
Nit: explicit

[bcwhite, 2016-04-06 00:58:16]

https://codereview.chromium.org/1780993002/diff/120001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1780993002/diff/120001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:233:
GlobalHistogramAllocator(scoped_ptr<PersistentMemoryAllocator> memory);
On 2016/04/05 15:56:00, Alexei Svitkine wrote:
> Nit: explicit

Done.

[bcwhite, 2016-04-06 01:48:50]

Description was changed from

==========
Break global impact of PersistentHistogramAllocator into a separate class.

BUG=546019
==========

to

==========
Break global impact of PersistentHistogramAllocator into a separate class.

BUG=546019
TBR=grt
grt: setup/installer (no logic changes, just names)
==========

[bcwhite, 2016-04-06 01:49:05]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-04-06 01:49:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1780993002/#ps180001
(title: "fixed bad formatting from upstream scoped_ptr change")

[commit-bot: I haz the power, 2016-04-06 01:49:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1780993002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1780993002/180001

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

Description was changed from

==========
Break global impact of PersistentHistogramAllocator into a separate class.

BUG=546019
TBR=grt
grt: setup/installer (no logic changes, just names)
==========

to

==========
Break global impact of PersistentHistogramAllocator into a separate class.

BUG=546019
TBR=grt
grt: setup/installer (no logic changes, just names)
==========

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

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-04-06 02:05:12]

Description was changed from

==========
Break global impact of PersistentHistogramAllocator into a separate class.

BUG=546019
TBR=grt
grt: setup/installer (no logic changes, just names)
==========

to

==========
Break global impact of PersistentHistogramAllocator into a separate class.

BUG=546019
TBR=grt
grt: setup/installer (no logic changes, just names)

Committed: https://crrev.com/5e748c6dca3b8f585c295956f192ca7c34cbe4b7
Cr-Commit-Position: refs/heads/master@{#385367}
==========

[commit-bot: I haz the power, 2016-04-06 02:05:14]

Patchset 6 (id:??) landed as
https://crrev.com/5e748c6dca3b8f585c295956f192ca7c34cbe4b7
Cr-Commit-Position: refs/heads/master@{#385367}