Remove async load concept for ToS from native code.

Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

To integrate ToS related code into one place, for the maintainability, this CL introduces
a class, TermsOfServicePage, which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page" to ArcSupportHost,
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
BUG=b/32424636
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/bbb459c611c30ecad20b6b2912df9489c395871f
Cr-Commit-Position: refs/heads/master@{#428375}

[hidehiko, 2016-10-25 14:18:22]

Description was changed from

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

For better management of the code structure, this CL introduces
a class TermsOfServicePage which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page",
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
TEST=Ran on test device manually. Ran bots.
==========

to

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

For better management of the code structure, this CL introduces
a class TermsOfServicePage which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page",
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hidehiko, 2016-10-25 14:22:10]

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

[commit-bot: I haz the power, 2016-10-25 14:22:31]

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

[hidehiko, 2016-10-25 14:27:02]

Description was changed from

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

For better management of the code structure, this CL introduces
a class TermsOfServicePage which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page",
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

To integrate ToS related code into one place, for the better maintainability,
this CL introduces
a class, TermsOfServicePage, which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page" to ArcSupportHost,
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-25 15:15:34]

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

[commit-bot: I haz the power, 2016-10-25 15:15:35]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-10-25 15:50:49]

Description was changed from

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

To integrate ToS related code into one place, for the better maintainability,
this CL introduces
a class, TermsOfServicePage, which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page" to ArcSupportHost,
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

To integrate ToS related code into one place, for the maintainability, this CL
introduces
a class, TermsOfServicePage, which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page" to ArcSupportHost,
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hidehiko, 2016-10-25 15:50:49]

hidehiko@chromium.org changed reviewers:
+ lhchavez@chromium.org, xiyuan@chromium.org

[hidehiko, 2016-10-25 15:52:14]

PTAL: Luis, Xiyuan.
FYI: Yury, but I'd appreciate if you reply to my inline comment.

