[Android] Add a native pull-to-refresh overscroll effect

[Android] Add a native pull-to-refresh overscroll effect

Add a restricted pull-to-refresh effect to Android. The effect is
triggered only when 1) the scroll starts when the page has no vertical
offset, 2) the scroll direction is upward and 3) the initial scroll
is not consumed by the page. Page reloads are triggered only when
the user pulls and releases beyond a certain threshold.

BUG=428429
Committed: https://crrev.com/9db1b9d8f01028f3badc34a2f4f6ce63fda87247
Cr-Commit-Position: refs/heads/master@{#304320}

[jdduke (slow), 2014-10-30 00:03:11]

jdduke@chromium.org changed reviewers:
+ aelias@chromium.org, tedchoc@chromium.org

[jdduke (slow), 2014-10-30 00:03:12]

PTAL, thanks.

tedchoc@: ui/base/android, content/public/android and content/browser/android
(in the last directory, particularly overscroll_controller_android* and
overscroll_refresh*, the other changes there should be fairly cosmetic
refactorings).

aelias: content/browser/renderer_host, ui/gfx/android,
ui/android/java/src/org/chromium/ui/gfx

[jdduke (slow), 2014-10-30 00:05:18]

On 2014/10/30 00:03:12, jdduke wrote:
> PTAL, thanks.
> 
> tedchoc@: ui/base/android, content/public/android and content/browser/android
> (in the last directory, particularly overscroll_controller_android* and
> overscroll_refresh*, the other changes there should be fairly cosmetic
> refactorings).
> 
> aelias: content/browser/renderer_host, ui/gfx/android,
> ui/android/java/src/org/chromium/ui/gfx

Hmm, actually, aelias@ could you review the overscroll_controller_android* and
overscroll_refresh* additions?

[aelias_OOO_until_Jul13, 2014-10-30 03:53:22]

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:93: // TODO(jdduke):
Figure out a cleaner way of suppressing a fling.
What would happen if we just let flings always go through to the renderer?

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_refresh.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh.cc:362: scroll_delta.y() < 0) {
I'm having trouble following what this is for, could you add a comment?

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

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:93: // TODO(jdduke):
Figure out a cleaner way of suppressing a fling.
On 2014/10/30 03:53:21, aelias wrote:
> What would happen if we just let flings always go through to the renderer?

That's the behavior I had originally, and it's certainly not a problem for
"upward" flings, as they immediately terminate.  However, Ted pointed out that
allowing a "downward" fling felt weird, and I agree with him. Perhaps it's
because we consume the downward touch scroll, so the fling motion at the end
feels discontinuous?

I almost rewired the whole |FilterInputEvent| chain to take an event pointer and
allow mutation, but that's just asking for trouble and I'd really prefer not to
open that up =/

[Ted C, 2014-10-31 23:52:06]

overall this seems very reasonable to me.  some small nits but nothing major

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:113:
content_view_core_->SetNeedsAnimate();
I wonder if we should just make GetRenderWidgetHostViewAndroid public in
ContentViewCore to avoid having to add this method there.

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_controller_android.h (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_controller_android.h:46: // Te be called upon
receipt of an overscroll event.
s/Te/To

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_refresh.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh.cc:39: const float
kPullActivationThreshold = .25f;
What is this a percentage of?  Screen size?  If so, I think it would be odd to
have to pull down more or less depending on if you rotated your device.

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh.cc:346: bool allow_activation =
scroll_velocity.y() > -500;
where does -500 come from?  should it be a constant?

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_refresh_unittest.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh_unittest.cc:32: bool
IsTriggeredRefreshActive() const override { return true; }
Should this return refresh_triggered_ instead?

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh_unittest.cc:123:
TEST_F(OverscrollRefreshTest, NotTriggeredIfInitialYOffsetIsZero) {
Is[Not]Zero?

https://codereview.chromium.org/679493002/diff/140001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/UIResources.java
(right):

https://codereview.chromium.org/679493002/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/UIResources.java:36:
int reqWidth,
this is c++ indenting...

I would put them all on the second line like getBitmap below.

https://codereview.chromium.org/679493002/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/UIResources.java:44:

remove this extra line

[jdduke (slow), 2014-11-06 00:10:37]

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:113:
content_view_core_->SetNeedsAnimate();
On 2014/10/31 23:52:05, Ted C wrote:
> I wonder if we should just make GetRenderWidgetHostViewAndroid public in
> ContentViewCore to avoid having to add this method there.

Your call, don't have a strong opinion here. It's nice not to depend on RWHVA
here, but maybe not as nice as not having to add the animate bridge... =/

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_controller_android.h (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_controller_android.h:46: // Te be called upon
receipt of an overscroll event.
On 2014/10/31 23:52:05, Ted C wrote:
> s/Te/To

Done.

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_refresh.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh.cc:39: const float
kPullActivationThreshold = .25f;
On 2014/10/31 23:52:05, Ted C wrote:
> What is this a percentage of?  Screen size?  If so, I think it would be odd to
> have to pull down more or less depending on if you rotated your device.

I think it's important to be at least somewhat coupled to screen size, but maybe
we should just use the smaller of the two dimensions to make it consistent?

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh.cc:346: bool allow_activation =
scroll_velocity.y() > -500;
On 2014/10/31 23:52:05, Ted C wrote:
> where does -500 come from?  should it be a constant?

I'll pull it out as a constant, it was from trial and error.

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh.cc:362: scroll_delta.y() < 0) {
On 2014/10/30 03:53:22, aelias wrote:
> I'm having trouble following what this is for, could you add a comment?

Done.

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_refresh_unittest.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh_unittest.cc:32: bool
IsTriggeredRefreshActive() const override { return true; }
On 2014/10/31 23:52:06, Ted C wrote:
> Should this return refresh_triggered_ instead?

Oops, yeah, I had a test for this codepath earlier then decided against it as we
really just want to make sure that the animation terminates after a timeout,
even when the client always reports that the refresh operation is live. Added a
comment for why return true is appropriate here.

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh_unittest.cc:123:
TEST_F(OverscrollRefreshTest, NotTriggeredIfInitialYOffsetIsZero) {
On 2014/10/31 23:52:05, Ted C wrote:
> Is[Not]Zero?

Done.

https://codereview.chromium.org/679493002/diff/140001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/UIResources.java
(right):

https://codereview.chromium.org/679493002/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/UIResources.java:36:
int reqWidth,
On 2014/10/31 23:52:06, Ted C wrote:
> this is c++ indenting...
> 
> I would put them all on the second line like getBitmap below.

Done.

https://codereview.chromium.org/679493002/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/UIResources.java:44:

On 2014/10/31 23:52:06, Ted C wrote:
> remove this extra line

Done.

[Ted C, 2014-11-06 19:25:40]

lgtm -- nothing major left from me

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:113:
content_view_core_->SetNeedsAnimate();
On 2014/11/06 00:10:37, jdduke wrote:
> On 2014/10/31 23:52:05, Ted C wrote:
> > I wonder if we should just make GetRenderWidgetHostViewAndroid public in
> > ContentViewCore to avoid having to add this method there.
> 
> Your call, don't have a strong opinion here. It's nice not to depend on RWHVA
> here, but maybe not as nice as not having to add the animate bridge... =/

I have a slight preference for exposing RWHVA.  Maybe we can stem the tide that
is the growth of the beast that is CVC.

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_refresh.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh.cc:39: const float
kPullActivationThreshold = .25f;
On 2014/11/06 00:10:37, jdduke wrote:
> On 2014/10/31 23:52:05, Ted C wrote:
> > What is this a percentage of?  Screen size?  If so, I think it would be odd
to
> > have to pull down more or less depending on if you rotated your device.
> 
> I think it's important to be at least somewhat coupled to screen size, but
maybe
> we should just use the smaller of the two dimensions to make it consistent?

In general, we should match the refresh widget used in the newer google apps. 
Capping it to the smaller of the two "seems" reasonable to me.

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_refresh_unittest.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh_unittest.cc:123:
TEST_F(OverscrollRefreshTest, NotTriggeredIfInitialYOffsetIsZero) {
On 2014/11/06 00:10:37, jdduke wrote:
> On 2014/10/31 23:52:05, Ted C wrote:
> > Is[Not]Zero?
> 
> Done.

Really?

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
File content/browser/android/overscroll_refresh.cc (right):

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.cc:262:
resource_manager_->GetUIResourceId(kIdleResourceType), size,
I was under the impression that these all did need to go on their own line (like
your previous patch).  Have I been living a lie?

Does that only apply for function declaration?

[aelias_OOO_until_Jul13, 2014-11-06 22:28:49]

lgtm, since almost all my comments below are just code/terminology clarity.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
File content/browser/android/overscroll_refresh.cc (right):

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.cc:84: if (layer->parent() != parent)
Please move these two if statements (reparenting and checking for
layer_tree_host()) above all the other early returns.  I think we should always
do this structural stuff even if nondrawable to make this easier to reason
about.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.cc:159: bool Update(base::TimeTicks
current_time, bool is_triggered_refresh_active) {
Please rename this to Animate.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.cc:353: if (scroll_consumption_state_
== ENABLED) {
I think this would be more readable as a switch statement, in order of the enum
(DISABLED, AWAITING_SCROLL_UPDATE_ACK, ENABLED) and terminating each with return
true/false instead of break.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
File content/browser/android/overscroll_refresh.h (right):

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:51: enum ActivationAllowance {
ALLOW_ACTIVATION, PREVENT_ACTIVATION };
This enum seems like an unnecessary abstraction.  Please just make
OnScrollUpdateAck take "bool was_consumed"

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:54: // Returns true if the supplied
|scroll_delta| is consumed, in which case it
Please avoid the slightly ambiguous passive tense here: "Returns true if the
refresh effect has consumed the |scroll_delta|"

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:72: bool IsActive() const;
Here you use "active" to mean "exists at all" and elsewhere you use the same
wording to mean "confirmed to cause a refresh".  Could you rename this one to
"Exists()" maybe?

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:75: bool IsPendingScrollUpdateAck()
const;
Please rename to IsAwaitingScrollUpdateAck, I think using the word "waiting"
makes this easier to follow.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:83: gfx::Vector2dF scroll_offset_;
I'd prefer this as a "bool scrolled_to_top_" as this is rich information and
raises the question of why the refresh effect needs it.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:87: PENDING_SCROLL_UPDATE_ACK,
Please rename to AWAITING_SCROLL_UPDATE_ACK.

[jdduke (slow), 2014-11-07 18:33:45]

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_controller_android.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_controller_android.cc:113:
content_view_core_->SetNeedsAnimate();
On 2014/11/06 19:25:40, Ted C wrote:
> On 2014/11/06 00:10:37, jdduke wrote:
> > On 2014/10/31 23:52:05, Ted C wrote:
> > > I wonder if we should just make GetRenderWidgetHostViewAndroid public in
> > > ContentViewCore to avoid having to add this method there.
> > 
> > Your call, don't have a strong opinion here. It's nice not to depend on
RWHVA
> > here, but maybe not as nice as not having to add the animate bridge... =/
> 
> I have a slight preference for exposing RWHVA.  Maybe we can stem the tide
that
> is the growth of the beast that is CVC.

I did us all a favor and removed all of the ContentViewCore/RWHVA dependencies.
Instead we take a WebContents, a WindowAndroidCompositor and a DPI scale.

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_refresh.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh.cc:39: const float
kPullActivationThreshold = .25f;
On 2014/11/06 19:25:40, Ted C wrote:
> On 2014/11/06 00:10:37, jdduke wrote:
> > On 2014/10/31 23:52:05, Ted C wrote:
> > > What is this a percentage of?  Screen size?  If so, I think it would be
odd
> to
> > > have to pull down more or less depending on if you rotated your device.
> > 
> > I think it's important to be at least somewhat coupled to screen size, but
> maybe
> > we should just use the smaller of the two dimensions to make it consistent?
> 
> In general, we should match the refresh widget used in the newer google apps. 
> Capping it to the smaller of the two "seems" reasonable to me.

Yeah, they use a threshold invariant to the orientation. V2 of this will have me
more faithfully emulate the look (growing arrows, etc..) of google apps.

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
File content/browser/android/overscroll_refresh_unittest.cc (right):

https://codereview.chromium.org/679493002/diff/140001/content/browser/android...
content/browser/android/overscroll_refresh_unittest.cc:123:
TEST_F(OverscrollRefreshTest, NotTriggeredIfInitialYOffsetIsZero) {
On 2014/11/06 19:25:40, Ted C wrote:
> On 2014/11/06 00:10:37, jdduke wrote:
> > On 2014/10/31 23:52:05, Ted C wrote:
> > > Is[Not]Zero?
> > 
> > Done.
> 
> Really?

No :( I'm a bad person.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
File content/browser/android/overscroll_refresh.cc (right):

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.cc:84: if (layer->parent() != parent)
On 2014/11/06 22:28:48, aelias wrote:
> Please move these two if statements (reparenting and checking for
> layer_tree_host()) above all the other early returns.  I think we should
always
> do this structural stuff even if nondrawable to make this easier to reason
> about.

Done.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.cc:159: bool Update(base::TimeTicks
current_time, bool is_triggered_refresh_active) {
On 2014/11/06 22:28:48, aelias wrote:
> Please rename this to Animate.

Done.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.cc:262:
resource_manager_->GetUIResourceId(kIdleResourceType), size,
On 2014/11/06 19:25:40, Ted C wrote:
> I was under the impression that these all did need to go on their own line
(like
> your previous patch).  Have I been living a lie?
> 
> Does that only apply for function declaration?

I ran "git cl format" and this is what it gave me, and I had the same reaction
as you... Double checking... Hmm, it's still packing arguments (but not the
declaration)... Something's fishy, going to manually reformat to the standard
aligned + line break for each arg...

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.cc:353: if (scroll_consumption_state_
== ENABLED) {
On 2014/11/06 22:28:48, aelias wrote:
> I think this would be more readable as a switch statement, in order of the
enum
> (DISABLED, AWAITING_SCROLL_UPDATE_ACK, ENABLED) and terminating each with
return
> true/false instead of break.

Done.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
File content/browser/android/overscroll_refresh.h (right):

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:51: enum ActivationAllowance {
ALLOW_ACTIVATION, PREVENT_ACTIVATION };
On 2014/11/06 22:28:49, aelias wrote:
> This enum seems like an unnecessary abstraction.  Please just make
> OnScrollUpdateAck take "bool was_consumed"

Hmm, yeah, I had a bool, then I needed a second bool, then a made it an enum,
and then I removed one of the enum states, so yeah, bool it is...

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:72: bool IsActive() const;
On 2014/11/06 22:28:49, aelias wrote:
> Here you use "active" to mean "exists at all" and elsewhere you use the same
> wording to mean "confirmed to cause a refresh".  Could you rename this one to
> "Exists()" maybe?

Yeah, the other |Active()| is prefixed to |IsTriggeredRefreshActive()|, but it
is a bit confusing. What about |IsAlive()|?

Actually, I'm not a huge fan of Exists or Alive, I'll rename the other Active
term to something else.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:75: bool IsPendingScrollUpdateAck()
const;
On 2014/11/06 22:28:48, aelias wrote:
> Please rename to IsAwaitingScrollUpdateAck, I think using the word "waiting"
> makes this easier to follow.

Done.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:83: gfx::Vector2dF scroll_offset_;
On 2014/11/06 22:28:49, aelias wrote:
> I'd prefer this as a "bool scrolled_to_top_" as this is rich information and
> raises the question of why the refresh effect needs it. 

Done.

https://codereview.chromium.org/679493002/diff/160001/content/browser/android...
content/browser/android/overscroll_refresh.h:87: PENDING_SCROLL_UPDATE_ACK,
On 2014/11/06 22:28:48, aelias wrote:
> Please rename to AWAITING_SCROLL_UPDATE_ACK.

Done.

[jdduke (slow), 2014-11-07 18:37:12]

jdduke@chromium.org changed reviewers:
+ sadrul@chromium.org

[jdduke (slow), 2014-11-07 18:37:13]

+sadrul for ui/base/BUILD.gn and ui/base/ui_base.gyp.

[sadrul, 2014-11-07 20:37:01]

said files lgtm

[jdduke (slow), 2014-11-14 17:08:00]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-11-14 17:08:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/679493002/240001

[commit-bot: I haz the power, 2014-11-14 17:19:31]

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

[commit-bot: I haz the power, 2014-11-14 17:19:32]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[jdduke (slow), 2014-11-14 17:42:29]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-11-14 17:42:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/679493002/260001

[commit-bot: I haz the power, 2014-11-14 18:09:41]

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

[commit-bot: I haz the power, 2014-11-14 18:09:44]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[jdduke (slow), 2014-11-14 18:28:00]

jdduke@chromium.org changed reviewers:
+ boliu@chromium.org

[jdduke (slow), 2014-11-14 18:28:01]

+boliu@ for android_webview/ review.

[boliu, 2014-11-14 18:49:24]

https://codereview.chromium.org/679493002/diff/300001/android_webview/java_li...
File android_webview/java_library_common.mk (right):

https://codereview.chromium.org/679493002/diff/300001/android_webview/java_li...
android_webview/java_library_common.mk:66: $(call
intermediates-dir-for,GYP,shared)/enums/system_ui_resource_type_java/org/chromium/ui/SystemUIResourceType.java
\
Following the pattern above, seems you missed
.../ui/*base/android/*SystemUIResourceType.java

[jdduke (slow), 2014-11-14 18:57:54]

https://codereview.chromium.org/679493002/diff/300001/android_webview/java_li...
File android_webview/java_library_common.mk (right):

https://codereview.chromium.org/679493002/diff/300001/android_webview/java_li...
android_webview/java_library_common.mk:66: $(call
intermediates-dir-for,GYP,shared)/enums/system_ui_resource_type_java/org/chromium/ui/SystemUIResourceType.java
\
On 2014/11/14 18:49:24, boliu wrote:
> Following the pattern above, seems you missed
> .../ui/*base/android/*SystemUIResourceType.java

Yeah, so it works as-is, but you're right in should be in the ui/base package.

[jdduke (slow), 2014-11-14 19:05:13]

On 2014/11/14 18:57:54, jdduke wrote:
>
https://codereview.chromium.org/679493002/diff/300001/android_webview/java_li...
> File android_webview/java_library_common.mk (right):
> 
>
https://codereview.chromium.org/679493002/diff/300001/android_webview/java_li...
> android_webview/java_library_common.mk:66: $(call
>
intermediates-dir-for,GYP,shared)/enums/system_ui_resource_type_java/org/chromium/ui/SystemUIResourceType.java
> \
> On 2014/11/14 18:49:24, boliu wrote:
> > Following the pattern above, seems you missed
> > .../ui/*base/android/*SystemUIResourceType.java
> 
> Yeah, so it works as-is, but you're right in should be in the ui/base package.

Done.

[boliu, 2014-11-14 19:07:40]

lgtm

[jdduke (slow), 2014-11-14 19:09:45]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-11-14 19:10:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/679493002/320001

[commit-bot: I haz the power, 2014-11-14 19:15:49]

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

[commit-bot: I haz the power, 2014-11-14 19:15:50]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[jdduke (slow), 2014-11-14 19:20:41]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-11-14 19:21:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/679493002/340001

[commit-bot: I haz the power, 2014-11-14 20:04:33]

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

[commit-bot: I haz the power, 2014-11-14 20:04:35]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[jdduke (slow), 2014-11-14 22:41:54]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-11-14 22:43:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/679493002/380001

[commit-bot: I haz the power, 2014-11-15 00:50:06]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2014-11-15 00:50:39]

Patchset 20 (id:??) landed as
https://crrev.com/9db1b9d8f01028f3badc34a2f4f6ce63fda87247
Cr-Commit-Position: refs/heads/master@{#304320}