[BlobAsync] Patch 2: Common Constants

[BlobAsync] Patch 2: Common Constants
This CL contains constants for the
Blob Async Transport refactor.

Note: This CL is currently a no-op, hookup is in the 'Hookup' patch.

Patches:
1: https://codereview.chromium.org/1287303002 (Committed!)
2: https://codereview.chromium.org/1288373002
3: https://codereview.chromium.org/1292523002
4: https://codereview.chromium.org/1098853003
Hookup: https://codereview.chromium.org/1234813004

BUG=375297
Committed: https://crrev.com/0140342861a6aa998ffbf44ecfdc360305f560bc
Cr-Commit-Position: refs/heads/master@{#357248}

[dmurph, 2015-08-13 12:57:05]

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

[dmurph, 2015-08-13 12:57:05]

Hello Michael & Kinuko,

This is the second patch of my patch breakup. This includes all of the messages
and structs that I'll be using in the new messaging logic. Notable changes:
 * Adding a new DataElement type, BYTES_DESCRIPTION
 * Supporting async population of blob data in the Builder
 * Adding logging support to all structs
 * BlobStorageMsg messages for async transportation

Thanks!
Daniel

[michaeln, 2015-08-18 03:04:42]

some prelim comments excavated from your orig mondo cl

https://codereview.chromium.org/1288373002/diff/1/content/common/blob_storage...
File content/common/blob_storage/blob_storage_messages.h (right):

https://codereview.chromium.org/1288373002/diff/1/content/common/blob_storage...
content/common/blob_storage/blob_storage_messages.h:22: #define
IPC_MESSAGE_START BlobMsgStart
BlobMsgStart is in use in webblob_messages.h. I think having it here too can
cause msg id collisions.

We could keep all of the blob related messages defined in one place to keep it
simple. The old ones can be named BlahMsg_OLD, or the new ones _NEW for the time
in which they overlap in the sources?

https://codereview.chromium.org/1288373002/diff/1/storage/common/blob_storage...
File storage/common/blob_storage/blob_item_bytes_request.h (right):

https://codereview.chromium.org/1288373002/diff/1/storage/common/blob_storage...
storage/common/blob_storage/blob_item_bytes_request.h:23: static
BlobItemBytesRequest CreateIPCRequest(size_t request_number,
Not sure the set of create helpers is needed? At the callsite, there's little
difference between invoking the constructor vs the helper. In this .h file, the
additional methods obscure that the class is POD.

https://codereview.chromium.org/1288373002/diff/1/storage/common/data_element.cc
File storage/common/data_element.cc (right):

https://codereview.chromium.org/1288373002/diff/1/storage/common/data_element...
storage/common/data_element.cc:57: std::ostream& operator<<(std::ostream& os,
const DataElement& x) {
is this for logging?

https://codereview.chromium.org/1288373002/diff/1/storage/common/data_element...
storage/common/data_element.cc:64: os << std::setfill('0') << std::setw(2) <<
+(x.bytes()[i]) << ' ';
not sure a hexdump of the contents of a blob is very helpful in a log file, if
the contents are large it would probably hurt more than help?

https://codereview.chromium.org/1288373002/diff/1/storage/common/data_element.h
File storage/common/data_element.h (right):

https://codereview.chromium.org/1288373002/diff/1/storage/common/data_element...
storage/common/data_element.h:52: char* mutable_bytes() { return &buf_[0]; }
maybe have SetToAllocatedBytes(n) return the char* instead of having this
seperate getter? i think that could be less error prone.

[dmurph, 2015-09-15 01:12:12]

https://codereview.chromium.org/1288373002/diff/1/content/common/blob_storage...
File content/common/blob_storage/blob_storage_messages.h (right):

https://codereview.chromium.org/1288373002/diff/1/content/common/blob_storage...
content/common/blob_storage/blob_storage_messages.h:22: #define
IPC_MESSAGE_START BlobMsgStart
On 2015/08/18 at 03:04:41, michaeln wrote:
> BlobMsgStart is in use in webblob_messages.h. I think having it here too can
cause msg id collisions.
> 
> We could keep all of the blob related messages defined in one place to keep it
simple. The old ones can be named BlahMsg_OLD, or the new ones _NEW for the time
in which they overlap in the sources?

Yes, I actually move all of the blob message over here and delete that class in
the final patch.  It did cause collisions.  Since this patch won't have this guy
be included at all, I though it was find to keep it like this.

https://codereview.chromium.org/1288373002/diff/1/storage/common/blob_storage...
File storage/common/blob_storage/blob_item_bytes_request.h (right):

https://codereview.chromium.org/1288373002/diff/1/storage/common/blob_storage...
storage/common/blob_storage/blob_item_bytes_request.h:23: static
BlobItemBytesRequest CreateIPCRequest(size_t request_number,
On 2015/08/18 at 03:04:41, michaeln wrote:
> Not sure the set of create helpers is needed? At the callsite, there's little
difference between invoking the constructor vs the helper. In this .h file, the
additional methods obscure that the class is POD.

You'll notice some constants below though, which having this helper removes (and
avoids people populating them with the wrong things).  We have
IPCBlobItemRequestStrategy::IPC, kInvalidIndex, and kInvalidSize.  I think it
makes is clearer, personally, as the method call also describes the type of
request we're creating.

https://codereview.chromium.org/1288373002/diff/1/storage/common/data_element.cc
File storage/common/data_element.cc (right):

https://codereview.chromium.org/1288373002/diff/1/storage/common/data_element...
storage/common/data_element.cc:57: std::ostream& operator<<(std::ostream& os,
const DataElement& x) {
On 2015/08/18 at 03:04:41, michaeln wrote:
> is this for logging?

Yes.  I'm being a good citizen with the basic data class.

https://codereview.chromium.org/1288373002/diff/1/storage/common/data_element...
storage/common/data_element.cc:64: os << std::setfill('0') << std::setw(2) <<
+(x.bytes()[i]) << ' ';
On 2015/08/18 at 03:04:41, michaeln wrote:
> not sure a hexdump of the contents of a blob is very helpful in a log file, if
the contents are large it would probably hurt more than help?

Yes, I think I'll add a cap of 40 bytes.

https://codereview.chromium.org/1288373002/diff/1/storage/common/data_element.h
File storage/common/data_element.h (right):

https://codereview.chromium.org/1288373002/diff/1/storage/common/data_element...
storage/common/data_element.h:52: char* mutable_bytes() { return &buf_[0]; }
On 2015/08/18 at 03:04:41, michaeln wrote:
> maybe have SetToAllocatedBytes(n) return the char* instead of having this
> seperate getter? i think that could be less error prone.

I had that, but unfortunately we need the bytes much later (after an IPC hop),
and I have no where sensible to store this pointer.

Example:
* We get the description, but not the data, so we need to allocate space.
* Later, we get the data, and we need to write to the allocated space.
* We need to have the space allocated in the first place because we need it to
be TYPE_BYTES, but if we don't have allocated memory then we're breaking a lot
of people's assumptions about that class and we might end up in a bad state.

[dmurph, 2015-10-12 20:22:34]

Michael:
I added the message category so this wouldn't look so weird.

[kinuko, 2015-10-16 14:24:13]

https://codereview.chromium.org/1288373002/diff/100001/content/child/blob_sto...
File content/child/blob_storage/blob_consolidation.cc (right):

https://codereview.chromium.org/1288373002/diff/100001/content/child/blob_sto...
content/child/blob_storage/blob_consolidation.cc:5: #include <algorithm>
(not related to this CL?)

https://codereview.chromium.org/1288373002/diff/100001/content/common/blob_st...
File content/common/blob_storage/blob_storage_messages.h (right):

https://codereview.chromium.org/1288373002/diff/100001/content/common/blob_st...
content/common/blob_storage/blob_storage_messages.h:5: // Multiply-included
message file, hence no include guard.
I know the intention was to split CL for smoother review (oops), but in order to
have meaningful security review on IPC messages I think we probably need
sender/receiver code too

https://codereview.chromium.org/1288373002/diff/100001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/100001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:155: std::ostream&
operator<<(std::ostream& os, const BlobDataBuilder& x) {
Are these << operators for logging?  Per style guide operator overloading is not
really encouraged, I think having a method like 'DebugString()' (that just
returns string) is the most common way in chrome (and in google3, if it hasn't
been changed much).  Do we have a strong reason not to do that?

https://code.google.com/p/chromium/codesearch#search/&q=debugstring&sq=packag...

https://codereview.chromium.org/1288373002/diff/100001/storage/common/blob_st...
File storage/common/blob_storage/blob_storage_constants.h (right):

https://codereview.chromium.org/1288373002/diff/100001/storage/common/blob_st...
storage/common/blob_storage/blob_storage_constants.h:19: constexpr size_t
kBlobStorageMaxBlobMemorySize =
constexpr is not allowed in chromium yet.

[dmurph, 2015-10-16 21:37:31]

Kinuko: Did you mean to not add a new IPC header? Or that I should move the IPC
changes to the Hookup patch, where they're actually used?

https://codereview.chromium.org/1288373002/diff/100001/content/child/blob_sto...
File content/child/blob_storage/blob_consolidation.cc (right):

https://codereview.chromium.org/1288373002/diff/100001/content/child/blob_sto...
content/child/blob_storage/blob_consolidation.cc:5: #include <algorithm>
On 2015/10/16 at 14:24:13, kinuko wrote:
> (not related to this CL?)

No, but it should be there. Removed to simplify CL.

https://codereview.chromium.org/1288373002/diff/100001/content/common/blob_st...
File content/common/blob_storage/blob_storage_messages.h (right):

https://codereview.chromium.org/1288373002/diff/100001/content/common/blob_st...
content/common/blob_storage/blob_storage_messages.h:5: // Multiply-included
message file, hence no include guard.
On 2015/10/16 at 14:24:13, kinuko wrote:
> I know the intention was to split CL for smoother review (oops), but in order
to have meaningful security review on IPC messages I think we probably need
sender/receiver code too

In that case, I'll just put the IPCs in the old file.

https://codereview.chromium.org/1288373002/diff/100001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/100001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:155: std::ostream&
operator<<(std::ostream& os, const BlobDataBuilder& x) {
On 2015/10/16 at 14:24:13, kinuko wrote:
> Are these << operators for logging?  Per style guide operator overloading is
not really encouraged, I think having a method like 'DebugString()' (that just
returns string) is the most common way in chrome (and in google3, if it hasn't
been changed much).  Do we have a strong reason not to do that?
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=debugstring&sq=packag...

I'm doing it for equality testing/printing.  See gtest here:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Teaching_Google_Test_...

I'll do PrintTo(const Bar& bar, ::std::ostream* os) instead, as it isn't
operator overloading. Is that better?

https://codereview.chromium.org/1288373002/diff/100001/storage/common/blob_st...
File storage/common/blob_storage/blob_storage_constants.h (right):

https://codereview.chromium.org/1288373002/diff/100001/storage/common/blob_st...
storage/common/blob_storage/blob_storage_constants.h:19: constexpr size_t
kBlobStorageMaxBlobMemorySize =
On 2015/10/16 at 14:24:13, kinuko wrote:
> constexpr is not allowed in chromium yet.

ok

[kinuko (google), 2015-10-18 03:19:52]

On 2015/10/16 21:37:31, dmurph wrote:
> Kinuko: Did you mean to not add a new IPC header? Or that I should move the
IPC
> changes to the Hookup patch, where they're actually used?

Either should work, or you could ask what security reviews would say. (In the
past I was suggested to add code that actually uses the msgs)

[dmurph, 2015-10-19 22:33:11]

Done, removed messages. I'll put these in the hookup patch.

[dmurph, 2015-10-19 22:34:04]

Description was changed from

==========
[BlobAsync] Patch 2: Messages & Constants
This CL contains messages and constants for the
Blob Async Transport refactor.

Note: This CL is currently a no-op, hookup is in the 'Hookup' patch.

Patches:
1: https://codereview.chromium.org/1287303002 (Committed!)
2: https://codereview.chromium.org/1288373002
3: https://codereview.chromium.org/1292523002
4: https://codereview.chromium.org/1098853003
Hookup: https://codereview.chromium.org/1234813004

BUG=375297
==========

to

==========
[BlobAsync] Patch 2: Common Constants
This CL contains constants for the
Blob Async Transport refactor.

Note: This CL is currently a no-op, hookup is in the 'Hookup' patch.

Patches:
1: https://codereview.chromium.org/1287303002 (Committed!)
2: https://codereview.chromium.org/1288373002
3: https://codereview.chromium.org/1292523002
4: https://codereview.chromium.org/1098853003
Hookup: https://codereview.chromium.org/1234813004

BUG=375297
==========

[dmurph, 2015-10-19 22:35:12]

dmurph@chromium.org changed reviewers:
+ jschuh@chromium.org

[dmurph, 2015-10-19 22:35:12]

+jschuh for content/common/resource_messages.cc

Justin, I'll probably add you for the hookup patch as well, as we chatted about
the Blob page earlier this year in MTV.

[michaeln, 2015-10-20 01:16:14]

lgtm

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:24: case DataElement::TYPE_BYTES:
There's code duplication. Do you intend to remove AppendData, AppendFile,
AppendFileSystemFile, and AppendBlob methods in favor of this one? It'd be nice
to not have duplication in the end.

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:80: auto& item = items_.at(index);
maybe be explicit about what item is
scoped_refptr<BlobDataItem>& item = items_.at(index);

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:82: // On the first call, we are
description, and allocate then.
I don't understand the comment. Is the first call the AppendFutureData call? Ah,
a comment that first explains a future data element can be populated
incrementally would have helped.

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:87: LOG(ERROR) << "Invalid item
type.";
Switch these to DVLOG(1)?
http://www.chromium.org/developers/coding-style

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.h (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:38: void AppendDataElement(const
DataElement& element);
Is this new method needed given the type specific methods of appending various
types of data? The new method doesn't provide any functionality not already
available by one of those.

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:52: //
**************************************
style nit: the asterisk heavy comments look out of place

https://codereview.chromium.org/1288373002/diff/140001/storage/common/data_el...
File storage/common/data_element.cc (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/common/data_el...
storage/common/data_element.cc:5: #include <iomanip>
might not be needed if base::HexEncode is used?

https://codereview.chromium.org/1288373002/diff/140001/storage/common/data_el...
storage/common/data_element.cc:63: os << "TYPE_BYTES, data: [" << std::hex;
could use << base::HexEncode(p, len) here

https://codereview.chromium.org/1288373002/diff/140001/storage/common/data_el...
File storage/common/data_element.h (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/common/data_el...
storage/common/data_element.h:169: return a.length() == b.length();
you can return true here since lengths are compared above

[kinuko, 2015-10-20 10:06:22]

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:77: return true;
I'm not fully sure why this returns true?

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:84:
element->SetToAllocatedBytes(element->length());
Um... should we return here?  Otherwise if the element type is
TYPE_BYTES_DESCRIPTION we call SetToAllocateBytes() then errors out online 88??

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.h (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:48: bool PopulateFutureData(size_t
index,
Could we add a comment when this returns true/false?

https://codereview.chromium.org/1288373002/diff/140001/storage/common/blob_st...
File storage/common/blob_storage/blob_item_bytes_request.h (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/common/blob_st...
storage/common/blob_storage/blob_item_bytes_request.h:19: // not using
std::numeric_limits<T>::max() because of non-C++11 builds.
nit: not -> Not

https://codereview.chromium.org/1288373002/diff/140001/storage/common/blob_st...
storage/common/blob_storage/blob_item_bytes_request.h:81: size_t request_number;
Is request_number something like a seq number for this request? The combination
of size_t type and this name makes it a little difficult for me to guess what
this is for.

[dmurph, 2015-10-20 18:28:28]

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:24: case DataElement::TYPE_BYTES:
On 2015/10/20 at 01:16:14, michaeln wrote:
> There's code duplication. Do you intend to remove AppendData, AppendFile,
AppendFileSystemFile, and AppendBlob methods in favor of this one? It'd be nice
to not have duplication in the end.

I made this method call the Append* methods.  It's useful to have this for the
places where we're adding an IPC-given DataElement, as otherwise I'm duplicating
switch statement and couple DCHECKs. I renamed it to be more clear. IS that
better?

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:77: return true;
On 2015/10/20 at 10:06:22, kinuko wrote:
> I'm not fully sure why this returns true?

I probably shouldn't exit early, you're right.

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:80: auto& item = items_.at(index);
On 2015/10/20 at 01:16:14, michaeln wrote:
> maybe be explicit about what item is
> scoped_refptr<BlobDataItem>& item = items_.at(index);

Removed, as we don't need it.

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:82: // On the first call, we are
description, and allocate then.
On 2015/10/20 at 01:16:14, michaeln wrote:
> I don't understand the comment. Is the first call the AppendFutureData call?
Ah, a comment that first explains a future data element can be populated
incrementally would have helped.

I added more documentation.

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:84:
element->SetToAllocatedBytes(element->length());
On 2015/10/20 at 10:06:22, kinuko wrote:
> Um... should we return here?  Otherwise if the element type is
TYPE_BYTES_DESCRIPTION we call SetToAllocateBytes() then errors out online 88??

It changes to TYPE_BYTES (this is lazy allocation). I added a comment to
clarify.

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:87: LOG(ERROR) << "Invalid item
type.";
On 2015/10/20 at 01:16:14, michaeln wrote:
> Switch these to DVLOG(1)?
> http://www.chromium.org/developers/coding-style

Done.

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.h (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:38: void AppendDataElement(const
DataElement& element);
On 2015/10/20 at 01:16:14, michaeln wrote:
> Is this new method needed given the type specific methods of appending various
types of data? The new method doesn't provide any functionality not already
available by one of those.

I renamed it to be mores specific for IPC data.  It now does the switch
statement, and does a couple DCHECKs. Later we have different spots that add an
IPC data element to the builder, and this allows us to not have to do those
switch statements everywhere.

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:48: bool PopulateFutureData(size_t
index,
On 2015/10/20 at 10:06:22, kinuko wrote:
> Could we add a comment when this returns true/false?

Done.

https://codereview.chromium.org/1288373002/diff/140001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:52: //
**************************************
On 2015/10/20 at 01:16:14, michaeln wrote:
> style nit: the asterisk heavy comments look out of place

Done.

https://codereview.chromium.org/1288373002/diff/140001/storage/common/blob_st...
File storage/common/blob_storage/blob_item_bytes_request.h (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/common/blob_st...
storage/common/blob_storage/blob_item_bytes_request.h:19: // not using
std::numeric_limits<T>::max() because of non-C++11 builds.
On 2015/10/20 at 10:06:22, kinuko wrote:
> nit: not -> Not

Done.

https://codereview.chromium.org/1288373002/diff/140001/storage/common/blob_st...
storage/common/blob_storage/blob_item_bytes_request.h:81: size_t request_number;
On 2015/10/20 at 10:06:22, kinuko wrote:
> Is request_number something like a seq number for this request? The
combination of size_t type and this name makes it a little difficult for me to
guess what this is for.

Not really, it's used by the blob async transportation system to keep track of
memory requests and responses (we need a separate index, as we can have multiple
requests per renderer item and multiple requests per browser item). I added a
comment to clarify.

https://codereview.chromium.org/1288373002/diff/140001/storage/common/data_el...
File storage/common/data_element.cc (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/common/data_el...
storage/common/data_element.cc:5: #include <iomanip>
On 2015/10/20 at 01:16:14, michaeln wrote:
> might not be needed if base::HexEncode is used?

Done.

https://codereview.chromium.org/1288373002/diff/140001/storage/common/data_el...
storage/common/data_element.cc:63: os << "TYPE_BYTES, data: [" << std::hex;
On 2015/10/20 at 01:16:14, michaeln wrote:
> could use << base::HexEncode(p, len) here

Nice catch.

https://codereview.chromium.org/1288373002/diff/140001/storage/common/data_el...
File storage/common/data_element.h (right):

https://codereview.chromium.org/1288373002/diff/140001/storage/common/data_el...
storage/common/data_element.h:169: return a.length() == b.length();
On 2015/10/20 at 01:16:14, michaeln wrote:
> you can return true here since lengths are compared above

Nice catch.

[michaeln, 2015-10-20 20:50:02]

https://codereview.chromium.org/1288373002/diff/240001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/240001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:56: return;
does this mess up index addressability at populate time? maybe callers shouldn't
append zero length elements, assert(length > 0)?

https://codereview.chromium.org/1288373002/diff/240001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.h (right):

https://codereview.chromium.org/1288373002/diff/240001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:40: void AppendIPCDataElement(const
DataElement& element);
nit: use same param name in .h and .cc

[dmurph, 2015-10-20 22:49:35]

Sorry about all of the patches, Android and Mac seem to hate me. Why can't we
just have one toolchain? grumble grumble

https://codereview.chromium.org/1288373002/diff/240001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/240001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:56: return;
On 2015/10/20 at 20:50:02, michaeln wrote:
> does this mess up index addressability at populate time? maybe callers
shouldn't append zero length elements, assert(length > 0)?

Done. I also now return the index of the item, so the caller will know for sure
which index they'll populate later.

https://codereview.chromium.org/1288373002/diff/240001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.h (right):

https://codereview.chromium.org/1288373002/diff/240001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:40: void AppendIPCDataElement(const
DataElement& element);
On 2015/10/20 at 20:50:02, michaeln wrote:
> nit: use same param name in .h and .cc

Done.

[kinuko, 2015-10-22 00:00:07]

lgtm.

[dmurph, 2015-10-22 00:17:47]

Justin: Any chance you could take a look? It's a small file change:
content/common/resource_messages.cc

Thanks Kinuko!

[dmurph, 2015-10-26 22:29:47]

dmurph@chromium.org changed reviewers:
+ cpu@chromium.org
- jschuh@chromium.org

[dmurph, 2015-10-26 22:29:48]

-jschuh@chromium.org, +cpu@chromium.org

Hi Carlos!
We chatted earlier this year about the Blob storage async transportation
refactor.  This is one of the CLs, I was wondering if you could do the review of
resource_messages.cc.
Later we'll be editing the main messages file, and I'll need a review there as
well.

Thanks!
Daniel

[cpu_(ooo_6.6-7.5), 2015-10-27 01:13:27]

resource_messages.cc lgtm

[dmurph, 2015-10-27 18:39:40]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2015-10-27 18:39:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1288373002/#ps310001
(title: "maybe?")

[commit-bot: I haz the power, 2015-10-27 18:40:04]

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

[commit-bot: I haz the power, 2015-10-27 18:54:39]

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

[commit-bot: I haz the power, 2015-10-27 18:54:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmurph, 2015-10-27 18:57:44]

dmurph@chromium.org changed reviewers:
+ palmer@chromium.org

[dmurph, 2015-10-27 18:57:45]

Whoops, Carlos wasn't an owner, my bad.
Chris: can you take a look at resource_messages.cc? It's a super small change.

[palmer, 2015-10-27 20:36:41]

https://codereview.chromium.org/1288373002/diff/310001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/310001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:23: AppendData(ipc_data.bytes(),
length);
Implicit conversion from uint64 to size_t here (|AppendData| takes a size_t, but
here |length| is a uint64). Consider enforcing a 32-bit size limit or using
base/numerics/safe_conversions.h.

https://codereview.chromium.org/1288373002/diff/310001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:84: if (offset + length >
element->length()) {
The expression offset + length could overflow. Consider setting maxima for
|offset| and |length| such that adding them cannot overflow, or use
base/numerics/safe_math.h.

[dmurph, 2015-10-28 00:56:21]

https://codereview.chromium.org/1288373002/diff/310001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/310001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:23: AppendData(ipc_data.bytes(),
length);
On 2015/10/27 at 20:36:41, palmer wrote:
> Implicit conversion from uint64 to size_t here (|AppendData| takes a size_t,
but here |length| is a uint64). Consider enforcing a 32-bit size limit or using
base/numerics/safe_conversions.h.

Done.

https://codereview.chromium.org/1288373002/diff/310001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:84: if (offset + length >
element->length()) {
On 2015/10/27 at 20:36:40, palmer wrote:
> The expression offset + length could overflow. Consider setting maxima for
|offset| and |length| such that adding them cannot overflow, or use
base/numerics/safe_math.h.

Done.

[palmer, 2015-10-30 23:11:43]

https://codereview.chromium.org/1288373002/diff/330001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/330001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:26: base::saturated_cast<size_t,
uint64>(length));
Hmm, is saturation the right semantics here? I don't think you have an
exploitable vulnerability here; I just wonder if checked_cast is what you want.

https://codereview.chromium.org/1288373002/diff/330001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:88: checked_end = checked_end +
length;
Nit: checked_end += length is more concise (IMO). But, either way.

[dmurph, 2015-10-30 23:22:07]

https://codereview.chromium.org/1288373002/diff/330001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/1288373002/diff/330001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:26: base::saturated_cast<size_t,
uint64>(length));
On 2015/10/30 at 23:11:43, palmer wrote:
> Hmm, is saturation the right semantics here? I don't think you have an
exploitable vulnerability here; I just wonder if checked_cast is what you want.

well, it's probably not valid if someone wants to copy data w/ length = the
entire address space, so I guess checked_cast would make more sense.  I'll
switch it.

https://codereview.chromium.org/1288373002/diff/330001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:88: checked_end = checked_end +
length;
On 2015/10/30 at 23:11:43, palmer wrote:
> Nit: checked_end += length is more concise (IMO). But, either way.

Done.

[palmer, 2015-10-30 23:32:38]

> well, it's probably not valid if someone wants to copy data w/ length = the
> entire address space, so I guess checked_cast would make more sense.  I'll
> switch it.

LGTM when you do that (don't need to wait for me).

[dmurph, 2015-10-30 23:34:13]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2015-10-30 23:34:14]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
michaeln@chromium.org, cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/1288373002/#ps350001
(title: "comments")

[commit-bot: I haz the power, 2015-10-30 23:35:39]

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

[commit-bot: I haz the power, 2015-10-31 01:04:59]

Committed patchset #19 (id:350001)

[commit-bot: I haz the power, 2015-10-31 01:05:47]

Patchset 19 (id:??) landed as
https://crrev.com/0140342861a6aa998ffbf44ecfdc360305f560bc
Cr-Commit-Position: refs/heads/master@{#357248}