Aura Overlay Scrollbars appear when mouse hovers over scroller edge

Aura Overlay Scrollbars appear when mouse hovers over scroller edge

In this patch, we add a deley fade in animation when mouse move from not hover
to hover. And cancel the deley fade in when mouse move from hover to not hover.

BUG=695580
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2716453005
Cr-Commit-Position: refs/heads/master@{#454266}
Committed: https://chromium.googlesource.com/chromium/src/+/748e8177decd9de78efe7350371f88197d7a9db8

[chaopeng, 2017-02-24 19:40:21]

Description was changed from

==========
Aura Overlay Scrollbars appear when mouse hovers over scroller edge

In this patch, we add a deley fade in animation when mouse move from not hover
to hover. And cancel the deley fade in when mouse move from hover to not hover.

BUG=695580
==========

to

==========
Aura Overlay Scrollbars appear when mouse hovers over scroller edge

In this patch, we add a deley fade in animation when mouse move from not hover
to hover. And cancel the deley fade in when mouse move from hover to not hover.

BUG=695580
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chaopeng, 2017-02-24 19:40:37]

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

[chaopeng, 2017-02-24 19:40:46]

Description was changed from

==========
Aura Overlay Scrollbars appear when mouse hovers over scroller edge

In this patch, we add a deley fade in animation when mouse move from not hover
to hover. And cancel the deley fade in when mouse move from hover to not hover.

BUG=695580
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Aura Overlay Scrollbars appear when mouse hovers over scroller edge

In this patch, we add a deley fade in animation when mouse move from not hover
to hover. And cancel the deley fade in when mouse move from hover to not hover.

BUG=695580
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chaopeng, 2017-02-24 19:40:46]

chaopeng@chromium.org changed reviewers:
+ bokan@chromium.org

[chaopeng, 2017-02-24 19:40:58]

PTAL, Thank you.

[commit-bot: I haz the power, 2017-02-24 19:41:10]

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, 2017-02-24 20:05:19]

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

[commit-bot: I haz the power, 2017-02-24 20:05:20]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chaopeng, 2017-02-24 20:19:19]

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

[commit-bot: I haz the power, 2017-02-24 20:20:06]

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, 2017-02-24 21:58:52]

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

[commit-bot: I haz the power, 2017-02-24 21:58:53]

Dry run: This issue passed the CQ dry run.

[chaopeng, 2017-02-27 20:53:58]

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

[chaopeng, 2017-02-27 20:54:20]

The CQ bit was unchecked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-02-27 20:54:20]

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, 2017-02-27 22:10:39]

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

[commit-bot: I haz the power, 2017-02-27 22:10:40]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-02-27 23:13:56]

Looks great! Just some minor comments. Also, just realised you probably haven't
seen this:
https://docs.google.com/document/d/1Lv8MF1pRfAI1k9QMxGADOeONBhB3ZdYwGKx3qsLBx...

