base: Rename discardable memory allocator interface and remove unnecessary class.

base: Rename discardable memory allocator interface and remove unnecessary class.

There's only one discardable memory allocator interface. Rename it
to DiscardableMemoryAllocator. DiscardableMemoryShmemChunk class
is unnecessary, remove it.

BUG=442945, 422953
Committed: https://crrev.com/1af05cb3770b3e3eb61d3731a09efcd1fbec242b
Cr-Commit-Position: refs/heads/master@{#321008}

[reveman, 2015-03-12 15:25:51]

reveman@chromium.org changed reviewers:
+ avi@chromium.org

[reveman, 2015-03-12 15:25:52]

Last cleanup patch.

[Avi (use Gerrit), 2015-03-12 16:05:53]

lgtm

stampity stamp

[reveman, 2015-03-12 16:55:10]

reveman@chromium.org changed reviewers:
+ danakj@chromium.org

[reveman, 2015-03-12 16:55:10]

+danakj for base/

[danakj, 2015-03-12 16:58:15]

LGTM

[reveman, 2015-03-12 19:38:51]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-03-12 19:39:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001873002/1

[reveman, 2015-03-12 21:04:25]

The CQ bit was unchecked by reveman@chromium.org

[reveman, 2015-03-16 21:06:56]

reveman@chromium.org changed reviewers:
+ oshima@chromium.org, stevenjb@chromium.org, thakis@chromium.org

[reveman, 2015-03-16 21:06:56]

+oshima for ash/test/
+thakis for chrome/
+stevenjb for ui/

[Nico, 2015-03-16 21:55:31]

lgtm

https://codereview.chromium.org/1001873002/diff/20001/base/memory/discardable...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/1001873002/diff/20001/base/memory/discardable...
base/memory/discardable_memory.cc:12: scoped_ptr<DiscardableMemory>
DiscardableMemory::CreateLockedMemory(
should this go too?

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
File base/test/test_discardable_memory_allocator.h (right):

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
base/test/test_discardable_memory_allocator.h:12: class
TestDiscardableMemoryAllocator : public DiscardableMemoryAllocator {
nit: StubDiscardableMemoryAllocator? Else it sounds a bit like a test. (but ok
as-is too. i'm aware it's what the old thing was called too)

[reveman, 2015-03-16 22:37:26]

https://codereview.chromium.org/1001873002/diff/20001/base/memory/discardable...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/1001873002/diff/20001/base/memory/discardable...
base/memory/discardable_memory.cc:12: scoped_ptr<DiscardableMemory>
DiscardableMemory::CreateLockedMemory(
On 2015/03/16 21:55:30, Nico (traveling) wrote:
> should this go too?

Yes, I considered that but decided to leave for a follow up for now.

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
File base/test/test_discardable_memory_allocator.h (right):

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
base/test/test_discardable_memory_allocator.h:12: class
TestDiscardableMemoryAllocator : public DiscardableMemoryAllocator {
On 2015/03/16 21:55:30, Nico (traveling) wrote:
> nit: StubDiscardableMemoryAllocator? Else it sounds a bit like a test. (but ok
> as-is too. i'm aware it's what the old thing was called too)

hm, the idea is that it's an implementation that is useful for testing purposes.
Not testing of discardable memory but testing of code that depends on
discardable memory. "Stub" sounds like it's not a real implementation but
something that would return a fake instances of discardable memory. I prefer
TestDiscardableMemoryAllocator for that reason.

[danakj, 2015-03-16 23:03:05]

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
File base/test/test_discardable_memory_allocator.h (right):

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
base/test/test_discardable_memory_allocator.h:12: class
TestDiscardableMemoryAllocator : public DiscardableMemoryAllocator {
On 2015/03/16 22:37:26, reveman wrote:
> On 2015/03/16 21:55:30, Nico (traveling) wrote:
> > nit: StubDiscardableMemoryAllocator? Else it sounds a bit like a test. (but
ok
> > as-is too. i'm aware it's what the old thing was called too)
> 
> hm, the idea is that it's an implementation that is useful for testing
purposes.
> Not testing of discardable memory but testing of code that depends on
> discardable memory. "Stub" sounds like it's not a real implementation but
> something that would return a fake instances of discardable memory. I prefer
> TestDiscardableMemoryAllocator for that reason.

If you want the name to give more character to say its useful in tests I'd
suggest a name that describes the actual implementation. Like,
MallocDiscardableMemoryAllocator.. or.. something..

[reveman, 2015-03-17 04:09:15]

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
File base/test/test_discardable_memory_allocator.h (right):

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
base/test/test_discardable_memory_allocator.h:12: class
TestDiscardableMemoryAllocator : public DiscardableMemoryAllocator {
On 2015/03/16 23:03:05, danakj wrote:
> On 2015/03/16 22:37:26, reveman wrote:
> > On 2015/03/16 21:55:30, Nico (traveling) wrote:
> > > nit: StubDiscardableMemoryAllocator? Else it sounds a bit like a test.
(but
> ok
> > > as-is too. i'm aware it's what the old thing was called too)
> > 
> > hm, the idea is that it's an implementation that is useful for testing
> purposes.
> > Not testing of discardable memory but testing of code that depends on
> > discardable memory. "Stub" sounds like it's not a real implementation but
> > something that would return a fake instances of discardable memory. I prefer
> > TestDiscardableMemoryAllocator for that reason.
> 
> If you want the name to give more character to say its useful in tests I'd
> suggest a name that describes the actual implementation. Like,
> MallocDiscardableMemoryAllocator.. or.. something..

OneShotDiscardableMemoryAllocator would reflect what the current implementation
does but I still prefer to keep it as TestDiscardableMemoryAllocator. We should
be able to change this to a different implementation if we determine that to be
more useful for testing purposes.

Anyhow, I think all this is beyond this patch, which does nothing but remove the
shmem sub-string from the name.

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
base/test/test_discardable_memory_allocator.h:12: class
TestDiscardableMemoryAllocator : public DiscardableMemoryAllocator {
On 2015/03/16 23:03:05, danakj wrote:
> On 2015/03/16 22:37:26, reveman wrote:
> > On 2015/03/16 21:55:30, Nico (traveling) wrote:
> > > nit: StubDiscardableMemoryAllocator? Else it sounds a bit like a test.
(but
> ok
> > > as-is too. i'm aware it's what the old thing was called too)
> > 
> > hm, the idea is that it's an implementation that is useful for testing
> purposes.
> > Not testing of discardable memory but testing of code that depends on
> > discardable memory. "Stub" sounds like it's not a real implementation but
> > something that would return a fake instances of discardable memory. I prefer
> > TestDiscardableMemoryAllocator for that reason.
> 
> If you want the name to give more character to say its useful in tests I'd
> suggest a name that describes the actual implementation. Like,
> MallocDiscardableMemoryAllocator.. or.. something..

OneShotDiscardableMemoryAllocator would reflect what the current implementation
does but I still prefer to keep it as TestDiscardableMemoryAllocator. We should
be able to change this to a different implementation if we determine that to be
a more useful for testing purposes.

Anyhow, I think all this is beyond this patch, which does nothing but remove the
shmem sub string from the name.

[reveman, 2015-03-17 14:02:41]

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
File base/test/test_discardable_memory_allocator.h (right):

https://codereview.chromium.org/1001873002/diff/20001/base/test/test_discarda...
base/test/test_discardable_memory_allocator.h:12: class
TestDiscardableMemoryAllocator : public DiscardableMemoryAllocator {
On 2015/03/17 04:09:15, reveman wrote:
> On 2015/03/16 23:03:05, danakj wrote:
> > On 2015/03/16 22:37:26, reveman wrote:
> > > On 2015/03/16 21:55:30, Nico (traveling) wrote:
> > > > nit: StubDiscardableMemoryAllocator? Else it sounds a bit like a test.
> (but
> > ok
> > > > as-is too. i'm aware it's what the old thing was called too)
> > > 
> > > hm, the idea is that it's an implementation that is useful for testing
> > purposes.
> > > Not testing of discardable memory but testing of code that depends on
> > > discardable memory. "Stub" sounds like it's not a real implementation but
> > > something that would return a fake instances of discardable memory. I
prefer
> > > TestDiscardableMemoryAllocator for that reason.
> > 
> > If you want the name to give more character to say its useful in tests I'd
> > suggest a name that describes the actual implementation. Like,
> > MallocDiscardableMemoryAllocator.. or.. something..
> 
> OneShotDiscardableMemoryAllocator would reflect what the current
implementation
> does but I still prefer to keep it as TestDiscardableMemoryAllocator. We
should
> be able to change this to a different implementation if we determine that to
be
> more useful for testing purposes.
> 
> Anyhow, I think all this is beyond this patch, which does nothing but remove
the
> shmem sub-string from the name.

I added a comment to this class in latest patch.

Btw, TestDiscardableMemoryAllocator is consistent with other "for testing"
implementations in base/test such as TestSimpleTaskRunner. As well as all for
testing implementations in cc/test/.

[Nico, 2015-03-17 14:11:23]

(as i said, i'm fine with the name as-is too)

On Tue, Mar 17, 2015 at 10:02 AM, <reveman@chromium.org> wrote:

>
> https://codereview.chromium.org/1001873002/diff/20001/
> base/test/test_discardable_memory_allocator.h
> File base/test/test_discardable_memory_allocator.h (right):
>
> https://codereview.chromium.org/1001873002/diff/20001/
> base/test/test_discardable_memory_allocator.h#newcode12
> base/test/test_discardable_memory_allocator.h:12: class
> TestDiscardableMemoryAllocator : public DiscardableMemoryAllocator {
> On 2015/03/17 04:09:15, reveman wrote:
>
>> On 2015/03/16 23:03:05, danakj wrote:
>> > On 2015/03/16 22:37:26, reveman wrote:
>> > > On 2015/03/16 21:55:30, Nico (traveling) wrote:
>> > > > nit: StubDiscardableMemoryAllocator? Else it sounds a bit like a
>>
> test.
>
>> (but
>> > ok
>> > > > as-is too. i'm aware it's what the old thing was called too)
>> > >
>> > > hm, the idea is that it's an implementation that is useful for
>>
> testing
>
>> > purposes.
>> > > Not testing of discardable memory but testing of code that depends
>>
> on
>
>> > > discardable memory. "Stub" sounds like it's not a real
>>
> implementation but
>
>> > > something that would return a fake instances of discardable
>>
> memory. I prefer
>
>> > > TestDiscardableMemoryAllocator for that reason.
>> >
>> > If you want the name to give more character to say its useful in
>>
> tests I'd
>
>> > suggest a name that describes the actual implementation. Like,
>> > MallocDiscardableMemoryAllocator.. or.. something..
>>
>
>  OneShotDiscardableMemoryAllocator would reflect what the current
>>
> implementation
>
>> does but I still prefer to keep it as TestDiscardableMemoryAllocator.
>>
> We should
>
>> be able to change this to a different implementation if we determine
>>
> that to be
>
>> more useful for testing purposes.
>>
>
>  Anyhow, I think all this is beyond this patch, which does nothing but
>>
> remove the
>
>> shmem sub-string from the name.
>>
>
> I added a comment to this class in latest patch.
>
> Btw, TestDiscardableMemoryAllocator is consistent with other "for
> testing" implementations in base/test such as TestSimpleTaskRunner. As
> well as all for testing implementations in cc/test/.
>
> https://codereview.chromium.org/1001873002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2015-03-17 18:26:16]

oshima, stevenjb, please take a look at the ash/ui changes when you have a
chance.

[oshima, 2015-03-17 18:28:46]

ash/test lgtm

[reveman, 2015-03-17 22:52:49]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-03-17 22:52:50]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
avi@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1001873002/#ps80001
(title: "fix child process DiscardableMemoryImpl")

[commit-bot: I haz the power, 2015-03-17 22:53:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001873002/80001

[commit-bot: I haz the power, 2015-03-17 23:18:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-17 23:19:15]

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