Get nacl building again for android

Get nacl building again for android

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3032

Committed: https://crrev.com/996e4238b31f621fabe490f34b67f5a9edb5872a
Cr-Commit-Position: refs/heads/master@{#313945}

[sehr, 2015-01-21 20:42:30]

sehr@chromium.org changed reviewers:
+ bbudge@chromium.org, thestig@chromium.org

[sehr, 2015-01-21 21:01:59]

Once more unto the breach.  Fix a few small issues that block building NaCl in
Chromium on Android.

[bbudge, 2015-01-21 21:22:07]

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:200: bool
NaClBrowserDelegateImpl::MapUrlToLocalFilePath(
It seems like the body of this fn could also be excluded when extensions aren't
enabled.

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.h (right):

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.h:17: namespace extensions {
It seems strange that this isn't in the above #ifdef.

[Lei Zhang, 2015-01-21 21:32:22]

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:143: void
NaClBrowserDelegateImpl::SetDebugPatterns(std::string debug_patterns) {
BTW, would you mind making |debug_patterns| const ref while you are here?

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:200: bool
NaClBrowserDelegateImpl::MapUrlToLocalFilePath(
On 2015/01/21 21:22:07, bbudge wrote:
> It seems like the body of this fn could also be excluded when extensions
aren't
> enabled.

Looks like it already is excluded.

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.h (right):

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.h:12: #include
"components/nacl/browser/nacl_browser_delegate.h"
nit: blank line to separate the #if from the above #includes.

https://codereview.chromium.org/831813004/diff/40001/chrome/common/pepper_per...
File chrome/common/pepper_permission_util.cc (right):

https://codereview.chromium.org/831813004/diff/40001/chrome/common/pepper_per...
chrome/common/pepper_permission_util.cc:13: #if defined(ENABLE_EXTENSIONS)
It looks like all the callers to IsExtensionOrSharedModuleWhitelisted() and
IsExtensionOrSharedModuleWhitelisted() are behind #if
defined(ENABLE_EXTENSIONS). How about we just don't build this file when
extensions are not enabled?

https://codereview.chromium.org/831813004/diff/40001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_uma_host.cc (right):

https://codereview.chromium.org/831813004/diff/40001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_host.cc:16: #if defined(ENABLE_EXTENSIONS)
nit: move these down to their own section.

https://codereview.chromium.org/831813004/diff/40001/components/nacl/browser/...
File components/nacl/browser/nacl_browser.cc (right):

https://codereview.chromium.org/831813004/diff/40001/components/nacl/browser/...
components/nacl/browser/nacl_browser.cc:151: #ifdef OS_ANDROID
nit: #if defined(OS_ANDROID)

[sehr, 2015-01-22 19:57:22]

Thanks for the reviews.  PTAL.

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:143: void
NaClBrowserDelegateImpl::SetDebugPatterns(std::string debug_patterns) {
On 2015/01/21 21:32:22, Lei Zhang wrote:
> BTW, would you mind making |debug_patterns| const ref while you are here?

Done.

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:200: bool
NaClBrowserDelegateImpl::MapUrlToLocalFilePath(
On 2015/01/21 21:22:07, bbudge wrote:
> It seems like the body of this fn could also be excluded when extensions
aren't
> enabled.

It is already excluded, I think.

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:200: bool
NaClBrowserDelegateImpl::MapUrlToLocalFilePath(
On 2015/01/21 21:32:22, Lei Zhang wrote:
> On 2015/01/21 21:22:07, bbudge wrote:
> > It seems like the body of this fn could also be excluded when extensions
> aren't
> > enabled.
> 
> Looks like it already is excluded.

Acknowledged.

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.h (right):

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.h:12: #include
"components/nacl/browser/nacl_browser_delegate.h"
On 2015/01/21 21:32:22, Lei Zhang wrote:
> nit: blank line to separate the #if from the above #includes.

Done.

https://codereview.chromium.org/831813004/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.h:17: namespace extensions {
On 2015/01/21 21:22:07, bbudge wrote:
> It seems strange that this isn't in the above #ifdef.

Merged into the ifdef.

https://codereview.chromium.org/831813004/diff/40001/chrome/common/pepper_per...
File chrome/common/pepper_permission_util.cc (right):

https://codereview.chromium.org/831813004/diff/40001/chrome/common/pepper_per...
chrome/common/pepper_permission_util.cc:13: #if defined(ENABLE_EXTENSIONS)
On 2015/01/21 21:32:22, Lei Zhang wrote:
> It looks like all the callers to IsExtensionOrSharedModuleWhitelisted() and
> IsExtensionOrSharedModuleWhitelisted() are behind #if
> defined(ENABLE_EXTENSIONS). How about we just don't build this file when
> extensions are not enabled?

Great suggestion.  Done.

https://codereview.chromium.org/831813004/diff/40001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_uma_host.cc (right):

https://codereview.chromium.org/831813004/diff/40001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_host.cc:16: #if defined(ENABLE_EXTENSIONS)
On 2015/01/21 21:32:22, Lei Zhang wrote:
> nit: move these down to their own section.

Done.

https://codereview.chromium.org/831813004/diff/40001/components/nacl/browser/...
File components/nacl/browser/nacl_browser.cc (right):

https://codereview.chromium.org/831813004/diff/40001/components/nacl/browser/...
components/nacl/browser/nacl_browser.cc:151: #ifdef OS_ANDROID
On 2015/01/21 21:32:22, Lei Zhang wrote:
> nit: #if defined(OS_ANDROID)

Done.

[Lei Zhang, 2015-01-22 22:00:47]

looking good...

https://codereview.chromium.org/831813004/diff/60001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/831813004/diff/60001/chrome/chrome_common.gyp...
chrome/chrome_common.gypi:414: ['enable_plugins==1 and enable_extensions==1', {
You probably need to make this change in chrome/common/BUILD.gn as well.

[sehr, 2015-01-23 00:05:04]

Thanks for catching that (sorry).  Trybots are trying...  PTAL.

https://codereview.chromium.org/831813004/diff/60001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/831813004/diff/60001/chrome/chrome_common.gyp...
chrome/chrome_common.gypi:414: ['enable_plugins==1 and enable_extensions==1', {
On 2015/01/22 22:00:47, Lei Zhang wrote:
> You probably need to make this change in chrome/common/BUILD.gn as well.

Done.

[Lei Zhang, 2015-01-23 00:06:32]

lgtm

[bbudge, 2015-01-23 00:28:28]

LGTM 1 comment

https://codereview.chromium.org/831813004/diff/80001/components/nacl/browser/...
File components/nacl/browser/nacl_browser.cc (right):

https://codereview.chromium.org/831813004/diff/80001/components/nacl/browser/...
components/nacl/browser/nacl_browser.cc:153: #endif
It might be a little nicer if we set this to false in the initializer, and then
reverse this #if sense and CheckEnvVar in the body. Up to you though.

[sehr, 2015-01-28 00:27:36]

sehr@chromium.org changed reviewers:
+ yoz@chromium.org

[sehr, 2015-01-30 01:24:00]

Addressed Bill's comments.  PTAL.  Yoyo, I think I need an LGTM from you also,
per presubmit scripts.

https://codereview.chromium.org/831813004/diff/80001/components/nacl/browser/...
File components/nacl/browser/nacl_browser.cc (right):

https://codereview.chromium.org/831813004/diff/80001/components/nacl/browser/...
components/nacl/browser/nacl_browser.cc:153: #endif
On 2015/01/23 00:28:28, bbudge wrote:
> It might be a little nicer if we set this to false in the initializer, and
then
> reverse this #if sense and CheckEnvVar in the body. Up to you though.

Done.

[Yoyo Zhou, 2015-01-30 01:31:17]

LGTM for extensions/

[sehr, 2015-01-30 17:30:02]

The CQ bit was checked by sehr@chromium.org

[commit-bot: I haz the power, 2015-01-30 17:31:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/831813004/160001

[commit-bot: I haz the power, 2015-01-30 18:16:14]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-30 18:17:46]

Patchset 9 (id:??) landed as
https://crrev.com/996e4238b31f621fabe490f34b67f5a9edb5872a
Cr-Commit-Position: refs/heads/master@{#313945}