New blobstoragecontext for use in the main browser process, not plugged in yet.

New BlobStorageContext for use in the main browser process. This class is not used yet, just added it with some tests in this CL.

BUG=174200
R=ericu@chromium.org, kinuko@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197720

[michaeln, 2013-04-17 01:23:48]

Either of you willing to review these changes? Eric, you'd have a leg up on blob
refactor by looking. And kinuko this inevitably overlaps with filesystem stuff.

I have to clean it u a little and add some tests, but I think this is basically
ready.

Worth noting that the changes to blob_data and data_element are somewhat
confusing since I'm adding a second way to identify a blob by uuid w/o removing
the existing url scheme. Thats intentional because the two-side patch plan calls
for both ways of identifying things to be used in parallel. See the comments in
the related BUG about that for more info.

https://chromiumcodereview.appspot.com/14139026/diff/5001/webkit/blob/blob_st...
File webkit/blob/blob_storage_context_unittest.cc (right):

https://chromiumcodereview.appspot.com/14139026/diff/5001/webkit/blob/blob_st...
webkit/blob/blob_storage_context_unittest.cc:75: // Make sure add/remove ref
Need to flesh out more tests
- add/remove ref
- host cleanup logic (cleaning up blobs in use in the deleted host)
- weakptr behavior if context is deleted prior to host

[michaeln, 2013-04-17 01:50:11]

If you want to see more context, you can look here...
https://chromiumcodereview.appspot.com/11410019/
... this is peeled off of that larger change.

[ericu, 2013-04-17 21:50:26]

I'll take a look.

On Tue, Apr 16, 2013 at 6:23 PM,  <michaeln@chromium.org> wrote:
> Reviewers: kinuko, ericu,
>
> Message:
> Either of you willing to review these changes? Eric, you'd have a leg up on
> blob
> refactor by looking. And kinuko this inevitably overlaps with filesystem
> stuff.
>
> I have to clean it u a little and add some tests, but I think this is
> basically
> ready.
>
> Worth noting that the changes to blob_data and data_element are somewhat
> confusing since I'm adding a second way to identify a blob by uuid w/o
> removing
> the existing url scheme. Thats intentional because the two-side patch plan
> calls
> for both ways of identifying things to be used in parallel. See the comments
> in
> the related BUG about that for more info.
>
>
>
https://chromiumcodereview.appspot.com/14139026/diff/5001/webkit/blob/blob_st...
> File webkit/blob/blob_storage_context_unittest.cc (right):
>
>
https://chromiumcodereview.appspot.com/14139026/diff/5001/webkit/blob/blob_st...
> webkit/blob/blob_storage_context_unittest.cc:75: // Make sure add/remove
> ref
> Need to flesh out more tests
> - add/remove ref
> - host cleanup logic (cleaning up blobs in use in the deleted host)
> - weakptr behavior if context is deleted prior to host
>
> Description:
> New BlobStorageContext for use in the main browser process. This class is
> not
> used yet, just added it with some tests in this CL.
>
> BUG=174200
>
> Please review this at https://chromiumcodereview.appspot.com/14139026/
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>   M     content/content_tests.gypi
>   M     webkit/base/data_element.h
>   M     webkit/base/data_element.cc
>   M     webkit/blob/blob_data.h
>   M     webkit/blob/blob_data.cc
>   A  +  webkit/blob/blob_storage_context.h
>   A  +  webkit/blob/blob_storage_context.cc
>   A  +  webkit/blob/blob_storage_context_unittest.cc
>   M     webkit/blob/webkit_blob.gypi
>
>

[ericu, 2013-04-18 02:01:58]

https://codereview.chromium.org/14139026/diff/5004/webkit/base/data_element.cc
File webkit/base/data_element.cc (right):

https://codereview.chromium.org/14139026/diff/5004/webkit/base/data_element.c...
webkit/base/data_element.cc:35: length_ = length;
Clear blob_uuid here, just in case.

https://codereview.chromium.org/14139026/diff/5004/webkit/base/data_element.h
File webkit/base/data_element.h (right):

