Re-land: base: Replace PurgeAndTruncate with Shrink function.

Re-land: base: Replace PurgeAndTruncate with Shrink function.

This cleans up the API a bit and allows the code to be unit tested.

This also prevents ftruncate from being used on Android where
shared memory is backed by Ashmem. Ashmem doesn't support ftruncate.

BUG=466509
TEST=base_unittests --gtest_filter=DiscardableSharedMemoryTest.Shrink

Committed: https://crrev.com/59d32f501694975c4011ea85a67060a41140d0f4
Cr-Commit-Position: refs/heads/master@{#323941}

[reveman, 2015-03-18 22:28:31]

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

[Avi (use Gerrit), 2015-03-18 22:36:03]

lgtm

[reveman, 2015-03-18 23:01:43]

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

[reveman, 2015-03-18 23:01:44]

+danakj for base/

[danakj, 2015-03-18 23:18:52]

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

https://codereview.chromium.org/1018743003/diff/20001/base/memory/discardable...
base/memory/discardable_shared_memory_unittest.cc:315: if
(!DiscardableSharedMemory::IsShrinkingSupported())
Do you think you could add platform #ifs here instead of early out based on
production code? Maybe you can verify this IsShrinkingSupported thing does what
you want too, to make people notice this test if they change that function?

[reveman, 2015-03-19 02:21:54]

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

https://codereview.chromium.org/1018743003/diff/20001/base/memory/discardable...
base/memory/discardable_shared_memory_unittest.cc:315: if
(!DiscardableSharedMemory::IsShrinkingSupported())
On 2015/03/18 at 23:18:52, danakj wrote:
> Do you think you could add platform #ifs here instead of early out based on
production code? Maybe you can verify this IsShrinkingSupported thing does what
you want too, to make people notice this test if they change that function?

I removed IsShrinkingSupported from latest patch and added #ifs around Shrink()
instead. That keeps things simple and I think it's more consistent with other
classes in base/ such as SharedMemory. PTAL.

[danakj, 2015-03-19 16:41:56]

https://codereview.chromium.org/1018743003/diff/40001/content/common/host_dis...
File content/common/host_discardable_shared_memory_manager.cc (right):

https://codereview.chromium.org/1018743003/diff/40001/content/common/host_dis...
content/common/host_discardable_shared_memory_manager.cc:386: #if
defined(OS_POSIX) && !defined(OS_ANDROID)
Hmm.. the only thing I like a bit less is these guards in content/ it would be
easy to let them get out of sync with the ones in base/. I would slightly prefer
that only the test can get out of sync and that it would fail when doing so.

[reveman, 2015-03-19 19:23:05]

PTAL

https://codereview.chromium.org/1018743003/diff/40001/content/common/host_dis...
File content/common/host_discardable_shared_memory_manager.cc (right):

https://codereview.chromium.org/1018743003/diff/40001/content/common/host_dis...
content/common/host_discardable_shared_memory_manager.cc:386: #if
defined(OS_POSIX) && !defined(OS_ANDROID)
On 2015/03/19 16:41:56, danakj wrote:
> Hmm.. the only thing I like a bit less is these guards in content/ it would be
> easy to let them get out of sync with the ones in base/. I would slightly
prefer
> that only the test can get out of sync and that it would fail when doing so.

Added DISCARDABLE_SHARED_MEMORY_SHRINKING define to prevent this from getting
out of sync. I could hide the ifdef inside the Shrink function and have Shrink
be a noop when not supported but I think I prefer to keep this explicit. This
code should be aware of the fact that shrinking is not always supported and we
might handle the case when it's not a bit differently in the future.

[reveman, 2015-03-23 18:53:21]

ping for review

[danakj, 2015-03-25 17:55:00]

I like the define approach so LGTM, with one suggestion:

https://codereview.chromium.org/1018743003/diff/60001/base/memory/discardable...
File base/memory/discardable_shared_memory.h (right):

https://codereview.chromium.org/1018743003/diff/60001/base/memory/discardable...
base/memory/discardable_shared_memory.h:19: 
If might be simpler to just #define DISCARDABLE_SHARED_MEMORY_SHRINKING here
instead of in the gypi file (does that work for the GN build?)

[reveman, 2015-03-25 18:26:42]

https://codereview.chromium.org/1018743003/diff/60001/base/memory/discardable...
File base/memory/discardable_shared_memory.h (right):

https://codereview.chromium.org/1018743003/diff/60001/base/memory/discardable...
base/memory/discardable_shared_memory.h:19: 
On 2015/03/25 17:55:00, danakj wrote:
> If might be simpler to just #define DISCARDABLE_SHARED_MEMORY_SHRINKING here
> instead of in the gypi file (does that work for the GN build?)

Sure, that works.

Having the define in build/ makes sense if we'd like to have some code that
doesn't need to include this file behave differently based on if this is
supported or not. We might need that in the future but we can keep it in here
for now.

[reveman, 2015-03-25 18:26:51]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-03-25 18:26:51]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1018743003/#ps80001
(title: "move define")

[commit-bot: I haz the power, 2015-03-25 18:27:12]

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

[commit-bot: I haz the power, 2015-03-25 19:21:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-25 19:22:04]

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

[reveman, 2015-04-01 15:14:50]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1057493005/ by reveman@chromium.org.

The reason for reverting is: Suspecting that this might be related to
crbug.com/471188..

[reveman, 2015-04-06 18:49:42]

Going to try re-land this now that I've updated the patch to not change the
behavior but instead just rename functions and avoid errors on Android where
ftruncate can't be used.

It's not clear yet why the change in behavior in the previous patch was causing
issues. I'll keep investigating.

[reveman, 2015-04-06 19:08:37]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-04-06 19:08:38]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1018743003/#ps120001
(title: "fix typo")

[commit-bot: I haz the power, 2015-04-06 19:09:25]

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

[commit-bot: I haz the power, 2015-04-06 20:11:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-06 20:12:27]

Patchset 7 (id:??) landed as
https://crrev.com/59d32f501694975c4011ea85a67060a41140d0f4
Cr-Commit-Position: refs/heads/master@{#323941}