arc: Provide progress to load TOS content for OptIn.

arc: Provide progress to load TOS content for OptIn.

This implements progress for loading TOS content for OptIn
workflow with error/retry support. Also fixes few UI bugs
in OptIn layout windows.

BUG=b/29419423
TEST=Manually on device. Emulate error conditions on different
     stages of OptIn workflow. OptIn is correctly recoverable
     in each case.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

[khmel, 2016-07-08 21:39:41]

Description was changed from

==========
arc: Provide progress to load TOS content for OptIn.

This implements progress for loading TOS content for OptIn
workflow with error/retry support. Also fixes few UI bugs
in OptIn layout windows.

BUG=b/29419423
TEST=Manually on device. Emulate error conditions on different
     stages of OptIn workflow. OptIn is correctly recoverable
     in each case.
==========

to

==========
arc: Provide progress to load TOS content for OptIn.

This implements progress for loading TOS content for OptIn
workflow with error/retry support. Also fixes few UI bugs
in OptIn layout windows.

BUG=b/29419423
TEST=Manually on device. Emulate error conditions on different
     stages of OptIn workflow. OptIn is correctly recoverable
     in each case.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[khmel, 2016-07-08 21:59:32]

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

[khmel, 2016-07-08 21:59:34]

Hi Xiyuan,

