[Android] Implementation of overscroll effect for Android L

[Android] Implementation of overscroll effect for Android L

Add an overscroll effect implementation that mimics that of Android L. The
primary differences are the use of a single rasterized arc layer and the
inclusion of motion orthogonal to overscroll in computing the effect offset.

BUG=389744
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290052

[jdduke (slow), 2014-07-17 16:05:38]

aelias@: PTAL.  I should have sent this out for review earlier, but I failed to
realize M38 was the Chrome version we'll be shipping for L.  This patch doesn't
pull in the customized overscroll color that we'll eventually want, but I figure
it's best to get review moving so we can land something in time for M38.

My biggest worry is the size of the SkBitmap (it's a good deal larger than the
bitmaps we've been using), but I'm hoping we can land powei@'s patch for using
shared UIResource id's and the relative cost will be diminished.

[aelias_OOO_until_Jul13, 2014-07-17 20:23:17]

Haven't looked in detail yet, but right, the bitmap size is a pretty big
concern.  We should switch to kAlpha_8_SkColorType and colorize in a shader. 
This will require a few CC changes.

https://codereview.chromium.org/367173003/diff/60001/content/browser/android/...
File content/browser/android/edge_effect_l.cc (right):

https://codereview.chromium.org/367173003/diff/60001/content/browser/android/...
content/browser/android/edge_effect_l.cc:98: const float radius = width * 0.75f
/ kSin;
Looks like this ends up multiplying by 1.5, for overall 3x screen size?  Math
error somewhere here?

[jdduke (slow), 2014-07-17 22:24:13]

On 2014/07/17 20:23:17, aelias wrote:
> Haven't looked in detail yet, but right, the bitmap size is a pretty big
> concern.  We should switch to kAlpha_8_SkColorType and colorize in a shader. 
> This will require a few CC changes.
> 

OK, I imagine I can just pattern after the existing color type support?

https://codereview.chromium.org/367173003/diff/60001/content/browser/android/...
File content/browser/android/edge_effect_l.cc (right):

https://codereview.chromium.org/367173003/diff/60001/content/browser/android/...
content/browser/android/edge_effect_l.cc:98: const float radius = width * 0.75f
/ kSin;
On 2014/07/17 20:23:17, aelias wrote:
> Looks like this ends up multiplying by 1.5, for overall 3x screen size?  Math
> error somewhere here?

Hmm, the (raw sized) effect should be larger than the screen length,
accommodating how the effect moves laterally on the non-overscrolling axis. 
However, 3x feels wrong, I'll take a closer look.

[jdduke (slow), 2014-08-11 19:51:10]

https://codereview.chromium.org/367173003/diff/60001/content/browser/android/...
File content/browser/android/edge_effect_l.cc (right):

https://codereview.chromium.org/367173003/diff/60001/content/browser/android/...
content/browser/android/edge_effect_l.cc:98: const float radius = width * 0.75f
/ kSin;
On 2014/07/17 20:23:17, aelias wrote:
> Looks like this ends up multiplying by 1.5, for overall 3x screen size?  Math
> error somewhere here?

1.5 is right, but the variable name |radius| is misleading; it's not not the
radius of the effect but of the arc (and we're only capturing a subset of that,
so it's a much smaller rect).  I think we can get away with taking the min
instead of the max of the screen dimensions.  Inspecting this on a Nexus 7
(1920x1200), the quality of the effect along the tablet sides (the longer
dimension) doesn't seem particularly degraded.  

Still, I wonder if that's too big (1800x240).  I'll play with smaller versions,
and see about moving this to the kA8_Config.

[jdduke (slow), 2014-08-14 19:40:50]

OK, I've rebased to the GL_ALPHA patch. I also went ahead and rewired the
DidOverscrollParams to include the event location; simply using the last
GestureScrollUpdate location seemed a bit presumptuous.  I'll add the requisite
owners once review settles.

[aelias_OOO_until_Jul13, 2014-08-14 22:54:20]

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
File content/browser/android/edge_effect_base.h (right):

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/edge_effect_base.h:32: virtual void Pull(base::TimeTicks
current_time, float delta_distance) = 0;
This variant seems unnecessary, can we remove it?  Note that overloading is
moderately discouraged by the style guide.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
File content/browser/android/edge_effect_l.cc (right):

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/edge_effect_l.cc:15: const int kRecedeTime = 1000;
Rename to kRecedeTimeMs

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/edge_effect_l.cc:18: const int kPullTime = 167;
Rename to kPullTimeMs

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
File content/browser/android/overscroll_glow.cc (right):

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/overscroll_glow.cc:25: const int kKitKatSDKVersion = 19;
Suggest rename to kKitKatMR2SDKVersion to avoid ambiguity.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/overscroll_glow.cc:84: bool UseEdgeEffectL() {
I don't like this kind of global factory policy.  For example, this can hamper
testability.  How about making the creator of OverscrollGlow first create the
EdgeEffects and pass them in the OverscrollGlow constructor?

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/overscroll_glow.cc:254:
gfx::Screen::GetNativeScreen()->GetPrimaryDisplay().device_scale_factor();
Can this also be passed in the OverscrollGlow constructor?

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
File content/browser/android/system_ui_resource_manager_impl.cc (right):

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:36:
paint.setColor((kThemeColor & 0xffffff) | 0x33000000);
Let's just do paint.setAlpha(0x33).  We shouldn't leave dead code lying around
until we're ready to support theming as currently it's potentially confusing.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:39: const float
screen_width =
"screen_width" sometimes being height seems confusing, I suggest getting rid of
this temporary and merging this into the arc_width line.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:44: gfx::Size
bounds(arc_width, height);
If I'm reading this correctly, this still ends up rather large even with A8,
right?  Like 500Kb on a phone?  Can we shrink the size further and make a
tradeoff about sharpness?

[jdduke (slow), 2014-08-15 19:15:06]

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
File content/browser/android/edge_effect_base.h (right):

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/edge_effect_base.h:32: virtual void Pull(base::TimeTicks
current_time, float delta_distance) = 0;
On 2014/08/14 22:54:19, aelias wrote:
> This variant seems unnecessary, can we remove it?  Note that overloading is
> moderately discouraged by the style guide.

Yeah, looks like they kept it in the Android framework for compatibility
reasons, but we don't need the old signature.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
File content/browser/android/edge_effect_l.cc (right):

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/edge_effect_l.cc:15: const int kRecedeTime = 1000;
On 2014/08/14 22:54:19, aelias wrote:
> Rename to kRecedeTimeMs

Done.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/edge_effect_l.cc:18: const int kPullTime = 167;
On 2014/08/14 22:54:19, aelias wrote:
> Rename to kPullTimeMs

Done.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
File content/browser/android/overscroll_glow.cc (right):

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/overscroll_glow.cc:25: const int kKitKatSDKVersion = 19;
On 2014/08/14 22:54:19, aelias wrote:
> Suggest rename to kKitKatMR2SDKVersion to avoid ambiguity.

Done.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/overscroll_glow.cc:84: bool UseEdgeEffectL() {
On 2014/08/14 22:54:19, aelias wrote:
> I don't like this kind of global factory policy.  For example, this can hamper
> testability.  How about making the creator of OverscrollGlow first create the
> EdgeEffects and pass them in the OverscrollGlow constructor?

Hmm, the EdgeEffects are currently setup to be lazily created, so if a site
never overscrolls they never pay the Layer/resource cost. I suppose I could pass
in a factory method... I considered also lazily creating the OverscrollGlow
itself, but there are some bookkeeping issues between frame metadata and the
timing of the overscroll event.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/overscroll_glow.cc:254:
gfx::Screen::GetNativeScreen()->GetPrimaryDisplay().device_scale_factor();
On 2014/08/14 22:54:19, aelias wrote:
> Can this also be passed in the OverscrollGlow constructor?

Hmm, if I pass in a factory method I think we can get away with not using the
scale factor here at all.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
File content/browser/android/system_ui_resource_manager_impl.cc (right):

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:36:
paint.setColor((kThemeColor & 0xffffff) | 0x33000000);
On 2014/08/14 22:54:20, aelias wrote:
> Let's just do paint.setAlpha(0x33).  We shouldn't leave dead code lying around
> until we're ready to support theming as currently it's potentially confusing.

Done.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:39: const float
screen_width =
On 2014/08/14 22:54:20, aelias wrote:
> "screen_width" sometimes being height seems confusing, I suggest getting rid
of
> this temporary and merging this into the arc_width line.

Done.

https://codereview.chromium.org/367173003/diff/180001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:44: gfx::Size
bounds(arc_width, height);
On 2014/08/14 22:54:20, aelias wrote:
> If I'm reading this correctly, this still ends up rather large even with A8,
> right?  Like 500Kb on a phone?  Can we shrink the size further and make a
> tradeoff about sharpness?

Where are you getting 500KB? On a 720 phone, we end up with 720x97x1=70KB, and
on a 1080 phone, 1080x145x1=156KB. That's larger than I'd like, but at least now
we're sharing the resource across all edges, and each edge only uses the
resource once.

I tried 0.25 instead of 0.5f in the width multiplier (the current multiplier,
0.5 / kSin is just 1, I wonder if we should just get rid of that multiplication
for simplicity), the artifacts become a good deal more apparent.  I guess I
could go down to 0.4f or something like that?

[jdduke (slow), 2014-08-15 19:19:30]

PTAL for owner review, thanks.

tsepez@: content/common/input_messages.h
yfriedman@: ui/base/android

[Yaron, 2014-08-15 19:28:49]

lgtm

[aelias_OOO_until_Jul13, 2014-08-15 19:29:44]

lgtm

[Tom Sepez, 2014-08-15 22:46:53]

Messages LGTM.

[jdduke (slow), 2014-08-15 22:47:33]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-08-15 22:50:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/367173003/240001

[commit-bot: I haz the power, 2014-08-15 23:38:27]

Committed patchset #11 (240001) as 290052