We want to use a region that's not exactly the scrollbar size - Sebastien
suggested a 50px region to start (so the distance would be 50px - scrollbar
width). Please add that to the mouse distance calculation in MouseMove.

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:123:
delayed_scrollbar_fade_in_.callback(), fade_in_delay_);
Can we say anything about the fade out animation? Is it guaranteed that it's not
posted? (If so DCHECK it). Or should we cancel and clear a fade out here?

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:127: base::TimeDelta delay =
on_resize ? fade_out_resize_delay_ : fade_out_delay_;
Ditto here for fade in

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller_unittest.cc (right):

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller_unittest.cc:997:
TEST_F(ScrollbarAnimationControllerAuraOverlayTest,
Please add another test that moving the mouse within the scrollbar region
doesn't enqueue a new fade in animation (nor cancel it). Also, once we've run
the fade in animation, moving the mouse while the scrollbars are showing
shouldn't enqueue the fade in animation.

https://codereview.chromium.org/2716453005/diff/40001/ui/native_theme/overlay...
File ui/native_theme/overlay_scrollbar_constants_aura.h (right):

https://codereview.chromium.org/2716453005/diff/40001/ui/native_theme/overlay...
ui/native_theme/overlay_scrollbar_constants_aura.h:17: constexpr base::TimeDelta
kOverlayScrollbarFadeInDelay =
Could you add a comment describing this value and mentioning that it's only for
the case where the mouse hovers near the screen edge.

[bokan, 2017-02-27 23:17:48]

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:262: bool is_over_scrollbar_now =
mouse_is_over_any_scrollbar();
Also - We shouldn't be doing any of this mouse related behavior on Android. From
a quick glance, I can't see any guards so I suspect if we plug in a mouse on
Android we might get some strange behavior. We should make sure we don't change
the fade in or out behavior as a result of mouse if we're on Android. If I'm
right that it's currently a problem, it's fine to fix that in a follow up patch.

[chaopeng, 2017-02-28 01:36:15]

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

[commit-bot: I haz the power, 2017-02-28 01:36:44]

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

[chaopeng, 2017-02-28 01:38:27]

bokan comments addressed. PTAL. Thank you.

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:127: base::TimeDelta delay =
on_resize ? fade_out_resize_delay_ : fade_out_delay_;
On 2017/02/27 23:13:56, bokan wrote:
> Ditto here for fade in

But we immediately fade in for resize and update scroll.

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:262: bool is_over_scrollbar_now =
mouse_is_over_any_scrollbar();
On 2017/02/27 23:17:48, bokan wrote:
> Also - We shouldn't be doing any of this mouse related behavior on Android.
From
> a quick glance, I can't see any guards so I suspect if we plug in a mouse on
> Android we might get some strange behavior. We should make sure we don't
change
> the fade in or out behavior as a result of mouse if we're on Android. If I'm
> right that it's currently a problem, it's fine to fix that in a follow up
patch.

We have a  early return in this method |!need_thinning_animation_| for Android.

[commit-bot: I haz the power, 2017-02-28 03:03:04]

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

[commit-bot: I haz the power, 2017-02-28 03:03:05]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-02-28 14:04:49]

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:127: base::TimeDelta delay =
on_resize ? fade_out_resize_delay_ : fade_out_delay_;
On 2017/02/28 01:38:27, chaopeng wrote:
> On 2017/02/27 23:13:56, bokan wrote:
> > Ditto here for fade in
> 
> But we immediately fade in for resize and update scroll.

Right, but that should happen immediately rather than posting a task. I'm
concerned about making sure we don't have both fade in and fade out tasks posted
at the same time.

https://codereview.chromium.org/2716453005/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:262: bool is_over_scrollbar_now =
mouse_is_over_any_scrollbar();
On 2017/02/28 01:38:27, chaopeng wrote:
> On 2017/02/27 23:17:48, bokan wrote:
> > Also - We shouldn't be doing any of this mouse related behavior on Android.
> From
> > a quick glance, I can't see any guards so I suspect if we plug in a mouse on
> > Android we might get some strange behavior. We should make sure we don't
> change
> > the fade in or out behavior as a result of mouse if we're on Android. If I'm
> > right that it's currently a problem, it's fine to fix that in a follow up
> patch.
> 
> We have a  early return in this method |!need_thinning_animation_| for
Android.

Ah, sorry, I missed that.

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:126:
DCHECK(delayed_scrollbar_fade_in_.IsCancelled());
It could also be null, right? I don't really care about the fade in task since
we reset it here. My comment was more about ensuring that if we're posting a
fade in, we don't already have a fade out posted.

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:303: if (scrollbar->ThumbThickness()
+ distance <
Nit: I think this would be clearer as |distance < kMouseMoveDistance -
ThumbThickness|

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller_unittest.cc (right):

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller_unittest.cc:996: // Scrollbars should
not schedule a new delay fade in when mouse hover hidden
Nit: "when mouse hover hidden scrollbar and move inside scrollbar" -> "when the
mouse hovers inside an already faded in scrollbar"

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller_unittest.cc:1011: // Move mouse over
inside scrollbar. Will throws an exception if post a new
I think you mean DCHECK rather than exception. Regardless, that's relying on the
production code to be a certain way. It'd be better if you compared the
start_fade() before and make sure that it's still the same start_fade() after
the second mouse move.

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller_unittest.cc:1032:
scrollbar_controller_->DidMouseMoveNear(VERTICAL, 50);
Make this a constant like the idleThicknessScale and others.

[chaopeng, 2017-02-28 15:56:43]

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

[chaopeng, 2017-02-28 15:56:57]

The CQ bit was unchecked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-02-28 15:56:57]

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

[chaopeng, 2017-02-28 15:57:21]

Updated, PTAL. Thank you.

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:126:
DCHECK(delayed_scrollbar_fade_in_.IsCancelled());
On 2017/02/28 14:04:49, bokan wrote:
> It could also be null, right? I don't really care about the fade in task since
> we reset it here. My comment was more about ensuring that if we're posting a
> fade in, we don't already have a fade out posted.

