[BlobAsync] Patch 3: Renderer Classes & Logic

[BlobAsync] Patch 3: Renderer Classes & Logic
Note: This patch is a no-op.

Patches:
1: https://codereview.chromium.org/1287303002
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/910d0ee4c7534c7366f3b1548c50102c67e56007
Cr-Commit-Position: refs/heads/master@{#357258}

[dmurph, 2015-08-13 13:14:42]

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

[dmurph, 2015-08-13 13:14:43]

Hello Michael & Kinuko,

Here is part 3 of the split up BlobAsync patchset.  This encompasses all
Renderer supporting classes & logic.

Thanks!
Daniel

[michaeln, 2015-08-18 02:50:35]

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_controller.cc:28: return
Singleton<BlobTransportController>::get();
Does base::LazyInstance<T>::Leaky work for this? See the following for a
discussion about Singleton vs LazyInstance. 

https://www.chromium.org/developers/coding-style/important-abstractions-and-d...

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_controller.cc:58: LOG(ERROR) <<
"Unable to find blob " << uuid << ".";
Consider switching these to DVLOG(1). See
http://www.chromium.org/developers/coding-style

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_controller.cc:123: // disk calls so
they don't call the sender.
should we clear the map of consolidations here and stop initiating new xfers?

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
File content/child/blob_storage/blob_transport_controller.h (right):

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_controller.h:54: void
OnChannelClose();
Shutdown() might be a better name. When this happens, we also want to stop
accepting new blobs for xfer. It happens when the channel is closed, but this
class doesn't care about that. It just needs to know that its time to
Shutdown().

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
File content/child/blob_storage/blob_transport_temporary_holder.h (right):

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_temporary_holder.h:39: class
CONTENT_EXPORT BlobTransportTemporaryHolder {
This class is mostly just a map. The GetDescriptions() and GetResponses()
methods seem like they'd fit very well on the Consolidation class itself, those
methods describe and then extract the data elements of the consolidation.

I'd vote for one less class and put the  map of consolidations directly in
the transport controller.

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_temporary_holder.h:77:
base::hash_map<std::string, BlobConsolidation*> blob_storage_;
mostly this map will be empty, maybe use a std::map<> to be more space efficient

[dmurph, 2015-10-07 00:32:36]

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_controller.cc:28: return
Singleton<BlobTransportController>::get();
On 2015/08/18 at 02:50:35, michaeln wrote:
> Does base::LazyInstance<T>::Leaky work for this? See the following for a
discussion about Singleton vs LazyInstance. 
> 
>
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...

Done.

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_controller.cc:58: LOG(ERROR) <<
"Unable to find blob " << uuid << ".";
On 2015/08/18 at 02:50:35, michaeln wrote:
> Consider switching these to DVLOG(1). See
http://www.chromium.org/developers/coding-style

Done.

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_controller.cc:123: // disk calls so
they don't call the sender.
On 2015/08/18 at 02:50:35, michaeln wrote:
> should we clear the map of consolidations here and stop initiating new xfers?

Since all of the async versions of the transfers are tied to the
BlobMessageFilter lifetime, I think we don't need to do anything here.

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
File content/child/blob_storage/blob_transport_controller.h (right):

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_controller.h:54: void
OnChannelClose();
On 2015/08/18 at 02:50:35, michaeln wrote:
> Shutdown() might be a better name. When this happens, we also want to stop
> accepting new blobs for xfer. It happens when the channel is closed, but this
> class doesn't care about that. It just needs to know that its time to
> Shutdown().

I actually don't need this anymore. We just use the BlobMessageFilter lifetime
for the disk callback.

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
File content/child/blob_storage/blob_transport_temporary_holder.h (right):

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_temporary_holder.h:39: class
CONTENT_EXPORT BlobTransportTemporaryHolder {
On 2015/08/18 at 02:50:35, michaeln wrote:
> This class is mostly just a map. The GetDescriptions() and GetResponses()
> methods seem like they'd fit very well on the Consolidation class itself,
those
> methods describe and then extract the data elements of the consolidation.
> 
> I'd vote for one less class and put the  map of consolidations directly in
> the transport controller.

As we discussed, I would prefer to keep all the logic here in one class,
especially because all of the disk logic will go here.  Perhaps we can have
better names though.

What about:
BlobTransportControl -> BlobChildTransportRouter or
BlobTransportControllerSingleton
BlobTransportTemporaryHolder -> BlobTransportController

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_temporary_holder.h:77:
base::hash_map<std::string, BlobConsolidation*> blob_storage_;
On 2015/08/18 at 02:50:35, michaeln wrote:
> mostly this map will be empty, maybe use a std::map<> to be more space
efficient

Done.

[michaeln, 2015-10-21 02:22:25]

i think this lgtm modulo the name (and/or perhaps smushing two classes together)

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
File content/child/blob_storage/blob_transport_temporary_holder.h (right):

https://codereview.chromium.org/1292523002/diff/1/content/child/blob_storage/...
content/child/blob_storage/blob_transport_temporary_holder.h:39: class
CONTENT_EXPORT BlobTransportTemporaryHolder {
On 2015/10/07 00:32:36, dmurph wrote:
> On 2015/08/18 at 02:50:35, michaeln wrote:
> > This class is mostly just a map. The GetDescriptions() and GetResponses()
> > methods seem like they'd fit very well on the Consolidation class itself,
> those
> > methods describe and then extract the data elements of the consolidation.
> > 
> > I'd vote for one less class and put the  map of consolidations directly in
> > the transport controller.
> 
> As we discussed, I would prefer to keep all the logic here in one class,
> especially because all of the disk logic will go here.  Perhaps we can have
> better names though.
> 
> What about:
> BlobTransportControl -> BlobChildTransportRouter or
> BlobTransportControllerSingleton
> BlobTransportTemporaryHolder -> BlobTransportController

Maybe this for names?

BlobMessageFilter (just like you have it now)
- this does the ipc message cracking and sending
- there are not many messages in the protocol so it's a very small class

BlobTransportController (just as you have it now)
- this is one step removed from ipc but otherwise has a method per ipc, called
by the filter upon msg receipt
- also, importantly, it has a lifeteime that is distinct from the filter which
is owned by the ipc system
- there are not many messages in the protocol so it's a very small class

BlobTransportLayer || BlobTransport[er] (what your currently calling
BlobTransportTemporaryHolder)
- a map of blob consolidations + methods to extract descriptions and then data
fitting those descriptions from an entry in its map

BlobConsolidation (just as you have it now)
- a vector of consolidated items comprising a blob with a method that allow
memory items to be read/visited.
- composed on other threads and handed off to the blobware on the IPC thread for
transporting

I think understand the division of labor your working on with these classes, but
some of it feels like over kill. I think having a hard time coming up with class
names might be a sign of that. Seems like you could merge the 'transportlayer'
into either the 'controller' or the 'consolidation' and have one less moving
part. Since you're no longer using Singleton<> for the controller (which can be
a pita for testing) is there any reason to not merge the 'controller' with the
'transporter'?

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

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...
content/child/blob_storage/blob_transport_temporary_holder.cc:38: if
(blob_storage_.find(uuid) != blob_storage_.end()) {
probably a DCHECK(blob_storage_.find(uuid) != blob_storage_.end()) would be
sufficient and a void return type.

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...
content/child/blob_storage/blob_transport_temporary_holder.cc:195: }
style nit: maybe could be more early returnish with fewer brackets

auto iter = blob_storage_.find(uuid);
if (iter == blob_storage_.end()) 
  return nullptr;
return iter->second;

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_temporary_holder.h (right):

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...
content/child/blob_storage/blob_transport_temporary_holder.h:56: // Returns
false if there already exists a blob with that uuid.
The return value probably isn't needed, none of the call sites test it nor
should they since there's no usecase for attempting to create a blob with the
same id.

[dmurph, 2015-10-21 21:47:57]

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

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...
content/child/blob_storage/blob_transport_temporary_holder.cc:38: if
(blob_storage_.find(uuid) != blob_storage_.end()) {
On 2015/10/21 at 02:22:24, michaeln wrote:
> probably a DCHECK(blob_storage_.find(uuid) != blob_storage_.end()) would be
sufficient and a void return type.

I want to make sure we exit early in production in case this ever happens, so I
still return bool here and DCHECK in the caller instead.

But GetDescirptions doesn't have a return anymore.

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...
content/child/blob_storage/blob_transport_temporary_holder.cc:195: }
On 2015/10/21 at 02:22:24, michaeln wrote:
> style nit: maybe could be more early returnish with fewer brackets
> 
> auto iter = blob_storage_.find(uuid);
> if (iter == blob_storage_.end()) 
>   return nullptr;
> return iter->second;

Done.

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_temporary_holder.h (right):

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...
content/child/blob_storage/blob_transport_temporary_holder.h:56: // Returns
false if there already exists a blob with that uuid.
On 2015/10/21 at 02:22:24, michaeln wrote:
> The return value probably isn't needed, none of the call sites test it nor
should they since there's no usecase for attempting to create a blob with the
same id.

Ok.  I'm going to keep it for Hold, so we can DCHECK/exit early.

[michaeln, 2015-10-22 21:10:22]

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_temporary_holder.h (right):

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...
content/child/blob_storage/blob_transport_temporary_holder.h:56: // Returns
false if there already exists a blob with that uuid.
On 2015/10/21 21:47:57, dmurph wrote:
> On 2015/10/21 at 02:22:24, michaeln wrote:
> > The return value probably isn't needed, none of the call sites test it nor
> should they since there's no usecase for attempting to create a blob with the
> same id.
> 
> Ok.  I'm going to keep it for Hold, so we can DCHECK/exit early.

Our style guide says "you should not handle DCHECK() failures", so DCHECK
followed by an early exit shouldn't be used.

https://codereview.chromium.org/1292523002/diff/100001/content/child/blob_sto...

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:53: bool not_found =
HoldBlobConsolidation(uuid, consolidation.Pass());
per the style-guide, to defend against duplicate uuid values in release builds
use CHECK

   auto insert_result = blob_storage_.insert(
       std::make_pair(uuid, consolidation.release()));
   CHECK(insert_result.second);

also, probably don't need the helper method to do that

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.h (right):

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:47: class CONTENT_EXPORT
BlobTransportController {
thnx for smushing the classes together!

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:118: void
ReleaseBlob(const std::string& uuid);
naming nit: Releaseonsolidation to harmonize with GetConsolidation.

[michaeln, 2015-10-22 21:12:42]

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.h (right):

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:42: // This function of
this class is to:
This s/b The

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:44: // * delegate calls
to the BlobTransportTemporaryHolder, and
comment is stale

[dmurph, 2015-10-22 23:43:04]

Note: all of the other changes are to fix the comparison operators so they're
linked correctly.

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:53: bool not_found =
HoldBlobConsolidation(uuid, consolidation.Pass());
On 2015/10/22 at 21:10:22, michaeln wrote:
> per the style-guide, to defend against duplicate uuid values in release builds
use CHECK
> 
>    auto insert_result = blob_storage_.insert(
>        std::make_pair(uuid, consolidation.release()));
>    CHECK(insert_result.second);
> 
> also, probably don't need the helper method to do that

Ok. I'll keep HoldBlob* as we use it in the tests.

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.h (right):

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:42: // This function of
this class is to:
On 2015/10/22 at 21:12:42, michaeln wrote:
> This s/b The

Done.

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:44: // * delegate calls
to the BlobTransportTemporaryHolder, and
On 2015/10/22 at 21:12:42, michaeln wrote:
> comment is stale

Fixed.

https://codereview.chromium.org/1292523002/diff/260001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:118: void
ReleaseBlob(const std::string& uuid);
On 2015/10/22 at 21:10:22, michaeln wrote:
> naming nit: Releaseonsolidation to harmonize with GetConsolidation.

Done.

[michaeln, 2015-10-23 23:52:12]

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:155: if
(blob_storage_.find(uuid) != blob_storage_.end()) {
nit: there's code here that doesn't need to be. since all of the tests are
friends of this class, can't they just as easily stuff values directly into the
map?

  controller.blob_storage_[uuid] = p;

[kinuko, 2015-10-26 06:44:22]

Not reviewed all files, some early comments

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:36: static
base::LazyInstance<BlobTransportController> controller_ =
nit: prefer prefixing with g_ without trailing _, or whatever that can be easily
distinguished from member variables

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:74: switch (status) {
For some of those errors I'm not fully sure how these could happen, assuming
that browser should be returning trustful responses.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:170: }
nit: should this be just DCHECK(consolidation) ?

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:230:
base::ScopedPtrHashMap<size_t, scoped_ptr<SharedMemory>> opened_memory;
Sorry, I'm probably missing something obvious... so we create SharedMemory from
the given handle, memcpy the data into it, and hold it as ScopedPtrHashMap. Then
is it ok to just release all these at the end of this method?

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:256:
DCHECK_LT(request.handle_index, memory_handles->size());
Is this handling DCHECK failure pattern?

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.h (right):

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:125:
std::map<std::string, BlobConsolidation*> blob_storage_;
nit: can we have a comment to note that this owns consolidations for inflight
transfers? (map to a raw pointer doesn't tell much about the storage)

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:125:
std::map<std::string, BlobConsolidation*> blob_storage_;
Well, or maybe use ScopedPtrHashMap.

[michaeln, 2015-10-26 20:25:48]

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.h (right):

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:125:
std::map<std::string, BlobConsolidation*> blob_storage_;
On 2015/10/26 06:44:22, kinuko wrote:
> Well, or maybe use ScopedPtrHashMap.

or ScopedPtrMap

[dmurph, 2015-10-28 01:00:52]

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:36: static
base::LazyInstance<BlobTransportController> controller_ =
On 2015/10/26 at 06:44:22, kinuko wrote:
> nit: prefer prefixing with g_ without trailing _, or whatever that can be
easily distinguished from member variables

Done.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:74: switch (status) {
On 2015/10/26 at 06:44:22, kinuko wrote:
> For some of those errors I'm not fully sure how these could happen, assuming
that browser should be returning trustful responses.

They *shouldn't*, but someone can screw up the blob system. Should I do
something different?

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:155: if
(blob_storage_.find(uuid) != blob_storage_.end()) {
On 2015/10/23 at 23:52:12, michaeln wrote:
> nit: there's code here that doesn't need to be. since all of the tests are
friends of this class, can't they just as easily stuff values directly into the
map?
> 
>   controller.blob_storage_[uuid] = p;
Done.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:170: }
On 2015/10/26 at 06:44:22, kinuko wrote:
> nit: should this be just DCHECK(consolidation) ?

Done.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:230:
base::ScopedPtrHashMap<size_t, scoped_ptr<SharedMemory>> opened_memory;
On 2015/10/26 at 06:44:22, kinuko wrote:
> Sorry, I'm probably missing something obvious... so we create SharedMemory
from the given handle, memcpy the data into it, and hold it as ScopedPtrHashMap.
Then is it ok to just release all these at the end of this method?

So we need to handle the case where multiple items are getting saved to the same
shared memory piece. That's why we have the index.  And yes, releasing the
SharedMemory object at the end of the method is what we want, as we no longer
need access to the memory in the renderer.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:256:
DCHECK_LT(request.handle_index, memory_handles->size());
On 2015/10/26 at 06:44:22, kinuko wrote:
> Is this handling DCHECK failure pattern?

Question:

How do I test for a failure case, but have a DCHECK there? How does this sound:
This method will return the error enums, and then we CHECK in the calling
method.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.h (right):

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.h:125:
std::map<std::string, BlobConsolidation*> blob_storage_;
On 2015/10/26 at 06:44:22, kinuko wrote:
> Well, or maybe use ScopedPtrHashMap.

Done.

[kinuko (google), 2015-10-28 03:53:36]

kinuko@google.com changed reviewers:
+ kinuko@google.com

[kinuko (google), 2015-10-28 03:53:36]

looking good % CHECK/DCHECK look a bit confusing to me

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:230:
base::ScopedPtrHashMap<size_t, scoped_ptr<SharedMemory>> opened_memory;
On 2015/10/28 01:00:51, dmurph wrote:
> On 2015/10/26 at 06:44:22, kinuko wrote:
> > Sorry, I'm probably missing something obvious... so we create SharedMemory
> from the given handle, memcpy the data into it, and hold it as
ScopedPtrHashMap.
> Then is it ok to just release all these at the end of this method?
> 
> So we need to handle the case where multiple items are getting saved to the
same
> shared memory piece. That's why we have the index.  And yes, releasing the
> SharedMemory object at the end of the method is what we want, as we no longer
> need access to the memory in the renderer.

Ah I see thanks, that makes it clearer why we have this ScopedPtrHashMap. Could
we have a brief comment about that (to save poor readers like me)?

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:251: <<
static_cast<int>(status);
Does this need to be CHECK?

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:256:
DCHECK_LT(request.handle_index, memory_handles->size());
On 2015/10/28 01:00:51, dmurph wrote:
> On 2015/10/26 at 06:44:22, kinuko wrote:
> > Is this handling DCHECK failure pattern?
> 
> Question:
> 
> How do I test for a failure case, but have a DCHECK there? How does this
sound:
> This method will return the error enums, and then we CHECK in the calling
> method.

Could this error happen without any coding error? If not let's just use DCHECK,
or CHECK if it has security implication. If the error could happen and we want
to handle the error case return an error code. If this shouldn't happen but we
still want to see if we have any crashes in release builds we could temporarily
use CHECK, but would be nice to have a comment about it too... is our common
guide, which probably you already know.

Actually this method has a lot of CHECK and DCHECKs and still returning some
error code that is handled by LOG(FATAL), which also terminates process. It'd be
nice if these are a bit more consistent.

https://codereview.chromium.org/1292523002/diff/320001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/320001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:97: break;
If we're basically doing LOG(FATAL) it feels we could just do DCHECK_EQ(SUCCESS,
status).

If we want to have debug-friendly output for the check I'd make a static routine
that takes ResponsesStatus and returns string.

[michaeln, 2015-10-29 01:24:53]

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:74: switch (status) {
On 2015/10/28 01:00:52, dmurph wrote:
> On 2015/10/26 at 06:44:22, kinuko wrote:
> > For some of those errors I'm not fully sure how these could happen, assuming
> that browser should be returning trustful responses.
> 
> They *shouldn't*, but someone can screw up the blob system. Should I do
> something different?

Are BLOB_NOT_FOUND and SHARED_MEMORY_MAP_FAILED the only ones that should be
expected to happen in a release build? If so, simply don't write code to handle
the other conditions besides DCHECK'ing deeper down the stack where the error is
apparent. If the condition could be expliotable, use CHECK instead of DCHECK.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:77: return;
should this be a break so we send the BlobStorageMsg_CancelBuildingBlob msg in
response?

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:256:
DCHECK_LT(request.handle_index, memory_handles->size());
On 2015/10/28 03:53:36, kinuko (google) wrote:
> On 2015/10/28 01:00:51, dmurph wrote:
> > On 2015/10/26 at 06:44:22, kinuko wrote:
> > > Is this handling DCHECK failure pattern?
> > 
> > Question:
> > 
> > How do I test for a failure case, but have a DCHECK there? How does this
> sound:
> > This method will return the error enums, and then we CHECK in the calling
> > method.
> 
> Could this error happen without any coding error? If not let's just use
DCHECK,
> or CHECK if it has security implication. If the error could happen and we want
> to handle the error case return an error code. If this shouldn't happen but we
> still want to see if we have any crashes in release builds we could
temporarily
> use CHECK, but would be nice to have a comment about it too... is our common
> guide, which probably you already know.
> 
> Actually this method has a lot of CHECK and DCHECKs and still returning some
> error code that is handled by LOG(FATAL), which also terminates process. It'd
be
> nice if these are a bit more consistent.

When it comes to CHECKs and DCHECKs and LOG'ing, our style guide generally has
us tread as lightly as possible in release builds to keep the size of the binary
down. Keeping extraneous strings, conditions, and branches out of an individual
class doesn't add up to much, but doing so across the large number of classes in
the project definitely adds up.

Here's a suggestion, whenever you see console output in a release build,
consider whether your mom would find that output useful :)

Am I right in thinking that many of the error returns in this function could
only be hit if the browser process sent us a very bad message or if the renderer
is in a seriously wrong state? For those kinds of possibilities, where we never
expect to see it happen, DCHECK() is a good choice.

The early returns on 238 and 240, is it true that browser should never ask to
read data from the wrong kind of item or a nonsense offset/range within the
right kind of item? DCHECK. Same question for the return here on line 257?

[dmurph, 2015-10-29 20:42:27]

Thanks for the comments guys, I think it's cleaned up a lot.

The dchecks mean that there are less tests for error conditions, but I guess if
we don't expect things to happen unless people are coding, then DCHECKS are
totally enough.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:74: switch (status) {
On 2015/10/29 at 01:24:53, michaeln wrote:
> On 2015/10/28 01:00:52, dmurph wrote:
> > On 2015/10/26 at 06:44:22, kinuko wrote:
> > > For some of those errors I'm not fully sure how these could happen,
assuming
> > that browser should be returning trustful responses.
> > 
> > They *shouldn't*, but someone can screw up the blob system. Should I do
> > something different?
> 
> Are BLOB_NOT_FOUND and SHARED_MEMORY_MAP_FAILED the only ones that should be
expected to happen in a release build? If so, simply don't write code to handle
the other conditions besides DCHECK'ing deeper down the stack where the error is
apparent. If the condition could be expliotable, use CHECK instead of DCHECK.

Done.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:77: return;
On 2015/10/29 at 01:24:53, michaeln wrote:
> should this be a break so we send the BlobStorageMsg_CancelBuildingBlob msg in
response?

Done now.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:230:
base::ScopedPtrHashMap<size_t, scoped_ptr<SharedMemory>> opened_memory;
On 2015/10/28 at 03:53:36, kinuko (google) wrote:
> On 2015/10/28 01:00:51, dmurph wrote:
> > On 2015/10/26 at 06:44:22, kinuko wrote:
> > > Sorry, I'm probably missing something obvious... so we create SharedMemory
> > from the given handle, memcpy the data into it, and hold it as
ScopedPtrHashMap.
> > Then is it ok to just release all these at the end of this method?
> > 
> > So we need to handle the case where multiple items are getting saved to the
same
> > shared memory piece. That's why we have the index.  And yes, releasing the
> > SharedMemory object at the end of the method is what we want, as we no
longer
> > need access to the memory in the renderer.
> 
> Ah I see thanks, that makes it clearer why we have this ScopedPtrHashMap.
Could we have a brief comment about that (to save poor readers like me)?

Done.  I changed it into a scoped vector as that's way more efficient.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:251: <<
static_cast<int>(status);
On 2015/10/28 at 03:53:36, kinuko (google) wrote:
> Does this need to be CHECK?

no, changed.

https://codereview.chromium.org/1292523002/diff/280001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:256:
DCHECK_LT(request.handle_index, memory_handles->size());
Done. I cleaned this up a lot and only have a couple error cases now.

https://codereview.chromium.org/1292523002/diff/320001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/320001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:97: break;
On 2015/10/28 at 03:53:36, kinuko (google) wrote:
> If we're basically doing LOG(FATAL) it feels we could just do
DCHECK_EQ(SUCCESS, status).
> 
> If we want to have debug-friendly output for the check I'd make a static
routine that takes ResponsesStatus and returns string.

I changed to DCHECKS in the method and then CHECKs for the important two - blob
not found, and memory map failed.

[michaeln, 2015-10-29 23:26:48]

lgtm, thnx for trimming it down

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:224: SharedMemory*
memory = opened_memory.get().at(request.handle_index);
Do you need the .get() to get at the scoped_vector? The the ScopedVector's
operator[]s work? If so, opened_memory[i] would be more readable.

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:233:
opened_memory.get()[request.handle_index] = shared_memory.release();
ditto opened_memory[i] = x;

[kinuko, 2015-10-30 02:43:38]

lgtm!

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:261: delete
consolidation;
wait... now that we use scopedptrmap no need to manually delete, right?

[michaeln, 2015-10-30 19:44:40]

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:9: #include
"base/containers/scoped_ptr_hash_map.h"
is this include needed now that your using a vector instead?

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:261: delete
consolidation;
On 2015/10/30 02:43:38, kinuko wrote:
> wait... now that we use scopedptrmap no need to manually delete, right?

Whoa! Good catch, this would be a dbl delete, right. I'm surprised the trybots
didn't complain about it? Ah, the unit tests don't cover the basics. There
probably should be a test that covers the basic container behavior exposed via
the public methods: InitiateBlobXfer, OnDone, and OnCancel.

[dmurph, 2015-10-30 22:55:33]

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:9: #include
"base/containers/scoped_ptr_hash_map.h"
On 2015/10/30 at 19:44:40, michaeln wrote:
> is this include needed now that your using a vector instead?

Done.

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:224: SharedMemory*
memory = opened_memory.get().at(request.handle_index);
On 2015/10/29 at 23:26:48, michaeln wrote:
> Do you need the .get() to get at the scoped_vector? The the ScopedVector's
operator[]s work? If so, opened_memory[i] would be more readable.

For some reason I didn't see these methods haha.

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:233:
opened_memory.get()[request.handle_index] = shared_memory.release();
On 2015/10/29 at 23:26:48, michaeln wrote:
> ditto opened_memory[i] = x;

Done.

https://codereview.chromium.org/1292523002/diff/340001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:261: delete
consolidation;
On 2015/10/30 at 19:44:40, michaeln wrote:
> On 2015/10/30 02:43:38, kinuko wrote:
> > wait... now that we use scopedptrmap no need to manually delete, right?
> 
> Whoa! Good catch, this would be a dbl delete, right. I'm surprised the trybots
didn't complain about it? Ah, the unit tests don't cover the basics. There
probably should be a test that covers the basic container behavior exposed via
the public methods: InitiateBlobXfer, OnDone, and OnCancel.

Done, and added tests.

[dmurph, 2015-10-31 01:07:18]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2015-10-31 01:07:18]

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

[commit-bot: I haz the power, 2015-10-31 01:07:48]

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

[commit-bot: I haz the power, 2015-10-31 02:10:32]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-10-31 02:11:22]

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