PTAL

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

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:110:
updateTermsHeight();
No longer needed here, we update height when activating 'terms' page.

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:300: // Apply
absolute dimension to webview tag in order to avoid UI glitch
You might remember that this was  done to avoid weird webview animation on LSO
load. Before we fixed it by setting fixed size as inner window bounds. Recently
image header was also added to this page and this calculation is no longer true.
As variant it is possible to set fixed width in css which fixes this as well.

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

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:178:
termsView.style.height = '0px';
For some reason getComputedStyle depends on webview height but based on html
layout (terms-container is marked as flex auto) should not. This causes
non-stable computation (each next call gives different result). As result,
layout is noticeable changed on next update and 'agree'/'cancel' buttons are
distorted. Having 0px height before computation stabilize layout.

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:318: var lsoError =
false;
Initially I was trying to call webview.stop() method on error in order to
prevent calling onLsoViewContentLoad. However in this case, webview is no longer
responsible afterword (when I try to assign new src, ERR_ABORTED is always
returned and webview.reload() does not help).

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:346: if
(results[0].length &&
Before error was not handled and results[0] generated error and stopping js
processing. This caused situation when technical info might be shown to user in
case of some error.

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/arc_support/main.css (right):

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/main.css:19: width: 960px;
Please see comments in .js

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/main.css:23: width: 468px;
the same as above.

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/main.css:62: padding: 8px 0 0 0;
Second checkbox section was added recently. However it broke layout
specification
(https://drive.google.com/a/google.com/folderview?id=0B6x6iYCtKinENXB6OTRtYlRP...).
This also includes changes in .section-terms below

[xiyuan, 2016-07-08 22:33:29]

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

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:110:
updateTermsHeight();
On 2016/07/08 21:59:33, khmel wrote:
> No longer needed here, we update height when activating 'terms' page.

Acknowledged.

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:300: // Apply
absolute dimension to webview tag in order to avoid UI glitch
On 2016/07/08 21:59:33, khmel wrote:
> You might remember that this was  done to avoid weird webview animation on LSO
> load. Before we fixed it by setting fixed size as inner window bounds.
Recently
> image header was also added to this page and this calculation is no longer
true.
> As variant it is possible to set fixed width in css which fixes this as well.

Acknowledged.

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

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:178:
termsView.style.height = '0px';
On 2016/07/08 21:59:33, khmel wrote:
> For some reason getComputedStyle depends on webview height but based on html
> layout (terms-container is marked as flex auto) should not. This causes
> non-stable computation (each next call gives different result). As result,
> layout is noticeable changed on next update and 'agree'/'cancel' buttons are
> distorted. Having 0px height before computation stabilize layout.

Strange since we have "height: 100%" on webview tag [1] and it should auto
resized with its parent container. Maybe adding "position: relative" to
#terms-container would get rid of this manual setting height code.

I probably have proposed this before. But anyway, in case it does not work, I am
fine with this work around. Just add a comment to doc why we are doing this.

[1]:
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/arc_su...

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:318: var lsoError =
false;
On 2016/07/08 21:59:33, khmel wrote:
> Initially I was trying to call webview.stop() method on error in order to
> prevent calling onLsoViewContentLoad. However in this case, webview is no
longer
> responsible afterword (when I try to assign new src, ERR_ABORTED is always
> returned and webview.reload() does not help).

Acknowledged.

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:346: if
(results[0].length &&
On 2016/07/08 21:59:33, khmel wrote:
> Before error was not handled and results[0] generated error and stopping js
> processing. This caused situation when technical info might be shown to user
in
> case of some error.

Should this be 
  results && results.length == 1 && typeof results[0] == 'string'
?

Otherwise, we still hits error when results is null, [] or non string in array.

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/arc_support/main.css (right):

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/main.css:19: width: 960px;
On 2016/07/08 21:59:34, khmel wrote:
> Please see comments in .js

Acknowledged.

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/main.css:23: width: 468px;
On 2016/07/08 21:59:34, khmel wrote:
> the same as above.

Acknowledged.

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/main.css:62: padding: 8px 0 0 0;
On 2016/07/08 21:59:33, khmel wrote:
> Second checkbox section was added recently. However it broke layout
> specification
>
(https://drive.google.com/a/google.com/folderview?id=0B6x6iYCtKinENXB6OTRtYlRP...).
> This also includes changes in .section-terms below

Acknowledged.

[khmel, 2016-07-08 22:52:55]

Thanks Xiyuan for comments,

PTAL

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

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:178:
termsView.style.height = '0px';
On 2016/07/08 22:33:29, xiyuan wrote:
> On 2016/07/08 21:59:33, khmel wrote:
> > For some reason getComputedStyle depends on webview height but based on html
> > layout (terms-container is marked as flex auto) should not. This causes
> > non-stable computation (each next call gives different result). As result,
> > layout is noticeable changed on next update and 'agree'/'cancel' buttons are
> > distorted. Having 0px height before computation stabilize layout.
> 
> Strange since we have "height: 100%" on webview tag [1] and it should auto
> resized with its parent container. Maybe adding "position: relative" to
> #terms-container would get rid of this manual setting height code.
> 
> I probably have proposed this before. But anyway, in case it does not work, I
am
> fine with this work around. Just add a comment to doc why we are doing this.
> 
> [1]:
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/arc_su...

Was good try but did not help :(. Added comment.

https://codereview.chromium.org/2137713002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:346: if
(results[0].length &&
On 2016/07/08 22:33:29, xiyuan wrote:
> On 2016/07/08 21:59:33, khmel wrote:
> > Before error was not handled and results[0] generated error and stopping js
> > processing. This caused situation when technical info might be shown to user
> in
> > case of some error.
> 
> Should this be 
>   results && results.length == 1 && typeof results[0] == 'string'
> ?
> 
> Otherwise, we still hits error when results is null, [] or non string in
array.

This is much better. Honestly speaking results[0].length was incorrect and I
forgot to update the code but you proposal is safer.

[khmel, 2016-07-08 22:52:57]

Thanks Xiyuan for comments,

PTAL

[xiyuan, 2016-07-08 23:11:32]

lgtm

[khmel, 2016-07-08 23:11:51]

The CQ bit was checked by khmel@chromium.org

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

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-07-09 00:44:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-09 00:47:09]

Description was changed from

==========
arc: Provide progress to load TOS content for OptIn.

This implements progress for loading TOS content for OptIn
workflow with error/retry support. Also fixes few UI bugs
in OptIn layout windows.

BUG=b/29419423
TEST=Manually on device. Emulate error conditions on different
     stages of OptIn workflow. OptIn is correctly recoverable
     in each case.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
arc: Provide progress to load TOS content for OptIn.

This implements progress for loading TOS content for OptIn
workflow with error/retry support. Also fixes few UI bugs
in OptIn layout windows.

BUG=b/29419423
TEST=Manually on device. Emulate error conditions on different
     stages of OptIn workflow. OptIn is correctly recoverable
     in each case.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-07-09 00:47:10]

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