CancelableClosure doesnot have is_null(), and its IsCanceled is checking
is_null.
https://cs.chromium.org/chromium/src/base/cancelable_callback.h?dr=CSs&l=91

[commit-bot: I haz the power, 2017-02-28 17:07:00]

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

[commit-bot: I haz the power, 2017-02-28 17:07:01]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-02-28 17:12:31]

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:126:
DCHECK(delayed_scrollbar_fade_in_.IsCancelled());
On 2017/02/28 15:57:20, chaopeng wrote:
> On 2017/02/28 14:04:49, bokan wrote:
> > It could also be null, right? I don't really care about the fade in task
since
> > we reset it here. My comment was more about ensuring that if we're posting a
> > fade in, we don't already have a fade out posted.
> 
> CancelableClosure doesnot have is_null(), and its IsCanceled is checking
> is_null.
> https://cs.chromium.org/chromium/src/base/cancelable_callback.h?dr=CSs&l=91

Ok, that's fine, but I was still saying we should be ensuring we don't have both
fade in and out tasks posted at the same time.

https://codereview.chromium.org/2716453005/diff/80001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.h (right):

https://codereview.chromium.org/2716453005/diff/80001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.h:81: static constexpr float
kMouseMoveDistanceToTriggerFadeInAnimation = 50.0f;
Sebastian actually corrected this to 30 so please change.

[chaopeng, 2017-02-28 18:15:17]

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

[chaopeng, 2017-02-28 18:15:25]

The CQ bit was unchecked by chaopeng@chromium.org

[chaopeng, 2017-02-28 18:16:56]

On 2017/02/28 17:12:31, bokan wrote:
>
https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
> File cc/input/scrollbar_animation_controller.cc (right):
> 
>
https://codereview.chromium.org/2716453005/diff/60001/cc/input/scrollbar_anim...
> cc/input/scrollbar_animation_controller.cc:126:
> DCHECK(delayed_scrollbar_fade_in_.IsCancelled());
> On 2017/02/28 15:57:20, chaopeng wrote:
> > On 2017/02/28 14:04:49, bokan wrote:
> > > It could also be null, right? I don't really care about the fade in task
> since
> > > we reset it here. My comment was more about ensuring that if we're posting
a
> > > fade in, we don't already have a fade out posted.
> > 
> > CancelableClosure doesnot have is_null(), and its IsCanceled is checking
> > is_null.
> > https://cs.chromium.org/chromium/src/base/cancelable_callback.h?dr=CSs&l=91
> 
> Ok, that's fine, but I was still saying we should be ensuring we don't have
both
> fade in and out tasks posted at the same time.
> 
>
https://codereview.chromium.org/2716453005/diff/80001/cc/input/scrollbar_anim...
> File cc/input/scrollbar_animation_controller.h (right):
> 
>
https://codereview.chromium.org/2716453005/diff/80001/cc/input/scrollbar_anim...
> cc/input/scrollbar_animation_controller.h:81: static constexpr float
> kMouseMoveDistanceToTriggerFadeInAnimation = 50.0f;
> Sebastian actually corrected this to 30 so please change.

I add a DCHECK in PostDelayedFadeIn/Out for ensuring we don't have both posted
at the same time.

Changed to 30px. 

PTAL. Thank you.

[bokan, 2017-02-28 18:36:03]

Thank you, lgtm

[chaopeng, 2017-02-28 18:44:06]

The CQ bit was checked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-02-28 18:44:39]

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

[bokan, 2017-02-28 18:50:36]

On 2017/02/28 18:44:39, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

You'll need cc/ and ui/ OWNER stamps too

[commit-bot: I haz the power, 2017-02-28 18:54:40]

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

[commit-bot: I haz the power, 2017-02-28 18:54:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chaopeng, 2017-02-28 19:01:25]

chaopeng@chromium.org changed reviewers:
+ weiliangc@chromium.org

[chaopeng, 2017-02-28 19:01:25]

weiliangc@chromium.org: PTAL

[chaopeng, 2017-02-28 20:04:37]

chaopeng@chromium.org changed reviewers:
+ alexmos@chromium.org, sadrul@chromium.org

[chaopeng, 2017-02-28 20:04:38]

alexmos@chromium.org: Please review changes in content/

sadrul@chromium.org: Please review changes in ui/

[alexmos, 2017-02-28 23:45:48]

content/ LGTM

