arc: Use overlay to show links from ToS.

arc: Use overlay to show links from ToS.

BUG=32247410
TEST=Locally on device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/845e02dc39e9283f6a13957bac034c0d4a3d172c
Cr-Commit-Position: refs/heads/master@{#426101}

[khmel, 2016-10-18 21:02:32]

Description was changed from

==========
arc: Use overlay to show links from ToS.

BUG=32247410
TEST=Locally on device.
==========

to

==========
arc: Use overlay to show links from ToS.

BUG=32247410
TEST=Locally on device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[khmel, 2016-10-18 21:14:25]

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

[khmel, 2016-10-18 21:14:25]

Hi Xiyuan,

PTAL

Thanks

[xiyuan, 2016-10-18 21:26:07]

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

https://codereview.chromium.org/2429793003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:432:
showOverlay('overlay-text');
Do we need to worry about the text content being shown when the overlay is used
again for an url? That is, what if user triggers showLearnMoreOverlay then
showURLOverlay, would the |learnMoreContent| be shown together with the webview?

We probably should either add css rules to make sure that only one of the <span>
and <webview> is visible or explicitly show/hide them.

https://codereview.chromium.org/2429793003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_support/main.html (right):

https://codereview.chromium.org/2429793003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/main.html:151: <span
id="learn-more-content"></span>
nit: Since we are making it generic, should we update this id too:
e.g.
  learn-more-content -> overlay-text-content

[khmel, 2016-10-18 22:18:12]

Thank you for your quick review!

PTAL

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

https://codereview.chromium.org/2429793003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:432:
showOverlay('overlay-text');
On 2016/10/18 21:26:07, xiyuan wrote:
> Do we need to worry about the text content being shown when the overlay is
used
> again for an url? That is, what if user triggers showLearnMoreOverlay then
> showURLOverlay, would the |learnMoreContent| be shown together with the
webview?
> 
> We probably should either add css rules to make sure that only one of the
<span>
> and <webview> is visible or explicitly show/hide them.

I added css rule as:

.overlay-text webview {
  display: none;
}

.overlay-url span {
  display: none;
}

I also re-tested this by clicking text and web links in random order and it
seems everything works as expected.

https://codereview.chromium.org/2429793003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_support/main.html (right):

https://codereview.chromium.org/2429793003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/main.html:151: <span
id="learn-more-content"></span>
On 2016/10/18 21:26:07, xiyuan wrote:
> nit: Since we are making it generic, should we update this id too:
> e.g.
>   learn-more-content -> overlay-text-content

Done.

[xiyuan, 2016-10-18 22:21:50]

lgtm

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

https://codereview.chromium.org/2429793003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:432:
showOverlay('overlay-text');
On 2016/10/18 22:18:12, khmel wrote:
> On 2016/10/18 21:26:07, xiyuan wrote:
> > Do we need to worry about the text content being shown when the overlay is
> used
> > again for an url? That is, what if user triggers showLearnMoreOverlay then
> > showURLOverlay, would the |learnMoreContent| be shown together with the
> webview?
> > 
> > We probably should either add css rules to make sure that only one of the
> <span>
> > and <webview> is visible or explicitly show/hide them.
> 
> I added css rule as:
> 
> .overlay-text webview {
>   display: none;
> }
> 
> .overlay-url span {
>   display: none;
> }
> 
> I also re-tested this by clicking text and web links in random order and it
> seems everything works as expected.

You are right. Mis-read that.

[khmel, 2016-10-18 22:26:05]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2016-10-18 22:26:35]

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-19 00:20:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:05:17]

Description was changed from

==========
arc: Use overlay to show links from ToS.

BUG=32247410
TEST=Locally on device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
arc: Use overlay to show links from ToS.

BUG=32247410
TEST=Locally on device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/845e02dc39e9283f6a13957bac034c0d4a3d172c
Cr-Commit-Position: refs/heads/master@{#426101}
==========

[commit-bot: I haz the power, 2016-10-21 13:05:18]

Patchset 3 (id:??) landed as
https://crrev.com/845e02dc39e9283f6a13957bac034c0d4a3d172c
Cr-Commit-Position: refs/heads/master@{#426101}