Remove stl_util's deletion functions from storage/.

Remove stl_util's deletion functions from storage/.

BUG=555865
Committed: https://crrev.com/cc88db2f55f5d9a5b9f48e687501438273c565b4
Cr-Commit-Position: refs/heads/master@{#423007}

[Avi (use Gerrit), 2016-10-01 23:20:21]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 23:20:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-02 00:02:08]

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

[commit-bot: I haz the power, 2016-10-02 00:02:09]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-02 00:14:18]

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

[Avi (use Gerrit), 2016-10-02 00:14:19]

Can you take a look? In particular, what is up with the function that ignores
its argument?

https://codereview.chromium.org/2389583002/diff/1/storage/browser/blob/blob_r...
File storage/browser/blob/blob_reader.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/blob/blob_r...
storage/browser/blob/blob_reader.cc:663: DCHECK_EQ(index, current_item_index_);
OK, what is going on here?

This function says "set the file reader at this index" but it **ignored** the
index passed in, and always used the current_item_index_. I'm checking that
they're equal out of paranoia, but this interface makes little sense to me.

[danakj, 2016-10-03 22:51:48]

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

[danakj, 2016-10-03 22:51:49]

https://codereview.chromium.org/2389583002/diff/1/storage/browser/blob/blob_r...
File storage/browser/blob/blob_reader.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/blob/blob_r...
storage/browser/blob/blob_reader.cc:663: DCHECK_EQ(index, current_item_index_);
On 2016/10/02 00:14:19, Avi wrote:
> OK, what is going on here?
> 
> This function says "set the file reader at this index" but it **ignored** the
> index passed in, and always used the current_item_index_. I'm checking that
> they're equal out of paranoia, but this interface makes little sense to me.

Good question. +dmurph who wrote this.

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/cop...
File storage/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/cop...
storage/browser/fileapi/copy_or_move_operation_delegate.cc:850: ;
extra;

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/cop...
storage/browser/fileapi/copy_or_move_operation_delegate.cc:909:
running_copy_set_.erase(
I assume this set is small and not being called much and thats why we're doing
std::find_if instead of a map where we could use better than O(n) map::find?
Maybe you can say this in a comment.

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/fil...
File storage/browser/fileapi/file_system_usage_cache.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/fil...
storage/browser/fileapi/file_system_usage_cache.cc:233: if (entry)
This is slightly different but it looks like we never have entries in the map
with a null. Can you say that in a comment?

https://codereview.chromium.org/2389583002/diff/1/storage/browser/quota/stora...
File storage/browser/quota/storage_monitor.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/quota/stora...
storage/browser/quota/storage_monitor.cc:259: if (!host_observers)
Ditto can you comment that there's never a null value in the map?

https://codereview.chromium.org/2389583002/diff/1/storage/browser/quota/stora...
storage/browser/quota/storage_monitor.cc:333: if (!type_observers)
dittos

https://codereview.chromium.org/2389583002/diff/1/storage/browser/quota/stora...
storage/browser/quota/storage_monitor.cc:361: return NULL;
nullptr while you're here?

[Avi (use Gerrit), 2016-10-04 15:56:04]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 15:56:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2016-10-04 15:56:22]

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/cop...
File storage/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/cop...
storage/browser/fileapi/copy_or_move_operation_delegate.cc:850: ;
On 2016/10/03 22:51:49, danakj wrote:
> extra;

