Fix PP_FileOpenFlags_Dev handling:

Fix PP_FileOpenFlags_Dev handling:
- rewrite the mapping from PP_FileOpenFlags_Dev to PlatformFileFlags.
- let ppb_flash_file_impl and ppb_file_io_impl use the same mapping logic.
- CreatePlatformFile: resolve the conflict between the win and posix implementation. Before this change, the win implementation didn't allow PLATFORM_FILE_TRUNCATE to be used with any of the (OPEN|CREATE)(_ALWAYS)? flags; while the posix implementation required it to be used with them.
- add more test cases to test the behavior of different PP_FileOpenFlags_Dev combinations.
- also unify the conversion from PlatformFileError to Pepper error.

BUG=68489
TEST=New test cases in test_file_io.cc

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86349

[darin (slow to review), 2011-05-20 21:02:13]

http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
base/platform_file_posix.cc:48: DCHECK(!open_flags);
so this means that it is not possible to specify PLATFORM_FILE_CREATE and
PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to specify
O_CREAT and O_TRUNC together.  while it may seem redundant to indicate that a
file should be truncated when created (since the file is 0 length anyways), i
don't see the harm in doing so.

http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
base/platform_file_posix.cc:53: if (!open_flags && !(flags & PLATFORM_FILE_OPEN)
&&
it looks like you might be breaking this check, which was trying to assert that
at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.  with your
change above, it is possible for PLATFORM_FILE_TRUNCATE to be specified without
having specified any of those other 4 flags.  is that really what you intended?

i always thought that truncate should be a modifier for opening an existing
file.

[yzshen1, 2011-05-20 21:29:24]

Hi, Darin.

Thanks for your reply. Please see my comments.

http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
base/platform_file_posix.cc:48: DCHECK(!open_flags);
On 2011/05/20 21:02:13, darin wrote:
> so this means that it is not possible to specify PLATFORM_FILE_CREATE and
> PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to specify
> O_CREAT and O_TRUNC together.  while it may seem redundant to indicate that a
> file should be truncated when created (since the file is 0 length anyways), i
> don't see the harm in doing so.

I think PlatformFileFlags are probably designed to look similar to Win flags.
As a result, the followings have an 1-1 mapping to Win flags:
PLATFORM_FILE_OPEN <-> OPEN_EXISTING
PLATFORM_FILE_CREATE <-> CREATE_NEW
PLATFORM_FILE_OPEN_ALWAYS <-> OPEN_ALWAYS
PLATFORM_FILE_CREATE_ALWAYS <-> CREATE_ALWAYS
PLATFORM_FILE_TRUNCATE <-> TRUNCATE_EXISTING
And platform_file_win enforces that these five PlatformFileFlags cannot be used
together with each other.
It would be good to be consistent.

The way to specify O_CREAT | O_TRUNC is PLATFORM_FILE_CREATE_ALWAYS.

http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
base/platform_file_posix.cc:53: if (!open_flags && !(flags & PLATFORM_FILE_OPEN)
&&
On 2011/05/20 21:02:13, darin wrote:
> it looks like you might be breaking this check, which was trying to assert
that
> at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
> PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.  with your
> change above, it is possible for PLATFORM_FILE_TRUNCATE to be specified
without
> having specified any of those other 4 flags.  is that really what you
intended?
> 
> i always thought that truncate should be a modifier for opening an existing
> file.

This is what I have described in the CL description:
CreatePlatformFile: resolve the conflict between the win and posix
implementation. Before this change, the win implementation didn't allow
PLATFORM_FILE_TRUNCATE to be used with any of the (OPEN|CREATE)(_ALWAYS)? flags;
while the posix implementation required it to be used with them.

I don't have a strong opinion about which way is better. But we have to keep the
win and posix implementation consistent.

[yzshen, 2011-05-20 21:35:41]

One more note:
Currently, PlatformFileFlags looks like Windows file open flags; while
PP_FileOpenFlags_Dev looks like Posix file open flags.
That is why the mapping
  PP_FileOpenFlags -> PlatfromFileFlags -> Posix file open flags
looks confusing.

On Fri, May 20, 2011 at 2:29 PM, <yzshen@chromium.org> wrote:

> Reviewers: darin,
>
> Message:
> Hi, Darin.
>
> Thanks for your reply. Please see my comments.
>
>
>
> http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
> File base/platform_file_posix.cc (right):
>
>
>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
> base/platform_file_posix.cc:48: DCHECK(!open_flags);
> On 2011/05/20 21:02:13, darin wrote:
>
>> so this means that it is not possible to specify PLATFORM_FILE_CREATE
>>
> and
>
>> PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to
>>
> specify
>
>> O_CREAT and O_TRUNC together.  while it may seem redundant to indicate
>>
> that a
>
>> file should be truncated when created (since the file is 0 length
>>
> anyways), i
>
>> don't see the harm in doing so.
>>
>
> I think PlatformFileFlags are probably designed to look similar to Win
> flags.
> As a result, the followings have an 1-1 mapping to Win flags:
> PLATFORM_FILE_OPEN <-> OPEN_EXISTING
> PLATFORM_FILE_CREATE <-> CREATE_NEW
> PLATFORM_FILE_OPEN_ALWAYS <-> OPEN_ALWAYS
> PLATFORM_FILE_CREATE_ALWAYS <-> CREATE_ALWAYS
> PLATFORM_FILE_TRUNCATE <-> TRUNCATE_EXISTING
> And platform_file_win enforces that these five PlatformFileFlags cannot
> be used together with each other.
> It would be good to be consistent.
>
> The way to specify O_CREAT | O_TRUNC is PLATFORM_FILE_CREATE_ALWAYS.
>
>
>
>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
> base/platform_file_posix.cc:53: if (!open_flags && !(flags &
> PLATFORM_FILE_OPEN) &&
> On 2011/05/20 21:02:13, darin wrote:
>
>> it looks like you might be breaking this check, which was trying to
>>
> assert that
>
>> at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
>> PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.  with
>>
> your
>
>> change above, it is possible for PLATFORM_FILE_TRUNCATE to be
>>
> specified without
>
>> having specified any of those other 4 flags.  is that really what you
>>
> intended?
>
>  i always thought that truncate should be a modifier for opening an
>>
> existing
>
>> file.
>>
>
> This is what I have described in the CL description:
> CreatePlatformFile: resolve the conflict between the win and posix
> implementation. Before this change, the win implementation didn't allow
> PLATFORM_FILE_TRUNCATE to be used with any of the
> (OPEN|CREATE)(_ALWAYS)? flags; while the posix implementation required
> it to be used with them.
>
> I don't have a strong opinion about which way is better. But we have to
> keep the win and posix implementation consistent.
>
> Description:
> Fix PP_FileOpenFlags_Dev handling:
> - rewrite the mapping from PP_FileOpenFlags_Dev to PlatformFileFlags.
> - let ppb_flash_file_impl and ppb_file_io_impl use the same mapping logic.
> - CreatePlatformFile: resolve the conflict between the win and posix
> implementation. Before this change, the win implementation didn't allow
> PLATFORM_FILE_TRUNCATE to be used with any of the (OPEN|CREATE)(_ALWAYS)?
> flags;
> while the posix implementation required it to be used with them.
> - add more test cases to test the behavior of different
> PP_FileOpenFlags_Dev
> combinations.
> - also unify the conversion from PlatformFileError to Pepper error.
>
> BUG=68489
> TEST=New test cases in test_file_io.cc
>
>
> Please review this at http://codereview.chromium.org/7038032/
>
> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>
> Affected files:
>  M base/platform_file_posix.cc
>  M ppapi/tests/test_file_io.h
>  M ppapi/tests/test_file_io.cc
>  M ppapi/tests/test_file_ref.cc
>  M webkit/glue/webkit_glue.gypi
>  D webkit/plugins/ppapi/error_util.h
>  D webkit/plugins/ppapi/error_util.cc
>  M webkit/plugins/ppapi/file_callbacks.cc
>  A webkit/plugins/ppapi/file_type_conversion.h
>  A webkit/plugins/ppapi/file_type_conversion.cc
>  M webkit/plugins/ppapi/ppb_file_io_impl.cc
>  M webkit/plugins/ppapi/ppb_flash_file_impl.cc
>
>
>


-- 
Best regards,
Yuzhu Shen.

[darin (slow to review), 2011-05-20 22:12:13]

OK, makes sense.  Thanks!

On Fri, May 20, 2011 at 2:35 PM, Yuzhu Shen <yzshen@google.com> wrote:

> One more note:
> Currently, PlatformFileFlags looks like Windows file open flags; while
> PP_FileOpenFlags_Dev looks like Posix file open flags.
> That is why the mapping
>   PP_FileOpenFlags -> PlatfromFileFlags -> Posix file open flags
> looks confusing.
>
> On Fri, May 20, 2011 at 2:29 PM, <yzshen@chromium.org> wrote:
>
>> Reviewers: darin,
>>
>> Message:
>> Hi, Darin.
>>
>> Thanks for your reply. Please see my comments.
>>
>>
>>
>> http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
>> File base/platform_file_posix.cc (right):
>>
>>
>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>> base/platform_file_posix.cc:48: DCHECK(!open_flags);
>> On 2011/05/20 21:02:13, darin wrote:
>>
>>> so this means that it is not possible to specify PLATFORM_FILE_CREATE
>>>
>> and
>>
>>> PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to
>>>
>> specify
>>
>>> O_CREAT and O_TRUNC together.  while it may seem redundant to indicate
>>>
>> that a
>>
>>> file should be truncated when created (since the file is 0 length
>>>
>> anyways), i
>>
>>> don't see the harm in doing so.
>>>
>>
>> I think PlatformFileFlags are probably designed to look similar to Win
>> flags.
>> As a result, the followings have an 1-1 mapping to Win flags:
>> PLATFORM_FILE_OPEN <-> OPEN_EXISTING
>> PLATFORM_FILE_CREATE <-> CREATE_NEW
>> PLATFORM_FILE_OPEN_ALWAYS <-> OPEN_ALWAYS
>> PLATFORM_FILE_CREATE_ALWAYS <-> CREATE_ALWAYS
>> PLATFORM_FILE_TRUNCATE <-> TRUNCATE_EXISTING
>> And platform_file_win enforces that these five PlatformFileFlags cannot
>> be used together with each other.
>> It would be good to be consistent.
>>
>> The way to specify O_CREAT | O_TRUNC is PLATFORM_FILE_CREATE_ALWAYS.
>>
>>
>>
>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>> base/platform_file_posix.cc:53: if (!open_flags && !(flags &
>> PLATFORM_FILE_OPEN) &&
>> On 2011/05/20 21:02:13, darin wrote:
>>
>>> it looks like you might be breaking this check, which was trying to
>>>
>> assert that
>>
>>> at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
>>> PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.  with
>>>
>> your
>>
>>> change above, it is possible for PLATFORM_FILE_TRUNCATE to be
>>>
>> specified without
>>
>>> having specified any of those other 4 flags.  is that really what you
>>>
>> intended?
>>
>>  i always thought that truncate should be a modifier for opening an
>>>
>> existing
>>
>>> file.
>>>
>>
>> This is what I have described in the CL description:
>> CreatePlatformFile: resolve the conflict between the win and posix
>> implementation. Before this change, the win implementation didn't allow
>> PLATFORM_FILE_TRUNCATE to be used with any of the
>> (OPEN|CREATE)(_ALWAYS)? flags; while the posix implementation required
>> it to be used with them.
>>
>> I don't have a strong opinion about which way is better. But we have to
>> keep the win and posix implementation consistent.
>>
>> Description:
>> Fix PP_FileOpenFlags_Dev handling:
>> - rewrite the mapping from PP_FileOpenFlags_Dev to PlatformFileFlags.
>> - let ppb_flash_file_impl and ppb_file_io_impl use the same mapping logic.
>> - CreatePlatformFile: resolve the conflict between the win and posix
>> implementation. Before this change, the win implementation didn't allow
>> PLATFORM_FILE_TRUNCATE to be used with any of the (OPEN|CREATE)(_ALWAYS)?
>> flags;
>> while the posix implementation required it to be used with them.
>> - add more test cases to test the behavior of different
>> PP_FileOpenFlags_Dev
>> combinations.
>> - also unify the conversion from PlatformFileError to Pepper error.
>>
>> BUG=68489
>> TEST=New test cases in test_file_io.cc
>>
>>
>> Please review this at http://codereview.chromium.org/7038032/
>>
>> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>>
>> Affected files:
>>  M base/platform_file_posix.cc
>>  M ppapi/tests/test_file_io.h
>>  M ppapi/tests/test_file_io.cc
>>  M ppapi/tests/test_file_ref.cc
>>  M webkit/glue/webkit_glue.gypi
>>  D webkit/plugins/ppapi/error_util.h
>>  D webkit/plugins/ppapi/error_util.cc
>>  M webkit/plugins/ppapi/file_callbacks.cc
>>  A webkit/plugins/ppapi/file_type_conversion.h
>>  A webkit/plugins/ppapi/file_type_conversion.cc
>>  M webkit/plugins/ppapi/ppb_file_io_impl.cc
>>  M webkit/plugins/ppapi/ppb_flash_file_impl.cc
>>
>>
>>
>
>
> --
> Best regards,
> Yuzhu Shen.
>
>

[darin (slow to review), 2011-05-20 22:26:19]

Actually, when I read platform_file.h, it is really not obvious that these
five flags are
mutually exclusive.  Take a look:

enum PlatformFileFlags {
  PLATFORM_FILE_OPEN = 1,
  PLATFORM_FILE_CREATE = 2,
  PLATFORM_FILE_OPEN_ALWAYS = 4,    // May create a new file.
  PLATFORM_FILE_CREATE_ALWAYS = 8,  // May overwrite an old file.
  PLATFORM_FILE_READ = 16,
  PLATFORM_FILE_WRITE = 32,
  PLATFORM_FILE_EXCLUSIVE_READ = 64,  // EXCLUSIVE is opposite of Windows
SHARE
  PLATFORM_FILE_EXCLUSIVE_WRITE = 128,
  PLATFORM_FILE_ASYNC = 256,
  PLATFORM_FILE_TEMPORARY = 512,        // Used on Windows only
  PLATFORM_FILE_HIDDEN = 1024,          // Used on Windows only
  PLATFORM_FILE_DELETE_ON_CLOSE = 2048,
  PLATFORM_FILE_TRUNCATE = 4096,
  PLATFORM_FILE_WRITE_ATTRIBUTES = 8192,  // Used on Windows only
  PLATFORM_FILE_ENUMERATE = 16384,  // May enumerate directory
};

The first four flags look mutually exclusive, but TRUNCATE doesn't appear
until far below.  At that point, it looks like something I should combine
with OPEN / OPEN_ALWAYS, and it looks like something that would be redundant
if specified along with CREATE / CREATE_ALWAYS.

At minimum, we need to clarify the documentation.

If TRUNCATE is not mutually exclusive, then on Windows, we'd need to do
extra work to support combining PLATFORM_FILE_OPEN_ALWAYS with
PLATFORM_FILE_TRUNCATE.

-Darin



On Fri, May 20, 2011 at 3:11 PM, Darin Fisher <darin@chromium.org> wrote:

> OK, makes sense.  Thanks!
>
>
> On Fri, May 20, 2011 at 2:35 PM, Yuzhu Shen <yzshen@google.com> wrote:
>
>> One more note:
>> Currently, PlatformFileFlags looks like Windows file open flags; while
>> PP_FileOpenFlags_Dev looks like Posix file open flags.
>> That is why the mapping
>>   PP_FileOpenFlags -> PlatfromFileFlags -> Posix file open flags
>> looks confusing.
>>
>> On Fri, May 20, 2011 at 2:29 PM, <yzshen@chromium.org> wrote:
>>
>>> Reviewers: darin,
>>>
>>> Message:
>>> Hi, Darin.
>>>
>>> Thanks for your reply. Please see my comments.
>>>
>>>
>>>
>>> http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
>>> File base/platform_file_posix.cc (right):
>>>
>>>
>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>> base/platform_file_posix.cc:48: DCHECK(!open_flags);
>>> On 2011/05/20 21:02:13, darin wrote:
>>>
>>>> so this means that it is not possible to specify PLATFORM_FILE_CREATE
>>>>
>>> and
>>>
>>>> PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to
>>>>
>>> specify
>>>
>>>> O_CREAT and O_TRUNC together.  while it may seem redundant to indicate
>>>>
>>> that a
>>>
>>>> file should be truncated when created (since the file is 0 length
>>>>
>>> anyways), i
>>>
>>>> don't see the harm in doing so.
>>>>
>>>
>>> I think PlatformFileFlags are probably designed to look similar to Win
>>> flags.
>>> As a result, the followings have an 1-1 mapping to Win flags:
>>> PLATFORM_FILE_OPEN <-> OPEN_EXISTING
>>> PLATFORM_FILE_CREATE <-> CREATE_NEW
>>> PLATFORM_FILE_OPEN_ALWAYS <-> OPEN_ALWAYS
>>> PLATFORM_FILE_CREATE_ALWAYS <-> CREATE_ALWAYS
>>> PLATFORM_FILE_TRUNCATE <-> TRUNCATE_EXISTING
>>> And platform_file_win enforces that these five PlatformFileFlags cannot
>>> be used together with each other.
>>> It would be good to be consistent.
>>>
>>> The way to specify O_CREAT | O_TRUNC is PLATFORM_FILE_CREATE_ALWAYS.
>>>
>>>
>>>
>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>> base/platform_file_posix.cc:53: if (!open_flags && !(flags &
>>> PLATFORM_FILE_OPEN) &&
>>> On 2011/05/20 21:02:13, darin wrote:
>>>
>>>> it looks like you might be breaking this check, which was trying to
>>>>
>>> assert that
>>>
>>>> at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
>>>> PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.  with
>>>>
>>> your
>>>
>>>> change above, it is possible for PLATFORM_FILE_TRUNCATE to be
>>>>
>>> specified without
>>>
>>>> having specified any of those other 4 flags.  is that really what you
>>>>
>>> intended?
>>>
>>>  i always thought that truncate should be a modifier for opening an
>>>>
>>> existing
>>>
>>>> file.
>>>>
>>>
>>> This is what I have described in the CL description:
>>> CreatePlatformFile: resolve the conflict between the win and posix
>>> implementation. Before this change, the win implementation didn't allow
>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>> (OPEN|CREATE)(_ALWAYS)? flags; while the posix implementation required
>>> it to be used with them.
>>>
>>> I don't have a strong opinion about which way is better. But we have to
>>> keep the win and posix implementation consistent.
>>>
>>> Description:
>>> Fix PP_FileOpenFlags_Dev handling:
>>> - rewrite the mapping from PP_FileOpenFlags_Dev to PlatformFileFlags.
>>> - let ppb_flash_file_impl and ppb_file_io_impl use the same mapping
>>> logic.
>>> - CreatePlatformFile: resolve the conflict between the win and posix
>>> implementation. Before this change, the win implementation didn't allow
>>> PLATFORM_FILE_TRUNCATE to be used with any of the (OPEN|CREATE)(_ALWAYS)?
>>> flags;
>>> while the posix implementation required it to be used with them.
>>> - add more test cases to test the behavior of different
>>> PP_FileOpenFlags_Dev
>>> combinations.
>>> - also unify the conversion from PlatformFileError to Pepper error.
>>>
>>> BUG=68489
>>> TEST=New test cases in test_file_io.cc
>>>
>>>
>>> Please review this at http://codereview.chromium.org/7038032/
>>>
>>> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>>>
>>> Affected files:
>>>  M base/platform_file_posix.cc
>>>  M ppapi/tests/test_file_io.h
>>>  M ppapi/tests/test_file_io.cc
>>>  M ppapi/tests/test_file_ref.cc
>>>  M webkit/glue/webkit_glue.gypi
>>>  D webkit/plugins/ppapi/error_util.h
>>>  D webkit/plugins/ppapi/error_util.cc
>>>  M webkit/plugins/ppapi/file_callbacks.cc
>>>  A webkit/plugins/ppapi/file_type_conversion.h
>>>  A webkit/plugins/ppapi/file_type_conversion.cc
>>>  M webkit/plugins/ppapi/ppb_file_io_impl.cc
>>>  M webkit/plugins/ppapi/ppb_flash_file_impl.cc
>>>
>>>
>>>
>>
>>
>> --
>> Best regards,
>> Yuzhu Shen.
>>
>>
>

[yzshen, 2011-05-20 22:43:12]

Hi, Darin.

On Fri, May 20, 2011 at 3:26 PM, Darin Fisher <darin@chromium.org> wrote:

> Actually, when I read platform_file.h, it is really not obvious that these
> five flags are
> mutually exclusive.  Take a look:
>
> enum PlatformFileFlags {
>   PLATFORM_FILE_OPEN = 1,
>   PLATFORM_FILE_CREATE = 2,
>   PLATFORM_FILE_OPEN_ALWAYS = 4,    // May create a new file.
>   PLATFORM_FILE_CREATE_ALWAYS = 8,  // May overwrite an old file.
>   PLATFORM_FILE_READ = 16,
>   PLATFORM_FILE_WRITE = 32,
>   PLATFORM_FILE_EXCLUSIVE_READ = 64,  // EXCLUSIVE is opposite of Windows
> SHARE
>   PLATFORM_FILE_EXCLUSIVE_WRITE = 128,
>   PLATFORM_FILE_ASYNC = 256,
>   PLATFORM_FILE_TEMPORARY = 512,        // Used on Windows only
>   PLATFORM_FILE_HIDDEN = 1024,          // Used on Windows only
>   PLATFORM_FILE_DELETE_ON_CLOSE = 2048,
>   PLATFORM_FILE_TRUNCATE = 4096,
>   PLATFORM_FILE_WRITE_ATTRIBUTES = 8192,  // Used on Windows only
>   PLATFORM_FILE_ENUMERATE = 16384,  // May enumerate directory
> };
>
> The first four flags look mutually exclusive, but TRUNCATE doesn't appear
> until far below.  At that point, it looks like something I should combine
> with OPEN / OPEN_ALWAYS, and it looks like something that would be redundant
> if specified along with CREATE / CREATE_ALWAYS.
>
> At minimum, we need to clarify the documentation.
>
I totally agree. I had to read the code to understand what's going on.


> If TRUNCATE is not mutually exclusive, then on Windows, we'd need to do
> extra work to support combining PLATFORM_FILE_OPEN_ALWAYS with
> PLATFORM_FILE_TRUNCATE.
>
If P_F_TRUNCATE is not mutually exclusive,  then
P_F_OPEN_ALWAYS + P_F_TRUNCATE  <-> P_F_CREATE_ALWAYS

I personally would like to keep the current semantics (mutually exclusive),
and add very clear comments to platform_file.h.

>
> -Darin
>
>
>
> On Fri, May 20, 2011 at 3:11 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> OK, makes sense.  Thanks!
>>
>>
>> On Fri, May 20, 2011 at 2:35 PM, Yuzhu Shen <yzshen@google.com> wrote:
>>
>>> One more note:
>>> Currently, PlatformFileFlags looks like Windows file open flags; while
>>> PP_FileOpenFlags_Dev looks like Posix file open flags.
>>> That is why the mapping
>>>   PP_FileOpenFlags -> PlatfromFileFlags -> Posix file open flags
>>> looks confusing.
>>>
>>> On Fri, May 20, 2011 at 2:29 PM, <yzshen@chromium.org> wrote:
>>>
>>>> Reviewers: darin,
>>>>
>>>> Message:
>>>> Hi, Darin.
>>>>
>>>> Thanks for your reply. Please see my comments.
>>>>
>>>>
>>>>
>>>>
>>>> http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
>>>> File base/platform_file_posix.cc (right):
>>>>
>>>>
>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>>> base/platform_file_posix.cc:48: DCHECK(!open_flags);
>>>> On 2011/05/20 21:02:13, darin wrote:
>>>>
>>>>> so this means that it is not possible to specify PLATFORM_FILE_CREATE
>>>>>
>>>> and
>>>>
>>>>> PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to
>>>>>
>>>> specify
>>>>
>>>>> O_CREAT and O_TRUNC together.  while it may seem redundant to indicate
>>>>>
>>>> that a
>>>>
>>>>> file should be truncated when created (since the file is 0 length
>>>>>
>>>> anyways), i
>>>>
>>>>> don't see the harm in doing so.
>>>>>
>>>>
>>>> I think PlatformFileFlags are probably designed to look similar to Win
>>>> flags.
>>>> As a result, the followings have an 1-1 mapping to Win flags:
>>>> PLATFORM_FILE_OPEN <-> OPEN_EXISTING
>>>> PLATFORM_FILE_CREATE <-> CREATE_NEW
>>>> PLATFORM_FILE_OPEN_ALWAYS <-> OPEN_ALWAYS
>>>> PLATFORM_FILE_CREATE_ALWAYS <-> CREATE_ALWAYS
>>>> PLATFORM_FILE_TRUNCATE <-> TRUNCATE_EXISTING
>>>> And platform_file_win enforces that these five PlatformFileFlags cannot
>>>> be used together with each other.
>>>> It would be good to be consistent.
>>>>
>>>> The way to specify O_CREAT | O_TRUNC is PLATFORM_FILE_CREATE_ALWAYS.
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>>> base/platform_file_posix.cc:53: if (!open_flags && !(flags &
>>>> PLATFORM_FILE_OPEN) &&
>>>> On 2011/05/20 21:02:13, darin wrote:
>>>>
>>>>> it looks like you might be breaking this check, which was trying to
>>>>>
>>>> assert that
>>>>
>>>>> at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
>>>>> PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.  with
>>>>>
>>>> your
>>>>
>>>>> change above, it is possible for PLATFORM_FILE_TRUNCATE to be
>>>>>
>>>> specified without
>>>>
>>>>> having specified any of those other 4 flags.  is that really what you
>>>>>
>>>> intended?
>>>>
>>>>  i always thought that truncate should be a modifier for opening an
>>>>>
>>>> existing
>>>>
>>>>> file.
>>>>>
>>>>
>>>> This is what I have described in the CL description:
>>>> CreatePlatformFile: resolve the conflict between the win and posix
>>>> implementation. Before this change, the win implementation didn't allow
>>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>>> (OPEN|CREATE)(_ALWAYS)? flags; while the posix implementation required
>>>> it to be used with them.
>>>>
>>>> I don't have a strong opinion about which way is better. But we have to
>>>> keep the win and posix implementation consistent.
>>>>
>>>> Description:
>>>> Fix PP_FileOpenFlags_Dev handling:
>>>> - rewrite the mapping from PP_FileOpenFlags_Dev to PlatformFileFlags.
>>>> - let ppb_flash_file_impl and ppb_file_io_impl use the same mapping
>>>> logic.
>>>> - CreatePlatformFile: resolve the conflict between the win and posix
>>>> implementation. Before this change, the win implementation didn't allow
>>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>>> (OPEN|CREATE)(_ALWAYS)? flags;
>>>> while the posix implementation required it to be used with them.
>>>> - add more test cases to test the behavior of different
>>>> PP_FileOpenFlags_Dev
>>>> combinations.
>>>> - also unify the conversion from PlatformFileError to Pepper error.
>>>>
>>>> BUG=68489
>>>> TEST=New test cases in test_file_io.cc
>>>>
>>>>
>>>> Please review this at http://codereview.chromium.org/7038032/
>>>>
>>>> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>>>>
>>>> Affected files:
>>>>  M base/platform_file_posix.cc
>>>>  M ppapi/tests/test_file_io.h
>>>>  M ppapi/tests/test_file_io.cc
>>>>  M ppapi/tests/test_file_ref.cc
>>>>  M webkit/glue/webkit_glue.gypi
>>>>  D webkit/plugins/ppapi/error_util.h
>>>>  D webkit/plugins/ppapi/error_util.cc
>>>>  M webkit/plugins/ppapi/file_callbacks.cc
>>>>  A webkit/plugins/ppapi/file_type_conversion.h
>>>>  A webkit/plugins/ppapi/file_type_conversion.cc
>>>>  M webkit/plugins/ppapi/ppb_file_io_impl.cc
>>>>  M webkit/plugins/ppapi/ppb_flash_file_impl.cc
>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Yuzhu Shen.
>>>
>>>
>>
>


-- 
Best regards,
Yuzhu Shen.

[yzshen1, 2011-05-20 23:08:41]

Hi, Darin.

I have added more comments in platform_file.h.
Please let me know whether that looks okay to you.
Thanks!

On 2011/05/20 22:43:12, yzshen wrote:
> Hi, Darin.
> 
> On Fri, May 20, 2011 at 3:26 PM, Darin Fisher <mailto:darin@chromium.org>
wrote:
> 
> > Actually, when I read platform_file.h, it is really not obvious that these
> > five flags are
> > mutually exclusive.  Take a look:
> >
> > enum PlatformFileFlags {
> >   PLATFORM_FILE_OPEN = 1,
> >   PLATFORM_FILE_CREATE = 2,
> >   PLATFORM_FILE_OPEN_ALWAYS = 4,    // May create a new file.
> >   PLATFORM_FILE_CREATE_ALWAYS = 8,  // May overwrite an old file.
> >   PLATFORM_FILE_READ = 16,
> >   PLATFORM_FILE_WRITE = 32,
> >   PLATFORM_FILE_EXCLUSIVE_READ = 64,  // EXCLUSIVE is opposite of Windows
> > SHARE
> >   PLATFORM_FILE_EXCLUSIVE_WRITE = 128,
> >   PLATFORM_FILE_ASYNC = 256,
> >   PLATFORM_FILE_TEMPORARY = 512,        // Used on Windows only
> >   PLATFORM_FILE_HIDDEN = 1024,          // Used on Windows only
> >   PLATFORM_FILE_DELETE_ON_CLOSE = 2048,
> >   PLATFORM_FILE_TRUNCATE = 4096,
> >   PLATFORM_FILE_WRITE_ATTRIBUTES = 8192,  // Used on Windows only
> >   PLATFORM_FILE_ENUMERATE = 16384,  // May enumerate directory
> > };
> >
> > The first four flags look mutually exclusive, but TRUNCATE doesn't appear
> > until far below.  At that point, it looks like something I should combine
> > with OPEN / OPEN_ALWAYS, and it looks like something that would be redundant
> > if specified along with CREATE / CREATE_ALWAYS.
> >
> > At minimum, we need to clarify the documentation.
> >
> I totally agree. I had to read the code to understand what's going on.
> 
> 
> > If TRUNCATE is not mutually exclusive, then on Windows, we'd need to do
> > extra work to support combining PLATFORM_FILE_OPEN_ALWAYS with
> > PLATFORM_FILE_TRUNCATE.
> >
> If P_F_TRUNCATE is not mutually exclusive,  then
> P_F_OPEN_ALWAYS + P_F_TRUNCATE  <-> P_F_CREATE_ALWAYS
> 
> I personally would like to keep the current semantics (mutually exclusive),
> and add very clear comments to platform_file.h.
> 
> >
> > -Darin
> >
> >
> >
> > On Fri, May 20, 2011 at 3:11 PM, Darin Fisher <mailto:darin@chromium.org>
wrote:
> >
> >> OK, makes sense.  Thanks!
> >>
> >>
> >> On Fri, May 20, 2011 at 2:35 PM, Yuzhu Shen <mailto:yzshen@google.com>
wrote:
> >>
> >>> One more note:
> >>> Currently, PlatformFileFlags looks like Windows file open flags; while
> >>> PP_FileOpenFlags_Dev looks like Posix file open flags.
> >>> That is why the mapping
> >>>   PP_FileOpenFlags -> PlatfromFileFlags -> Posix file open flags
> >>> looks confusing.
> >>>
> >>> On Fri, May 20, 2011 at 2:29 PM, <mailto:yzshen@chromium.org> wrote:
> >>>
> >>>> Reviewers: darin,
> >>>>
> >>>> Message:
> >>>> Hi, Darin.
> >>>>
> >>>> Thanks for your reply. Please see my comments.
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
> >>>> File base/platform_file_posix.cc (right):
> >>>>
> >>>>
> >>>>
>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
> >>>> base/platform_file_posix.cc:48: DCHECK(!open_flags);
> >>>> On 2011/05/20 21:02:13, darin wrote:
> >>>>
> >>>>> so this means that it is not possible to specify PLATFORM_FILE_CREATE
> >>>>>
> >>>> and
> >>>>
> >>>>> PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to
> >>>>>
> >>>> specify
> >>>>
> >>>>> O_CREAT and O_TRUNC together.  while it may seem redundant to indicate
> >>>>>
> >>>> that a
> >>>>
> >>>>> file should be truncated when created (since the file is 0 length
> >>>>>
> >>>> anyways), i
> >>>>
> >>>>> don't see the harm in doing so.
> >>>>>
> >>>>
> >>>> I think PlatformFileFlags are probably designed to look similar to Win
> >>>> flags.
> >>>> As a result, the followings have an 1-1 mapping to Win flags:
> >>>> PLATFORM_FILE_OPEN <-> OPEN_EXISTING
> >>>> PLATFORM_FILE_CREATE <-> CREATE_NEW
> >>>> PLATFORM_FILE_OPEN_ALWAYS <-> OPEN_ALWAYS
> >>>> PLATFORM_FILE_CREATE_ALWAYS <-> CREATE_ALWAYS
> >>>> PLATFORM_FILE_TRUNCATE <-> TRUNCATE_EXISTING
> >>>> And platform_file_win enforces that these five PlatformFileFlags cannot
> >>>> be used together with each other.
> >>>> It would be good to be consistent.
> >>>>
> >>>> The way to specify O_CREAT | O_TRUNC is PLATFORM_FILE_CREATE_ALWAYS.
> >>>>
> >>>>
> >>>>
> >>>>
>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
> >>>> base/platform_file_posix.cc:53: if (!open_flags && !(flags &
> >>>> PLATFORM_FILE_OPEN) &&
> >>>> On 2011/05/20 21:02:13, darin wrote:
> >>>>
> >>>>> it looks like you might be breaking this check, which was trying to
> >>>>>
> >>>> assert that
> >>>>
> >>>>> at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
> >>>>> PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.  with
> >>>>>
> >>>> your
> >>>>
> >>>>> change above, it is possible for PLATFORM_FILE_TRUNCATE to be
> >>>>>
> >>>> specified without
> >>>>
> >>>>> having specified any of those other 4 flags.  is that really what you
> >>>>>
> >>>> intended?
> >>>>
> >>>>  i always thought that truncate should be a modifier for opening an
> >>>>>
> >>>> existing
> >>>>
> >>>>> file.
> >>>>>
> >>>>
> >>>> This is what I have described in the CL description:
> >>>> CreatePlatformFile: resolve the conflict between the win and posix
> >>>> implementation. Before this change, the win implementation didn't allow
> >>>> PLATFORM_FILE_TRUNCATE to be used with any of the
> >>>> (OPEN|CREATE)(_ALWAYS)? flags; while the posix implementation required
> >>>> it to be used with them.
> >>>>
> >>>> I don't have a strong opinion about which way is better. But we have to
> >>>> keep the win and posix implementation consistent.
> >>>>
> >>>> Description:
> >>>> Fix PP_FileOpenFlags_Dev handling:
> >>>> - rewrite the mapping from PP_FileOpenFlags_Dev to PlatformFileFlags.
> >>>> - let ppb_flash_file_impl and ppb_file_io_impl use the same mapping
> >>>> logic.
> >>>> - CreatePlatformFile: resolve the conflict between the win and posix
> >>>> implementation. Before this change, the win implementation didn't allow
> >>>> PLATFORM_FILE_TRUNCATE to be used with any of the
> >>>> (OPEN|CREATE)(_ALWAYS)? flags;
> >>>> while the posix implementation required it to be used with them.
> >>>> - add more test cases to test the behavior of different
> >>>> PP_FileOpenFlags_Dev
> >>>> combinations.
> >>>> - also unify the conversion from PlatformFileError to Pepper error.
> >>>>
> >>>> BUG=68489
> >>>> TEST=New test cases in test_file_io.cc
> >>>>
> >>>>
> >>>> Please review this at http://codereview.chromium.org/7038032/
> >>>>
> >>>> SVN Base: http://git.chromium.org/git/chromium.git%40trunk
> >>>>
> >>>> Affected files:
> >>>>  M base/platform_file_posix.cc
> >>>>  M ppapi/tests/test_file_io.h
> >>>>  M ppapi/tests/test_file_io.cc
> >>>>  M ppapi/tests/test_file_ref.cc
> >>>>  M webkit/glue/webkit_glue.gypi
> >>>>  D webkit/plugins/ppapi/error_util.h
> >>>>  D webkit/plugins/ppapi/error_util.cc
> >>>>  M webkit/plugins/ppapi/file_callbacks.cc
> >>>>  A webkit/plugins/ppapi/file_type_conversion.h
> >>>>  A webkit/plugins/ppapi/file_type_conversion.cc
> >>>>  M webkit/plugins/ppapi/ppb_file_io_impl.cc
> >>>>  M webkit/plugins/ppapi/ppb_flash_file_impl.cc
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>> --
> >>> Best regards,
> >>> Yuzhu Shen.
> >>>
> >>>
> >>
> >
> 
> 
> -- 
> Best regards,
> Yuzhu Shen.

[darin (slow to review), 2011-05-20 23:24:35]

On Fri, May 20, 2011 at 3:43 PM, Yuzhu Shen <yzshen@google.com> wrote:

> Hi, Darin.
>
> On Fri, May 20, 2011 at 3:26 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> Actually, when I read platform_file.h, it is really not obvious that these
>> five flags are
>> mutually exclusive.  Take a look:
>>
>> enum PlatformFileFlags {
>>   PLATFORM_FILE_OPEN = 1,
>>   PLATFORM_FILE_CREATE = 2,
>>   PLATFORM_FILE_OPEN_ALWAYS = 4,    // May create a new file.
>>   PLATFORM_FILE_CREATE_ALWAYS = 8,  // May overwrite an old file.
>>   PLATFORM_FILE_READ = 16,
>>   PLATFORM_FILE_WRITE = 32,
>>   PLATFORM_FILE_EXCLUSIVE_READ = 64,  // EXCLUSIVE is opposite of Windows
>> SHARE
>>   PLATFORM_FILE_EXCLUSIVE_WRITE = 128,
>>   PLATFORM_FILE_ASYNC = 256,
>>   PLATFORM_FILE_TEMPORARY = 512,        // Used on Windows only
>>   PLATFORM_FILE_HIDDEN = 1024,          // Used on Windows only
>>   PLATFORM_FILE_DELETE_ON_CLOSE = 2048,
>>   PLATFORM_FILE_TRUNCATE = 4096,
>>   PLATFORM_FILE_WRITE_ATTRIBUTES = 8192,  // Used on Windows only
>>   PLATFORM_FILE_ENUMERATE = 16384,  // May enumerate directory
>> };
>>
>> The first four flags look mutually exclusive, but TRUNCATE doesn't appear
>> until far below.  At that point, it looks like something I should combine
>> with OPEN / OPEN_ALWAYS, and it looks like something that would be redundant
>> if specified along with CREATE / CREATE_ALWAYS.
>>
>> At minimum, we need to clarify the documentation.
>>
> I totally agree. I had to read the code to understand what's going on.
>
>
>> If TRUNCATE is not mutually exclusive, then on Windows, we'd need to do
>> extra work to support combining PLATFORM_FILE_OPEN_ALWAYS with
>> PLATFORM_FILE_TRUNCATE.
>>
> If P_F_TRUNCATE is not mutually exclusive,  then
> P_F_OPEN_ALWAYS + P_F_TRUNCATE  <-> P_F_CREATE_ALWAYS
>


good point!



> I personally would like to keep the current semantics (mutually exclusive),
> and add very clear comments to platform_file.h.
>

OK, maybe we should also move P_F_TRUNCATE to be next to the others?  maybe
it should be renamed P_F_TRUNCATE_EXISTING, or maybe P_F_OPEN_TRUNCATED
would be better (so that it looks more parallel to P_F_OPEN and
P_F_OPEN_ALWAYS)?

-darin



>
>> -Darin
>>
>>
>>
>> On Fri, May 20, 2011 at 3:11 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>>> OK, makes sense.  Thanks!
>>>
>>>
>>> On Fri, May 20, 2011 at 2:35 PM, Yuzhu Shen <yzshen@google.com> wrote:
>>>
>>>> One more note:
>>>> Currently, PlatformFileFlags looks like Windows file open flags; while
>>>> PP_FileOpenFlags_Dev looks like Posix file open flags.
>>>> That is why the mapping
>>>>   PP_FileOpenFlags -> PlatfromFileFlags -> Posix file open flags
>>>> looks confusing.
>>>>
>>>> On Fri, May 20, 2011 at 2:29 PM, <yzshen@chromium.org> wrote:
>>>>
>>>>> Reviewers: darin,
>>>>>
>>>>> Message:
>>>>> Hi, Darin.
>>>>>
>>>>> Thanks for your reply. Please see my comments.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
>>>>> File base/platform_file_posix.cc (right):
>>>>>
>>>>>
>>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>>>> base/platform_file_posix.cc:48: DCHECK(!open_flags);
>>>>> On 2011/05/20 21:02:13, darin wrote:
>>>>>
>>>>>> so this means that it is not possible to specify PLATFORM_FILE_CREATE
>>>>>>
>>>>> and
>>>>>
>>>>>> PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to
>>>>>>
>>>>> specify
>>>>>
>>>>>> O_CREAT and O_TRUNC together.  while it may seem redundant to indicate
>>>>>>
>>>>> that a
>>>>>
>>>>>> file should be truncated when created (since the file is 0 length
>>>>>>
>>>>> anyways), i
>>>>>
>>>>>> don't see the harm in doing so.
>>>>>>
>>>>>
>>>>> I think PlatformFileFlags are probably designed to look similar to Win
>>>>> flags.
>>>>> As a result, the followings have an 1-1 mapping to Win flags:
>>>>> PLATFORM_FILE_OPEN <-> OPEN_EXISTING
>>>>> PLATFORM_FILE_CREATE <-> CREATE_NEW
>>>>> PLATFORM_FILE_OPEN_ALWAYS <-> OPEN_ALWAYS
>>>>> PLATFORM_FILE_CREATE_ALWAYS <-> CREATE_ALWAYS
>>>>> PLATFORM_FILE_TRUNCATE <-> TRUNCATE_EXISTING
>>>>> And platform_file_win enforces that these five PlatformFileFlags cannot
>>>>> be used together with each other.
>>>>> It would be good to be consistent.
>>>>>
>>>>> The way to specify O_CREAT | O_TRUNC is PLATFORM_FILE_CREATE_ALWAYS.
>>>>>
>>>>>
>>>>>
>>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>>>> base/platform_file_posix.cc:53: if (!open_flags && !(flags &
>>>>> PLATFORM_FILE_OPEN) &&
>>>>> On 2011/05/20 21:02:13, darin wrote:
>>>>>
>>>>>> it looks like you might be breaking this check, which was trying to
>>>>>>
>>>>> assert that
>>>>>
>>>>>> at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
>>>>>> PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.  with
>>>>>>
>>>>> your
>>>>>
>>>>>> change above, it is possible for PLATFORM_FILE_TRUNCATE to be
>>>>>>
>>>>> specified without
>>>>>
>>>>>> having specified any of those other 4 flags.  is that really what you
>>>>>>
>>>>> intended?
>>>>>
>>>>>  i always thought that truncate should be a modifier for opening an
>>>>>>
>>>>> existing
>>>>>
>>>>>> file.
>>>>>>
>>>>>
>>>>> This is what I have described in the CL description:
>>>>> CreatePlatformFile: resolve the conflict between the win and posix
>>>>> implementation. Before this change, the win implementation didn't allow
>>>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>>>> (OPEN|CREATE)(_ALWAYS)? flags; while the posix implementation required
>>>>> it to be used with them.
>>>>>
>>>>> I don't have a strong opinion about which way is better. But we have to
>>>>> keep the win and posix implementation consistent.
>>>>>
>>>>> Description:
>>>>> Fix PP_FileOpenFlags_Dev handling:
>>>>> - rewrite the mapping from PP_FileOpenFlags_Dev to PlatformFileFlags.
>>>>> - let ppb_flash_file_impl and ppb_file_io_impl use the same mapping
>>>>> logic.
>>>>> - CreatePlatformFile: resolve the conflict between the win and posix
>>>>> implementation. Before this change, the win implementation didn't allow
>>>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>>>> (OPEN|CREATE)(_ALWAYS)? flags;
>>>>> while the posix implementation required it to be used with them.
>>>>> - add more test cases to test the behavior of different
>>>>> PP_FileOpenFlags_Dev
>>>>> combinations.
>>>>> - also unify the conversion from PlatformFileError to Pepper error.
>>>>>
>>>>> BUG=68489
>>>>> TEST=New test cases in test_file_io.cc
>>>>>
>>>>>
>>>>> Please review this at http://codereview.chromium.org/7038032/
>>>>>
>>>>> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>>>>>
>>>>> Affected files:
>>>>>  M base/platform_file_posix.cc
>>>>>  M ppapi/tests/test_file_io.h
>>>>>  M ppapi/tests/test_file_io.cc
>>>>>  M ppapi/tests/test_file_ref.cc
>>>>>  M webkit/glue/webkit_glue.gypi
>>>>>  D webkit/plugins/ppapi/error_util.h
>>>>>  D webkit/plugins/ppapi/error_util.cc
>>>>>  M webkit/plugins/ppapi/file_callbacks.cc
>>>>>  A webkit/plugins/ppapi/file_type_conversion.h
>>>>>  A webkit/plugins/ppapi/file_type_conversion.cc
>>>>>  M webkit/plugins/ppapi/ppb_file_io_impl.cc
>>>>>  M webkit/plugins/ppapi/ppb_flash_file_impl.cc
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Best regards,
>>>> Yuzhu Shen.
>>>>
>>>>
>>>
>>
>
>
> --
> Best regards,
> Yuzhu Shen.
>
>

[yzshen, 2011-05-20 23:28:42]

Hi, Darin.

On Fri, May 20, 2011 at 4:24 PM, Darin Fisher <darin@chromium.org> wrote:

> On Fri, May 20, 2011 at 3:43 PM, Yuzhu Shen <yzshen@google.com> wrote:
>
>> Hi, Darin.
>>
>> On Fri, May 20, 2011 at 3:26 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>>> Actually, when I read platform_file.h, it is really not obvious that
>>> these five flags are
>>> mutually exclusive.  Take a look:
>>>
>>> enum PlatformFileFlags {
>>>   PLATFORM_FILE_OPEN = 1,
>>>   PLATFORM_FILE_CREATE = 2,
>>>   PLATFORM_FILE_OPEN_ALWAYS = 4,    // May create a new file.
>>>   PLATFORM_FILE_CREATE_ALWAYS = 8,  // May overwrite an old file.
>>>   PLATFORM_FILE_READ = 16,
>>>   PLATFORM_FILE_WRITE = 32,
>>>   PLATFORM_FILE_EXCLUSIVE_READ = 64,  // EXCLUSIVE is opposite of Windows
>>> SHARE
>>>   PLATFORM_FILE_EXCLUSIVE_WRITE = 128,
>>>   PLATFORM_FILE_ASYNC = 256,
>>>   PLATFORM_FILE_TEMPORARY = 512,        // Used on Windows only
>>>   PLATFORM_FILE_HIDDEN = 1024,          // Used on Windows only
>>>   PLATFORM_FILE_DELETE_ON_CLOSE = 2048,
>>>   PLATFORM_FILE_TRUNCATE = 4096,
>>>   PLATFORM_FILE_WRITE_ATTRIBUTES = 8192,  // Used on Windows only
>>>   PLATFORM_FILE_ENUMERATE = 16384,  // May enumerate directory
>>> };
>>>
>>> The first four flags look mutually exclusive, but TRUNCATE doesn't appear
>>> until far below.  At that point, it looks like something I should combine
>>> with OPEN / OPEN_ALWAYS, and it looks like something that would be redundant
>>> if specified along with CREATE / CREATE_ALWAYS.
>>>
>>> At minimum, we need to clarify the documentation.
>>>
>> I totally agree. I had to read the code to understand what's going on.
>>
>>
>>> If TRUNCATE is not mutually exclusive, then on Windows, we'd need to do
>>> extra work to support combining PLATFORM_FILE_OPEN_ALWAYS with
>>> PLATFORM_FILE_TRUNCATE.
>>>
>> If P_F_TRUNCATE is not mutually exclusive,  then
>> P_F_OPEN_ALWAYS + P_F_TRUNCATE  <-> P_F_CREATE_ALWAYS
>>
>
>
> good point!
>
>
>
>> I personally would like to keep the current semantics (mutually
>> exclusive), and add very clear comments to platform_file.h.
>>
>
> OK, maybe we should also move P_F_TRUNCATE to be next to the others?  maybe
> it should be renamed P_F_TRUNCATE_EXISTING, or maybe P_F_OPEN_TRUNCATED
> would be better (so that it looks more parallel to P_F_OPEN and
> P_F_OPEN_ALWAYS)?
>
Thanks for the suggestion!
I will rename&move the flag. And because the enum value for each flag will
be changed, I will also take a look at whether that will affect anything.


>
> -darin
>
>
>
>>
>>> -Darin
>>>
>>>
>>>
>>> On Fri, May 20, 2011 at 3:11 PM, Darin Fisher <darin@chromium.org>wrote:
>>>
>>>> OK, makes sense.  Thanks!
>>>>
>>>>
>>>> On Fri, May 20, 2011 at 2:35 PM, Yuzhu Shen <yzshen@google.com> wrote:
>>>>
>>>>> One more note:
>>>>> Currently, PlatformFileFlags looks like Windows file open flags; while
>>>>> PP_FileOpenFlags_Dev looks like Posix file open flags.
>>>>> That is why the mapping
>>>>>   PP_FileOpenFlags -> PlatfromFileFlags -> Posix file open flags
>>>>> looks confusing.
>>>>>
>>>>> On Fri, May 20, 2011 at 2:29 PM, <yzshen@chromium.org> wrote:
>>>>>
>>>>>> Reviewers: darin,
>>>>>>
>>>>>> Message:
>>>>>> Hi, Darin.
>>>>>>
>>>>>> Thanks for your reply. Please see my comments.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
>>>>>> File base/platform_file_posix.cc (right):
>>>>>>
>>>>>>
>>>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>>>>> base/platform_file_posix.cc:48: DCHECK(!open_flags);
>>>>>> On 2011/05/20 21:02:13, darin wrote:
>>>>>>
>>>>>>> so this means that it is not possible to specify PLATFORM_FILE_CREATE
>>>>>>>
>>>>>> and
>>>>>>
>>>>>>> PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to
>>>>>>>
>>>>>> specify
>>>>>>
>>>>>>> O_CREAT and O_TRUNC together.  while it may seem redundant to
>>>>>>> indicate
>>>>>>>
>>>>>> that a
>>>>>>
>>>>>>> file should be truncated when created (since the file is 0 length
>>>>>>>
>>>>>> anyways), i
>>>>>>
>>>>>>> don't see the harm in doing so.
>>>>>>>
>>>>>>
>>>>>> I think PlatformFileFlags are probably designed to look similar to Win
>>>>>> flags.
>>>>>> As a result, the followings have an 1-1 mapping to Win flags:
>>>>>> PLATFORM_FILE_OPEN <-> OPEN_EXISTING
>>>>>> PLATFORM_FILE_CREATE <-> CREATE_NEW
>>>>>> PLATFORM_FILE_OPEN_ALWAYS <-> OPEN_ALWAYS
>>>>>> PLATFORM_FILE_CREATE_ALWAYS <-> CREATE_ALWAYS
>>>>>> PLATFORM_FILE_TRUNCATE <-> TRUNCATE_EXISTING
>>>>>> And platform_file_win enforces that these five PlatformFileFlags
>>>>>> cannot
>>>>>> be used together with each other.
>>>>>> It would be good to be consistent.
>>>>>>
>>>>>> The way to specify O_CREAT | O_TRUNC is PLATFORM_FILE_CREATE_ALWAYS.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>>>>> base/platform_file_posix.cc:53: if (!open_flags && !(flags &
>>>>>> PLATFORM_FILE_OPEN) &&
>>>>>> On 2011/05/20 21:02:13, darin wrote:
>>>>>>
>>>>>>> it looks like you might be breaking this check, which was trying to
>>>>>>>
>>>>>> assert that
>>>>>>
>>>>>>> at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
>>>>>>> PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.  with
>>>>>>>
>>>>>> your
>>>>>>
>>>>>>> change above, it is possible for PLATFORM_FILE_TRUNCATE to be
>>>>>>>
>>>>>> specified without
>>>>>>
>>>>>>> having specified any of those other 4 flags.  is that really what you
>>>>>>>
>>>>>> intended?
>>>>>>
>>>>>>  i always thought that truncate should be a modifier for opening an
>>>>>>>
>>>>>> existing
>>>>>>
>>>>>>> file.
>>>>>>>
>>>>>>
>>>>>> This is what I have described in the CL description:
>>>>>> CreatePlatformFile: resolve the conflict between the win and posix
>>>>>> implementation. Before this change, the win implementation didn't
>>>>>> allow
>>>>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>>>>> (OPEN|CREATE)(_ALWAYS)? flags; while the posix implementation required
>>>>>> it to be used with them.
>>>>>>
>>>>>> I don't have a strong opinion about which way is better. But we have
>>>>>> to
>>>>>> keep the win and posix implementation consistent.
>>>>>>
>>>>>> Description:
>>>>>> Fix PP_FileOpenFlags_Dev handling:
>>>>>> - rewrite the mapping from PP_FileOpenFlags_Dev to PlatformFileFlags.
>>>>>> - let ppb_flash_file_impl and ppb_file_io_impl use the same mapping
>>>>>> logic.
>>>>>> - CreatePlatformFile: resolve the conflict between the win and posix
>>>>>> implementation. Before this change, the win implementation didn't
>>>>>> allow
>>>>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>>>>> (OPEN|CREATE)(_ALWAYS)? flags;
>>>>>> while the posix implementation required it to be used with them.
>>>>>> - add more test cases to test the behavior of different
>>>>>> PP_FileOpenFlags_Dev
>>>>>> combinations.
>>>>>> - also unify the conversion from PlatformFileError to Pepper error.
>>>>>>
>>>>>> BUG=68489
>>>>>> TEST=New test cases in test_file_io.cc
>>>>>>
>>>>>>
>>>>>> Please review this at http://codereview.chromium.org/7038032/
>>>>>>
>>>>>> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>>>>>>
>>>>>> Affected files:
>>>>>>  M base/platform_file_posix.cc
>>>>>>  M ppapi/tests/test_file_io.h
>>>>>>  M ppapi/tests/test_file_io.cc
>>>>>>  M ppapi/tests/test_file_ref.cc
>>>>>>  M webkit/glue/webkit_glue.gypi
>>>>>>  D webkit/plugins/ppapi/error_util.h
>>>>>>  D webkit/plugins/ppapi/error_util.cc
>>>>>>  M webkit/plugins/ppapi/file_callbacks.cc
>>>>>>  A webkit/plugins/ppapi/file_type_conversion.h
>>>>>>  A webkit/plugins/ppapi/file_type_conversion.cc
>>>>>>  M webkit/plugins/ppapi/ppb_file_io_impl.cc
>>>>>>  M webkit/plugins/ppapi/ppb_flash_file_impl.cc
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Best regards,
>>>>> Yuzhu Shen.
>>>>>
>>>>>
>>>>
>>>
>>
>>
>> --
>> Best regards,
>> Yuzhu Shen.
>>
>>
>


-- 
Best regards,
Yuzhu Shen.

[darin (slow to review), 2011-05-20 23:29:42]

On Fri, May 20, 2011 at 4:28 PM, Yuzhu Shen <yzshen@google.com> wrote:

> Hi, Darin.
>
> On Fri, May 20, 2011 at 4:24 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> On Fri, May 20, 2011 at 3:43 PM, Yuzhu Shen <yzshen@google.com> wrote:
>>
>>> Hi, Darin.
>>>
>>> On Fri, May 20, 2011 at 3:26 PM, Darin Fisher <darin@chromium.org>wrote:
>>>
>>>> Actually, when I read platform_file.h, it is really not obvious that
>>>> these five flags are
>>>> mutually exclusive.  Take a look:
>>>>
>>>> enum PlatformFileFlags {
>>>>   PLATFORM_FILE_OPEN = 1,
>>>>   PLATFORM_FILE_CREATE = 2,
>>>>   PLATFORM_FILE_OPEN_ALWAYS = 4,    // May create a new file.
>>>>   PLATFORM_FILE_CREATE_ALWAYS = 8,  // May overwrite an old file.
>>>>   PLATFORM_FILE_READ = 16,
>>>>   PLATFORM_FILE_WRITE = 32,
>>>>   PLATFORM_FILE_EXCLUSIVE_READ = 64,  // EXCLUSIVE is opposite of
>>>> Windows SHARE
>>>>   PLATFORM_FILE_EXCLUSIVE_WRITE = 128,
>>>>   PLATFORM_FILE_ASYNC = 256,
>>>>   PLATFORM_FILE_TEMPORARY = 512,        // Used on Windows only
>>>>   PLATFORM_FILE_HIDDEN = 1024,          // Used on Windows only
>>>>   PLATFORM_FILE_DELETE_ON_CLOSE = 2048,
>>>>   PLATFORM_FILE_TRUNCATE = 4096,
>>>>   PLATFORM_FILE_WRITE_ATTRIBUTES = 8192,  // Used on Windows only
>>>>   PLATFORM_FILE_ENUMERATE = 16384,  // May enumerate directory
>>>> };
>>>>
>>>> The first four flags look mutually exclusive, but TRUNCATE doesn't
>>>> appear until far below.  At that point, it looks like something I should
>>>> combine with OPEN / OPEN_ALWAYS, and it looks like something that would be
>>>> redundant if specified along with CREATE / CREATE_ALWAYS.
>>>>
>>>> At minimum, we need to clarify the documentation.
>>>>
>>> I totally agree. I had to read the code to understand what's going on.
>>>
>>>
>>>> If TRUNCATE is not mutually exclusive, then on Windows, we'd need to do
>>>> extra work to support combining PLATFORM_FILE_OPEN_ALWAYS with
>>>> PLATFORM_FILE_TRUNCATE.
>>>>
>>> If P_F_TRUNCATE is not mutually exclusive,  then
>>> P_F_OPEN_ALWAYS + P_F_TRUNCATE  <-> P_F_CREATE_ALWAYS
>>>
>>
>>
>> good point!
>>
>>
>>
>>> I personally would like to keep the current semantics (mutually
>>> exclusive), and add very clear comments to platform_file.h.
>>>
>>
>>  OK, maybe we should also move P_F_TRUNCATE to be next to the others?
>>  maybe it should be renamed P_F_TRUNCATE_EXISTING, or maybe
>> P_F_OPEN_TRUNCATED would be better (so that it looks more parallel to
>> P_F_OPEN and P_F_OPEN_ALWAYS)?
>>
> Thanks for the suggestion!
> I will rename&move the flag. And because the enum value for each flag will
> be changed, I will also take a look at whether that will affect anything.
>


Thanks!


>
>
>>
>> -darin
>>
>>
>>
>>>
>>>> -Darin
>>>>
>>>>
>>>>
>>>> On Fri, May 20, 2011 at 3:11 PM, Darin Fisher <darin@chromium.org>wrote:
>>>>
>>>>> OK, makes sense.  Thanks!
>>>>>
>>>>>
>>>>> On Fri, May 20, 2011 at 2:35 PM, Yuzhu Shen <yzshen@google.com> wrote:
>>>>>
>>>>>> One more note:
>>>>>> Currently, PlatformFileFlags looks like Windows file open flags; while
>>>>>> PP_FileOpenFlags_Dev looks like Posix file open flags.
>>>>>> That is why the mapping
>>>>>>   PP_FileOpenFlags -> PlatfromFileFlags -> Posix file open flags
>>>>>> looks confusing.
>>>>>>
>>>>>> On Fri, May 20, 2011 at 2:29 PM, <yzshen@chromium.org> wrote:
>>>>>>
>>>>>>> Reviewers: darin,
>>>>>>>
>>>>>>> Message:
>>>>>>> Hi, Darin.
>>>>>>>
>>>>>>> Thanks for your reply. Please see my comments.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
>>>>>>> File base/platform_file_posix.cc (right):
>>>>>>>
>>>>>>>
>>>>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>>>>>> base/platform_file_posix.cc:48: DCHECK(!open_flags);
>>>>>>> On 2011/05/20 21:02:13, darin wrote:
>>>>>>>
>>>>>>>> so this means that it is not possible to specify
>>>>>>>> PLATFORM_FILE_CREATE
>>>>>>>>
>>>>>>> and
>>>>>>>
>>>>>>>> PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to
>>>>>>>>
>>>>>>> specify
>>>>>>>
>>>>>>>> O_CREAT and O_TRUNC together.  while it may seem redundant to
>>>>>>>> indicate
>>>>>>>>
>>>>>>> that a
>>>>>>>
>>>>>>>> file should be truncated when created (since the file is 0 length
>>>>>>>>
>>>>>>> anyways), i
>>>>>>>
>>>>>>>> don't see the harm in doing so.
>>>>>>>>
>>>>>>>
>>>>>>> I think PlatformFileFlags are probably designed to look similar to
>>>>>>> Win
>>>>>>> flags.
>>>>>>> As a result, the followings have an 1-1 mapping to Win flags:
>>>>>>> PLATFORM_FILE_OPEN <-> OPEN_EXISTING
>>>>>>> PLATFORM_FILE_CREATE <-> CREATE_NEW
>>>>>>> PLATFORM_FILE_OPEN_ALWAYS <-> OPEN_ALWAYS
>>>>>>> PLATFORM_FILE_CREATE_ALWAYS <-> CREATE_ALWAYS
>>>>>>> PLATFORM_FILE_TRUNCATE <-> TRUNCATE_EXISTING
>>>>>>> And platform_file_win enforces that these five PlatformFileFlags
>>>>>>> cannot
>>>>>>> be used together with each other.
>>>>>>> It would be good to be consistent.
>>>>>>>
>>>>>>> The way to specify O_CREAT | O_TRUNC is PLATFORM_FILE_CREATE_ALWAYS.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>>>>>> base/platform_file_posix.cc:53: if (!open_flags && !(flags &
>>>>>>> PLATFORM_FILE_OPEN) &&
>>>>>>> On 2011/05/20 21:02:13, darin wrote:
>>>>>>>
>>>>>>>> it looks like you might be breaking this check, which was trying to
>>>>>>>>
>>>>>>> assert that
>>>>>>>
>>>>>>>> at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
>>>>>>>> PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.
>>>>>>>>  with
>>>>>>>>
>>>>>>> your
>>>>>>>
>>>>>>>> change above, it is possible for PLATFORM_FILE_TRUNCATE to be
>>>>>>>>
>>>>>>> specified without
>>>>>>>
>>>>>>>> having specified any of those other 4 flags.  is that really what
>>>>>>>> you
>>>>>>>>
>>>>>>> intended?
>>>>>>>
>>>>>>>  i always thought that truncate should be a modifier for opening an
>>>>>>>>
>>>>>>> existing
>>>>>>>
>>>>>>>> file.
>>>>>>>>
>>>>>>>
>>>>>>> This is what I have described in the CL description:
>>>>>>> CreatePlatformFile: resolve the conflict between the win and posix
>>>>>>> implementation. Before this change, the win implementation didn't
>>>>>>> allow
>>>>>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>>>>>> (OPEN|CREATE)(_ALWAYS)? flags; while the posix implementation
>>>>>>> required
>>>>>>> it to be used with them.
>>>>>>>
>>>>>>> I don't have a strong opinion about which way is better. But we have
>>>>>>> to
>>>>>>> keep the win and posix implementation consistent.
>>>>>>>
>>>>>>> Description:
>>>>>>> Fix PP_FileOpenFlags_Dev handling:
>>>>>>> - rewrite the mapping from PP_FileOpenFlags_Dev to PlatformFileFlags.
>>>>>>> - let ppb_flash_file_impl and ppb_file_io_impl use the same mapping
>>>>>>> logic.
>>>>>>> - CreatePlatformFile: resolve the conflict between the win and posix
>>>>>>> implementation. Before this change, the win implementation didn't
>>>>>>> allow
>>>>>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>>>>>> (OPEN|CREATE)(_ALWAYS)? flags;
>>>>>>> while the posix implementation required it to be used with them.
>>>>>>> - add more test cases to test the behavior of different
>>>>>>> PP_FileOpenFlags_Dev
>>>>>>> combinations.
>>>>>>> - also unify the conversion from PlatformFileError to Pepper error.
>>>>>>>
>>>>>>> BUG=68489
>>>>>>> TEST=New test cases in test_file_io.cc
>>>>>>>
>>>>>>>
>>>>>>> Please review this at http://codereview.chromium.org/7038032/
>>>>>>>
>>>>>>> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>>>>>>>
>>>>>>> Affected files:
>>>>>>>  M base/platform_file_posix.cc
>>>>>>>  M ppapi/tests/test_file_io.h
>>>>>>>  M ppapi/tests/test_file_io.cc
>>>>>>>  M ppapi/tests/test_file_ref.cc
>>>>>>>  M webkit/glue/webkit_glue.gypi
>>>>>>>  D webkit/plugins/ppapi/error_util.h
>>>>>>>  D webkit/plugins/ppapi/error_util.cc
>>>>>>>  M webkit/plugins/ppapi/file_callbacks.cc
>>>>>>>  A webkit/plugins/ppapi/file_type_conversion.h
>>>>>>>  A webkit/plugins/ppapi/file_type_conversion.cc
>>>>>>>  M webkit/plugins/ppapi/ppb_file_io_impl.cc
>>>>>>>  M webkit/plugins/ppapi/ppb_flash_file_impl.cc
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Best regards,
>>>>>> Yuzhu Shen.
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Yuzhu Shen.
>>>
>>>
>>
>
>
> --
> Best regards,
> Yuzhu Shen.
>
>

[yzshen1, 2011-05-21 02:20:05]

Hi, Darin.

I have updated the CL. Please take another look. Thank you!

On Fri, May 20, 2011 at 4:29 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Fri, May 20, 2011 at 4:28 PM, Yuzhu Shen <yzshen@google.com> wrote:
>
>> Hi, Darin.
>>
>> On Fri, May 20, 2011 at 4:24 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>>> On Fri, May 20, 2011 at 3:43 PM, Yuzhu Shen <yzshen@google.com> wrote:
>>>
>>>> Hi, Darin.
>>>>
>>>> On Fri, May 20, 2011 at 3:26 PM, Darin Fisher <darin@chromium.org>wrote:
>>>>
>>>>> Actually, when I read platform_file.h, it is really not obvious that
>>>>> these five flags are
>>>>> mutually exclusive.  Take a look:
>>>>>
>>>>> enum PlatformFileFlags {
>>>>>   PLATFORM_FILE_OPEN = 1,
>>>>>   PLATFORM_FILE_CREATE = 2,
>>>>>   PLATFORM_FILE_OPEN_ALWAYS = 4,    // May create a new file.
>>>>>   PLATFORM_FILE_CREATE_ALWAYS = 8,  // May overwrite an old file.
>>>>>   PLATFORM_FILE_READ = 16,
>>>>>   PLATFORM_FILE_WRITE = 32,
>>>>>   PLATFORM_FILE_EXCLUSIVE_READ = 64,  // EXCLUSIVE is opposite of
>>>>> Windows SHARE
>>>>>   PLATFORM_FILE_EXCLUSIVE_WRITE = 128,
>>>>>   PLATFORM_FILE_ASYNC = 256,
>>>>>   PLATFORM_FILE_TEMPORARY = 512,        // Used on Windows only
>>>>>   PLATFORM_FILE_HIDDEN = 1024,          // Used on Windows only
>>>>>   PLATFORM_FILE_DELETE_ON_CLOSE = 2048,
>>>>>   PLATFORM_FILE_TRUNCATE = 4096,
>>>>>   PLATFORM_FILE_WRITE_ATTRIBUTES = 8192,  // Used on Windows only
>>>>>   PLATFORM_FILE_ENUMERATE = 16384,  // May enumerate directory
>>>>> };
>>>>>
>>>>> The first four flags look mutually exclusive, but TRUNCATE doesn't
>>>>> appear until far below.  At that point, it looks like something I should
>>>>> combine with OPEN / OPEN_ALWAYS, and it looks like something that would be
>>>>> redundant if specified along with CREATE / CREATE_ALWAYS.
>>>>>
>>>>> At minimum, we need to clarify the documentation.
>>>>>
>>>> I totally agree. I had to read the code to understand what's going on.
>>>>
>>>>
>>>>> If TRUNCATE is not mutually exclusive, then on Windows, we'd need to do
>>>>> extra work to support combining PLATFORM_FILE_OPEN_ALWAYS with
>>>>> PLATFORM_FILE_TRUNCATE.
>>>>>
>>>> If P_F_TRUNCATE is not mutually exclusive,  then
>>>> P_F_OPEN_ALWAYS + P_F_TRUNCATE  <-> P_F_CREATE_ALWAYS
>>>>
>>>
>>>
>>> good point!
>>>
>>>
>>>
>>>> I personally would like to keep the current semantics (mutually
>>>> exclusive), and add very clear comments to platform_file.h.
>>>>
>>>
>>>  OK, maybe we should also move P_F_TRUNCATE to be next to the others?
>>>  maybe it should be renamed P_F_TRUNCATE_EXISTING, or maybe
>>> P_F_OPEN_TRUNCATED would be better (so that it looks more parallel to
>>> P_F_OPEN and P_F_OPEN_ALWAYS)?
>>>
>> Thanks for the suggestion!
>> I will rename&move the flag. And because the enum value for each flag will
>> be changed, I will also take a look at whether that will affect anything.
>>
>
>
> Thanks!
>
>
>>
>>
>>>
>>> -darin
>>>
>>>
>>>
>>>>
>>>>> -Darin
>>>>>
>>>>>
>>>>>
>>>>> On Fri, May 20, 2011 at 3:11 PM, Darin Fisher <darin@chromium.org>wrote:
>>>>>
>>>>>> OK, makes sense.  Thanks!
>>>>>>
>>>>>>
>>>>>> On Fri, May 20, 2011 at 2:35 PM, Yuzhu Shen <yzshen@google.com>wrote:
>>>>>>
>>>>>>> One more note:
>>>>>>> Currently, PlatformFileFlags looks like Windows file open flags;
>>>>>>> while PP_FileOpenFlags_Dev looks like Posix file open flags.
>>>>>>> That is why the mapping
>>>>>>>   PP_FileOpenFlags -> PlatfromFileFlags -> Posix file open flags
>>>>>>> looks confusing.
>>>>>>>
>>>>>>> On Fri, May 20, 2011 at 2:29 PM, <yzshen@chromium.org> wrote:
>>>>>>>
>>>>>>>> Reviewers: darin,
>>>>>>>>
>>>>>>>> Message:
>>>>>>>> Hi, Darin.
>>>>>>>>
>>>>>>>> Thanks for your reply. Please see my comments.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc
>>>>>>>> File base/platform_file_posix.cc (right):
>>>>>>>>
>>>>>>>>
>>>>>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>>>>>>> base/platform_file_posix.cc:48: DCHECK(!open_flags);
>>>>>>>> On 2011/05/20 21:02:13, darin wrote:
>>>>>>>>
>>>>>>>>> so this means that it is not possible to specify
>>>>>>>>> PLATFORM_FILE_CREATE
>>>>>>>>>
>>>>>>>> and
>>>>>>>>
>>>>>>>>> PLATFORM_FILE_TRUNCATE at the same time.  but, POSIX allows you to
>>>>>>>>>
>>>>>>>> specify
>>>>>>>>
>>>>>>>>> O_CREAT and O_TRUNC together.  while it may seem redundant to
>>>>>>>>> indicate
>>>>>>>>>
>>>>>>>> that a
>>>>>>>>
>>>>>>>>> file should be truncated when created (since the file is 0 length
>>>>>>>>>
>>>>>>>> anyways), i
>>>>>>>>
>>>>>>>>> don't see the harm in doing so.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I think PlatformFileFlags are probably designed to look similar to
>>>>>>>> Win
>>>>>>>> flags.
>>>>>>>> As a result, the followings have an 1-1 mapping to Win flags:
>>>>>>>> PLATFORM_FILE_OPEN <-> OPEN_EXISTING
>>>>>>>> PLATFORM_FILE_CREATE <-> CREATE_NEW
>>>>>>>> PLATFORM_FILE_OPEN_ALWAYS <-> OPEN_ALWAYS
>>>>>>>> PLATFORM_FILE_CREATE_ALWAYS <-> CREATE_ALWAYS
>>>>>>>> PLATFORM_FILE_TRUNCATE <-> TRUNCATE_EXISTING
>>>>>>>> And platform_file_win enforces that these five PlatformFileFlags
>>>>>>>> cannot
>>>>>>>> be used together with each other.
>>>>>>>> It would be good to be consistent.
>>>>>>>>
>>>>>>>> The way to specify O_CREAT | O_TRUNC is PLATFORM_FILE_CREATE_ALWAYS.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
http://codereview.chromium.org/7038032/diff/1/base/platform_file_posix.cc#new...
>>>>>>>> base/platform_file_posix.cc:53: if (!open_flags && !(flags &
>>>>>>>> PLATFORM_FILE_OPEN) &&
>>>>>>>> On 2011/05/20 21:02:13, darin wrote:
>>>>>>>>
>>>>>>>>> it looks like you might be breaking this check, which was trying to
>>>>>>>>>
>>>>>>>> assert that
>>>>>>>>
>>>>>>>>> at least one of PLATFORM_FILE_CREATE, PLATFORM_FILE_CREATE_ALWAYS,
>>>>>>>>> PLATFORM_FILE_OPEN, or PLATFORM_FILE_OPEN_ALWAYS was specified.
>>>>>>>>>  with
>>>>>>>>>
>>>>>>>> your
>>>>>>>>
>>>>>>>>> change above, it is possible for PLATFORM_FILE_TRUNCATE to be
>>>>>>>>>
>>>>>>>> specified without
>>>>>>>>
>>>>>>>>> having specified any of those other 4 flags.  is that really what
>>>>>>>>> you
>>>>>>>>>
>>>>>>>> intended?
>>>>>>>>
>>>>>>>>  i always thought that truncate should be a modifier for opening an
>>>>>>>>>
>>>>>>>> existing
>>>>>>>>
>>>>>>>>> file.
>>>>>>>>>
>>>>>>>>
>>>>>>>> This is what I have described in the CL description:
>>>>>>>> CreatePlatformFile: resolve the conflict between the win and posix
>>>>>>>> implementation. Before this change, the win implementation didn't
>>>>>>>> allow
>>>>>>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>>>>>>> (OPEN|CREATE)(_ALWAYS)? flags; while the posix implementation
>>>>>>>> required
>>>>>>>> it to be used with them.
>>>>>>>>
>>>>>>>> I don't have a strong opinion about which way is better. But we have
>>>>>>>> to
>>>>>>>> keep the win and posix implementation consistent.
>>>>>>>>
>>>>>>>> Description:
>>>>>>>> Fix PP_FileOpenFlags_Dev handling:
>>>>>>>> - rewrite the mapping from PP_FileOpenFlags_Dev to
>>>>>>>> PlatformFileFlags.
>>>>>>>> - let ppb_flash_file_impl and ppb_file_io_impl use the same mapping
>>>>>>>> logic.
>>>>>>>> - CreatePlatformFile: resolve the conflict between the win and posix
>>>>>>>> implementation. Before this change, the win implementation didn't
>>>>>>>> allow
>>>>>>>> PLATFORM_FILE_TRUNCATE to be used with any of the
>>>>>>>> (OPEN|CREATE)(_ALWAYS)? flags;
>>>>>>>> while the posix implementation required it to be used with them.
>>>>>>>> - add more test cases to test the behavior of different
>>>>>>>> PP_FileOpenFlags_Dev
>>>>>>>> combinations.
>>>>>>>> - also unify the conversion from PlatformFileError to Pepper error.
>>>>>>>>
>>>>>>>> BUG=68489
>>>>>>>> TEST=New test cases in test_file_io.cc
>>>>>>>>
>>>>>>>>
>>>>>>>> Please review this at http://codereview.chromium.org/7038032/
>>>>>>>>
>>>>>>>> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>>>>>>>>
>>>>>>>> Affected files:
>>>>>>>>  M base/platform_file_posix.cc
>>>>>>>>  M ppapi/tests/test_file_io.h
>>>>>>>>  M ppapi/tests/test_file_io.cc
>>>>>>>>  M ppapi/tests/test_file_ref.cc
>>>>>>>>  M webkit/glue/webkit_glue.gypi
>>>>>>>>  D webkit/plugins/ppapi/error_util.h
>>>>>>>>  D webkit/plugins/ppapi/error_util.cc
>>>>>>>>  M webkit/plugins/ppapi/file_callbacks.cc
>>>>>>>>  A webkit/plugins/ppapi/file_type_conversion.h
>>>>>>>>  A webkit/plugins/ppapi/file_type_conversion.cc
>>>>>>>>  M webkit/plugins/ppapi/ppb_file_io_impl.cc
>>>>>>>>  M webkit/plugins/ppapi/ppb_flash_file_impl.cc
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Best regards,
>>>>>>> Yuzhu Shen.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Best regards,
>>>> Yuzhu Shen.
>>>>
>>>>
>>>
>>
>>
>> --
>> Best regards,
>> Yuzhu Shen.
>>
>>
>

[darin (slow to review), 2011-05-21 04:32:25]

Looks great.  Just some nits.

http://codereview.chromium.org/7038032/diff/2004/ppapi/tests/test_file_io.cc
File ppapi/tests/test_file_io.cc (right):

http://codereview.chromium.org/7038032/diff/2004/ppapi/tests/test_file_io.cc#...
ppapi/tests/test_file_io.cc:136: false,   // invalid_combination
instead of writing these comments, i think it would be better to define an enum
(bit mask).  that will make the code way more self-documenting.  you won't need
these comments!

http://codereview.chromium.org/7038032/diff/2004/webkit/plugins/ppapi/file_ty...
File webkit/plugins/ppapi/file_type_conversion.h (right):

http://codereview.chromium.org/7038032/diff/2004/webkit/plugins/ppapi/file_ty...
webkit/plugins/ppapi/file_type_conversion.h:6: #define
WEBKIT_PLUGINS_PPAPI_FILE_TYPE_CONVERSION_H_
nit: conversion -> conversions ... since this file contains an array of
conversion routines.

[yzshen1, 2011-05-23 00:53:01]

Hi, Darin.
Thanks for your suggestions! Please take another look.

http://codereview.chromium.org/7038032/diff/2004/ppapi/tests/test_file_io.cc
File ppapi/tests/test_file_io.cc (right):

http://codereview.chromium.org/7038032/diff/2004/ppapi/tests/test_file_io.cc#...
ppapi/tests/test_file_io.cc:136: false,   // invalid_combination
On 2011/05/21 04:32:25, darin wrote:
> instead of writing these comments, i think it would be better to define an
enum
> (bit mask).  that will make the code way more self-documenting.  you won't
need
> these comments!

Done.

http://codereview.chromium.org/7038032/diff/2004/webkit/plugins/ppapi/file_ty...
File webkit/plugins/ppapi/file_type_conversion.h (right):

http://codereview.chromium.org/7038032/diff/2004/webkit/plugins/ppapi/file_ty...
webkit/plugins/ppapi/file_type_conversion.h:6: #define
WEBKIT_PLUGINS_PPAPI_FILE_TYPE_CONVERSION_H_
On 2011/05/21 04:32:25, darin wrote:
> nit: conversion -> conversions ... since this file contains an array of
> conversion routines.

Done.

[darin (slow to review), 2011-05-23 18:17:42]

LGTM

http://codereview.chromium.org/7038032/diff/8001/base/platform_file.h
File base/platform_file.h (right):

http://codereview.chromium.org/7038032/diff/8001/base/platform_file.h#newcode32
base/platform_file.h:32: // exactly one of the five (possibly combining with
other flags) when openning
nit: openning -> opening

http://codereview.chromium.org/7038032/diff/8001/ppapi/tests/test_file_io.h
File ppapi/tests/test_file_io.h (right):

http://codereview.chromium.org/7038032/diff/8001/ppapi/tests/test_file_io.h#n...
ppapi/tests/test_file_io.h:27: NOT_CREATE_IF_NOT_EXIST = 1 << 1,
nit: this reads funny with the two NOTs... also, the use of NOT instead of
DONT/DOESNT hurts readability.... maybe this helps?

 CREATE_IF_DOESNT_EXIST
 DONT_CREATE_IF_DOESNT_EXIST
 ...

http://codereview.chromium.org/7038032/diff/8001/ppapi/tests/test_file_io.h#n...
ppapi/tests/test_file_io.h:32: END_OF_PAIR = NOT_TRUNCATE_IF_EXIST,
nit: i'm having trouble making sense of END_OF_PAIR.... END_OF_OPEN_EXPECTATIONS
might be better.

[yzshen1, 2011-05-23 19:48:06]

http://codereview.chromium.org/7038032/diff/8001/base/platform_file.h
File base/platform_file.h (right):

http://codereview.chromium.org/7038032/diff/8001/base/platform_file.h#newcode32
base/platform_file.h:32: // exactly one of the five (possibly combining with
other flags) when openning
On 2011/05/23 18:17:42, darin wrote:
> nit: openning -> opening

Done.

http://codereview.chromium.org/7038032/diff/8001/ppapi/tests/test_file_io.h
File ppapi/tests/test_file_io.h (right):

http://codereview.chromium.org/7038032/diff/8001/ppapi/tests/test_file_io.h#n...
ppapi/tests/test_file_io.h:27: NOT_CREATE_IF_NOT_EXIST = 1 << 1,
Done. Thanks! It looks much better this way. :)

On 2011/05/23 18:17:42, darin wrote:
> nit: this reads funny with the two NOTs... also, the use of NOT instead of
> DONT/DOESNT hurts readability.... maybe this helps?
> 
>  CREATE_IF_DOESNT_EXIST
>  DONT_CREATE_IF_DOESNT_EXIST
>  ...

http://codereview.chromium.org/7038032/diff/8001/ppapi/tests/test_file_io.h#n...
ppapi/tests/test_file_io.h:32: END_OF_PAIR = NOT_TRUNCATE_IF_EXIST,
INVALID_FLAG_COMBINATION is also one kind of open expectation, so changing
END_OF_PAIR to END_OF_OPEN_EXPECTATIONS might be confusing, too.

Since I would like to emphasize that "all values above are defined in pairs:
<some_expectation> and DONT_<some_expectation>", I changed it to
END_OF_OPEN_EXPECTATION_PAIRS. I have also added comments for it.

Thanks!

On 2011/05/23 18:17:42, darin wrote:
> nit: i'm having trouble making sense of END_OF_PAIR....
END_OF_OPEN_EXPECTATIONS
> might be better.

[darin (slow to review), 2011-05-23 21:20:39]

On Mon, May 23, 2011 at 12:48 PM, <yzshen@chromium.org> wrote:

>
> http://codereview.chromium.org/7038032/diff/8001/base/platform_file.h
> File base/platform_file.h (right):
>
>
>
http://codereview.chromium.org/7038032/diff/8001/base/platform_file.h#newcode32
> base/platform_file.h:32: // exactly one of the five (possibly combining
> with other flags) when openning
> On 2011/05/23 18:17:42, darin wrote:
>
>> nit: openning -> opening
>>
>
> Done.
>
>
> http://codereview.chromium.org/7038032/diff/8001/ppapi/tests/test_file_io.h
> File ppapi/tests/test_file_io.h (right):
>
>
>
http://codereview.chromium.org/7038032/diff/8001/ppapi/tests/test_file_io.h#n...
> ppapi/tests/test_file_io.h:27: NOT_CREATE_IF_NOT_EXIST = 1 << 1,
> Done. Thanks! It looks much better this way. :)
>
>
> On 2011/05/23 18:17:42, darin wrote:
>
>> nit: this reads funny with the two NOTs... also, the use of NOT
>>
> instead of
>
>> DONT/DOESNT hurts readability.... maybe this helps?
>>
>
>   CREATE_IF_DOESNT_EXIST
>>  DONT_CREATE_IF_DOESNT_EXIST
>>  ...
>>
>
>
>
http://codereview.chromium.org/7038032/diff/8001/ppapi/tests/test_file_io.h#n...
> ppapi/tests/test_file_io.h:32: END_OF_PAIR = NOT_TRUNCATE_IF_EXIST,
> INVALID_FLAG_COMBINATION is also one kind of open expectation, so
> changing END_OF_PAIR to END_OF_OPEN_EXPECTATIONS might be confusing,
> too.
>
> Since I would like to emphasize that "all values above are defined in
> pairs: <some_expectation> and DONT_<some_expectation>", I changed it to
> END_OF_OPEN_EXPECTATION_PAIRS. I have also added comments for it.
>
> Thanks!
>
>
Oh, I was confused about the intent of END_OF_PAIR.  This clears it up for
me,
and I agree that END_OF_OPEN_EXPECTATION_PAIRS sounds good.

-Darin


>
> On 2011/05/23 18:17:42, darin wrote:
>
>> nit: i'm having trouble making sense of END_OF_PAIR....
>>
> END_OF_OPEN_EXPECTATIONS
>
>> might be better.
>>
>
> http://codereview.chromium.org/7038032/
>

[darin (slow to review), 2011-05-23 21:23:00]

LGTM