https://codereview.chromium.org/14139026/diff/5004/webkit/base/data_element.h...
webkit/base/data_element.h:107: GURL blob_url_;
I don't see any reason that you need 2 GURL members; they can still share space
until you stop using url_ for blobs.

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:35: //
base::SysInfo::AmountOfPhysicalMemoryMB()
What does this comment mean?  Is this a TODO?

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:36: static const int64 kMaxMemoryUsage = 500
* 1024 * 1024;  // Half a gig.
How'd you pick this number?  I see it used to be twice that much; what's going
to break?

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:53: BlobDataHandle::~BlobDataHandle() {
There's no need to call the PostTask either if !context_->get().  Perhaps pull
that check up front?

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:101: }
Stylistically, I'd probably revoke the public URL for a blob before removing its
last local reference, but it's just a nit.

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:116: DCHECK_EQ(blobs_inuse_map_[uuid], 1);
How about a TODO for something harsher here [and perhaps elsewhere in this file]
than a DCHECK?
This could indicate a compromised renderer.

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:136: void
BlobStorageHost::AddFinishedBlob(const BlobData* blob_data) {
This function stands out as different.  Where is it used?  Where did the caller
get a BlobData that's not referenced by this host's process?

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:209: DCHECK(unfinalized_blob_map_.find(uuid)
== unfinalized_blob_map_.end());
This one definitely needs something stronger than a DCHECK; it allows a bad
renderer to run the browser out of memory.

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:225: // 1) The Data item is denoted by the
raw data and the range.
I see no range for the TYPE_BYTES.

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:230: // All the Blob items in the passing
blob data are resolved and expanded into
What is "the passing blob data"?

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:253: case BlobData::Item::TYPE_BLOB: {
If we always convert BlobData::Items to lowest form, how do we ever get one of
these?  Does the basic Blob constructor not come through this code?

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:272: // until it does, we'll prevent memory
usage over a max amount.
We don't really prevent it--we just prevent it from lasting past the end of this
function.  We should really stop it *before* it gets huge, or at least within 1
BlobData::Item of going over the line.  Add a TODO?

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:293: DCHECK(unfinalized_blob_map_.find(uuid)
!= unfinalized_blob_map_.end());
Also too dangerous for just DCHECK.

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:328: DCHECK_EQ(found->second.second->uuid(),
uuid);
This DCHECK_EQ could be hoisted to line 327.

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:339: public_blob_urls_[blob_url] = uuid;
What if this URL is already in the map?

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.h (right):

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.h:50: base::SequencedTaskRunner* task_runner);
Should we document that it can only be created on the IO thread?

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.h:62: // There is generally one instance per
child process. When the child process
Generally, or always?

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context_unittest.cc (right):

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context_unittest.cc:35: // Make sure its there, finish
building implies a ref of one.
s/its/it's

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context_unittest.cc:68: // Drop the hosts ref to it.
s/hosts/host's/

[kinuko, 2013-04-18 02:36:35]

I'm still on the way reading this, but since Eric seems to have done a good
first review I'm sending my incomplete comments..

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context.h (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:41: class WEBKIT_STORAGE_EXPORT
BlobDataHandle
As a generic comment can we split files for these new classes?

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:57:
scoped_ptr<base::WeakPtr<BlobStorageContext> > context_;
whoa... I'll see when I go on reading the patch but why a handle to BlobData
owns the lifetime of (weakptr of) per-process context?

[michaeln, 2013-04-18 20:42:19]

no new snapshot yet, i'll upload one later today with some changes in light of
comments

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context.h (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:41: class WEBKIT_STORAGE_EXPORT
BlobDataHandle
On 2013/04/18 02:36:35, kinuko wrote:
> As a generic comment can we split files for these new classes?

We could, would it help? I'm refactoring the current Controller class into these
three new tightly coupled classes, so have them in the same .h .cc module for
now at least. Certainly could split them out.

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:57:
scoped_ptr<base::WeakPtr<BlobStorageContext> > context_;
On 2013/04/18 02:36:35, kinuko wrote:
> whoa... I'll see when I go on reading the patch but why a handle to BlobData
> owns the lifetime of (weakptr of) per-process context?

Better than a raw ptr and a comment that sayz "foo shouldn't outlive bar", cross
your fingers and pray.

These 3 new classes all have delete semantics. I'm making it so there are no
restrictions on the order of destruction because those kind of restrictions
historically have been difficult to ensure in higher level consuming code
(content lib).

So this guy keeps a weakptr to the context. It holds that weakptr in a scopedptr
because weakptrs are not threadsafe (iirc, but maybe that's changed?), and this
class takes care to utilize the weakptr only on its thread, including
destruction of the weakptr.

[michaeln, 2013-04-18 20:45:15]

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context.cc (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:61: base::Bind(&DeleteHelper,
base::Passed(&context_), blob_data_));
blob_data_ is not being handled correctly here since its not a threadsafe
refcounted obj... i'll fix that in the next snapshot too

[michaeln, 2013-04-19 21:55:51]

thnx for looking, still no snapshot, but today (gcl upload works today)

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context.cc (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:35: //
base::SysInfo::AmountOfPhysicalMemoryMB()
On 2013/04/18 02:01:58, ericu wrote:
> What does this comment mean?  Is this a TODO?

it is a todo, related to that bug you're cc'd on, just a note to self about the
name of that function

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:53: BlobDataHandle::~BlobDataHandle() {
On 2013/04/18 02:01:58, ericu wrote:
> There's no need to call the PostTask either if !context_->get().  Perhaps pull
> that check up front?

i gotta look more closely, are weakptrs threadsafe these days?

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:101: }
On 2013/04/18 02:01:58, ericu wrote:
> Stylistically, I'd probably revoke the public URL for a blob before removing
its
> last local reference, but it's just a nit.

Done.

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:116: DCHECK_EQ(blobs_inuse_map_[uuid], 1);
On 2013/04/18 02:01:58, ericu wrote:
> How about a TODO for something harsher here [and perhaps elsewhere in this
file]
> than a DCHECK?
> This could indicate a compromised renderer.

i'll just make these changes now and put the return values in and such

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:136: void
BlobStorageHost::AddFinishedBlob(const BlobData* blob_data) {
On 2013/04/18 02:01:58, ericu wrote:
> This function stands out as different.  Where is it used?  Where did the
caller
> get a BlobData that's not referenced by this host's process?

this one is different, it's not in direct support of renderer delivered ipcs.
test cases use this

i need to provide a recipe for creating a blob in the browser process to be sent
to a renderer process. i think the similar context method is involved in that,
but you're right... this host method probably does not belong.

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:225: // 1) The Data item is denoted by the
raw data and the range.
On 2013/04/18 02:01:58, ericu wrote:
> I see no range for the TYPE_BYTES.

i'll look more closely at these comments? the code its talking about is not
changing

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:272: // until it does, we'll prevent memory
usage over a max amount.
On 2013/04/18 02:01:58, ericu wrote:
> We don't really prevent it--we just prevent it from lasting past the end of
this
> function.  We should really stop it *before* it gets huge, or at least within
1
> BlobData::Item of going over the line.  Add a TODO?

good point, i can test above instead of after the fact

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:328: DCHECK_EQ(found->second.second->uuid(),
uuid);
On 2013/04/18 02:01:58, ericu wrote:
> This DCHECK_EQ could be hoisted to line 327.

Done.

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:339: public_blob_urls_[blob_url] = uuid;
On 2013/04/18 02:01:58, ericu wrote:
> What if this URL is already in the map?

yup... another of those 'what you talkin about willis' error conditions that a
renderer may present us with

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context.h (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:50: base::SequencedTaskRunner* task_runner);
On 2013/04/18 02:01:58, ericu wrote:
> Should we document that it can only be created on the IO thread?

should we? i hadn't since it's private and pretty clear that the context is
where these are constructed which is documented as IO thread only... telling
consumers they can only construct just so, when they can't construct at all
didn't seem useful to me.

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:62: // There is generally one instance per
child process. When the child process
On 2013/04/18 02:01:58, ericu wrote:
> Generally, or always?

always i guess, but i can imagine other use cases too. you could cons up a
'host' per idb transaction that mucks with blobs if you don't want to work with
collections of 'handles' for example. upon xaction completion delete that host.

[michaeln, 2013-04-20 00:31:02]

not ready yet, i'm still poking at the consistency checks. I'm leaning towards
making all of those tests being handled by the hosts class instead of having the
responsibility split between the context and host, but that's not reflected in
the CL yet.

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context.h (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:62: // There is generally one instance per
child process. When the child process
On 2013/04/19 21:55:51, michaeln wrote:
> On 2013/04/18 02:01:58, ericu wrote:
> > Generally, or always?
> 
> always i guess, but i can imagine other use cases too. you could cons up a
> 'host' per idb transaction that mucks with blobs if you don't want to work
with
> collections of 'handles' for example. upon xaction completion delete that
host.

Done.

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context_unittest.cc (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context_unittest.cc:35: // Make sure its there, finish
building implies a ref of one.
On 2013/04/18 02:01:58, ericu wrote:
> s/its/it's

Done.

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context_unittest.cc:68: // Drop the hosts ref to it.
On 2013/04/18 02:01:58, ericu wrote:
> s/hosts/host's/

Done.

[ericu, 2013-04-22 17:25:14]

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context.cc (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.cc:53: BlobDataHandle::~BlobDataHandle() {
On 2013/04/19 21:55:51, michaeln wrote:
> On 2013/04/18 02:01:58, ericu wrote:
> > There's no need to call the PostTask either if !context_->get().  Perhaps
pull
> > that check up front?
> 
> i gotta look more closely, are weakptrs threadsafe these days?

Ah, I was assuming that they were; I could be completely wrong about that.

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context.h (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:50: base::SequencedTaskRunner* task_runner);
On 2013/04/19 21:55:51, michaeln wrote:
> On 2013/04/18 02:01:58, ericu wrote:
> > Should we document that it can only be created on the IO thread?
> 
> should we? i hadn't since it's private and pretty clear that the context is
> where these are constructed which is documented as IO thread only... telling
> consumers they can only construct just so, when they can't construct at all
> didn't seem useful to me.

OK.

[michaeln, 2013-04-23 02:03:15]

> i'm still poking at the consistency checks. I'm leaning towards
> making all of those tests being handled by the hosts class instead of having
the
> responsibility split between the context and host, but that's not reflected in
> the CL yet.

I've hoisted the checks into the 'host' class. There are two things i want to
address still in this CL (inline below). The second one may cause me to change
things up again?

https://codereview.chromium.org/14139026/diff/45001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/45001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:62: // Currently, this is incorrect.
want to take care of this prior to commiting

https://codereview.chromium.org/14139026/diff/45001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:117: // legitimately get messages to operate
on things that do not exist.
ditto, prior to commit

[michaeln, 2013-04-23 02:05:52]

eric, i know there are at least two comments from you that i have not addressed
yet too (filesysmte_url_ vs blob_url_ and weakptr spelunking).

[kinuko, 2013-04-23 05:30:39]

(cosmetic comments only)

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context.h (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:41: class WEBKIT_STORAGE_EXPORT
BlobDataHandle
On 2013/04/18 20:42:19, michaeln wrote:
> On 2013/04/18 02:36:35, kinuko wrote:
> > As a generic comment can we split files for these new classes?
> 
> We could, would it help? I'm refactoring the current Controller class into
these
> three new tightly coupled classes, so have them in the same .h .cc module for
> now at least. Certainly could split them out.

I usually find it helpful to find files/classes. The cleanup can be put off
later though.

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:57:
scoped_ptr<base::WeakPtr<BlobStorageContext> > context_;
On 2013/04/18 20:42:19, michaeln wrote:
> On 2013/04/18 02:36:35, kinuko wrote:
> > whoa... I'll see when I go on reading the patch but why a handle to BlobData
> > owns the lifetime of (weakptr of) per-process context?
> 
> Better than a raw ptr and a comment that sayz "foo shouldn't outlive bar",
cross
> your fingers and pray.
> 
> These 3 new classes all have delete semantics. I'm making it so there are no
> restrictions on the order of destruction because those kind of restrictions
> historically have been difficult to ensure in higher level consuming code
> (content lib).

Yup, if this was a mere weak_ptr it would have been easier for me to understand
the semantics.

> So this guy keeps a weakptr to the context. It holds that weakptr in a
scopedptr
> because weakptrs are not threadsafe (iirc, but maybe that's changed?), and
this
> class takes care to utilize the weakptr only on its thread, including
> destruction of the weakptr.

As far as I looked into the comment in weak_ptr.h it says destruction of WeakPtr
can happen on any thread (while dereference on other threads is naturally
prohibited).

If we still need this scoped_ptr<> could we have some comments about that? To be
nicer for future readers... thanks.

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/base/data_e...
File webkit/base/data_element.h (right):

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/base/data_e...
webkit/base/data_element.h:8: #include <vector>
nit: #include <string>
(per lint)

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/base/data_e...
webkit/base/data_element.h:39: // TODO(michaeln): fully switch to using string
uuids for blob identifiers.
nit: can you also link to the issue?

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context_unittest.cc (right):

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/blob/blob_s...
webkit/blob/blob_storage_context_unittest.cc:24: }  // anon namespace
nit: '//  namespace' seems to be more common thesedays in chrome

[michaeln, 2013-04-23 19:27:16]

no new snapshot yet, but the 'done' changes are made locally

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context.h (right):

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:41: class WEBKIT_STORAGE_EXPORT
BlobDataHandle
> I usually find it helpful to find files/classes. The cleanup can be put off
> later though.

ok, will do in a later snapshot

https://chromiumcodereview.appspot.com/14139026/diff/23001/webkit/blob/blob_s...
webkit/blob/blob_storage_context.h:57:
scoped_ptr<base::WeakPtr<BlobStorageContext> > context_;
The comments in weak_ptr.h read as if we don't need the scoped_ptr here. Maybe
I've grown to accustomed to hyper-nannystat'ism in some of our infrastructure
classes :)

I'll use base::WP<T> here directly inline.

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/base/data_e...
File webkit/base/data_element.h (right):

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/base/data_e...
webkit/base/data_element.h:8: #include <vector>
On 2013/04/23 05:30:39, kinuko wrote:
> nit: #include <string>
> (per lint)

Done.

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/base/data_e...
webkit/base/data_element.h:8: #include <vector>
On 2013/04/23 05:30:39, kinuko wrote:
> nit: #include <string>
> (per lint)

Done.

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/base/data_e...
webkit/base/data_element.h:39: // TODO(michaeln): fully switch to using string
uuids for blob identifiers.
On 2013/04/23 05:30:39, kinuko wrote:
> nit: can you also link to the issue?

Done.

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/blob/blob_s...
File webkit/blob/blob_storage_context_unittest.cc (right):

https://chromiumcodereview.appspot.com/14139026/diff/45001/webkit/blob/blob_s...
webkit/blob/blob_storage_context_unittest.cc:24: }  // anon namespace
On 2013/04/23 05:30:39, kinuko wrote:
> nit: '//  namespace' seems to be more common thesedays in chrome

Done.

[michaeln, 2013-04-23 21:11:10]

https://codereview.chromium.org/14139026/diff/5004/webkit/base/data_element.cc
File webkit/base/data_element.cc (right):

https://codereview.chromium.org/14139026/diff/5004/webkit/base/data_element.c...
webkit/base/data_element.cc:35: length_ = length;
On 2013/04/18 02:01:58, ericu wrote:
> Clear blob_uuid here, just in case.

Done. (and the reciprocal in the uuid variant)

https://codereview.chromium.org/14139026/diff/5004/webkit/base/data_element.h
File webkit/base/data_element.h (right):

https://codereview.chromium.org/14139026/diff/5004/webkit/base/data_element.h...
webkit/base/data_element.h:107: GURL blob_url_;
true, not strictly needed, but i think makes removing the dead code easier later
and lends greater clarity now... at least that's the idea... i don't feel
strongly but i think the extra storage for the empty GURL is worth it for those
benefits mentioned

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:53: BlobDataHandle::~BlobDataHandle() {
weakptrs may be deleted on the 'wrong thread', but they cannot be deref'd (or
consequently tested) on the wrong thread... so we can't test context_->get()
early.

[michaeln, 2013-04-23 21:15:10]

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:253: case BlobData::Item::TYPE_BLOB: {
we'll get here by way of script...

var compoundBlob = new Blob(["hello\n", existingBlob])

[michaeln, 2013-04-23 22:12:48]

New snapshot, but i still have a TODO to take care of...

// TODO: Gotta think thru the case where the client tries to
// build too large a blob, and we kill it off. In that case, we could
// legitimately get messages to operate on things that do not exist.

[michaeln, 2013-04-24 18:36:32]

Took care of that TODO...

A blob entry remains in the map under that uuid, but the data is released and
the entry is flagged as having exceeded memory. It can be refed and derefd and
such, but attemtps to read the data will fail with 404 errors. I think that
maintains present day behavior for this condition.

See diffs between 9 and 8 on blob_storage_context.* for details.

> New snapshot, but i still have a TODO to take care of...
> 
> // TODO: Gotta think thru the case where the client tries to
> // build too large a blob, and we kill it off. In that case, we could
> // legitimately get messages to operate on things that do not exist.

[michaeln, 2013-04-24 19:22:46]

I'll wait for review comments prior to splitting out files per class.

[ericu, 2013-04-24 23:54:43]

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:209: DCHECK(unfinalized_blob_map_.find(uuid)
== unfinalized_blob_map_.end());
On 2013/04/18 02:01:58, ericu wrote:
> This one definitely needs something stronger than a DCHECK; it allows a bad
> renderer to run the browser out of memory.

Still there.

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:253: case BlobData::Item::TYPE_BLOB: {
On 2013/04/23 21:15:10, michaeln wrote:
> we'll get here by way of script...
> 
> var compoundBlob = new Blob(["hello\n", existingBlob])

Thx.

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:61: blob_data_->Release();
Wouldn't this be a bit cleaner with a scoped_refptr for blob_data_, and only
handling it specially if we're on the wrong thread?  This manually dealing with
refcounts feels awkward.  What about ReleaseSoon?

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:178: return IsInUse(uuid) &&
(blobs_inuse_map_[uuid] == 1);
Given how often this is called, you might want to optimize it to make only a
single lookup in the map, rather than using IsInUse here.

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:243: if (found->second.flags &
EXCEEDED_MEMORY)
Check for BEING_BUILT too?

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:314: found->second.flags &= ~BEING_BUILT;
DCHECK before clearing it?

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.h (right):

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.h:56: BlobData* blob_data_;  // Intentionally a
raw ptr to a non-thread-safe ref.
Why?

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.h:77: bool CancelBuildingBlob(const
std::string& uuid);
What would you think about WARN_UNUSED_RETURN on these?

[michaeln, 2013-04-25 00:08:00]

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:225: // 1) The Data item is denoted by the
raw data and the range.
> i'll look more closely at these comments? the code its talking about is not
> changing

Changed the comment to read...

  // The blob data is stored in the canonical way which only contains a
  // list of Data, File, and FileSystem items. Aggregated TYPE_BLOB items
  // are expanded into the primitive constituent types.
  // 1) The Data item is denoted by the raw data and length.
  // 2) The File item is denoted by the file path, the range and the expected
  //    modification time.
  // 3) The FileSystem File item is denoted by the FileSystem URL, the range
  //    and the expected modification time.
  // 4) The Blob items are expanded.
  // TODO(michaeln): Would be nice to avoid copying Data items when expanding.

[michaeln, 2013-04-25 01:05:01]

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:272: // until it does, we'll prevent memory
usage over a max amount.
On 2013/04/18 02:01:58, ericu wrote:
> We don't really prevent it--we just prevent it from lasting past the end of
this
> function.  We should really stop it *before* it gets huge, or at least within
1
> BlobData::Item of going over the line.  Add a TODO?

Done.

[michaeln, 2013-04-25 01:21:37]

thnx for looking

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/23001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:209: DCHECK(unfinalized_blob_map_.find(uuid)
== unfinalized_blob_map_.end());
On 2013/04/24 23:54:43, ericu wrote:
> On 2013/04/18 02:01:58, ericu wrote:
> > This one definitely needs something stronger than a DCHECK; it allows a bad
> > renderer to run the browser out of memory.
> 
> Still there.

See the Host class where renderer inputs are examined prior to calling this
private method. The gate keeper is the Host class.

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:61: blob_data_->Release();
On 2013/04/24 23:54:43, ericu wrote:
> Wouldn't this be a bit cleaner with a scoped_refptr for blob_data_, and only
> handling it specially if we're on the wrong thread?  This manually dealing
with
> refcounts feels awkward.  What about ReleaseSoon?

It would not be cleaner because there is no api to tell a scoped_refptr to
fuggedaboutit. There is no way to use ReleaseSoon w/o altering the refcount on
the 'wrong thread'. Here's the recipe for ReleaseSoon.

    DatabaseTracker* tracker = db_tracker_;
    tracker->AddRef();   <<<< addref here on the wrong thread
    db_tracker_ = NULL;  <<<< deref there on the wrong thread
    if (!db_tracker_thread_->ReleaseSoon(FROM_HERE, tracker))
      tracker->Release();

I could have a scoped_ptr to a refptr which is Passed to the other thread, but
thats not really cleaner or clearer.

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:178: return IsInUse(uuid) &&
(blobs_inuse_map_[uuid] == 1);
will do when splitting files

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:243: if (found->second.flags &
EXCEEDED_MEMORY)
On 2013/04/24 23:54:43, ericu wrote:
> Check for BEING_BUILT too?

i'll add a DCHECK here, and an IsBeingBuilt() for use in the Host.

[michaeln, 2013-04-25 21:22:46]

i think i'm at the point of diminishing returns... wdyt?

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:243: if (found->second.flags &
EXCEEDED_MEMORY)
I've massaged the rel and dbg runtime checks a little. My goal with those checks
are...

1) Prevent the blob storage data structs from getting out of whack when given
untrusted renderer inputs.

2) Notice when a renderer is providing bad inputs and return false to the
caller. This will be used in later CLs to terminate the bad renderer.

3) Dbg only checks to let developers know when they're providing unexpected
inputs.

I'm not defending against the consequences of buggy code with that last one in
rel, just helping to identify the buggy code at dbg time (so it never makes it
to rel). Nothing a renderer can say should lead to a crash or trip a DCHECK in
the context.

[michaeln, 2013-04-25 23:37:27]

Took care of signed'ness compile errors and also got rid of n2'edness around
GetMemoryUsage() calculations in #15. The only files that changed where the 2
main files.

[michaeln, 2013-04-26 00:26:57]

https://codereview.chromium.org/14139026/diff/128020/webkit/blob/blob_storage...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/128020/webkit/blob/blob_storage...
webkit/blob/blob_storage_context.cc:298: DCHECK(src.get());
in exceeded_mem situations, this dcheck will trip... i'll remove it

[michaeln, 2013-04-26 00:37:59]

https://codereview.chromium.org/14139026/diff/128020/webkit/blob/blob_storage...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/128020/webkit/blob/blob_storage...
webkit/blob/blob_storage_context.cc:58: // Note: Do not test context_ on the
wrong thread.
changed this code comment to

    // Note: Do not test context_ or alter the blob_data_ refcount
    // on the wrong thread.

[ericu, 2013-04-26 22:17:37]

LGTM

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:61: blob_data_->Release();
On 2013/04/25 01:21:37, michaeln wrote:
> On 2013/04/24 23:54:43, ericu wrote:
> > Wouldn't this be a bit cleaner with a scoped_refptr for blob_data_, and only
> > handling it specially if we're on the wrong thread?  This manually dealing
> with
> > refcounts feels awkward.  What about ReleaseSoon?
> 
> It would not be cleaner because there is no api to tell a scoped_refptr to
> fuggedaboutit. There is no way to use ReleaseSoon w/o altering the refcount on
> the 'wrong thread'. Here's the recipe for ReleaseSoon.
> 
>     DatabaseTracker* tracker = db_tracker_;
>     tracker->AddRef();   <<<< addref here on the wrong thread
>     db_tracker_ = NULL;  <<<< deref there on the wrong thread
>     if (!db_tracker_thread_->ReleaseSoon(FROM_HERE, tracker))
>       tracker->Release();
> 
> I could have a scoped_ptr to a refptr which is Passed to the other thread, but
> thats not really cleaner or clearer.

Arg.  OK, that's clearly not better.  Nevermind.

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:178: return IsInUse(uuid) &&
(blobs_inuse_map_[uuid] == 1);
On 2013/04/25 01:21:37, michaeln wrote:
> will do when splitting files

OK; if that's not going to be in the next upload, please add a TODO.  That way I
can be lazy and not have to remember to check for it ;'>.

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.h (right):

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.h:56: BlobData* blob_data_;  // Intentionally a
raw ptr to a non-thread-safe ref.
On 2013/04/24 23:54:43, ericu wrote:
> Why?

This is still worth a more-helpful comment, though.

[michaeln, 2013-04-29 18:32:48]

i'll run it by the trybots to see if i missed checking any return values (with
msvc WARN_UNUSED_RESULT is a noop), and then do the split into three files.

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.cc:178: return IsInUse(uuid) &&
(blobs_inuse_map_[uuid] == 1);
On 2013/04/26 22:17:37, ericu wrote:
> On 2013/04/25 01:21:37, michaeln wrote:
> > will do when splitting files
> 
> OK; if that's not going to be in the next upload, please add a TODO.  That way
I
> can be lazy and not have to remember to check for it ;'>.

this is done elsewhere, see later comment about where

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
File webkit/blob/blob_storage_context.h (right):

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.h:56: BlobData* blob_data_;  // Intentionally a
raw ptr to a non-thread-safe ref.
> This is still worth a more-helpful comment, though.

I added that greater explanation comment in the .cc file in the vicinity of
where blob_data_ is used.

https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
webkit/blob/blob_storage_context.h:77: bool CancelBuildingBlob(const
std::string& uuid);
On 2013/04/24 23:54:43, ericu wrote:
> What would you think about WARN_UNUSED_RETURN on these?

Done.

https://codereview.chromium.org/14139026/diff/100006/webkit/blob/blob_storage...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/100006/webkit/blob/blob_storage...
webkit/blob/blob_storage_context.cc:460: BlobMap::iterator found =
blob_map_.find(uuid);
Here's the HasUseCountOfOne replacement which is smarter about only doing one
lookup.

[ericu, 2013-04-30 00:45:28]

I haven't checked your split into 3 sets of files--I'm assuming that's
just a mechanical copy-paste with the headers fixed up--but the small
tweaks from my last review to the version before that look good.

On Mon, Apr 29, 2013 at 11:32 AM,  <michaeln@chromium.org> wrote:
> i'll run it by the trybots to see if i missed checking any return values
> (with
> msvc WARN_UNUSED_RESULT is a noop), and then do the split into three files.
>
>
>
>
https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
> File webkit/blob/blob_storage_context.cc (right):
>
>
https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
> webkit/blob/blob_storage_context.cc:178: return IsInUse(uuid) &&
> (blobs_inuse_map_[uuid] == 1);
> On 2013/04/26 22:17:37, ericu wrote:
>>
>> On 2013/04/25 01:21:37, michaeln wrote:
>> > will do when splitting files
>
>
>> OK; if that's not going to be in the next upload, please add a TODO.
>
> That way I
>>
>> can be lazy and not have to remember to check for it ;'>.
>
>
> this is done elsewhere, see later comment about where
>
>
>
https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
> File webkit/blob/blob_storage_context.h (right):
>
>
https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
> webkit/blob/blob_storage_context.h:56: BlobData* blob_data_;  //
> Intentionally a raw ptr to a non-thread-safe ref.
>>
>> This is still worth a more-helpful comment, though.
>
>
> I added that greater explanation comment in the .cc file in the vicinity
> of where blob_data_ is used.
>
>
>
https://codereview.chromium.org/14139026/diff/71001/webkit/blob/blob_storage_...
> webkit/blob/blob_storage_context.h:77: bool CancelBuildingBlob(const
> std::string& uuid);
> On 2013/04/24 23:54:43, ericu wrote:
>>
>> What would you think about WARN_UNUSED_RETURN on these?
>
>
> Done.
>
>
https://codereview.chromium.org/14139026/diff/100006/webkit/blob/blob_storage...
> File webkit/blob/blob_storage_context.cc (right):
>
>
https://codereview.chromium.org/14139026/diff/100006/webkit/blob/blob_storage...
> webkit/blob/blob_storage_context.cc:460: BlobMap::iterator found =
> blob_map_.find(uuid);
> Here's the HasUseCountOfOne replacement which is smarter about only
> doing one lookup.
>
> https://codereview.chromium.org/14139026/

[michaeln, 2013-04-30 01:18:00]

yup, the split was purely mechanical

[kinuko, 2013-04-30 15:13:27]

lgtm

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_data_ha...
File webkit/blob/blob_data_handle.h (right):

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_data_ha...
webkit/blob/blob_data_handle.h:28: class WEBKIT_STORAGE_EXPORT BlobDataHandle
Thanks for splitting the files!!

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
webkit/blob/blob_storage_context.cc:77: BlobUrlHasRef(url) ?
ClearBlobUrlRef(url) : url);
nit: looks like it's same if we just use ClearBlobUrlRef(url)?

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
webkit/blob/blob_storage_context.cc:307: bool BlobStorageContext::IsInUse(const
std::string& uuid) {
(I felt IsAlive() or something might be more intuitive, but then found we also
have corresponding IsInUseHost which refer inuse_map-- so probably it's the
right term it should be.)

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
File webkit/blob/blob_storage_context.h (right):

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
webkit/blob/blob_storage_context.h:30: // threaded and should  only be used on
the IO thread.
nit: extra space between 'should' and 'only'

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
File webkit/blob/blob_storage_context_unittest.cc (right):

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
webkit/blob/blob_storage_context_unittest.cc:87: 
nit: extra empty line

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
webkit/blob/blob_storage_context_unittest.cc:132:
EXPECT_TRUE(*(blob_data_handle->data()) ==*canonicalized_blob_data2);
nit: space after ==

[michaeln, 2013-04-30 18:14:07]

hey, thnx for looking

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
File webkit/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
webkit/blob/blob_storage_context.cc:77: BlobUrlHasRef(url) ?
ClearBlobUrlRef(url) : url);
On 2013/04/30 15:13:27, kinuko wrote:
> nit: looks like it's same if we just use ClearBlobUrlRef(url)?

one produces a new GURL and does assignments and the such, while the other
doesn't... just avoiding the extra copies to cater to the common case (no refs)

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
File webkit/blob/blob_storage_context.h (right):

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
webkit/blob/blob_storage_context.h:30: // threaded and should  only be used on
the IO thread.
On 2013/04/30 15:13:27, kinuko wrote:
> nit: extra space between 'should' and 'only'

Done.

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
File webkit/blob/blob_storage_context_unittest.cc (right):

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
webkit/blob/blob_storage_context_unittest.cc:87: 
On 2013/04/30 15:13:27, kinuko wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/14139026/diff/165001/webkit/blob/blob_storage...
webkit/blob/blob_storage_context_unittest.cc:132:
EXPECT_TRUE(*(blob_data_handle->data()) ==*canonicalized_blob_data2);
On 2013/04/30 15:13:27, kinuko wrote:
> nit: space after ==

Done.

[michaeln, 2013-05-01 21:28:37]

Committed patchset #18 manually as r197720.