Additional file descriptor mappings for Android

Additional file descriptor mappings.

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

[acleung, 2012-08-24 21:22:39]

The git upload script suggested your name. Feel free to redirect to anyone who
you think is appropriated.

Thanks.

[Ben Goodger (Google), 2012-08-27 15:46:07]

Please use svn blame to find a more appropriate reviewer. Thanks.

[acleung, 2012-08-29 17:27:59]

On 2012/08/27 15:46:07, Ben Goodger (Google) wrote:
> Please use svn blame to find a more appropriate reviewer. Thanks.

@cpu @agl, would you mind taking a look?

[cpu_(ooo_6.6-7.5), 2012-08-30 00:59:03]

I am not qualified to review unix-like changes. I barely know what an fd means.
Consider asking jcampan@ who besides agl can review this.

[agl, 2012-08-30 01:31:47]

http://codereview.chromium.org/10876056/diff/2001/chrome/app/chrome_main_dele...
File chrome/app/chrome_main_delegate.cc (right):

http://codereview.chromium.org/10876056/diff/2001/chrome/app/chrome_main_dele...
chrome/app/chrome_main_delegate.cc:599: // The renderer sandboxing make it that
we can't access any files even that
// The renderer sandbox prevents us from accessing our .pak files directly.
Therefore file descriptors to the .pak files that we need as passed in at
process creation time.

