[Android] Fix icu_use_data_file = false.

[Android] Fix icu_use_data_file = false.

There are compiler errors while building Android WebView Shell with icu_use_data_file = false.

BUG=

Committed: https://crrev.com/0ca26a775cdf42c2a32815df10283b80352a6b36
Cr-Commit-Position: refs/heads/master@{#413686}

[Eden Wang, 2016-08-12 07:07:57]

Description was changed from

==========
[Android] Fix icu_use_data_file = false.

There are compiler errors while building Android WebView Shell with
icu_use_data_file = false.

BUG=
==========

to

==========
[Android] Fix icu_use_data_file = false.

There are compiler errors while building Android WebView Shell with
icu_use_data_file = false.

BUG=
==========

[Eden Wang, 2016-08-12 07:07:57]

nedenwang@tencent.com changed reviewers:
+ altimin@chromium.org, jamesxwei@tencent.com

[Eden Wang, 2016-08-12 07:10:42]

Hi altimin,
  I have got some compiler errors while building Android WebView Shell with
icu_use_data_file = false, base::i18n::InitializeICUWithFileDescriptor and
base::i18n::GetIcuDataFileHandle are defined if ICU_UTIL_DATA_IMPL ==
ICU_UTIL_DATA_FILE.

[altimin, 2016-08-12 10:35:38]

altimin@chromium.org changed reviewers:
+ avi@chromium.org, jshin@chromium.org

[altimin, 2016-08-12 10:35:38]

Thanks for the patch!

Non-owner lgtm.

+jshin@ for icu.
+avi@ for content/

[altimin, 2016-08-12 10:36:05]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 10:36:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 10:36:28]

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

[commit-bot: I haz the power, 2016-08-12 10:36:29]

Dry run: The author nedenwang@tencent.com has not signed Google Contributor
License Agreement. Please visit https://cla.developers.google.com to sign and
manage CLA.

[Avi (use Gerrit), 2016-08-12 15:47:35]

On 2016/08/12 10:35:38, altimin wrote:
> Thanks for the patch!
> 
> Non-owner lgtm.
> 
> +jshin@ for icu.
> +avi@ for content/

Eden—

My files don't show that you're covered by a CLA. Can you sign at
https://cla.developers.google.com/ ?

If you already have signed, I was looking with the email address
"nedenwang@tencent.com". What email address would it be under?

[Wei Xiaohai, 2016-08-12 17:06:04]

avi，I had signed the Corprate CLA on behalf of Tencent before, I forwarded the
stamped CLA to you in another email.

We already had two members listed in the  AUTHORS file(penghu@tencent.com
perryuwang@tencent.com).

james


---原始邮件---
发件人: "avi@chromium.org "<avi@chromium.org>
发送时间: 2016年8月12日 23:47:38
收件人:
"jshin@chromium.org"<jshin@chromium.org>;"jamesxwei(魏晓海)"<jamesxwei@tencent.c...;
抄送:
"jam@chromium.org"<jam@chromium.org>;"darin-cc@chromium.org"<darin-cc@chromiu...;
主题: Re: [Android] Fix icu_use_data_file = false. (issue 2241753002 by
nedenwang@tencent.com)(Internet mail)

On 2016/08/12 10:35:38, altimin wrote:
> Thanks for the patch!
>
> Non-owner lgtm.
>
> +jshin@ for icu.
> +avi@ for content/

Eden—

My files don't show that you're covered by a CLA. Can you sign at
https://cla.developers.google.com/ ?

If you already have signed, I was looking with the email address
"nedenwang@tencent.com"<mailto:"nedenwang@tencent.com">;. What email address
would it be under?

https://codereview.chromium.org/2241753002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2016-08-12 17:23:37]

Unfortunately not all reviewers are properly checking CLAs before allowing
changes to the AUTHORS file, so I can't use that as precedent.

This is indeed weird; I cannot find your corporate CLA either. I asked our
CLA people for help here and will let you know how it goes.

Thank you for your patience.

Avi

On Fri, Aug 12, 2016 at 1:05 PM, jamesxwei(魏晓海) <jamesxwei@tencent.com>
wrote:

> avi，I had signed the Corprate CLA on behalf of Tencent
> before, I forwarded the stamped CLA to you in another email.
>
> We already had two members listed in the  AUTHORS file(penghu@tencent.com
> perryuwang@tencent.com).
>
> james
>
>
> ---原始邮件---
> *发件人:* "avi@chromium.org "<avi@chromium.org>
> *发送时间:* 2016年8月12日 23:47:38
> *收件人:* "jshin@chromium.org"<jshin@chromium.org>;"jamesxwei(魏晓海)"<
> jamesxwei@tencent.com>;"altimin@chromium.org"<altimin@chromium.org
> >;"nedenwang(王传东)"<nedenwang@tencent.com>;
> *抄送:* "jam@chromium.org"<jam@chromium.org>;"darin-cc@chromium.org"<
> darin-cc@chromium.org>;"chromium-reviews@chromium.org"<chromium
> -reviews@chromium.org>;
> *主题:* Re: [Android] Fix icu_use_data_file = false. (issue 2241753002 by
> nedenwang@tencent.com)(Internet mail)
>
> On 2016/08/12 10:35:38, altimin wrote:
> > Thanks for the patch!
> >
> > Non-owner lgtm.
> >
> > +jshin@ for icu.
> > +avi@ for content/
>
> Eden—
>
> My files don't show that you're covered by a CLA. Can you sign at
> https://cla.developers.google.com/ ?
>
> If you already have signed, I was looking with the email address
> "nedenwang@tencent.com";. What email address would it be under?
>
> https://codereview.chromium.org/2241753002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jungshik at Google, 2016-08-16 00:15:46]

Eden, 

The change looks good. Let's wait for the clarification with CLA. 

Thank you for your patience and contribution. 

------------
Thanks, Avi, for your due diligence ! :-)

[Eden Wang, 2016-08-19 02:15:36]

The CQ bit was checked by nedenwang@tencent.com

[commit-bot: I haz the power, 2016-08-19 02:16:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 02:16:03]

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

[commit-bot: I haz the power, 2016-08-19 02:16:04]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Eden Wang, 2016-08-19 04:14:37]

Hi avi,
  We got CLA and I submitted pathset 2 to fix unused variable 'g_fds' while
icu_use_data_file = false and v8_use_external_startup_data = false.
  The AUTHORS file has commited in another code review.

[jungshik at Google, 2016-08-19 23:10:48]

Thanks for the patience. 
I've confirmed that we could accept your contribution (CLA-wise).

https://codereview.chromium.org/2241753002/diff/20001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2241753002/diff/20001/content/app/content_mai...
content/app/content_main_runner.cc:474: ALLOW_UNUSED_LOCAL(g_fds);
I wonder if it'd be better to enclose |base::Glo...g_fds = ...| with 

#if !defined(OS_ANDROID) || (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE)

instead of ALLOW_UNUSED_LOCAL(g_fds).

[Eden Wang, 2016-08-20 00:41:42]

https://codereview.chromium.org/2241753002/diff/20001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2241753002/diff/20001/content/app/content_mai...
content/app/content_main_runner.cc:474: ALLOW_UNUSED_LOCAL(g_fds);
On 2016/08/19 23:10:48, jungshik at google wrote:
> I wonder if it'd be better to enclose |base::Glo...g_fds = ...| with 
> 
> #if !defined(OS_ANDROID) || (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE)
> 
> instead of ALLOW_UNUSED_LOCAL(g_fds). 

I think it would be a very long statement to enclose |base::Glo...g_fds = ...|,
because g_fds is used for

!defined(OS_ANDROID)
defined(OS_LINUX) || defined(OS_OPENBSD)
defined(OS_ANDROID) && (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE)
defined(V8_USE_EXTERNAL_STARTUP_DATA) && defined(OS_POSIX) &&
!defined(OS_MACOSX)

and this statement is not stable, we need add more conditions after any other fd
has been accessed.

[jungshik at Google, 2016-08-22 20:02:00]

https://codereview.chromium.org/2241753002/diff/20001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2241753002/diff/20001/content/app/content_mai...
content/app/content_main_runner.cc:474: ALLOW_UNUSED_LOCAL(g_fds);
On 2016/08/20 00:41:42, Eden Wang wrote:
> On 2016/08/19 23:10:48, jungshik at google wrote:
> > I wonder if it'd be better to enclose |base::Glo...g_fds = ...| with 
> > 
> > #if !defined(OS_ANDROID) || (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE)
> > 
> > instead of ALLOW_UNUSED_LOCAL(g_fds). 
> 
> I think it would be a very long statement to enclose |base::Glo...g_fds =
...|,
> because g_fds is used for
> 
> !defined(OS_ANDROID)
> defined(OS_LINUX) || defined(OS_OPENBSD)
> defined(OS_ANDROID) && (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE)
> defined(V8_USE_EXTERNAL_STARTUP_DATA) && defined(OS_POSIX) &&
> !defined(OS_MACOSX)
> 
> and this statement is not stable, we need add more conditions after any other
fd
> has been accessed.

