Add Stream support to WebBlobRegistry and FileAPIMessageFilter.

Add Stream support to WebBlobRegistry and FileAPIMessageFilter.

Some of existing IPCs and methods for Blob are renamed for readability.

Note
- raw data sending code in webblobregistry_impl.cc are duplicated intentionally
not to bother michaeln's refactoring

Bonus: Stopped over-optimization of sharing webkit_blob::BlobData::Item instances in webblobregistry_impl.cc

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

[tyoshino (SeeGerritForStatus), 2013-06-24 14:26:08]

Hi michaeln, could you please do initial review focusing on overall design?

Thanks

[michaeln, 2013-06-24 20:48:17]

https://codereview.chromium.org/15817013/diff/24004/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/15817013/diff/24004/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:196:
scoped_refptr<StreamContext> stream_context_;
This separate 'context' class in the content layer will definitely make things
more difficult for the BlobDataHandle class and its usage below the content
layer more difficult. I'm not sure what to do about that and am wondering what
your thoughts are?

I'm referring to changes along the lines of this uncommitted CL.
https://codereview.chromium.org/11410019/diff/87001/content/browser/fileapi/f...

I kinda wish content/browser/stream was in webkit/browser/stream instead to make
it more possible to plugin these things together.

https://codereview.chromium.org/15817013/diff/24004/content/child/webblobregi...
File content/child/webblobregistry_impl.cc (right):

https://codereview.chromium.org/15817013/diff/24004/content/child/webblobregi...
content/child/webblobregistry_impl.cc:36: webkit_blob::BlobData::Item* item) {
Do callers of this method use in any way the modified 'item' value?

If not, i think it would make sense to remove the ptr from the param list and to
just define one locally on the stack. Wdyt?

[tyoshino (SeeGerritForStatus), 2013-06-25 08:38:00]

https://codereview.chromium.org/15817013/diff/24004/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/15817013/diff/24004/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:196:
scoped_refptr<StreamContext> stream_context_;
On 2013/06/24 20:48:17, michaeln wrote:
> This separate 'context' class in the content layer will definitely make things
> more difficult for the BlobDataHandle class and its usage below the content
> layer more difficult. I'm not sure what to do about that and am wondering what
> your thoughts are?

I think we can merge them into one. I'll try.

> 
> I'm referring to changes along the lines of this uncommitted CL.
>
https://codereview.chromium.org/11410019/diff/87001/content/browser/fileapi/f...
> 
> I kinda wish content/browser/stream was in webkit/browser/stream instead to
make
> it more possible to plugin these things together.

Looks like it's not too hard to move stream to webkit/ dir. But how should
everything be eventually? src/webkit/ will be kept for components that are
independent from content implementation detail, or will be merged into the
content tree?

https://codereview.chromium.org/15817013/diff/24004/content/child/webblobregi...
File content/child/webblobregistry_impl.cc (right):

https://codereview.chromium.org/15817013/diff/24004/content/child/webblobregi...
content/child/webblobregistry_impl.cc:36: webkit_blob::BlobData::Item* item) {
On 2013/06/24 20:48:17, michaeln wrote:
> Do callers of this method use in any way the modified 'item' value?
> 
> If not, i think it would make sense to remove the ptr from the param list and
to
> just define one locally on the stack. Wdyt?

One Item instance is shared between code for each type. There's no interaction
between them. Done.

[tyoshino (SeeGerritForStatus), 2013-06-25 14:04:27]

On 2013/06/25 08:38:00, tyoshino wrote:
> On 2013/06/24 20:48:17, michaeln wrote:
> > This separate 'context' class in the content layer will definitely make
things
> > more difficult for the BlobDataHandle class and its usage below the content
> > layer more difficult. I'm not sure what to do about that and am wondering
what
> > your thoughts are?
> 
> I think we can merge them into one. I'll try.

Created https://codereview.chromium.org/17653005/. Now testing.

[tyoshino (SeeGerritForStatus), 2013-07-03 08:02:10]

On 2013/06/25 08:38:00, tyoshino wrote:
> Looks like it's not too hard to move stream to webkit/ dir. But how should
> everything be eventually? src/webkit/ will be kept for components that are
> independent from content implementation detail, or will be merged into the
> content tree?

Had some chat with kinuko@. How about moving stream stuff to webkit/ as
tentative refactoring?

[tyoshino (SeeGerritForStatus), 2013-07-03 12:37:43]

Added unittests

[tyoshino (SeeGerritForStatus), 2013-07-17 03:44:03]

ping?

I'd like to enable Stream first and then do moves. I can help you resolving
issues on your work which our work introduces.

[kinuko, 2013-07-18 06:06:55]

Michael seems to be out or busy, so let me do the first review.

https://codereview.chromium.org/15817013/diff/42001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/42001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:186:
IPC_MESSAGE_HANDLER(BlobHostMsg_StartBuildingStream, OnStartBuildingStream)
Sometimes we interchangeably use the term 'blob' and 'stream' (especially when
talking about BlobURL) but sometimes not, which confuses me a bit.

We seem to start building a stream by OnStartBuildingStream, but finish it with
OnFinishBuildingBlob, which feels weird to me.

Similarly OnAppendBlobDataItem can also add data to a stream feels confusing.

If 'Stream' is a blob I feel OnStartBuildingStream should be named in the same
manner, like OnStartBuildingBlobForStream, and the receiving side should have a
single thin 'BlobURLRegistry' or 'BlobURLHandler' class which performs the
actual demux for blob_storage_context and stream_context internally (so that
FileAPIMessageHandler can just focus on IPC message handling).

Otherwise I'd probably add separate IPC messages/stub methods for
OnAppendStreamDataItem, OnFinishBuildingStream and separate the process more
clearly.

https://codereview.chromium.org/15817013/diff/42001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:618: stream->AddData(buffer,
buffer_size);
nit: it might be easier to read if Stream class just has a method to add bytes
data (which internally creates IOBuffer and takes its ownership)

