Cleanup: Get rid of OS_WIN/OS_POSIX ifdefs from MojoApplication

Cleanup: Get rid of OS_WIN/OS_POSIX ifdefs from MojoApplication

This CL gets rid of some OS_WIN/OS_POSIX ifdefs by using
base::PlatformFile type. It also simplifies ChannelMojo as well.

R=darin@chromium.org, viettrunluu@chromium.org
TBR=thakis@chomium.org
TEST=none
BUG=none

[Hajime Morrita, 2014-09-11 20:43:53]

morrita@chromium.org changed reviewers:
+ viettrungluu@chromium.org
- viettrunluu@chromium.org

[Hajime Morrita, 2014-09-11 20:44:00]

PTAL? This small cleanup is a preparation of my ChannelMojo for windows work
https://codereview.chromium.org/553283002/

[viettrungluu, 2014-09-12 00:12:45]

https://codereview.chromium.org/555503007/diff/1/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/555503007/diff/1/base/files/file.h#newcode34
base/files/file.h:34: #if defined(OS_BSD) || defined(OS_MACOSX) ||
defined(OS_NACL)
This stuff was all inside the defined(OS_POSIX) block.

https://codereview.chromium.org/555503007/diff/1/base/files/file.h#newcode37
base/files/file.h:37: typedef struct stat64 stat_wrapper_t;
In particular, you shouldn't define this on Windows.

https://codereview.chromium.org/555503007/diff/1/base/files/platform_file.h
File base/files/platform_file.h (right):

https://codereview.chromium.org/555503007/diff/1/base/files/platform_file.h#n...
base/files/platform_file.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
nit: Arguably, you should update this to 2014 (and no "(c)").

