net: Update FileStream to use base::File instead of PlatformFile.

net: Update FileStream to use base::File instead of PlatformFile.

As collateral damage, OpenContentUriForRead is now returning File instead of
a plain file descriptor.

BUG=322664
TEST=net_unittests
R=willchan@chromium.org

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

[rvargas (doing something else), 2014-03-12 18:59:49]

PTAL

[willchan no longer on Chromium, 2014-03-14 21:50:13]

Hey Ricardo, I'm just returning from vacation and will be backlogged next week.
If you can wait, that's great. Otherwise, I already skimmed the base/ stuff and
the net/ stuff other than the FileStream refactoring. So if you can get someone
else like mmenke to review that, feel free to TBR the rest to me since they're
fine.

[rvargas (doing something else), 2014-03-18 19:13:30]

On 2014/03/14 21:50:13, willchan wrote:
> Hey Ricardo, I'm just returning from vacation and will be backlogged next
week.
> If you can wait, that's great. Otherwise, I already skimmed the base/ stuff
and
> the net/ stuff other than the FileStream refactoring. So if you can get
someone
> else like mmenke to review that, feel free to TBR the rest to me since they're
> fine.

That's OK, I was not expecting a quick review. Besides, the actual changes to
FileStream are for the most part mechanical.

[willchan no longer on Chromium, 2014-03-18 20:36:55]

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream.cc
File net/base/file_stream.cc (left):

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream.cc...
net/base/file_stream.cc:60: base::ThreadRestrictions::AssertIOAllowed();
Shouldn't we keep the ThreadRestrictions in the sync case?

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream_co...
File net/base/file_stream_context.h (right):

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream_co...
net/base/file_stream_context.h:144: class OpenResult {
I chatted with jyasskin (c++ committee member and ex-readability reviewer) and
he thinks stylistically we should keep this as a struct, even if you added the
move emulation here. I tend to agree. WDYT about switching this back to a
struct?

[rvargas (doing something else), 2014-03-19 01:03:18]

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream.cc
File net/base/file_stream.cc (left):

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream.cc...
net/base/file_stream.cc:60: base::ThreadRestrictions::AssertIOAllowed();
On 2014/03/18 20:36:56, willchan wrote:
> Shouldn't we keep the ThreadRestrictions in the sync case?

This object doesn't own the file... the context does. At the end of the call
stack base::File is checking for thread restrictions on destruction, but it does
so only if there is something to close.

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream_co...
File net/base/file_stream_context.h (right):

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream_co...
net/base/file_stream_context.h:144: class OpenResult {
On 2014/03/18 20:36:56, willchan wrote:
> I chatted with jyasskin (c++ committee member and ex-readability reviewer) and
> he thinks stylistically we should keep this as a struct, even if you added the
> move emulation here. I tend to agree. WDYT about switching this back to a
> struct?

I guess it looks weird in any case. I mean, having a struct with private and
public sections (and operators) really feels like going against the "use a
struct for trivial cases... everything else is a class". On the other hand,
having a class with the members declared as public goes against the "everything
private, declare accessors".

Just so that it's clear to me, what is your concern about making it a class?

[rvargas (doing something else), 2014-03-19 19:01:47]

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream_co...
File net/base/file_stream_context.h (right):

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream_co...
net/base/file_stream_context.h:144: class OpenResult {
Tentatively back to struct

[willchan no longer on Chromium, 2014-03-19 22:36:35]

lgtm

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream.cc
File net/base/file_stream.cc (left):

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream.cc...
net/base/file_stream.cc:60: base::ThreadRestrictions::AssertIOAllowed();
On 2014/03/19 01:03:18, rvargas wrote:
> On 2014/03/18 20:36:56, willchan wrote:
> > Shouldn't we keep the ThreadRestrictions in the sync case?
> 
> This object doesn't own the file... the context does. At the end of the call
> stack base::File is checking for thread restrictions on destruction, but it
does
> so only if there is something to close.
> 

Aha, OK. That makes sense. Wasn't sure if this was an accidental deletion.

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream_co...
File net/base/file_stream_context.h (right):

https://codereview.chromium.org/189393002/diff/180001/net/base/file_stream_co...
net/base/file_stream_context.h:144: class OpenResult {
On 2014/03/19 19:01:48, rvargas wrote:
> Tentatively back to struct

It's just a minor preference. The reason is that these public member functions
are purely there for move support, and our idiomatic use of struct is to
indicate that it's just a encapsulation of a bunch of data, rather than
associated functionality.

[rvargas (doing something else), 2014-03-19 23:00:04]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-03-19 23:01:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/189393002/200001

[commit-bot: I haz the power, 2014-03-20 00:16:58]

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

[commit-bot: I haz the power, 2014-03-20 00:16:59]

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

[rvargas (doing something else), 2014-03-20 21:03:29]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-03-20 21:04:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/189393002/220001

[commit-bot: I haz the power, 2014-03-21 00:41:34]

Change committed as 258472