I don't think you need that long condition. The short condition I gave 
seems sufficient because g_fds is used right below for setlocale when OS !=
Android. 
Having to change the condition (when that's not the case any more) is a
downside, though. 

Anyway, it's not a big deal. I'm ok with ALLOW..LOCALE.

[Avi (use Gerrit), 2016-08-22 20:05:12]

I see the CLA, LGTM.

Get Junshik's approal.

[jungshik at Google, 2016-08-22 20:30:18]

On 2016/08/22 20:02:00, jungshik at google wrote:
>
https://codereview.chromium.org/2241753002/diff/20001/content/app/content_mai...
> File content/app/content_main_runner.cc (right):
> 
>
https://codereview.chromium.org/2241753002/diff/20001/content/app/content_mai...
> content/app/content_main_runner.cc:474: ALLOW_UNUSED_LOCAL(g_fds);
> On 2016/08/20 00:41:42, Eden Wang wrote:
> > On 2016/08/19 23:10:48, jungshik at google wrote:
> > > I wonder if it'd be better to enclose |base::Glo...g_fds = ...| with 
> > > 
> > > #if !defined(OS_ANDROID) || (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE)
> > > 
> > > instead of ALLOW_UNUSED_LOCAL(g_fds). 
> > 
> > I think it would be a very long statement to enclose |base::Glo...g_fds =
> ...|,
> > because g_fds is used for
> > 
> > !defined(OS_ANDROID)
> > defined(OS_LINUX) || defined(OS_OPENBSD)
> > defined(OS_ANDROID) && (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE)
> > defined(V8_USE_EXTERNAL_STARTUP_DATA) && defined(OS_POSIX) &&
> > !defined(OS_MACOSX)
> > 
> > and this statement is not stable, we need add more conditions after any
other
> fd
> > has been accessed.
> 
> I don't think you need that long condition. The short condition I gave 
> seems sufficient because g_fds is used right below for setlocale when OS !=
> Android. 
> Having to change the condition (when that's not the case any more) is a
> downside, though. 
> 
> Anyway, it's not a big deal. I'm ok with ALLOW..LOCALE.

LGTM

[Eden Wang, 2016-08-22 20:44:52]

The CQ bit was checked by nedenwang@tencent.com

[Eden Wang, 2016-08-22 20:44:52]

The patchset sent to the CQ was uploaded after l-g-t-m from altimin@chromium.org
Link to the patchset: https://codereview.chromium.org/2241753002/#ps20001
(title: "fix unused variable 'g_fds'")

[commit-bot: I haz the power, 2016-08-22 20:45:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 23:06:03]

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

[commit-bot: I haz the power, 2016-08-22 23:06:04]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Eden Wang, 2016-08-23 04:03:42]

On 2016/08/22 23:06:04, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Hi Avi & jungshik,
   interactive_ui_tests was also broken on the main waterfall. Is this the
reason that win_chromium_rel_ng failed ?

[Avi (use Gerrit), 2016-08-23 05:08:00]

On 2016/08/23 04:03:42, Eden Wang wrote:
> On 2016/08/22 23:06:04, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> 
> Hi Avi & jungshik,
>    interactive_ui_tests was also broken on the main waterfall. Is this the
> reason that win_chromium_rel_ng failed ?

Yes; if a test is failing on the main waterfall it'll be broken on the trybots.
In a case like that feel free to commit again.

[Eden Wang, 2016-08-23 06:21:39]

The CQ bit was checked by nedenwang@tencent.com

[commit-bot: I haz the power, 2016-08-23 06:21:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 07:34:23]

Description was changed from

==========
[Android] Fix icu_use_data_file = false.

There are compiler errors while building Android WebView Shell with
icu_use_data_file = false.

BUG=
==========

to

==========
[Android] Fix icu_use_data_file = false.

There are compiler errors while building Android WebView Shell with
icu_use_data_file = false.

BUG=
==========

[commit-bot: I haz the power, 2016-08-23 07:34:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-23 07:36:27]

Description was changed from

==========
[Android] Fix icu_use_data_file = false.

There are compiler errors while building Android WebView Shell with
icu_use_data_file = false.

BUG=
==========

to

==========
[Android] Fix icu_use_data_file = false.

There are compiler errors while building Android WebView Shell with
icu_use_data_file = false.

BUG=

Committed: https://crrev.com/0ca26a775cdf42c2a32815df10283b80352a6b36
Cr-Commit-Position: refs/heads/master@{#413686}
==========

[commit-bot: I haz the power, 2016-08-23 07:36:28]

Patchset 2 (id:??) landed as
https://crrev.com/0ca26a775cdf42c2a32815df10283b80352a6b36
Cr-Commit-Position: refs/heads/master@{#413686}