https://codereview.chromium.org/15817013/diff/42001/content/browser/streams/s...
File content/browser/streams/stream.h (right):

https://codereview.chromium.org/15817013/diff/42001/content/browser/streams/s...
content/browser/streams/stream.h:80: // pass right data (e.g.
FileAPIMessageFilter).
Do you have an issue number or link to the discussion?

[tyoshino (SeeGerritForStatus), 2013-07-23 07:27:34]

https://codereview.chromium.org/15817013/diff/42001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/42001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:186:
IPC_MESSAGE_HANDLER(BlobHostMsg_StartBuildingStream, OnStartBuildingStream)
On 2013/07/18 06:06:55, kinuko wrote:
> Sometimes we interchangeably use the term 'blob' and 'stream' (especially when
> talking about BlobURL) but sometimes not, which confuses me a bit.

Darin's idea was making Blob a subclass of the Stream. So, following this idea,
ideally and ultimately .*Blob should be renamed to .*Stream.

But the discussion is suspended. It's too early to rename them to .*Stream. I
think
it's reasonable to use .*Blob as namespace for both Blob and Stream for a while.

> 
> We seem to start building a stream by OnStartBuildingStream, but finish it
with
> OnFinishBuildingBlob, which feels weird to me.

OK. Let me add BlobHostMsg_FinishBuildingStream as
BlobHostMsg_FinishBuildingBlob
and BlobHostMsg_FinishBuildingStream have different parameters.

Re: Clone and Remove, may I rename them by removing Blob from them? They're
common
between Blob and Stream and have the same parameters as well as
AppendBlobDataItem and
SyncAppendSharedMemory.

> 
> Similarly OnAppendBlobDataItem can also add data to a stream feels confusing.

I don't think it's so much confusing. BlobDataItem means what data source is.
Not the type of objective of the operation. Of course, it's better if we could
give more generic name to BlobDataItem.

> 
> If 'Stream' is a blob I feel OnStartBuildingStream should be named in the same
> manner, like OnStartBuildingBlobForStream, and the receiving side should have
a
> single thin 'BlobURLRegistry' or 'BlobURLHandler' class which performs the
> actual demux for blob_storage_context and stream_context internally (so that
> FileAPIMessageHandler can just focus on IPC message handling).
> 

Sounds good refactoring.

> Otherwise I'd probably add separate IPC messages/stub methods for
> OnAppendStreamDataItem, OnFinishBuildingStream and separate the process more
> clearly.
>

[kinuko, 2013-07-23 08:07:42]