https://codereview.chromium.org/2450823002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/2450823002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:317:
setTimeout(function() {
khmel@, this is introduced in your CL. Could you tell me why setTimeout is
needed? It's better to keep the comment here, I think.


Note: here arrow function cannot be used, just because git cl presubmit fails to
parse it, unfortunately.

[khmel, 2016-10-25 15:59:11]

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

[khmel, 2016-10-25 15:59:12]

https://codereview.chromium.org/2450823002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/2450823002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:317:
setTimeout(function() {
On 2016/10/25 15:52:14, hidehiko wrote:
> khmel@, this is introduced in your CL. Could you tell me why setTimeout is
> needed? It's better to keep the comment here, I think.
> 
> 
> Note: here arrow function cannot be used, just because git cl presubmit fails
to
> parse it, unfortunately.

setTimeout is used to async update webview style to prevent webview animation
glitch and wrong layout caused by changing whole page layout (for example caused
by different UMA text). It is cannot be calculated correctly
(window.getComputedStyle) if applied inline.

[khmel, 2016-10-25 16:16:12]

Please make sure that you handle all cases for loading ToS. There is
onBeforeRequest discarded which showed 'terms-loading', it seems you break
current UX feature when we show loading progress in case ToS is reloaded inline
(you may reload ToS by choosing different language at the bottom of ToS).

[hidehiko, 2016-10-26 11:50:15]

On 2016/10/25 16:16:12, khmel wrote:
> Please make sure that you handle all cases for loading ToS. There is
> onBeforeRequest discarded which showed 'terms-loading', it seems you break
> current UX feature when we show loading progress in case ToS is reloaded
inline
> (you may reload ToS by choosing different language at the bottom of ToS).

Oh, good catch.

Though, it's difficult for me to guess the expected behavior from the current
behavior.
I filed a bug for that (CC'ed you), and let's hear the PM's opinion.

[xiyuan, 2016-10-26 20:27:30]

lgtm

https://codereview.chromium.org/2450823002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/2450823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:226: * @param
{boolean} isManaged Set true if ARC is under managed.
nit: "is under managed" -> "is managed" or "is under management"

https://codereview.chromium.org/2450823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:740:
termsPage.startTermsViewLoading();
nit: This call seems redundant since showPage('terms') calls termsPage.onShow()
which would call startTermsViewLoading() when necessary.

[khmel, 2016-10-27 00:49:36]

lgtm

[hidehiko, 2016-10-27 07:52:47]

Description was changed from

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

To integrate ToS related code into one place, for the maintainability, this CL
introduces
a class, TermsOfServicePage, which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page" to ArcSupportHost,
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

To integrate ToS related code into one place, for the maintainability, this CL
introduces
a class, TermsOfServicePage, which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page" to ArcSupportHost,
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
BUG=b/32424636
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hidehiko, 2016-10-27 07:55:51]

Thank you for review.

As for loading progress bar on language switching, fixed the behavior to the
khmel@'s original intention based on the discussion of the bug thread.

PTAL, Luis?

https://codereview.chromium.org/2450823002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/2450823002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:317:
setTimeout(function() {
On 2016/10/25 15:59:12, khmel wrote:
> On 2016/10/25 15:52:14, hidehiko wrote:
> > khmel@, this is introduced in your CL. Could you tell me why setTimeout is
> > needed? It's better to keep the comment here, I think.
> > 
> > 
> > Note: here arrow function cannot be used, just because git cl presubmit
fails
> to
> > parse it, unfortunately.
> 
> setTimeout is used to async update webview style to prevent webview animation
> glitch and wrong layout caused by changing whole page layout (for example
caused
> by different UMA text). It is cannot be calculated correctly
> (window.getComputedStyle) if applied inline.

Thanks! Commented.

https://codereview.chromium.org/2450823002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/2450823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:226: * @param
{boolean} isManaged Set true if ARC is under managed.
On 2016/10/26 20:27:30, xiyuan wrote:
> nit: "is under managed" -> "is managed" or "is under management"

Done.

https://codereview.chromium.org/2450823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:740:
termsPage.startTermsViewLoading();
On 2016/10/26 20:27:30, xiyuan wrote:
> nit: This call seems redundant since showPage('terms') calls
termsPage.onShow()
> which would call startTermsViewLoading() when necessary.

Oops. Done.

[hidehiko, 2016-10-27 08:08:30]

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

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

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-10-27 10:07:17]

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

[commit-bot: I haz the power, 2016-10-27 10:07:18]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-10-27 15:58:03]

sorry, i missed this review somehow :/

chrome/browser/chromeos/arc lgtm

https://codereview.chromium.org/2450823002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/2450823002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:317:
setTimeout(function() {
On 2016/10/27 07:55:51, hidehiko wrote:
> On 2016/10/25 15:59:12, khmel wrote:
> > On 2016/10/25 15:52:14, hidehiko wrote:
> > > khmel@, this is introduced in your CL. Could you tell me why setTimeout is
> > > needed? It's better to keep the comment here, I think.
> > > 
> > > 
> > > Note: here arrow function cannot be used, just because git cl presubmit
> fails
> > to
> > > parse it, unfortunately.
> > 
> > setTimeout is used to async update webview style to prevent webview
animation
> > glitch and wrong layout caused by changing whole page layout (for example
> caused
> > by different UMA text). It is cannot be calculated correctly
> > (window.getComputedStyle) if applied inline.
> 
> Thanks! Commented.

Re: git cl presubmit failing to parse something. That sounds like a bug in the
presubmit check. Can you see if there's already a bug for it, or open a new one?

[hidehiko, 2016-10-28 13:55:47]

Thank you for review. Submitting.

On 2016/10/27 15:58:03, Luis Héctor Chávez wrote:
> sorry, i missed this review somehow :/
> 
> chrome/browser/chromeos/arc lgtm
> 
>
https://codereview.chromium.org/2450823002/diff/1/chrome/browser/resources/ch...
> File chrome/browser/resources/chromeos/arc_support/background.js (right):
> 
>
https://codereview.chromium.org/2450823002/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/arc_support/background.js:317:
> setTimeout(function() {
> On 2016/10/27 07:55:51, hidehiko wrote:
> > On 2016/10/25 15:59:12, khmel wrote:
> > > On 2016/10/25 15:52:14, hidehiko wrote:
> > > > khmel@, this is introduced in your CL. Could you tell me why setTimeout
is
> > > > needed? It's better to keep the comment here, I think.
> > > > 
> > > > 
> > > > Note: here arrow function cannot be used, just because git cl presubmit
> > fails
> > > to
> > > > parse it, unfortunately.
> > > 
> > > setTimeout is used to async update webview style to prevent webview
> animation
> > > glitch and wrong layout caused by changing whole page layout (for example
> > caused
> > > by different UMA text). It is cannot be calculated correctly
> > > (window.getComputedStyle) if applied inline.
> > 
> > Thanks! Commented.
> 
> Re: git cl presubmit failing to parse something. That sounds like a bug in the
> presubmit check. Can you see if there's already a bug for it, or open a new
one?

Sure. I couldn't find the bug, so I'll file it.
Let me land this first, so that I can create small repro CL on top of this.

[hidehiko, 2016-10-28 13:55:56]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-10-28 13:55:57]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
khmel@chromium.org
Link to the patchset: https://codereview.chromium.org/2450823002/#ps60001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-10-28 13:56:12]

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-10-28 15:15:23]

Description was changed from

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

To integrate ToS related code into one place, for the maintainability, this CL
introduces
a class, TermsOfServicePage, which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page" to ArcSupportHost,
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
BUG=b/32424636
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

To integrate ToS related code into one place, for the maintainability, this CL
introduces
a class, TermsOfServicePage, which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page" to ArcSupportHost,
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
BUG=b/32424636
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-28 15:15:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-28 15:18:15]

Description was changed from

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

To integrate ToS related code into one place, for the maintainability, this CL
introduces
a class, TermsOfServicePage, which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page" to ArcSupportHost,
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
BUG=b/32424636
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Remove async load concept for ToS from native code.

This CL removes the async-load concept for ToS from native
ArcSupportHost / ArcAuthService, because it is unused.
Now, it is encapsulated in the arc_support App.

To integrate ToS related code into one place, for the maintainability, this CL
introduces
a class, TermsOfServicePage, which implements the ToS page
of the arc_support App.

After this CL, native can just say "Show ToS page" to ArcSupportHost,
so that arc_support app handles async loading.

This is the preparation to resolve the dependency between
ArcAuthService and ArcSupportHost.

BUG=657687
BUG=b/31079732
BUG=b/32424636
TEST=Ran on test device manually. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/bbb459c611c30ecad20b6b2912df9489c395871f
Cr-Commit-Position: refs/heads/master@{#428375}
==========

[commit-bot: I haz the power, 2016-10-28 15:18:16]

Patchset 4 (id:??) landed as
https://crrev.com/bbb459c611c30ecad20b6b2912df9489c395871f
Cr-Commit-Position: refs/heads/master@{#428375}