[sadrul, 2017-03-01 15:34:54]

ui lgtm

[weiliangc, 2017-03-01 16:58:11]

https://codereview.chromium.org/2716453005/diff/100001/cc/input/scrollbar_ani...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2716453005/diff/100001/cc/input/scrollbar_ani...
cc/input/scrollbar_animation_controller.cc:165: float progress =
delta.InSecondsF() / fade_out_duration_.InSecondsF();
Quick question, why is there fade out duration, but no fade in duration?

[bokan, 2017-03-01 17:13:59]

https://codereview.chromium.org/2716453005/diff/100001/cc/input/scrollbar_ani...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2716453005/diff/100001/cc/input/scrollbar_ani...
cc/input/scrollbar_animation_controller.cc:165: float progress =
delta.InSecondsF() / fade_out_duration_.InSecondsF();
On 2017/03/01 16:58:10, weiliangc wrote:
> Quick question, why is there fade out duration, but no fade in duration?

Fade-in is instant. Perhaps "fade-in" should be replaced with "show" or another
verb.

[weiliangc, 2017-03-01 19:06:42]

On 2017/03/01 at 17:13:59, bokan wrote:
>
https://codereview.chromium.org/2716453005/diff/100001/cc/input/scrollbar_ani...
> File cc/input/scrollbar_animation_controller.cc (right):
> 
>
https://codereview.chromium.org/2716453005/diff/100001/cc/input/scrollbar_ani...
> cc/input/scrollbar_animation_controller.cc:165: float progress =
delta.InSecondsF() / fade_out_duration_.InSecondsF();
> On 2017/03/01 16:58:10, weiliangc wrote:
> > Quick question, why is there fade out duration, but no fade in duration?
> 
> Fade-in is instant. Perhaps "fade-in" should be replaced with "show" or
another verb.

Yeah using "show" sounds like a good idea. Thanks for clarifying this.

[chaopeng, 2017-03-01 19:38:28]

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

[commit-bot: I haz the power, 2017-03-01 19:39:00]

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

[chaopeng, 2017-03-01 19:39:07]

On 2017/03/01 19:06:42, weiliangc wrote:
> On 2017/03/01 at 17:13:59, bokan wrote:
> >
>
https://codereview.chromium.org/2716453005/diff/100001/cc/input/scrollbar_ani...
> > File cc/input/scrollbar_animation_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/2716453005/diff/100001/cc/input/scrollbar_ani...
> > cc/input/scrollbar_animation_controller.cc:165: float progress =
> delta.InSecondsF() / fade_out_duration_.InSecondsF();
> > On 2017/03/01 16:58:10, weiliangc wrote:
> > > Quick question, why is there fade out duration, but no fade in duration?
> > 
> > Fade-in is instant. Perhaps "fade-in" should be replaced with "show" or
> another verb.
> 
> Yeah using "show" sounds like a good idea. Thanks for clarifying this.

Renamed all fade in to show. PTAL. Thank you.

[commit-bot: I haz the power, 2017-03-01 21:35:41]

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

[commit-bot: I haz the power, 2017-03-01 21:35:42]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2017-03-02 14:49:24]

Thank you. LGTM.

[chaopeng, 2017-03-02 15:24:10]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2017-03-02 15:24:10]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
sadrul@chromium.org, alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2716453005/#ps120001
(title: "rename fade in to show")

[commit-bot: I haz the power, 2017-03-02 15:24:26]

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-03-02 15:30:41]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488468250036700,
"parent_rev": "07e27841f0a5a7c76d0192dd0a509a0c22aa5763", "commit_rev":
"748e8177decd9de78efe7350371f88197d7a9db8"}

[commit-bot: I haz the power, 2017-03-02 15:31:18]

Description was changed from

==========
Aura Overlay Scrollbars appear when mouse hovers over scroller edge

In this patch, we add a deley fade in animation when mouse move from not hover
to hover. And cancel the deley fade in when mouse move from hover to not hover.

BUG=695580
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Aura Overlay Scrollbars appear when mouse hovers over scroller edge

In this patch, we add a deley fade in animation when mouse move from not hover
to hover. And cancel the deley fade in when mouse move from hover to not hover.

BUG=695580
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2716453005
Cr-Commit-Position: refs/heads/master@{#454266}
Committed:
https://chromium.googlesource.com/chromium/src/+/748e8177decd9de78efe7350371f...
==========

[commit-bot: I haz the power, 2017-03-02 15:31:20]

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