Add CreateTemporaryFile to PPB_Flash_File_ModuleLocal.

Add CreateTemporaryFile to PPB_Flash_File_ModuleLocal.

BUG=129807
TEST=None

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

[yzshen1, 2012-06-07 18:15:45]

Hi, Trung.

Would you please take a look? Thanks!

[viettrungluu, 2012-06-07 20:17:36]

https://chromiumcodereview.appspot.com/10534045/diff/1/content/browser/render...
File content/browser/renderer_host/pepper_file_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10534045/diff/1/content/browser/render...
content/browser/renderer_host/pepper_file_message_filter.cc:248:
base::PLATFORM_FILE_CREATE_ALWAYS | base::PLATFORM_FILE_EXCLUSIVE_READ |
Why do you want exclusive read and write?

https://chromiumcodereview.appspot.com/10534045/diff/1/ppapi/c/private/ppb_fl...
File ppapi/c/private/ppb_flash_file.h (right):

https://chromiumcodereview.appspot.com/10534045/diff/1/ppapi/c/private/ppb_fl...
ppapi/c/private/ppb_flash_file.h:114: int32_t (*CreateTemporaryFile)(PP_Instance
instance,
Why don't we just add this to OpenFile()'s functionality? We could presumably
add a flag to |mode| and a note that for the temporary-file case |path| means
the directory path. Also, I don't see a good reason for returning the file name
-- it's typically meaningless on POSIX-ish systems, where temporary files are
created by making a file (with whatever name) and then unlink()ing it
immediately.

[yzshen1, 2012-06-07 21:42:06]

Thanks, Trung!

https://chromiumcodereview.appspot.com/10534045/diff/1/content/browser/render...
File content/browser/renderer_host/pepper_file_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10534045/diff/1/content/browser/render...
content/browser/renderer_host/pepper_file_message_filter.cc:248:
base::PLATFORM_FILE_CREATE_ALWAYS | base::PLATFORM_FILE_EXCLUSIVE_READ |
I am not sure. Do you know about any scenario that this shouldn't be so? :)

On 2012/06/07 20:17:36, viettrungluu wrote:
> Why do you want exclusive read and write?

https://chromiumcodereview.appspot.com/10534045/diff/1/ppapi/c/private/ppb_fl...
File ppapi/c/private/ppb_flash_file.h (right):

https://chromiumcodereview.appspot.com/10534045/diff/1/ppapi/c/private/ppb_fl...
ppapi/c/private/ppb_flash_file.h:114: int32_t (*CreateTemporaryFile)(PP_Instance
instance,
On 2012/06/07 20:17:36, viettrungluu wrote:
> Why don't we just add this to OpenFile()'s functionality? We could presumably
> add a flag to |mode| and a note that for the temporary-file case |path| means
> the directory path. Also, I don't see a good reason for returning the file
name
> -- it's typically meaningless on POSIX-ish systems, where temporary files are
> created by making a file (with whatever name) and then unlink()ing it
> immediately.

I considered adding this to OpenFile(), but I didn't do it that way because:
- We cannot easily add new flag to PP_FileOpenFlags, which is an official enum.
- The different meaning of |path| seems significant to me to justify a new
method.
- If we would like to return the resulting file name, it would be a good reason
to have a new method as well. I took a look at the file system API in Flash, it
has a method to get file name.

[yzshen1, 2012-06-08 01:23:26]

Hi, Trung.

Please take another look. Thanks!

[viettrungluu, 2012-06-11 17:31:46]

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_file_message_filter.cc:250:
(!file_util::DirectoryExists(validated_dir_path) &&
Why don't we have to do any of this for Open?

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_file_message_filter.cc:262:
base::PLATFORM_FILE_CREATE_ALWAYS | base::PLATFORM_FILE_EXCLUSIVE_READ |
Sorry I didn't answer your question earlier; in general, locking is more
expensive than not locking, so we should probably avoid ..._EXCLUSIVE_... unless
there's a compelling reason for it.

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_file_message_filter.cc:268: file_handle ==
base::kInvalidPlatformFileValue) {
Do you really think file_handle could be kInvalidPlatformFileValue? (If not,
CHECK or DCHECK.)

http://codereview.chromium.org/10534045/diff/3002/webkit/plugins/ppapi/ppb_fl...
File webkit/plugins/ppapi/ppb_flash_impl.cc (right):

http://codereview.chromium.org/10534045/diff/3002/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_impl.cc:537: base::PlatformFile temp_file =
base::kInvalidPlatformFileValue;
Why do you need temp_file at all?

[yzshen1, 2012-06-12 23:53:06]

Hi, Trung.

Thanks!

I have also added tests. Please take a look.

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_file_message_filter.cc:250:
(!file_util::DirectoryExists(validated_dir_path) &&
On 2012/06/11 17:31:46, viettrungluu wrote:
> Why don't we have to do any of this for Open?

The folder of module-local root is not guaranteed to exist at this point, right?
Because the users of this method has no control over (or know about) where the
temp file locates, it is our responsibility to help them to make sure the folder
exists.

On the other hand, the users of Open can test whether the dir exists and (if
not) create it.

(But having to create the root directory explicitly does seem a little bit
weird. Do you suggest to treat Open-ing a file in the root directory specially?)

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_file_message_filter.cc:262:
base::PLATFORM_FILE_CREATE_ALWAYS | base::PLATFORM_FILE_EXCLUSIVE_READ |
Changed. Thanks for explanation.
On 2012/06/11 17:31:46, viettrungluu wrote:
> Sorry I didn't answer your question earlier; in general, locking is more
> expensive than not locking, so we should probably avoid ..._EXCLUSIVE_...
unless
> there's a compelling reason for it.

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_file_message_filter.cc:268: file_handle ==
base::kInvalidPlatformFileValue) {
No. Changed.

On 2012/06/11 17:31:46, viettrungluu wrote:
> Do you really think file_handle could be kInvalidPlatformFileValue? (If not,
> CHECK or DCHECK.)

http://codereview.chromium.org/10534045/diff/3002/webkit/plugins/ppapi/ppb_fl...
File webkit/plugins/ppapi/ppb_flash_impl.cc (right):

http://codereview.chromium.org/10534045/diff/3002/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_impl.cc:537: base::PlatformFile temp_file =
base::kInvalidPlatformFileValue;
Done.

On 2012/06/11 17:31:46, viettrungluu wrote:
> Why do you need temp_file at all?

[viettrungluu, 2012-06-13 22:46:32]

Sorry for the delay.

LGTM with a question (I find the whole directory-creating thing very odd), and a
couple of issues. We should probably get this in soon though.

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_file_message_filter.cc:250:
(!file_util::DirectoryExists(validated_dir_path) &&
On 2012/06/12 23:53:06, yzshen1 wrote:
> On 2012/06/11 17:31:46, viettrungluu wrote:
> > Why don't we have to do any of this for Open?
> 
> The folder of module-local root is not guaranteed to exist at this point,
right?
> Because the users of this method has no control over (or know about) where the
> temp file locates, it is our responsibility to help them to make sure the
folder
> exists.
> 
> On the other hand, the users of Open can test whether the dir exists and (if
> not) create it.
> 
> (But having to create the root directory explicitly does seem a little bit
> weird. Do you suggest to treat Open-ing a file in the root directory
specially?)

Now that I think of it, I have no idea how anything at all works. Someone must
be ensuring that the directory exists -- could you figure out who?

http://codereview.chromium.org/10534045/diff/13003/ppapi/tests/test_flash_fil...
File ppapi/tests/test_flash_file.cc (right):

http://codereview.chromium.org/10534045/diff/13003/ppapi/tests/test_flash_fil...
ppapi/tests/test_flash_file.cc:34: ssize_t bytes_written = write(file_handle,
contents.c_str(), contents.size());
You should use HANDLE_EINTR (or something equivalent) for write().

http://codereview.chromium.org/10534045/diff/13003/ppapi/tests/test_flash_fil...
ppapi/tests/test_flash_file.cc:57: bytes_read = read(file_handle, buffer,
kBufferSize);
Ditto for read().

(lseek() is not interruptible, so it's okay.)

[yzshen1, 2012-06-14 18:25:38]

Thanks, Trung.

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_file_message_filter.cc:250:
(!file_util::DirectoryExists(validated_dir_path) &&
The answer is no. :P
The pepper layer doesn't do anything to ensure that the module-local root
directory exists.

What I have tried:
I deleted the Pepper Data directory and then ran the youtube page.
At the beginning, Flash tried to open some files, while the module-local root
didn't exist. Some time later, it called CreateDirectory() (which created
directory recursively), the module-local root finally got created.

On 2012/06/13 22:46:32, viettrungluu wrote:
> On 2012/06/12 23:53:06, yzshen1 wrote:
> > On 2012/06/11 17:31:46, viettrungluu wrote:
> > > Why don't we have to do any of this for Open?
> > 
> > The folder of module-local root is not guaranteed to exist at this point,
> right?
> > Because the users of this method has no control over (or know about) where
the
> > temp file locates, it is our responsibility to help them to make sure the
> folder
> > exists.
> > 
> > On the other hand, the users of Open can test whether the dir exists and (if
> > not) create it.
> > 
> > (But having to create the root directory explicitly does seem a little bit
> > weird. Do you suggest to treat Open-ing a file in the root directory
> specially?)
> 
> Now that I think of it, I have no idea how anything at all works. Someone must
> be ensuring that the directory exists -- could you figure out who?

http://codereview.chromium.org/10534045/diff/13003/ppapi/tests/test_flash_fil...
File ppapi/tests/test_flash_file.cc (right):

http://codereview.chromium.org/10534045/diff/13003/ppapi/tests/test_flash_fil...
ppapi/tests/test_flash_file.cc:34: ssize_t bytes_written = write(file_handle,
contents.c_str(), contents.size());
On 2012/06/13 22:46:32, viettrungluu wrote:
> You should use HANDLE_EINTR (or something equivalent) for write().

Done.

http://codereview.chromium.org/10534045/diff/13003/ppapi/tests/test_flash_fil...
ppapi/tests/test_flash_file.cc:57: bytes_read = read(file_handle, buffer,
kBufferSize);
On 2012/06/13 22:46:32, viettrungluu wrote:
> Ditto for read().
> 
> (lseek() is not interruptible, so it's okay.)

Done.

[viettrungluu, 2012-06-14 20:47:22]

Still LGTM. Thanks.

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_file_message_filter.cc:250:
(!file_util::DirectoryExists(validated_dir_path) &&
Probably we (by which I mean you) should abstract out an
"EnsureModuleLocalRootExists" and call it for Open() as well. Could you do this
in a follow-up CL?

On 2012/06/14 18:25:38, yzshen1 wrote:
> The answer is no. :P
> The pepper layer doesn't do anything to ensure that the module-local root
> directory exists.
> 
> What I have tried:
> I deleted the Pepper Data directory and then ran the youtube page.
> At the beginning, Flash tried to open some files, while the module-local root
> didn't exist. Some time later, it called CreateDirectory() (which created
> directory recursively), the module-local root finally got created.
> 
> On 2012/06/13 22:46:32, viettrungluu wrote:
> > On 2012/06/12 23:53:06, yzshen1 wrote:
> > > On 2012/06/11 17:31:46, viettrungluu wrote:
> > > > Why don't we have to do any of this for Open?
> > > 
> > > The folder of module-local root is not guaranteed to exist at this point,
> > right?
> > > Because the users of this method has no control over (or know about) where
> the
> > > temp file locates, it is our responsibility to help them to make sure the
> > folder
> > > exists.
> > > 
> > > On the other hand, the users of Open can test whether the dir exists and
(if
> > > not) create it.
> > > 
> > > (But having to create the root directory explicitly does seem a little bit
> > > weird. Do you suggest to treat Open-ing a file in the root directory
> > specially?)
> > 
> > Now that I think of it, I have no idea how anything at all works. Someone
must
> > be ensuring that the directory exists -- could you figure out who?
>

[yzshen, 2012-06-14 20:48:11]

On Thu, Jun 14, 2012 at 1:47 PM, <viettrungluu@chromium.org> wrote:

> Still LGTM. Thanks.
>
>
>
> http://codereview.chromium.**org/10534045/diff/3002/**
>
content/browser/renderer_host/**pepper_file_message_filter.cc<http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_host/pepper_file_message_filter.cc>
> File content/browser/renderer_host/**pepper_file_message_filter.cc
> (right):
>
> http://codereview.chromium.**org/10534045/diff/3002/**
>
content/browser/renderer_host/**pepper_file_message_filter.cc#**newcode250<http://codereview.chromium.org/10534045/diff/3002/content/browser/renderer_host/pepper_file_message_filter.cc#newcode250>
> content/browser/renderer_host/**pepper_file_message_filter.cc:**250:
> (!file_util::DirectoryExists(**validated_dir_path) &&
> Probably we (by which I mean you) should abstract out an
> "EnsureModuleLocalRootExists" and call it for Open() as well. Could you
> do this in a follow-up CL?


Sure. Thanks!


>
>
> On 2012/06/14 18:25:38, yzshen1 wrote:
>
>> The answer is no. :P
>> The pepper layer doesn't do anything to ensure that the module-local
>>
> root
>
>> directory exists.
>>
>
>  What I have tried:
>> I deleted the Pepper Data directory and then ran the youtube page.
>> At the beginning, Flash tried to open some files, while the
>>
> module-local root
>
>> didn't exist. Some time later, it called CreateDirectory() (which
>>
> created
>
>> directory recursively), the module-local root finally got created.
>>
>
>  On 2012/06/13 22:46:32, viettrungluu wrote:
>> > On 2012/06/12 23:53:06, yzshen1 wrote:
>> > > On 2012/06/11 17:31:46, viettrungluu wrote:
>> > > > Why don't we have to do any of this for Open?
>> > >
>> > > The folder of module-local root is not guaranteed to exist at this
>>
> point,
>
>> > right?
>> > > Because the users of this method has no control over (or know
>>
> about) where
>
>> the
>> > > temp file locates, it is our responsibility to help them to make
>>
> sure the
>
>> > folder
>> > > exists.
>> > >
>> > > On the other hand, the users of Open can test whether the dir
>>
> exists and (if
>
>> > > not) create it.
>> > >
>> > > (But having to create the root directory explicitly does seem a
>>
> little bit
>
>> > > weird. Do you suggest to treat Open-ing a file in the root
>>
> directory
>
>> > specially?)
>> >
>> > Now that I think of it, I have no idea how anything at all works.
>>
> Someone must
>
>> > be ensuring that the directory exists -- could you figure out who?
>>
>
>
>
http://codereview.chromium.**org/10534045/<http://codereview.chromium.org/105...
>



-- 
Best regards,
Yuzhu Shen.

[yzshen1, 2012-06-14 23:15:40]

Hi, Brett.

Could you please do an owner review for:
content/browser/renderer_host
content/renderer

Thanks!

[brettw, 2012-06-15 20:31:47]

lgtm