[BlobAsync] Fixed shared memory mapping

[BlobAsync] Fixed shared memory mapping

The test was designed to catch this case, but because the memory was mapped before we sent it to the class, the incorrect call to Map was a no-op. Test has been fixed.

BUG=619217
Committed: https://crrev.com/8731540b822e48124e782a976ef5d44632757a29
Cr-Commit-Position: refs/heads/master@{#401053}

[dmurph, 2016-06-21 16:40:05]

Description was changed from

==========
[BlobAsync] Fixed shared memory mapping

BUG=619217
==========

to

==========
[BlobAsync] Fixed shared memory mapping

The test was designed to catch this case, but because the memory was mapped
before we sent it to the class, the incorrect call to Map was a no-op. Test has
been fixed.

BUG=619217
==========

[dmurph, 2016-06-21 16:40:32]

dmurph@chromium.org changed reviewers:
+ mek@chromium.org, michaeln@chromium.org

[dmurph, 2016-06-21 16:40:33]

Hello! Blob P0 Fix PTAL!

[Marijn Kruisselbrink, 2016-06-21 16:52:36]

lgtm, with some nits

https://codereview.chromium.org/2086033002/diff/20001/content/child/blob_stor...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/2086033002/diff/20001/content/child/blob_stor...
content/child/blob_storage/blob_transport_controller.cc:217: std::vector<size_t>
shared_memory_sizes;
nit: Why not just pass memory_handles->size() directly to the vector
constructor, rather than the split between constructing and resizing?

https://codereview.chromium.org/2086033002/diff/20001/content/child/blob_stor...
content/child/blob_storage/blob_transport_controller.cc:226:
static_cast<size_t>(request.size + request.handle_offset));
rather than the static_cast<size_t> you could also just explicitly pass the
template parameter to std::max (so std::max<size_t>(..., ...))

https://codereview.chromium.org/2086033002/diff/20001/content/child/blob_stor...
content/child/blob_storage/blob_transport_controller.cc:253:
DCHECK_LT(request.handle_index, memory_handles->size())
Should you move/copy this DCHECK to your new loop?

[dmurph, 2016-06-21 17:33:03]

Thanks!

https://codereview.chromium.org/2086033002/diff/20001/content/child/blob_stor...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/2086033002/diff/20001/content/child/blob_stor...
content/child/blob_storage/blob_transport_controller.cc:217: std::vector<size_t>
shared_memory_sizes;
On 2016/06/21 at 16:52:35, Marijn Kruisselbrink wrote:
> nit: Why not just pass memory_handles->size() directly to the vector
constructor, rather than the split between constructing and resizing?

Ah, right, thanks.

https://codereview.chromium.org/2086033002/diff/20001/content/child/blob_stor...
content/child/blob_storage/blob_transport_controller.cc:226:
static_cast<size_t>(request.size + request.handle_offset));
On 2016/06/21 at 16:52:35, Marijn Kruisselbrink wrote:
> rather than the static_cast<size_t> you could also just explicitly pass the
template parameter to std::max (so std::max<size_t>(..., ...))

Done.

https://codereview.chromium.org/2086033002/diff/20001/content/child/blob_stor...
content/child/blob_storage/blob_transport_controller.cc:253:
DCHECK_LT(request.handle_index, memory_handles->size())
On 2016/06/21 at 16:52:35, Marijn Kruisselbrink wrote:
> Should you move/copy this DCHECK to your new loop?

Done.

[dmurph, 2016-06-21 17:33:07]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2016-06-21 17:33:07]

The patchset sent to the CQ was uploaded after l-g-t-m from mek@chromium.org
Link to the patchset: https://codereview.chromium.org/2086033002/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2016-06-21 17:33:41]

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

[commit-bot: I haz the power, 2016-06-21 18:43:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-21 18:46:35]

Description was changed from

==========
[BlobAsync] Fixed shared memory mapping

The test was designed to catch this case, but because the memory was mapped
before we sent it to the class, the incorrect call to Map was a no-op. Test has
been fixed.

BUG=619217
==========

to

==========
[BlobAsync] Fixed shared memory mapping

The test was designed to catch this case, but because the memory was mapped
before we sent it to the class, the incorrect call to Map was a no-op. Test has
been fixed.

BUG=619217

Committed: https://crrev.com/8731540b822e48124e782a976ef5d44632757a29
Cr-Commit-Position: refs/heads/master@{#401053}
==========

[commit-bot: I haz the power, 2016-06-21 18:46:36]

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