Update toast behavior and apperance according to UX requirement

Update toast behavior and apperance according to UX requirement

BUG=736761
Review-Url: https://codereview.chromium.org/2957173002
Cr-Commit-Position: refs/heads/master@{#483281}
Committed: https://chromium.googlesource.com/chromium/src/+/14d5ed2dd6b7ca8b6eea5875c9c64e142846e4b6

[bshe, 2017-06-28 12:43:06]

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

[commit-bot: I haz the power, 2017-06-28 12:43:17]

Dry run: CQ is trying da patch.

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

[bshe, 2017-06-28 13:16:23]

bshe@chromium.org changed reviewers:
+ cjgrant@chromium.org

[bshe, 2017-06-28 13:16:24]

Hey Chris. Do you mind to take a look? Thanks.

[bshe, 2017-06-28 13:16:29]

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

[commit-bot: I haz the power, 2017-06-28 13:16:36]

Dry run: CQ is trying da patch.

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

[cjgrant, 2017-06-28 13:55:35]

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/exclusive_screen_toast_texture.cc
(right):

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exclusive_screen_toast_texture.cc:24:
constexpr float kRadius = 0.004;
Possibly kCornerRadius for clarity?

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/exclusive_screen_toast.cc
(right):

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/exclusive_screen_toast.cc:21: float
x = drawn_size.width() / drawn_size.height() * size().y();
s/x/width/ ?  'x' generally sounds like a position.

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:527: if (fullscreen_ &&
!web_vr_mode_) {
Why not put this block into ConfigureExclusiveScreenToast, and call it here? 
IMO, that method's job is to do exactly what this code is doing.

[commit-bot: I haz the power, 2017-06-28 14:51:50]

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

[commit-bot: I haz the power, 2017-06-28 14:51:51]

Dry run: This issue passed the CQ dry run.

[amp, 2017-06-28 16:47:21]

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

[amp, 2017-06-28 16:47:21]

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:439:
element->set_size({kToastWidthDMM, kToastHeightDMM, 1});
Don't the DMM need to be multiplied by the distance before being used?

Or is that what set_scale does below?  (in which case why don't we do that for
other elements?)

[bshe, 2017-06-28 20:01:58]

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/exclusive_screen_toast_texture.cc
(right):

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exclusive_screen_toast_texture.cc:24:
constexpr float kRadius = 0.004;
On 2017/06/28 13:55:34, cjgrant wrote:
> Possibly kCornerRadius for clarity?

Done.

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/exclusive_screen_toast.cc
(right):

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/exclusive_screen_toast.cc:21: float
x = drawn_size.width() / drawn_size.height() * size().y();
On 2017/06/28 13:55:34, cjgrant wrote:
> s/x/width/ ?  'x' generally sounds like a position.

Done.

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:439:
element->set_size({kToastWidthDMM, kToastHeightDMM, 1});
On 2017/06/28 16:47:21, amp wrote:
> Don't the DMM need to be multiplied by the distance before being used?
> 
> Or is that what set_scale does below?  (in which case why don't we do that for
> other elements?)

It is what set_scale does. It is unclear to me if we have a preference of one
over the other in each individual case. 
In my case, use set_scale makes much more sense. The real element size was
dynamically calculated based on the string length. And only the height of toast
stays the same. After calculation, I shouldn't change the size at all.

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:527: if (fullscreen_ &&
!web_vr_mode_) {
On 2017/06/28 13:55:34, cjgrant wrote:
> Why not put this block into ConfigureExclusiveScreenToast, and call it here? 
> IMO, that method's job is to do exactly what this code is doing.

I am trying to make rebase easier. Your refactor patch will get rid of
ConfigureExclusiveScreenToast, right?

[cjgrant, 2017-06-28 20:24:51]

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:527: if (fullscreen_ &&
!web_vr_mode_) {
On 2017/06/28 20:01:58, bshe wrote:
> On 2017/06/28 13:55:34, cjgrant wrote:
> > Why not put this block into ConfigureExclusiveScreenToast, and call it here?

> > IMO, that method's job is to do exactly what this code is doing.
> 
> I am trying to make rebase easier. Your refactor patch will get rid of
> ConfigureExclusiveScreenToast, right?

Yes, it does, currently, but when there's this much code, it should continue to
exist.

[cjgrant, 2017-06-28 20:27:24]

lgtm after existing comments are addressed

[bshe, 2017-06-29 01:34:12]

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2957173002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:527: if (fullscreen_ &&
!web_vr_mode_) {
On 2017/06/28 20:24:50, cjgrant wrote:
> On 2017/06/28 20:01:58, bshe wrote:
> > On 2017/06/28 13:55:34, cjgrant wrote:
> > > Why not put this block into ConfigureExclusiveScreenToast, and call it
here?
> 
> > > IMO, that method's job is to do exactly what this code is doing.
> > 
> > I am trying to make rebase easier. Your refactor patch will get rid of
> > ConfigureExclusiveScreenToast, right?
> 
> Yes, it does, currently, but when there's this much code, it should continue
to
> exist.

Done.

[bshe, 2017-06-29 01:34:19]

The CQ bit was checked by bshe@chromium.org

[bshe, 2017-06-29 01:34:20]

The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2957173002/#ps120001
(title: "review")

[commit-bot: I haz the power, 2017-06-29 01:34: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, 2017-06-29 03:36:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498700059469150,
"parent_rev": "33dbb18a885ca3abab18cbda03706dd89df4dc4b", "commit_rev":
"14d5ed2dd6b7ca8b6eea5875c9c64e142846e4b6"}

[commit-bot: I haz the power, 2017-06-29 03:36:15]

Description was changed from

==========
Update toast behavior and apperance according to UX requirement

BUG=736761
==========

to

==========
Update toast behavior and apperance according to UX requirement

BUG=736761

Review-Url: https://codereview.chromium.org/2957173002
Cr-Commit-Position: refs/heads/master@{#483281}
Committed:
https://chromium.googlesource.com/chromium/src/+/14d5ed2dd6b7ca8b6eea5875c9c6...
==========

[commit-bot: I haz the power, 2017-06-29 03:36:19]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/14d5ed2dd6b7ca8b6eea5875c9c6...