https://codereview.chromium.org/15817013/diff/42001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/42001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:186:
IPC_MESSAGE_HANDLER(BlobHostMsg_StartBuildingStream, OnStartBuildingStream)
On 2013/07/23 07:27:34, tyoshino wrote:
> On 2013/07/18 06:06:55, kinuko wrote:
> > Sometimes we interchangeably use the term 'blob' and 'stream' (especially
when
> > talking about BlobURL) but sometimes not, which confuses me a bit.
> 
> Darin's idea was making Blob a subclass of the Stream. So, following this
idea,
> ideally and ultimately .*Blob should be renamed to .*Stream.
>
> But the discussion is suspended. It's too early to rename them to .*Stream. I
> think
> it's reasonable to use .*Blob as namespace for both Blob and Stream for a
while.
> 
> > We seem to start building a stream by OnStartBuildingStream, but finish it
> with
> > OnFinishBuildingBlob, which feels weird to me.
> 
> OK. Let me add BlobHostMsg_FinishBuildingStream as
> BlobHostMsg_FinishBuildingBlob
> and BlobHostMsg_FinishBuildingStream have different parameters.

This sounds good to me.

(I initially thought we could probably just rename StartBuildingStream to
StartBuildingBlobForStream to clarify that we use the term 'blob' for stream and
keep everything else as is, but if it doesn't work for you)

> Re: Clone and Remove, may I rename them by removing Blob from them? They're
> common
> between Blob and Stream and have the same parameters as well as
> AppendBlobDataItem and
> SyncAppendSharedMemory.

Does it make sense to simply name them to like: OnCloneBlobOrStream,
OnRemoveBlobOrStream?

(I'm afraid Remove conflicts with Remove for FileSystem API. Msg names are ok as
they're prefixed, but method names are problematic)

> > Similarly OnAppendBlobDataItem can also add data to a stream feels
confusing.
> 
> I don't think it's so much confusing. BlobDataItem means what data source is.
> Not the type of objective of the operation. Of course, it's better if we could
> give more generic name to BlobDataItem.

Ok, works for me.

> > If 'Stream' is a blob I feel OnStartBuildingStream should be named in the
same
> > manner, like OnStartBuildingBlobForStream, and the receiving side should
have
> a
> > single thin 'BlobURLRegistry' or 'BlobURLHandler' class which performs the
> > actual demux for blob_storage_context and stream_context internally (so that
> > FileAPIMessageHandler can just focus on IPC message handling).
> > 
> 
> Sounds good refactoring.
> 
> > Otherwise I'd probably add separate IPC messages/stub methods for
> > OnAppendStreamDataItem, OnFinishBuildingStream and separate the process more
> > clearly.
> >  
>

[tyoshino (SeeGerritForStatus), 2013-07-23 11:54:38]

https://codereview.chromium.org/15817013/diff/42001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/42001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:186:
IPC_MESSAGE_HANDLER(BlobHostMsg_StartBuildingStream, OnStartBuildingStream)
On 2013/07/23 08:07:42, kinuko wrote:
> Does it make sense to simply name them to like: OnCloneBlobOrStream,
> OnRemoveBlobOrStream?

Done

> 
> (I'm afraid Remove conflicts with Remove for FileSystem API. Msg names are ok
as
> they're prefixed, but method names are problematic)
> 

Yes..

> > > Similarly OnAppendBlobDataItem can also add data to a stream feels
> confusing.
> > 
> > I don't think it's so much confusing. BlobDataItem means what data source
is.
> > Not the type of objective of the operation. Of course, it's better if we
could
> > give more generic name to BlobDataItem.

https://codereview.chromium.org/15817013/diff/42001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:618: stream->AddData(buffer,
buffer_size);
On 2013/07/18 06:06:55, kinuko wrote:
> nit: it might be easier to read if Stream class just has a method to add bytes
> data (which internally creates IOBuffer and takes its ownership)

Done.

https://codereview.chromium.org/15817013/diff/42001/content/browser/streams/s...
File content/browser/streams/stream.h (right):

https://codereview.chromium.org/15817013/diff/42001/content/browser/streams/s...
content/browser/streams/stream.h:80: // pass right data (e.g.
FileAPIMessageFilter).
On 2013/07/18 06:06:55, kinuko wrote:
> Do you have an issue number or link to the discussion?

Just filed. crbug.com/263342

[tyoshino (SeeGerritForStatus), 2013-07-23 12:01:44]

Hi jam, please review files excluding _messages.h and fileapi/ subdir.

Hi tsepez, please review webblob_messages.h

Kinuko, please keep reviewing fileapi/ subdir.

[kinuko, 2013-07-23 13:57:23]

fileapi changes lgtm modulo some nits.

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:114: }
nit: please match the method order in .cc and .h (i.e. this needs to be placed
after HasPermissionsForFile)

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:552: url.path(),
"blobinternal%3A///", true /* case_sensitive*/));
nit: space between case_sensitive and '*/'

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:639: stream->Finalize();
Should we DCHECK(stream.get()) here?

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:56: // FileSystemHostMsg_
family IPCs into separate classes.
nit: would be nicer to file a bug and put a shorter comment here

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter_unittest.cc (right):

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter_unittest.cc:69: return
static_cast<IPC::ChannelProxy::MessageFilter*>(filter_.get());
nit: since we almost always call OnMessageReceived() after this cast, it might
be simpler to just add a helper method like:

bool OnMessageReceived(const IPC::Message& message) {
  return static_cast<...>(filter_.get())->OnMessageReceived(message);
}

https://codereview.chromium.org/15817013/diff/68001/content/browser/streams/s...
File content/browser/streams/stream.h (right):

https://codereview.chromium.org/15817013/diff/68001/content/browser/streams/s...
content/browser/streams/stream.h:59: // Adds data of |size| at |data| to the
stream.
nit: can you explicitly comment this copies |data| into its internal buffer?

[jam, 2013-07-23 15:52:36]

lgtm

https://codereview.chromium.org/15817013/diff/68001/content/child/webblobregi...
File content/child/webblobregistry_impl.cc (right):

https://codereview.chromium.org/15817013/diff/68001/content/child/webblobregi...
content/child/webblobregistry_impl.cc:129:
DCHECK(ChildThread::current()->message_loop() ==
nit: here and in the rest of the file, ChildThread::current() returns non-NULL
only when you're on that thread, so this can just be written as

DCHECK(ChildThread::current());

[Tom Sepez, 2013-07-23 18:08:24]

Messages themselves look OK, but a couple of security concerns about how we
process them.

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:551: DCHECK(StartsWithASCII(
What happens if a compromised renderer violates this assumption? DCHECK isn't
strong enough to protect us here in the absence of other checks later on.
Perhaps call BadMessageReceived here.

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:557: GURL() /* security_origin
*/,
Does this mean all streams share the same origin?

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:602:
DCHECK(base::SharedMemory::IsHandleValid(handle));
What happens if a compromised renderer either lies about buffer_size, or passes
back one of its handles not associated with shared memory?

[michaeln, 2013-07-23 20:19:41]

> I'd like to enable Stream first and then do moves

Is there an webapp that's blocked right now on the functionality provided by
this CL?


Given these changes, I'm kinda stymied with how to proceed with these
refactoring changes and wonder what you would recommend?

https://codereview.chromium.org/11410019/
https://codereview.chromium.org/11192017/

I'd like to spend the next couple of weeks getting those changes updated and
committed.

[michaeln, 2013-07-23 22:14:38]

What functionality actually gets enabled by this CL?

The use case I'm aware of is the case where /chrome siphons off the response of
navigation resource load to an "app" that has signed up to handle the mime type
of the response. These additions look unrelated to that case. That case is
covered by existing code. Also, I see a skeletal Stream.idl (and .cc .h) file in
blink, and some logic in FileReaderLoader that will accept Stream as input but i
think that code is currently unreachable. I don't see any other points of
integration that are reachable?

I might not be seeing everything, but it looks like this CL by itself doesn't
quite enable anything and there's additional work to be done prior to actually
being able to script Streams meaningfully. Is that right?

If that is right, I'd like to propose doing the blob refactoring/revamping work
first. Wdyt?


On 2013/07/23 20:19:41, michaeln wrote:
> > I'd like to enable Stream first and then do moves
> 
> Is there an webapp that's blocked right now on the functionality provided by
> this CL?
> 
> 
> Given these changes, I'm kinda stymied with how to proceed with these
> refactoring changes and wonder what you would recommend?
> 
> https://codereview.chromium.org/11410019/
> https://codereview.chromium.org/11192017/
> 
> I'd like to spend the next couple of weeks getting those changes updated and
> committed.

[kinuko, 2013-07-24 02:20:20]

On 2013/07/23 22:14:38, michaeln wrote:
> What functionality actually gets enabled by this CL?
> 
> The use case I'm aware of is the case where /chrome siphons off the response
of
> navigation resource load to an "app" that has signed up to handle the mime
type
> of the response. These additions look unrelated to that case. That case is
> covered by existing code. Also, I see a skeletal Stream.idl (and .cc .h) file
in
> blink, and some logic in FileReaderLoader that will accept Stream as input but
i
> think that code is currently unreachable. I don't see any other points of
> integration that are reachable?
> 
> I might not be seeing everything, but it looks like this CL by itself doesn't
> quite enable anything and there's additional work to be done prior to actually
> being able to script Streams meaningfully. Is that right?
> 
> If that is right, I'd like to propose doing the blob refactoring/revamping
work
> first. Wdyt?

tyoshino will give details, but afaik this feature (making Streams work with
XHR) is strongly requested by YouTube and his team wants to make it work in this
quarter.

But if you're resuming your Blob refactoring, I think Streams side should
probably just add another set of IPCs and stub functions for Streams but not to
overload Blob's ones for now, as it'll likely collide in the air.  They don't
share impls in the chrome side (yet), so we should just handle them separately. 
tyoshino, michaeln: wdyt?

> On 2013/07/23 20:19:41, michaeln wrote:
> > > I'd like to enable Stream first and then do moves
> > 
> > Is there an webapp that's blocked right now on the functionality provided by
> > this CL?
> > 
> > 
> > Given these changes, I'm kinda stymied with how to proceed with these
> > refactoring changes and wonder what you would recommend?
> > 
> > https://codereview.chromium.org/11410019/
> > https://codereview.chromium.org/11192017/
> > 
> > I'd like to spend the next couple of weeks getting those changes updated and
> > committed.

[michaeln, 2013-07-24 03:38:30]

> tyoshino will give details, but afaik this feature (making Streams work with
> XHR) is strongly requested by YouTube and his team wants to make it work in
this
> quarter.

I'd like to know more about the specific use case and functionality.
XHR.responseStream, and then what does youtube do with that value?

> But if you're resuming your Blob refactoring, I think Streams side should
> probably just add another set of IPCs and stub functions for Streams but not
to
> overload Blob's ones for now, as it'll likely collide in the air.  They don't
> share impls in the chrome side (yet), so we should just handle them
separately. 
> tyoshino, michaeln: wdyt?

Doing both of these at the same time is really puzzling. I think we either
serialize the work where we revamp the problematic blob infrastructure first
before adding to it. Or decouple them entirely so we can work in work in
parallel.

Fyi: The scheme by which Streams are being identified and accounted for (a clone
of how its currently done for blobs) is going to make it  difficult to
reasonably implement functionality like
postMessage(streamsOrObjsThatReferToStreams).

[tyoshino (SeeGerritForStatus), 2013-07-24 04:04:32]

> What functionality actually gets enabled by this CL?

As Kinuko said, I'm asked by people working on Media Source Extensions
(https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html)
to finish XHR/Stream integration. See SourceBuffer::appendStream
(https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html...).
They might use FileReaderLoader for reading Stream.

Another ongoing work is making Stream readable directly by JavaScript.
I have this experimental CL adding Stream support to FileReader
https://codereview.chromium.org/18635004/ but I'm going to keep it just for test
and move forward to implement it using Promises
http://htmlpreview.github.io/?https://github.com/tyoshino/stream/blob/master/...
This work is not urgent. I'm waiting for progress on ProgressPromise
implementation.

> I might not be seeing everything, but it looks like this CL by itself doesn't
> quite enable anything and there's additional work to be done prior to actually
> being able to script Streams meaningfully. Is that right?

I want to move on. I chatted with Kinuko and understand that it's not productive
that keeping both effort going with step-by-step merging. I'd like to copy code
for Blob necessary for Stream for now not to block your refactoring.

[tyoshino (SeeGerritForStatus), 2013-07-24 04:04:56]

> What functionality actually gets enabled by this CL?

As Kinuko said, I'm asked by people working on Media Source Extensions
(https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html)
to finish XHR/Stream integration. See SourceBuffer::appendStream
(https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html...).
They might use FileReaderLoader for reading Stream.

Another ongoing work is making Stream readable directly by JavaScript.
I have this experimental CL adding Stream support to FileReader
https://codereview.chromium.org/18635004/ but I'm going to keep it just for test
and move forward to implement it using Promises
http://htmlpreview.github.io/?https://github.com/tyoshino/stream/blob/master/...
This work is not urgent. I'm waiting for progress on ProgressPromise
implementation.

> I might not be seeing everything, but it looks like this CL by itself doesn't
> quite enable anything and there's additional work to be done prior to actually
> being able to script Streams meaningfully. Is that right?

I want to move on. I chatted with Kinuko and understand that it's not productive
that keeping both effort going with step-by-step merging. I'd like to copy code
for Blob necessary for Stream for now not to block your refactoring.

[tyoshino (SeeGerritForStatus), 2013-07-24 12:14:10]

Discussed plan with Michael. It seems that I should folk Blob code for Stream
and allow Michael to make progress on his Blob refactoring and both move forward
in parallel.

Sorry but reviewers, please take a look again.

[tyoshino (SeeGerritForStatus), 2013-07-24 12:14:19]

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:114: }
On 2013/07/23 13:57:23, kinuko wrote:
> nit: please match the method order in .cc and .h (i.e. this needs to be placed
> after HasPermissionsForFile)

Done.

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:551: DCHECK(StartsWithASCII(
On 2013/07/23 18:08:25, Tom Sepez wrote:
> What happens if a compromised renderer violates this assumption? DCHECK isn't
> strong enough to protect us here in the absence of other checks later on.
> Perhaps call BadMessageReceived here.

Done.

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:552: url.path(),
"blobinternal%3A///", true /* case_sensitive*/));
On 2013/07/23 13:57:23, kinuko wrote:
> nit: space between case_sensitive and '*/'

Done.

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:557: GURL() /* security_origin
*/,
On 2013/07/23 18:08:25, Tom Sepez wrote:
> Does this mean all streams share the same origin?  

Yes. But we have another origin checking system.

That said, I started thinking this harmful. I wrote a CL to remove
security_origin member variable.
https://codereview.chromium.org/19798012/

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:602:
DCHECK(base::SharedMemory::IsHandleValid(handle));
On 2013/07/23 18:08:25, Tom Sepez wrote:
> What happens if a compromised renderer either lies about buffer_size, or
passes
> back one of its handles not associated with shared memory?

Filed a bug and CC-ed you.

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:639: stream->Finalize();
On 2013/07/23 13:57:23, kinuko wrote:
> Should we DCHECK(stream.get()) here?

Added NOTREACHED

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:56: // FileSystemHostMsg_
family IPCs into separate classes.
On 2013/07/23 13:57:23, kinuko wrote:
> nit: would be nicer to file a bug and put a shorter comment here

Done.

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter_unittest.cc (right):

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter_unittest.cc:69: return
static_cast<IPC::ChannelProxy::MessageFilter*>(filter_.get());
On 2013/07/23 13:57:23, kinuko wrote:
> nit: since we almost always call OnMessageReceived() after this cast, it might
> be simpler to just add a helper method like:
> 
> bool OnMessageReceived(const IPC::Message& message) {
>   return static_cast<...>(filter_.get())->OnMessageReceived(message);
> }

Done.

https://codereview.chromium.org/15817013/diff/68001/content/browser/streams/s...
File content/browser/streams/stream.h (right):

https://codereview.chromium.org/15817013/diff/68001/content/browser/streams/s...
content/browser/streams/stream.h:59: // Adds data of |size| at |data| to the
stream.
On 2013/07/23 13:57:23, kinuko wrote:
> nit: can you explicitly comment this copies |data| into its internal buffer?

Done.

https://codereview.chromium.org/15817013/diff/68001/content/child/webblobregi...
File content/child/webblobregistry_impl.cc (right):

https://codereview.chromium.org/15817013/diff/68001/content/child/webblobregi...
content/child/webblobregistry_impl.cc:129:
DCHECK(ChildThread::current()->message_loop() ==
On 2013/07/23 15:52:36, jam wrote:
> nit: here and in the rest of the file, ChildThread::current() returns non-NULL
> only when you're on that thread, so this can just be written as
> 
> DCHECK(ChildThread::current());

oh. Done.

[tyoshino (SeeGerritForStatus), 2013-07-24 12:26:28]

Sorry. fileapi_message_filter_unittest.cc was not updated. Fixed.

Notes for reviewers

- raw data sending code in webblobregistry_impl.cc are duplicated intentionally
not to bother michaeln's refactoring
- I'm going to remove Stream's security_origin parameter before this CL. See
https://codereview.chromium.org/19798012/

[kinuko, 2013-07-24 13:11:05]

This version's much easier to follow :)

FileAPIMessageFilter gets bloated while streams stuff doesn't look necessary to
be there. I'm fine with having them there to start with, but it'd be better to
refactor later (you seem to have filed a related cleanup bug).

(Some of my comments below look obsolete as I've stepped in before you upload
newer versions, sending them together anyway)

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:548: 
nit: this empty line's not necessary?

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:620: url.path(),
"blobinternal%3A///", true /* case_sensitive */)) {
Does this check add any value?  We aren't even checking if url's scheme is blob.
 The internal BlobURL is going to be deprecated in the coming Blob refactoring,
and this check may start to fail when we change Blink side to use different URL
format (before we have tests it's easy to be missed).

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:621: NOTREACHED() <<
"Malformed Stream URL: " << url.path();
nit: url.spec() would be better for logging

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:134: // used for Stream.
nit: this comment'd better be placed after line 144?

https://codereview.chromium.org/15817013/diff/86001/content/browser/streams/s...
File content/browser/streams/stream.h (right):

https://codereview.chromium.org/15817013/diff/86001/content/browser/streams/s...
content/browser/streams/stream.h:45: // mechanism).
nit: this comment should be added in/after
https://codereview.chromium.org/19798012/ ?

https://codereview.chromium.org/15817013/diff/92001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/92001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:613: // TODO(tyoshino): Set
|content_type| to the stream.
nit: I think common comment style is to have method-level comment in .h or
comment in the method body in .cc.  Maybe you can move this comment inside the
method?

[tyoshino (SeeGerritForStatus), 2013-07-24 13:56:21]

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:548: 
On 2013/07/24 13:11:05, kinuko wrote:
> nit: this empty line's not necessary?

already removed (the patch uploaded after you started reviewing, sorry)

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:620: url.path(),
"blobinternal%3A///", true /* case_sensitive */)) {
On 2013/07/24 13:11:05, kinuko wrote:
> Does this check add any value?  We aren't even checking if url's scheme is
blob.
>  The internal BlobURL is going to be deprecated in the coming Blob
refactoring,
> and this check may start to fail when we change Blink side to use different
URL

When to make that deprecation happen this expectation should be updated
accordingly. If it's going to be uuid string as michaeln's current CL, we can
just remove this as you say.

Until then, I think, it's not greatly useful but ok to have this here. This line
work as a small mark that here old internal Blob URL is expected now.

> format (before we have tests it's easy to be missed).

I'll write a small layout test for XHR.responseStream. Then it goes through this
path.

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:621: NOTREACHED() <<
"Malformed Stream URL: " << url.path();
On 2013/07/24 13:11:05, kinuko wrote:
> nit: url.spec() would be better for logging

Done.

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/15817013/diff/86001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:134: // used for Stream.
On 2013/07/24 13:11:05, kinuko wrote:
> nit: this comment'd better be placed after line 144?

Done.

https://codereview.chromium.org/15817013/diff/86001/content/browser/streams/s...
File content/browser/streams/stream.h (right):

https://codereview.chromium.org/15817013/diff/86001/content/browser/streams/s...
content/browser/streams/stream.h:45: // mechanism).
On 2013/07/24 13:11:05, kinuko wrote:
> nit: this comment should be added in/after
> https://codereview.chromium.org/19798012/ ?

ditto. already removed

https://codereview.chromium.org/15817013/diff/92001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/92001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:613: // TODO(tyoshino): Set
|content_type| to the stream.
On 2013/07/24 13:11:05, kinuko wrote:
> nit: I think common comment style is to have method-level comment in .h or
> comment in the method body in .cc.  Maybe you can move this comment inside the
> method?

Moved to .h thanks

[michaeln, 2013-07-24 21:07:52]

lgtm modulo the OVERRIDE comment

https://codereview.chromium.org/15817013/diff/100001/content/child/webblobreg...
File content/child/webblobregistry_impl.h (right):

https://codereview.chromium.org/15817013/diff/100001/content/child/webblobreg...
content/child/webblobregistry_impl.h:31: virtual void unregisterBlobURL(const
WebKit::WebURL& url) OVERRIDE;
Please remove the OVERRIDE annotations for this WebKit interface derivative.
They make it more difficult to create and commit so called two sided patches so
we avoid them in these sort of classes.

[tyoshino (SeeGerritForStatus), 2013-07-25 04:03:52]

https://codereview.chromium.org/15817013/diff/92001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/92001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:613: // TODO(tyoshino): Set
|content_type| to the stream.
Oops. Removal was not applied yet. Done again.

https://codereview.chromium.org/15817013/diff/100001/content/child/webblobreg...
File content/child/webblobregistry_impl.h (right):

https://codereview.chromium.org/15817013/diff/100001/content/child/webblobreg...
content/child/webblobregistry_impl.h:31: virtual void unregisterBlobURL(const
WebKit::WebURL& url) OVERRIDE;
On 2013/07/24 21:07:52, michaeln wrote:
> Please remove the OVERRIDE annotations for this WebKit interface derivative.
> They make it more difficult to create and commit so called two sided patches
so
> we avoid them in these sort of classes.

Done.

[kinuko, 2013-07-25 04:21:34]

lgtm

[tyoshino (SeeGerritForStatus), 2013-07-25 06:31:47]

Added CONTENT_EXPORT to StreamContext* GetFor(BrowserContext* browser_context)
so that unittests can use it.

[tyoshino (SeeGerritForStatus), 2013-07-29 07:44:00]

PTAL, tsepez, jam.

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15817013/diff/68001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:557: GURL() /* security_origin
*/,
On 2013/07/24 12:14:20, tyoshino wrote:
> On 2013/07/23 18:08:25, Tom Sepez wrote:
> > Does this mean all streams share the same origin?  
> 
> Yes. But we have another origin checking system.
> 
> That said, I started thinking this harmful. I wrote a CL to remove
> security_origin member variable.
> https://codereview.chromium.org/19798012/

Merged the fix.

[tyoshino (SeeGerritForStatus), 2013-07-29 07:45:16]

On 2013/07/24 13:11:05, kinuko wrote:
> FileAPIMessageFilter gets bloated while streams stuff doesn't look necessary
to
> be there. I'm fine with having them there to start with, but it'd be better to
> refactor later (you seem to have filed a related cleanup bug).

Yes. I'll do that at some point. Even methods for BlobHostMsg_ could also be
separated now.

[Tom Sepez, 2013-07-29 17:18:53]

lgtm

[tyoshino (SeeGerritForStatus), 2013-07-30 18:51:21]

Thank you.

John, does this still look lg to you?

[tyoshino (SeeGerritForStatus), 2013-07-31 16:57:44]

On 2013/07/30 18:51:21, tyoshino wrote:
> Thank you.
> 
> John, does this still look lg to you?

FYI, the patchset you lgtm-ed is 18. Please check diff between 18 and 29. Thanks

[tyoshino (SeeGerritForStatus), 2013-07-31 16:59:34]

On 2013/07/31 16:57:44, tyoshino wrote:
> On 2013/07/30 18:51:21, tyoshino wrote:
> > Thank you.
> > 
> > John, does this still look lg to you?
> 
> FYI, the patchset you lgtm-ed is 18. Please check diff between 18 and 29.
Thanks

Delta between 18 and 24 would be better. Patchsets 25-29 are mostly rebasing.

[tyoshino (SeeGerritForStatus), 2013-08-02 07:47:20]

got lgtm from jam offline. submitting

[commit-bot: I haz the power, 2013-08-02 12:19:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoshino@chromium.org/15817013/143001

[commit-bot: I haz the power, 2013-08-02 14:18:44]

Retried try job too often on win7_aura for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[tyoshino (SeeGerritForStatus), 2013-08-06 03:53:41]

Kinuko,

Could you please take a look at this diff?
https://codereview.chromium.org/15817013/diff2/130001:175001/content/browser/...

Win builds failed because of handle passing issue in the unittest I wrote.

[kinuko, 2013-08-06 04:27:56]

On 2013/08/06 03:53:41, tyoshino wrote:
> Kinuko,
> 
> Could you please take a look at this diff?
>
https://codereview.chromium.org/15817013/diff2/130001:175001/content/browser/...
> 
> Win builds failed because of handle passing issue in the unittest I wrote.

diff lgtm

[commit-bot: I haz the power, 2013-08-06 04:31:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoshino@chromium.org/15817013/175001

[commit-bot: I haz the power, 2013-08-06 04:41:27]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-08-06 05:17:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoshino@chromium.org/15817013/198001

[commit-bot: I haz the power, 2013-08-06 06:10:44]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-08-06 06:19:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoshino@chromium.org/15817013/198001

[commit-bot: I haz the power, 2013-08-06 07:45:27]

Change committed as 215842