Remove PlatformFile from components/nacl

Remove PlatformFile from components/nacl

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

[rvargas (doing something else), 2014-06-03 17:57:23]

PTAL

https://codereview.chromium.org/307173002/diff/60001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/307173002/diff/60001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1123: // This file was not
being closed!
Is this OK?

[Mark Seaborn, 2014-06-03 18:23:21]

+dschuff/jvoung for nacl/browser/pnacl_host.*
+dmichael for nacl/renderer/*

I don't know those parts of the code well enough to review this well.

https://codereview.chromium.org/307173002/diff/60001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/307173002/diff/60001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1123: // This file was not
being closed!
On 2014/06/03 17:57:23, rvargas wrote:
> Is this OK?

I don't understand the comment.  Is it referring to what the code used to do? 
If you're fixing something, doesn't this explanation belong in the commit
message instead?

[dmichael (off chromium), 2014-06-03 19:14:45]

+teravest

[rvargas (doing something else), 2014-06-03 19:59:49]

https://codereview.chromium.org/307173002/diff/60001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/307173002/diff/60001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1123: // This file was not
being closed!
On 2014/06/03 18:23:21, Mark Seaborn wrote:
> On 2014/06/03 17:57:23, rvargas wrote:
> > Is this OK?
> 
> I don't understand the comment.  Is it referring to what the code used to do? 
> If you're fixing something, doesn't this explanation belong in the commit
> message instead?

In any case, the comment will be removed... it was a line to remind me to point
that out on the review. There was a couple of bugs in the code, but I'm not
familiar with this code to know if there was a reason to leak the file in this
code path.

So, rephrasing... is it OK to close the file here, or was the leak intentional?

[teravest, 2014-06-03 20:04:54]

lgtm

https://codereview.chromium.org/307173002/diff/60001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/307173002/diff/60001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1123: // This file was not
being closed!
On 2014/06/03 19:59:49, rvargas wrote:
> On 2014/06/03 18:23:21, Mark Seaborn wrote:
> > On 2014/06/03 17:57:23, rvargas wrote:
> > > Is this OK?
> > 
> > I don't understand the comment.  Is it referring to what the code used to
do? 
> > If you're fixing something, doesn't this explanation belong in the commit
> > message instead?
> 
> In any case, the comment will be removed... it was a line to remind me to
point
> that out on the review. There was a couple of bugs in the code, but I'm not
> familiar with this code to know if there was a reason to leak the file in this
> code path.
> 
> So, rephrasing... is it OK to close the file here, or was the leak
intentional?

It should be OK to close the file there.

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

Thanks!

https://codereview.chromium.org/307173002/diff/60001/components/nacl/browser/...
File components/nacl/browser/pnacl_host.cc (right):

https://codereview.chromium.org/307173002/diff/60001/components/nacl/browser/...
components/nacl/browser/pnacl_host.cc:410: // Does this has to be executed after
the previous line?
This was also a comment for the review... but I decided that the order is not
important, and I also updated the callback signature.

[rvargas (doing something else), 2014-06-03 21:31:34]

https://codereview.chromium.org/307173002/diff/60001/components/nacl/browser/...
File components/nacl/browser/pnacl_host.cc (right):

https://codereview.chromium.org/307173002/diff/60001/components/nacl/browser/...
components/nacl/browser/pnacl_host.cc:410: // Does this has to be executed after
the previous line?
On 2014/06/03 21:29:43, rvargas wrote:
> This was also a comment for the review... but I decided that the order is not
> important, and I also updated the callback signature.

(let me know if the order is indeed important)

[jvoung (off chromium), 2014-06-03 23:00:09]

lgtm

https://codereview.chromium.org/307173002/diff/60001/components/nacl/browser/...
File components/nacl/browser/pnacl_host.cc (right):

https://codereview.chromium.org/307173002/diff/60001/components/nacl/browser/...
components/nacl/browser/pnacl_host.cc:410: // Does this has to be executed after
the previous line?
On 2014/06/03 21:31:34, rvargas wrote:
> On 2014/06/03 21:29:43, rvargas wrote:
> > This was also a comment for the review... but I decided that the order is
not
> > important, and I also updated the callback signature.
> 
> (let me know if the order is indeed important)

I think it will be okay (dschuff can confirm, but is OoO), since this code
shouldn't be handling another request until this thread is done running the
erase().

[rvargas (doing something else), 2014-06-04 01:38:03]

Thanks!

[rvargas (doing something else), 2014-06-04 01:38:07]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-06-04 01:38:42]

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

[commit-bot: I haz the power, 2014-06-04 12:15:58]

Change committed as 274783