https://codereview.chromium.org/555503007/diff/1/base/files/platform_file.h#n...
base/files/platform_file.h:18: inline PlatformFile InvalidPlatformFile() {
Why not just use a const?

[Hajime Morrita, 2014-09-12 00:39:01]

Thanks for the review! PTAL?

https://codereview.chromium.org/555503007/diff/1/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/555503007/diff/1/base/files/file.h#newcode34
base/files/file.h:34: #if defined(OS_BSD) || defined(OS_MACOSX) ||
defined(OS_NACL)
On 2014/09/12 00:12:45, viettrungluu wrote:
> This stuff was all inside the defined(OS_POSIX) block.

Acknowledged.

https://codereview.chromium.org/555503007/diff/1/base/files/file.h#newcode37
base/files/file.h:37: typedef struct stat64 stat_wrapper_t;
On 2014/09/12 00:12:45, viettrungluu wrote:
> In particular, you shouldn't define this on Windows.

Done.

https://codereview.chromium.org/555503007/diff/1/base/files/platform_file.h
File base/files/platform_file.h (right):

https://codereview.chromium.org/555503007/diff/1/base/files/platform_file.h#n...
base/files/platform_file.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2014/09/12 00:12:45, viettrungluu wrote:
> nit: Arguably, you should update this to 2014 (and no "(c)").

Aww. Done.

https://codereview.chromium.org/555503007/diff/1/base/files/platform_file.h#n...
base/files/platform_file.h:18: inline PlatformFile InvalidPlatformFile() {
On 2014/09/12 00:12:45, viettrungluu wrote:
> Why not just use a const?

Done.

[viettrungluu, 2014-09-12 16:39:07]

lgtm w/nit, but I'm not an owner of most of these directories

https://codereview.chromium.org/555503007/diff/20001/ipc/ipc_channel_handle.h
File ipc/ipc_channel_handle.h (right):

https://codereview.chromium.org/555503007/diff/20001/ipc/ipc_channel_handle.h...
ipc/ipc_channel_handle.h:11: #include "base/files/platform_file.h"
nit: include order wrong

[Hajime Morrita, 2014-09-12 19:45:31]

Thanks for the review Trung! I updated the CL.
Darin, could you rubberstamp this?
This is almost for MojoApplication/MojoApplicationHost.

[Hajime Morrita, 2014-09-15 17:31:27]

morrita@chromium.org changed reviewers:
+ jam@chromium.org

[Hajime Morrita, 2014-09-15 17:35:13]

morrita@chromium.org changed reviewers:
+ brettw@chromium.org

[Hajime Morrita, 2014-09-15 17:35:14]

John: Could you take at ipc/ and content/ ? change on content/ just follows the
ipc/ change.

Brett: Could you see base/ change? It just extracted PlatformFile from file.h.

[jam, 2014-09-15 22:40:12]

lgtm

[Hajime Morrita, 2014-09-15 23:54:29]

morrita@chromium.org changed reviewers:
+ thakis@chomium.org
- brettw@chromium.org

[Hajime Morrita, 2014-09-15 23:54:29]

Nico: Could you rubberstamp base/ change?
It just extracts platform_file.h.

[Hajime Morrita, 2014-09-16 01:52:50]

I learned that this type of change is OK to use TBR.

[Hajime Morrita, 2014-09-16 01:52:54]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-09-16 02:19:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/555503007/40001

[commit-bot: I haz the power, 2014-09-16 02:41:49]

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

[commit-bot: I haz the power, 2014-09-16 02:41:50]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[brettw, 2014-09-16 16:49:18]

brettw@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2014-09-16 16:49:19]

File additions to base definitely aren't OK to TBR

[Hajime Morrita, 2014-09-16 16:57:21]

On 2014/09/16 16:49:19, brettw wrote:
> File additions to base definitely aren't OK to TBR
I'm sorry about that. Apparently I should learn more about Chromium way before
start using TBR.
Could you take a look?

[brettw, 2014-09-16 17:22:02]

brettw@chromium.org changed reviewers:
+ rvargas@chromium.org

[brettw, 2014-09-16 17:22:03]

Ricardo did most of the base file moving around. I'd want to check that he's OK
with this before committing since I'm not sure what other changes he's planning.
Owners LGTM

https://codereview.chromium.org/555503007/diff/40001/base/files/platform_file.h
File base/files/platform_file.h (right):

https://codereview.chromium.org/555503007/diff/40001/base/files/platform_file...
base/files/platform_file.h:8: #include "build/build_config.h"
Blank line after this.

https://codereview.chromium.org/555503007/diff/40001/base/files/platform_file...
base/files/platform_file.h:15: #if defined(OS_WIN)
I would personally find it more clear to but blank lines around the
#preprocessor directives.

[Hajime Morrita, 2014-09-16 17:50:24]

Thanks for the review!
I'll wait for Ricardo's comment.

https://codereview.chromium.org/555503007/diff/40001/base/files/platform_file.h
File base/files/platform_file.h (right):

https://codereview.chromium.org/555503007/diff/40001/base/files/platform_file...
base/files/platform_file.h:8: #include "build/build_config.h"
On 2014/09/16 17:22:03, brettw wrote:
> Blank line after this.

Done.

https://codereview.chromium.org/555503007/diff/40001/base/files/platform_file...
base/files/platform_file.h:15: #if defined(OS_WIN)
On 2014/09/16 17:22:03, brettw wrote:
> I would personally find it more clear to but blank lines around the
> #preprocessor directives.

Done.

[rvargas (doing something else), 2014-09-17 04:51:46]

https://codereview.chromium.org/555503007/diff/60001/base/files/platform_file.h
File base/files/platform_file.h (right):

https://codereview.chromium.org/555503007/diff/60001/base/files/platform_file...
base/files/platform_file.h:19: const PlatformFile kInvalidPlatformFile =
INVALID_HANDLE_VALUE;
I'm not really happy reviving PlatformFile and kInvalidPlatformFile as first
class citizens of base.

The main reason behind moving PlatformFile behind base::File was to make it
extremely obvious that File is what should be used in almost all cases and
HANDLE/fd should be avoided... to the point that code that really wants to use
raw handles has to redefine kInvalidPlatformFile (because really, there should
be an object somewhere with an IsValid method to check)

In fact I was planning removing PlatformFile from the definition of
PlatforFileForTransit and make users pass a const File& when they want to share
a file with another process. The only reason I haven't is that IPC is being
replaced by mojo ipc.

Improper sharing of handles (that ends up in double close) is still very high on
the reasons for Chrome crashing, and it is almost impossible to debug.

[Hajime Morrita, 2014-09-17 17:02:16]

Hi Ricard, thanks for the review and shharing your thought!

On 2014/09/17 04:51:46, rvargas wrote:
>
https://codereview.chromium.org/555503007/diff/60001/base/files/platform_file.h
> File base/files/platform_file.h (right):
> 
>
https://codereview.chromium.org/555503007/diff/60001/base/files/platform_file...
> base/files/platform_file.h:19: const PlatformFile kInvalidPlatformFile =
> INVALID_HANDLE_VALUE;
> I'm not really happy reviving PlatformFile and kInvalidPlatformFile as first
> class citizens of base.
> 
> The main reason behind moving PlatformFile behind base::File was to make it
> extremely obvious that File is what should be used in almost all cases and
> HANDLE/fd should be avoided... to the point that code that really wants to use
> raw handles has to redefine kInvalidPlatformFile (because really, there should
> be an object somewhere with an IsValid method to check)
> 

This makes sense.

> In fact I was planning removing PlatformFile from the definition of
> PlatforFileForTransit and make users pass a const File& when they want to
share
> a file with another process. The only reason I haven't is that IPC is being
> replaced by mojo ipc.

An irony here is that this patch is a part of Mojo-IPC work...
As Mojo-in-Chrome uses IPC::Channel for bootstrapping its pipe,
we won't be able t o kill it anytime soon.

Probably I should kill PlatformFile in PlatformFileInTransit as you mentioned.
Let me try.