Done.

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/cop...
storage/browser/fileapi/copy_or_move_operation_delegate.cc:909:
running_copy_set_.erase(
On 2016/10/03 22:51:49, danakj wrote:
> I assume this set is small and not being called much and thats why we're doing
> std::find_if instead of a map where we could use better than O(n) map::find?
> Maybe you can say this in a comment.

Switching to map.

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/fil...
File storage/browser/fileapi/file_system_usage_cache.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/fil...
storage/browser/fileapi/file_system_usage_cache.cc:233: if (entry)
On 2016/10/03 22:51:49, danakj wrote:
> This is slightly different but it looks like we never have entries in the map
> with a null. Can you say that in a comment?

The old code is weird. It does an insert with a null just to see if an insert
succeeds, and if the insert succeeds then it constructs a file.

Because the old code is weird, I rewrote it when I switched containers. I don't
see why we need to bring over the weirdness to the new code; the default
assumption that I see is that of course the entry in the map wouldn't be null.

https://codereview.chromium.org/2389583002/diff/1/storage/browser/quota/stora...
File storage/browser/quota/storage_monitor.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/quota/stora...
storage/browser/quota/storage_monitor.cc:259: if (!host_observers)
On 2016/10/03 22:51:49, danakj wrote:
> Ditto can you comment that there's never a null value in the map?

Again, I don't get it. When we have a key->value map, the values are never null.
Why would I need to comment it?

This is a shortcut; if the operator [] put in a new value, it'll be null, and I
use that to know to insert a new unique_ptr. This code looks to me to be obvious
and not needed of commentary; do you see what I'm doing as unclear? :(

https://codereview.chromium.org/2389583002/diff/1/storage/browser/quota/stora...
storage/browser/quota/storage_monitor.cc:333: if (!type_observers)
On 2016/10/03 22:51:49, danakj wrote:
> dittos

dittos

https://codereview.chromium.org/2389583002/diff/1/storage/browser/quota/stora...
storage/browser/quota/storage_monitor.cc:361: return NULL;
On 2016/10/03 22:51:49, danakj wrote:
> nullptr while you're here?

Done.

[commit-bot: I haz the power, 2016-10-04 16:40:12]

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

[commit-bot: I haz the power, 2016-10-04 16:40:13]

Dry run: This issue passed the CQ dry run.

[dmurph, 2016-10-04 19:23:24]

blob/ lgtm

You can remove the dcheck in the blob reader, and keep it using the index arg.

https://codereview.chromium.org/2389583002/diff/1/storage/browser/blob/blob_r...
File storage/browser/blob/blob_reader.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/blob/blob_r...
storage/browser/blob/blob_reader.cc:663: DCHECK_EQ(index, current_item_index_);
On 2016/10/03 22:51:49, danakj wrote:
> On 2016/10/02 00:14:19, Avi wrote:
> > OK, what is going on here?
> > 
> > This function says "set the file reader at this index" but it **ignored**
the
> > index passed in, and always used the current_item_index_. I'm checking that
> > they're equal out of paranoia, but this interface makes little sense to me.
> 
> Good question. +dmurph who wrote this.

Ah, sorry for this. You can remove the DCHECK, this is only ever called w/
current_item_index_ and it should have been changed.

[Avi (use Gerrit), 2016-10-04 19:55:24]

dmurph, thank you for the clarification. Done.

Dana?

[Avi (use Gerrit), 2016-10-04 19:55:37]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[danakj, 2016-10-04 19:55:52]

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/fil...
File storage/browser/fileapi/file_system_usage_cache.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/fil...
storage/browser/fileapi/file_system_usage_cache.cc:233: if (entry)
On 2016/10/04 15:56:22, Avi wrote:
> On 2016/10/03 22:51:49, danakj wrote:
> > This is slightly different but it looks like we never have entries in the
map
> > with a null. Can you say that in a comment?
> 
> The old code is weird. It does an insert with a null just to see if an insert
> succeeds, and if the insert succeeds then it constructs a file.
> 
> Because the old code is weird, I rewrote it when I switched containers. I
don't
> see why we need to bring over the weirdness to the new code; the default
> assumption that I see is that of course the entry in the map wouldn't be null.

I just think it would be helpful when reading this code to see a comment about
the assumption its making about the rest of the class, that null is not a valid
thing in the container. I think it's a fine assumption, but documenting that
it's being made.

[commit-bot: I haz the power, 2016-10-04 19:56:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-10-04 19:56:39]

LGTM

[commit-bot: I haz the power, 2016-10-04 21:55:22]

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

[commit-bot: I haz the power, 2016-10-04 21:55:23]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-04 22:06:07]

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/fil...
File storage/browser/fileapi/file_system_usage_cache.cc (right):

https://codereview.chromium.org/2389583002/diff/1/storage/browser/fileapi/fil...
storage/browser/fileapi/file_system_usage_cache.cc:233: if (entry)
On 2016/10/04 19:55:52, danakj wrote:
> On 2016/10/04 15:56:22, Avi wrote:
> > On 2016/10/03 22:51:49, danakj wrote:
> > > This is slightly different but it looks like we never have entries in the
> map
> > > with a null. Can you say that in a comment?
> > 
> > The old code is weird. It does an insert with a null just to see if an
insert
> > succeeds, and if the insert succeeds then it constructs a file.
> > 
> > Because the old code is weird, I rewrote it when I switched containers. I
> don't
> > see why we need to bring over the weirdness to the new code; the default
> > assumption that I see is that of course the entry in the map wouldn't be
null.
> 
> I just think it would be helpful when reading this code to see a comment about
> the assumption its making about the rest of the class, that null is not a
valid
> thing in the container. I think it's a fine assumption, but documenting that
> it's being made.

Done.

[Avi (use Gerrit), 2016-10-04 22:06:24]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-10-04 22:06:25]

The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2389583002/#ps60001
(title: "nits")

[commit-bot: I haz the power, 2016-10-04 22:07:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 23:52:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-04 23:58:27]

Description was changed from

==========
Remove stl_util's deletion functions from storage/.

BUG=555865
==========

to

==========
Remove stl_util's deletion functions from storage/.

BUG=555865

Committed: https://crrev.com/cc88db2f55f5d9a5b9f48e687501438273c565b4
Cr-Commit-Position: refs/heads/master@{#423007}
==========

[commit-bot: I haz the power, 2016-10-04 23:58:28]

Patchset 4 (id:??) landed as
https://crrev.com/cc88db2f55f5d9a5b9f48e687501438273c565b4
Cr-Commit-Position: refs/heads/master@{#423007}