http://codereview.chromium.org/10876056/diff/2001/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10876056/diff/2001/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:1700: if (f ==
base::kInvalidPlatformFileValue) {
This looks like a DCHECK. Also, if this fails then I wouldn't be surprised that
something else in the path CHECKS because of the -1 file descriptor. Consider
whether this is a CHECK.

http://codereview.chromium.org/10876056/diff/2001/content/public/common/descr...
File content/public/common/descriptors.h (right):

http://codereview.chromium.org/10876056/diff/2001/content/public/common/descr...
content/public/common/descriptors.h:12: const int kDescriptorStart =
kPrimaryIPCChannel + 100;
This is a rather generic, unnamespaced name.

[acleung, 2012-08-31 23:36:33]

Thanks! I have address the comments downstream. Could you please take another
look?

https://chromiumcodereview.appspot.com/10876056/diff/2001/chrome/app/chrome_m...
File chrome/app/chrome_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/10876056/diff/2001/chrome/app/chrome_m...
chrome/app/chrome_main_delegate.cc:599: // The renderer sandboxing make it that
we can't access any files even that
On 2012/08/30 01:31:47, agl wrote:
> // The renderer sandbox prevents us from accessing our .pak files directly.
> Therefore file descriptors to the .pak files that we need as passed in at
> process creation time.

Done.

https://chromiumcodereview.appspot.com/10876056/diff/2001/chrome/browser/chro...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10876056/diff/2001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:1700: if (f ==
base::kInvalidPlatformFileValue) {
On 2012/08/30 01:31:47, agl wrote:
> This looks like a DCHECK. Also, if this fails then I wouldn't be surprised
that
> something else in the path CHECKS because of the -1 file descriptor. Consider
> whether this is a CHECK.

Done.

https://chromiumcodereview.appspot.com/10876056/diff/2001/content/public/comm...
File content/public/common/descriptors.h (right):

https://chromiumcodereview.appspot.com/10876056/diff/2001/content/public/comm...
content/public/common/descriptors.h:12: const int kDescriptorStart =
kPrimaryIPCChannel + 100;
On 2012/08/30 01:31:47, agl wrote:
> This is a rather generic, unnamespaced name.

Done.

[agl, 2012-08-31 23:43:00]

lgtm

[commit-bot: I haz the power, 2012-09-05 00:25:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@google.com/10876056/18001

[commit-bot: I haz the power, 2012-09-05 00:25:42]

Presubmit check for 10876056-18001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    content/public

Presubmit checks took 1.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[acleung, 2012-09-05 00:34:32]

@jam / @brettw

I need some more LGTM to check in. Could you two have a look?

[jam, 2012-09-05 01:31:35]

https://codereview.chromium.org/10876056/diff/18001/content/public/common/des...
File content/public/common/descriptors.h (right):

https://codereview.chromium.org/10876056/diff/18001/content/public/common/des...
content/public/common/descriptors.h:13: const int kDescriptorStart =
kPrimaryIPCChannel + 100;
don't add a new file to the content api, instead reuse
content\public\common\content_descriptors.h and add a 
kContentIPCDescriptorMax or something similar

[acleung, 2012-09-19 23:06:27]

John.

Picked out all the down stream changes in the other review.

Can I haz approval?

[acleung, 2012-10-02 05:50:06]

Updated from conflicts introduced by http://codereview.chromium.org/10876056

On 2012/09/19 23:06:27, acleung wrote:
> John.
> 
> Picked out all the down stream changes in the other review.
> 
> Can I haz approval?

[jam, 2012-10-03 16:04:05]

https://codereview.chromium.org/10876056/diff/29001/chrome/common/descriptors...
File chrome/common/descriptors_android.h (right):

https://codereview.chromium.org/10876056/diff/29001/chrome/common/descriptors...
chrome/common/descriptors_android.h:12: // be incremented based on its ordinal.
ditto

https://codereview.chromium.org/10876056/diff/29001/chrome/common/descriptors...
chrome/common/descriptors_android.h:13: kAndroidChromePakDescriptor =
kContentIPCDescriptorMax + 1,
you put the android specific values in an ifdef in src\content, why is this not
matching?

https://codereview.chromium.org/10876056/diff/29001/content/public/common/con...
File content/public/common/content_descriptors.h (right):

https://codereview.chromium.org/10876056/diff/29001/content/public/common/con...
content/public/common/content_descriptors.h:13: // Sets the initial descriptor
value. Subsequence descriptors will
this comment is just describing the code, so not needed

[acleung, 2012-10-03 18:29:39]

@john

Please have another look.

https://codereview.chromium.org/10876056/diff/29001/chrome/common/descriptors...
File chrome/common/descriptors_android.h (right):

https://codereview.chromium.org/10876056/diff/29001/chrome/common/descriptors...
chrome/common/descriptors_android.h:12: // be incremented based on its ordinal.
On 2012/10/03 16:04:06, John Abd-El-Malek wrote:
> ditto

Done.

https://codereview.chromium.org/10876056/diff/29001/chrome/common/descriptors...
chrome/common/descriptors_android.h:13: kAndroidChromePakDescriptor =
kContentIPCDescriptorMax + 1,
On 2012/10/03 16:04:06, John Abd-El-Malek wrote:
> you put the android specific values in an ifdef in src\content, why is this
not
> matching?

Done.

https://codereview.chromium.org/10876056/diff/29001/content/public/common/con...
File content/public/common/content_descriptors.h (right):

https://codereview.chromium.org/10876056/diff/29001/content/public/common/con...
content/public/common/content_descriptors.h:13: // Sets the initial descriptor
value. Subsequence descriptors will
On 2012/10/03 16:04:06, John Abd-El-Malek wrote:
> this comment is just describing the code, so not needed

Done.

[jam, 2012-10-03 20:57:41]

lgtm

https://codereview.chromium.org/10876056/diff/36001/chrome/common/descriptors...
File chrome/common/descriptors_android.h (right):

https://codereview.chromium.org/10876056/diff/36001/chrome/common/descriptors...
chrome/common/descriptors_android.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: list this file in chrome_common.gypi so it can be found by IDEs

[acleung, 2012-10-03 21:45:41]

On 2012/10/03 20:57:41, John Abd-El-Malek wrote:
> lgtm
> 
>
https://codereview.chromium.org/10876056/diff/36001/chrome/common/descriptors...
> File chrome/common/descriptors_android.h (right):
> 
>
https://codereview.chromium.org/10876056/diff/36001/chrome/common/descriptors...
> chrome/common/descriptors_android.h:1: // Copyright (c) 2012 The Chromium
> Authors. All rights reserved.
> nit: list this file in chrome_common.gypi so it can be found by IDEs

Done

[commit-bot: I haz the power, 2012-10-03 21:46:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@google.com/10876056/39001

[commit-bot: I haz the power, 2012-10-03 23:57:41]

Change committed as 160013