[Andorid] Migrate to ClipDrawable progress bar.

[Andorid] Migrate to ClipDrawable progress bar.

Using ClipDrawable progress bar helps future changes for
http://crbug.com/461148 as we only need pass to color parameters.

Also it has minor performance benefit.

BUG=466140, 455891, 461148
Committed: https://crrev.com/ee231168bb231ee117485ecd1e7cb16a1f632bae
Cr-Commit-Position: refs/heads/master@{#339598}

[Kibeom Kim (inactive), 2015-07-07 22:03:56]

kkimlabs@chromium.org changed reviewers:
+ dtrainor@chromium.org, tedchoc@chromium.org

[Kibeom Kim (inactive), 2015-07-07 22:07:50]

I need to debug ToolbarProgressBarTest but other than that I think patchset 20
is ready for review.

This is not necessary for fixing crbug.com/461148 , since I can just stretch the
9-patch drawable on compositor side but just having color API is simpler and
cleaner, and also has some minor performance benefit so I did it.

[Kibeom Kim (inactive), 2015-07-07 23:43:25]

Fixed ToolbarProgressBarTest

[David Trainor- moved to gerrit, 2015-07-08 17:32:47]

A few small nits/issues but in general lgtm.

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java
(right):

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:69:
super.setImageDrawable(new ClipDrawable(new ColorDrawable(mProgressBarColor),
Gravity.START,
No need for super.  Same with below?

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:153:
if (mListener != null && (visibility == VISIBLE) != (getVisibility() ==
VISIBLE)) {
Do you have to cache getVisibility() before calling super for this check to
work?

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
File
chrome/android/javatests_shell/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java
(right):

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:73:
CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
I might be overly worried, but since we have an animation running this is kind
of relying on the fact that the default timeout for polling is greater than the
animation duration.  That seems slightly scary could we be more explicit about
our timeout here since we *know* it's going to take X ms to at least animate? 
IE: default time out + X or something?  Same below.  wdyt?

[Ted C, 2015-07-08 18:17:34]

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
(right):

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:486:
mProgressBar.setProgress(mProgressBar.getMax() / 100 * progress);
add a method like set[Load/Toolbar]Progress on the progress bar that isolates
the need for this to do the max calculations.  That seems like internal state
and we should really only be operating on 0 <-> 1.0.  I actually think getMax()
shouldn't need to be exposed and should be handled entirely within the progress
bar, but I do not know the implications of that.

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java
(right):

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:29:
private Rect mDirtyBound = new Rect();
final?

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:32:
* Interface for listening drawing validation signals.
listening [to] drawing [in]validation

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:42:
public ClipDrawableProgressBar(Context context) {
do you actually need the three constructors?

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:86:
* @param progress The new progress, between 0 and getMax().
add asserts that this is done correctly

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:123:
public int getProgressBarBackgroundColor() {
javadoc all public methods

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:142:
public void setProgressChangeListener(InvalidationListener listener) {
the comment and name of this method should be changed since it is tracking
invalidations not progress changes.

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java
(right):

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:32:
setAlpha(getProgress() > 0 ? 1.0f : 0.0f);
why set alpha instead of setting invisible?

Is alpha 0 as efficient as INVISIBLE (i'd be surprised if they were)?

https://chromiumcodereview.appspot.com/1170843002/diff/400001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:39:
animate().alpha(progress > 0 ? 1.0f : 0.0f).setDuration(SHOW_HIDE_DURATION_MS);
hmm...won't subsequent calls to this always cancel out the previous one (further
extending the duration)?

Does this do nothing if the value is already the final value?  Or does it tick
off useless alpha updates for the duration anyway?

We should also make sure that we don't get multiple calls with progress equal to
max/1 such that the progress stays much longer than it should since this would
cancel the clearing animation.  Although that might have already been the case
before, but it seems like we shouldn't be doing that.

If we get a call to set the progress to max without it ever decreasing, we
shouldn't reshow the progress bar.

[Kibeom Kim (inactive), 2015-07-13 13:34:09]

Also fixed alpha blending accumulation issue that I missed.

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
(right):

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:486:
mProgressBar.setProgress(mProgressBar.getMax() / 100 * progress);
On 2015/07/08 18:17:33, Ted C wrote:
> add a method like set[Load/Toolbar]Progress on the progress bar that isolates
> the need for this to do the max calculations.  That seems like internal state
> and we should really only be operating on 0 <-> 1.0.  I actually think
getMax()
> shouldn't need to be exposed and should be handled entirely within the
progress
> bar, but I do not know the implications of that.

Done.

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java
(right):

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:29:
private Rect mDirtyBound = new Rect();
On 2015/07/08 18:17:34, Ted C wrote:
> final?

Done.

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:32:
* Interface for listening drawing validation signals.
On 2015/07/08 18:17:33, Ted C wrote:
> listening [to] drawing [in]validation

Done.

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:42:
public ClipDrawableProgressBar(Context context) {
On 2015/07/08 18:17:34, Ted C wrote:
> do you actually need the three constructors?

Done.

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:69:
super.setImageDrawable(new ClipDrawable(new ColorDrawable(mProgressBarColor),
Gravity.START,
On 2015/07/08 17:32:47, David Trainor wrote:
> No need for super.  Same with below?

Done.

Context: I was thinking disabling setImageDrawable() setBackgroundColor() by
|assert false'| in this class since we don't want people to call them.

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:86:
* @param progress The new progress, between 0 and getMax().
On 2015/07/08 18:17:34, Ted C wrote:
> add asserts that this is done correctly

Done.

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:123:
public int getProgressBarBackgroundColor() {
On 2015/07/08 18:17:33, Ted C wrote:
> javadoc all public methods

Done.

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:142:
public void setProgressChangeListener(InvalidationListener listener) {
On 2015/07/08 18:17:33, Ted C wrote:
> the comment and name of this method should be changed since it is tracking
> invalidations not progress changes.

Done.

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:153:
if (mListener != null && (visibility == VISIBLE) != (getVisibility() ==
VISIBLE)) {
On 2015/07/08 17:32:47, David Trainor wrote:
> Do you have to cache getVisibility() before calling super for this check to
> work?

Done. Thanks for catching!

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java
(right):

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:32:
setAlpha(getProgress() > 0 ? 1.0f : 0.0f);
On 2015/07/08 18:17:34, Ted C wrote:
> why set alpha instead of setting invisible?
> 
> Is alpha 0 as efficient as INVISIBLE (i'd be surprised if they were)?

Done. (handled inside ClipDrawableProgressBar)

I've read "Note that setting alpha to a translucent value (0<alpha<1) can have
significant performance implications" from
http://developer.android.com/reference/android/view/View.html#setAlpha(float).
So I assumed setting alpha 0 should be fine, but I don't know for sure. Though,
I've found some code path in ViewGroup that INVISIBLE seems at least slightly
more efficient...

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:39:
animate().alpha(progress > 0 ? 1.0f : 0.0f).setDuration(SHOW_HIDE_DURATION_MS);
On 2015/07/08 18:17:34, Ted C wrote:
> hmm...won't subsequent calls to this always cancel out the previous one
(further
> extending the duration)?
> 
Yeah right.

> Does this do nothing if the value is already the final value?  Or does it tick
> off useless alpha updates for the duration anyway?
> 
I'm not sure on the rendering code path, but at least there are some Android
animation code that would run unnecessarily. Done.

> We should also make sure that we don't get multiple calls with progress equal
to
> max/1 such that the progress stays much longer than it should since this would
> cancel the clearing animation.  Although that might have already been the case
> before, but it seems like we shouldn't be doing that.
> 
> If we get a call to set the progress to max without it ever decreasing, we
> shouldn't reshow the progress bar.

I noticed that indeed we can get multiple 100 progress calls, and the last one
is caused by page load event. So I think we should just listen to page
start/load event to for showing/hiding. Done.

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/javates...
File
chrome/android/javatests_shell/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java
(right):

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/javates...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:73:
CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
On 2015/07/08 17:32:47, David Trainor wrote:
> I might be overly worried, but since we have an animation running this is kind
> of relying on the fact that the default timeout for polling is greater than
the
> animation duration.  That seems slightly scary could we be more explicit about
> our timeout here since we *know* it's going to take X ms to at least animate? 
> IE: default time out + X or something?  Same below.  wdyt?

Done.

[Kibeom Kim (inactive), 2015-07-13 19:49:02]

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/javates...
File
chrome/android/javatests_shell/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java
(right):

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/javates...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:73:
CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
On 2015/07/13 13:34:09, Kibeom Kim wrote:
> On 2015/07/08 17:32:47, David Trainor wrote:
> > I might be overly worried, but since we have an animation running this is
kind
> > of relying on the fact that the default timeout for polling is greater than
> the
> > animation duration.  That seems slightly scary could we be more explicit
about
> > our timeout here since we *know* it's going to take X ms to at least
animate? 
> > IE: default time out + X or something?  Same below.  wdyt?
> 
> Done.

Actually, the new patchset doesn't address the concern :( I'll think about how
to incorporate that.

[Kibeom Kim (inactive), 2015-07-13 20:27:17]

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

[commit-bot: I haz the power, 2015-07-13 20:28:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1170843002/460001

[Kibeom Kim (inactive), 2015-07-13 20:32:49]

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/javates...
File
chrome/android/javatests_shell/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java
(right):

https://codereview.chromium.org/1170843002/diff/400001/chrome/android/javates...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:73:
CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
On 2015/07/13 19:49:02, Kibeom Kim wrote:
> On 2015/07/13 13:34:09, Kibeom Kim wrote:
> > On 2015/07/08 17:32:47, David Trainor wrote:
> > > I might be overly worried, but since we have an animation running this is
> kind
> > > of relying on the fact that the default timeout for polling is greater
than
> > the
> > > animation duration.  That seems slightly scary could we be more explicit
> about
> > > our timeout here since we *know* it's going to take X ms to at least
> animate? 
> > > IE: default time out + X or something?  Same below.  wdyt?
> > 
> > Done.
> 
> Actually, the new patchset doesn't address the concern :( I'll think about how
> to incorporate that.

Done.

(I was on the fence putting the duration constants in 
 1. test file: no memory waste
 2. progress bar file: better co-location as duration is implementation detail,
less chance to mistake when changing the implementation.
 and marginally preferred 2.)

[commit-bot: I haz the power, 2015-07-13 21:19:53]

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

[commit-bot: I haz the power, 2015-07-13 21:19:55]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Ted C, 2015-07-15 02:41:12]

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java:157:
mProgressView.getAlphaBound(mAlphaBound);
getAlphaBound[s]

or maybe

getAlphaDrawRegion

or

getAlphaRegion

It's a hard thing to convey, so I don't know if there is a "correct thing here".

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(left):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:967:
if (currentTab.getProgress() == 100 || currentTab.isShowingInterstitialPage()) {
what was the previous behavior if you were on a tab that was loaded to 50% and
you switched to a 100% loaded tab?  Did the progress bar just disappear or did
it just to 100% and fade away?

What happens now?  To me it looks like it will do the latter and I don't know if
that differs from the previous behavior.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:335:
finishLoadProgress();
both failed and crash used to reset the progress to 0.  Now this just hides it
right?

Does that look the same?  Intentional?

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:356:
updateTabLoadingState(true);
this used to call with false...intentional?

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:59:
if (attrs != null) {
shouldn't we assert that the attrs are non-null?

And don't we need both of the values to be present?

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:62:
mProgressBarColor =
a.getColor(R.styleable.ClipDrawableProgressBar_progressBarColor, 0);
along with the comment about, use TRANSPARENT instead of 0 to make it easier to
see.

add an assert that progressBarColor is some other value.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:69:
setBackgroundColor(mBackgroundColor);
if background color is transparent, should we be setting it at all?  Does that
cause overdraw (I would hope android would treat this as no need to draw).

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:88:

early exit if mProgress == progress?

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:125:
public void setProgressBarBackgroundColor(int color) {
I would just override setBackgroundColor explicitly as it will prevent callers
from unexpectedly calling the wrong thing.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:199:
public void setVisibility(int visibility) {
probably worth adding a javadoc that this value will not necessarily be
respected

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:51:
mAlphaAnimator.setTimeListener(new TimeListener() {
What is the benefit of the alpha animator?

Doesn't that lose us the ability to set an interpolator?

Wondering why this animation is different than other places?  Is it with the
goal that a total animation from 0 -> 1 should take X seconds?  And if you only
need to animate from 0.5 -> 1 then it should take 1/2X?

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:84:
mIsStarted = false;
Is page load finished always going to happen right after progress hits 100%?  We
aren't delaying the hiding more by having this be driven by something other than
set progress are we?

[Kibeom Kim (inactive), 2015-07-15 11:22:46]

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java:157:
mProgressView.getAlphaBound(mAlphaBound);
On 2015/07/15 02:41:12, Ted C wrote:
> getAlphaBound[s]
> 
> or maybe
> 
> getAlphaDrawRegion
> 
> or
> 
> getAlphaRegion
> 
> It's a hard thing to convey, so I don't know if there is a "correct thing
here".

Done. getAlphaDrawRegion sounds better..

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(left):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:967:
if (currentTab.getProgress() == 100 || currentTab.isShowingInterstitialPage()) {
On 2015/07/15 02:41:12, Ted C wrote:
> what was the previous behavior if you were on a tab that was loaded to 50% and
> you switched to a 100% loaded tab?  Did the progress bar just disappear or did
> it just to 100% and fade away?
> 
> What happens now?  To me it looks like it will do the latter and I don't know
if
> that differs from the previous behavior.

Yeah right, thanks for catching this.

I think the existing code was fading out too, but only background so it must be
hard to notice.  Done.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:335:
finishLoadProgress();
On 2015/07/15 02:41:12, Ted C wrote:
> both failed and crash used to reset the progress to 0.  Now this just hides it
> right?
> 
> Does that look the same?  Intentional?

It looks different. This is fairly subjective, but I thought setting 0 is
double-hiding since it was fading out the background anyways. I think we should
either hide suddenly or fade out smoothly. Mixing them (hiding progress bar
suddenly while fading out its background) sounds little strange to me. (but
again I guess it's subjective)

Added an argument to control animation. Done.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:356:
updateTabLoadingState(true);
On 2015/07/15 02:41:12, Ted C wrote:
> this used to call with false...intentional?

Yes. Existing code was:

private void onTabCrash() {
    updateTabLoadingState(false, false);    
    updateLoadProgressInternal(0);    
    updateButtonStatus();
}

private void updateTabLoadingState(boolean isLoading, boolean updateUrl) {
    Tab currentTab = mToolbarModel.getTab();
    mLocationBar.updateLoadingState(updateUrl);
    if (isLoading) updateLoadProgress(currentTab.getProgress());
    if (updateUrl) updateButtonStatus();
}

So if I set |updateUrl| to be true, then updateButtonStatus(); is called
anyways.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:59:
if (attrs != null) {
On 2015/07/15 02:41:12, Ted C wrote:
> shouldn't we assert that the attrs are non-null?
> 
> And don't we need both of the values to be present?

Done. (I think fallback transparent background color should be fine even if user
doesn't specify?)

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:62:
mProgressBarColor =
a.getColor(R.styleable.ClipDrawableProgressBar_progressBarColor, 0);
On 2015/07/15 02:41:12, Ted C wrote:
> along with the comment about, use TRANSPARENT instead of 0 to make it easier
to
> see.
> 
> add an assert that progressBarColor is some other value.

Done.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:69:
setBackgroundColor(mBackgroundColor);
On 2015/07/15 02:41:12, Ted C wrote:
> if background color is transparent, should we be setting it at all?  Does that
> cause overdraw (I would hope android would treat this as no need to draw).

Hmm, from what I've read, it still sets ColorDrawable and I don't see
optimization code there. Done. (setBackgroundColor function below)

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:88:

On 2015/07/15 02:41:12, Ted C wrote:
> early exit if mProgress == progress?

Done.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:125:
public void setProgressBarBackgroundColor(int color) {
On 2015/07/15 02:41:12, Ted C wrote:
> I would just override setBackgroundColor explicitly as it will prevent callers
> from unexpectedly calling the wrong thing.

Done.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:199:
public void setVisibility(int visibility) {
On 2015/07/15 02:41:12, Ted C wrote:
> probably worth adding a javadoc that this value will not necessarily be
> respected

Done.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:51:
mAlphaAnimator.setTimeListener(new TimeListener() {
On 2015/07/15 02:41:12, Ted C wrote:
> What is the benefit of the alpha animator?
> 
> Doesn't that lose us the ability to set an interpolator?
> 
> Wondering why this animation is different than other places?  Is it with the
> goal that a total animation from 0 -> 1 should take X seconds?  And if you
only
> need to animate from 0.5 -> 1 then it should take 1/2X?

I like TimeAnimator in general, I don't know why I did that here, I must be
drunk. Done.

Though, I think interpolation doesn't matter as the animation is too short to
notice the difference. Set it back anyways.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:84:
mIsStarted = false;
On 2015/07/15 02:41:12, Ted C wrote:
> Is page load finished always going to happen right after progress hits 100%? 
We
> aren't delaying the hiding more by having this be driven by something other
than
> set progress are we?

I think, regardless of happening right after 100% or not, page load finished
event is the correct place to finish progress. At least it seems desktop spinner
has done in that way.
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

And for the question, I don't know for sure :/ though it doesn't seem guaranteed
to me.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

[Ted C, 2015-07-15 17:42:59]

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:356:
updateTabLoadingState(true);
On 2015/07/15 11:22:45, Kibeom Kim wrote:
> On 2015/07/15 02:41:12, Ted C wrote:
> > this used to call with false...intentional?
> 
> Yes. Existing code was:
> 
> private void onTabCrash() {
>     updateTabLoadingState(false, false);    
>     updateLoadProgressInternal(0);    
>     updateButtonStatus();
> }
> 
> private void updateTabLoadingState(boolean isLoading, boolean updateUrl) {
>     Tab currentTab = mToolbarModel.getTab();
>     mLocationBar.updateLoadingState(updateUrl);
>     if (isLoading) updateLoadProgress(currentTab.getProgress());
>     if (updateUrl) updateButtonStatus();
> }
> 
> So if I set |updateUrl| to be true, then updateButtonStatus(); is called
> anyways.

Correct, but previously it was calling updateLoadingState on the locationbar
with false.  This changes that to true and will result in a URL update.

That is probably fine, but it seems to have changed.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:59:
if (attrs != null) {
On 2015/07/15 11:22:45, Kibeom Kim wrote:
> On 2015/07/15 02:41:12, Ted C wrote:
> > shouldn't we assert that the attrs are non-null?
> > 
> > And don't we need both of the values to be present?
> 
> Done. (I think fallback transparent background color should be fine even if
user
> doesn't specify?)

Agreed, a transparent background is fine.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:84:
mIsStarted = false;
On 2015/07/15 11:22:46, Kibeom Kim wrote:
> On 2015/07/15 02:41:12, Ted C wrote:
> > Is page load finished always going to happen right after progress hits 100%?

> We
> > aren't delaying the hiding more by having this be driven by something other
> than
> > set progress are we?
> 
> I think, regardless of happening right after 100% or not, page load finished
> event is the correct place to finish progress. At least it seems desktop
spinner
> has done in that way.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

Not sure I agree with the "correctness" assertion.

The desktop spinner don't have a finished state while the progress bar does.  If
they rotate a bit extra than the user won't really notice.  If our blue bar
stays on the screen for a couple extra seconds then it will be much more
noticeable to the user.

> 
> And for the question, I don't know for sure :/ though it doesn't seem
guaranteed
> to me.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

[Kibeom Kim (inactive), 2015-07-16 04:45:48]

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:356:
updateTabLoadingState(true);
On 2015/07/15 17:42:59, Ted C wrote:
> On 2015/07/15 11:22:45, Kibeom Kim wrote:
> > On 2015/07/15 02:41:12, Ted C wrote:
> > > this used to call with false...intentional?
> > 
> > Yes. Existing code was:
> > 
> > private void onTabCrash() {
> >     updateTabLoadingState(false, false);    
> >     updateLoadProgressInternal(0);    
> >     updateButtonStatus();
> > }
> > 
> > private void updateTabLoadingState(boolean isLoading, boolean updateUrl) {
> >     Tab currentTab = mToolbarModel.getTab();
> >     mLocationBar.updateLoadingState(updateUrl);
> >     if (isLoading) updateLoadProgress(currentTab.getProgress());
> >     if (updateUrl) updateButtonStatus();
> > }
> > 
> > So if I set |updateUrl| to be true, then updateButtonStatus(); is called
> > anyways.
> 
> Correct, but previously it was calling updateLoadingState on the locationbar
> with false.  This changes that to true and will result in a URL update.
> 
> That is probably fine, but it seems to have changed.

Oh, yeah you're right. Done.

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java
(right):

https://codereview.chromium.org/1170843002/diff/460001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:84:
mIsStarted = false;
On 2015/07/15 17:42:59, Ted C wrote:
> On 2015/07/15 11:22:46, Kibeom Kim wrote:
> > On 2015/07/15 02:41:12, Ted C wrote:
> > > Is page load finished always going to happen right after progress hits
100%?
> 
> > We
> > > aren't delaying the hiding more by having this be driven by something
other
> > than
> > > set progress are we?
> > 
> > I think, regardless of happening right after 100% or not, page load finished
> > event is the correct place to finish progress. At least it seems desktop
> spinner
> > has done in that way.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
> 
> Not sure I agree with the "correctness" assertion.
> 
> The desktop spinner don't have a finished state while the progress bar does. 
If
> they rotate a bit extra than the user won't really notice.  If our blue bar
> stays on the screen for a couple extra seconds then it will be much more
> noticeable to the user.
> 
> > 
> > And for the question, I don't know for sure :/ though it doesn't seem
> guaranteed
> > to me.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> 

Investigation so far:

In general, progress can be updated from these two functions.
1.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
2.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

1 is coupled with DidFinishLoad event
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
. But for 2, seems like it only tracks total downloaded bytes % per estimation.
My impression is that it's not a sufficient condition for DidFinishLoad event to
be fired as DidFinishLoad has to pass these additional checks.

-
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
-
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...


Notes:
- If 100% progress is happened by 2, not 1, Frame#m_isLoading variable will be
still true.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
setIsLoading(false); is only called in 1. 
- Though, I tried many pages locally, but couldn't find an example that the
progress bar reaches 100% independent from DidFinishLoad event.

[Ted C, 2015-07-17 23:42:44]

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java
(right):

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java:140:
private final Rect mAlphaBound = new Rect();
update the name of this variable

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
(right):

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:496:
protected void finishLoadProgress(boolean fadeOut) {
should we use delayed here to be consistent with the sub class?  Also, javadoc.

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java
(right):

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:56:
CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
assertTrue here?

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:64:
CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
assertTrue?

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:70:
CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL + ToolbarProgressBar.HIDING_DURATION_MS,
I would have the ability to overwrite these delays for tests to make them run
faster.

Also the indenting here is really odd.

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:83:
CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
assertTrue and all others.

[Kibeom Kim (inactive), 2015-07-19 08:44:40]

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

[commit-bot: I haz the power, 2015-07-19 08:44:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1170843002/520001

[Kibeom Kim (inactive), 2015-07-19 08:48:55]

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java
(right):

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java:140:
private final Rect mAlphaBound = new Rect();
On 2015/07/17 23:42:44, Ted C wrote:
> update the name of this variable

Done.

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
(right):

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:496:
protected void finishLoadProgress(boolean fadeOut) {
On 2015/07/17 23:42:44, Ted C wrote:
> should we use delayed here to be consistent with the sub class?  Also,
javadoc.

Done.

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java
(right):

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:56:
CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
On 2015/07/17 23:42:44, Ted C wrote:
> assertTrue here?

Done.

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:64:
CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
On 2015/07/17 23:42:44, Ted C wrote:
> assertTrue?

Done.

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:70:
CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL + ToolbarProgressBar.HIDING_DURATION_MS,
On 2015/07/17 23:42:44, Ted C wrote:
> I would have the ability to overwrite these delays for tests to make them run
> faster.
> 
> Also the indenting here is really odd.

Sorry I don't understand the overwriting delays suggestion? But anyways, changed
from polling, to listening animation end event. I think this is better.

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:83:
CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
On 2015/07/17 23:42:44, Ted C wrote:
> assertTrue and all others.

Done.

[Ted C, 2015-07-20 18:23:45]

lgtm

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java
(right):

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:70:
CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL + ToolbarProgressBar.HIDING_DURATION_MS,
On 2015/07/19 08:48:55, Kibeom Kim wrote:
> On 2015/07/17 23:42:44, Ted C wrote:
> > I would have the ability to overwrite these delays for tests to make them
run
> > faster.
> > 
> > Also the indenting here is really odd.
> 
> Sorry I don't understand the overwriting delays suggestion? But anyways,
changed
> from polling, to listening animation end event. I think this is better.

Sorry for being unclear.  But I meant have a method that changes the animation
durations for the toolbar progress bar.  Then in tests, it can be 10ms instead
of the 200ms (or whatever) it is for normal code.

I'm just trying to make sure these tests aren't slower than they need to be.

https://codereview.chromium.org/1170843002/diff/540001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java
(right):

https://codereview.chromium.org/1170843002/diff/540001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:22:

unnecessary extra blank line?

[Kibeom Kim (inactive), 2015-07-20 19:09:12]

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java
(right):

https://codereview.chromium.org/1170843002/diff/500001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:70:
CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL + ToolbarProgressBar.HIDING_DURATION_MS,
On 2015/07/20 18:23:45, Ted C wrote:
> On 2015/07/19 08:48:55, Kibeom Kim wrote:
> > On 2015/07/17 23:42:44, Ted C wrote:
> > > I would have the ability to overwrite these delays for tests to make them
> run
> > > faster.
> > > 
> > > Also the indenting here is really odd.
> > 
> > Sorry I don't understand the overwriting delays suggestion? But anyways,
> changed
> > from polling, to listening animation end event. I think this is better.
> 
> Sorry for being unclear.  But I meant have a method that changes the animation
> durations for the toolbar progress bar.  Then in tests, it can be 10ms instead
> of the 200ms (or whatever) it is for normal code.
> 
> I'm just trying to make sure these tests aren't slower than they need to be.

Done.

https://codereview.chromium.org/1170843002/diff/540001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java
(right):

https://codereview.chromium.org/1170843002/diff/540001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ToolbarProgressBarTest.java:22:

On 2015/07/20 18:23:45, Ted C wrote:
> unnecessary extra blank line?

Done.

[Kibeom Kim (inactive), 2015-07-20 19:09:18]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-07-20 19:09:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1170843002/#ps560001
(title: " ")

[commit-bot: I haz the power, 2015-07-20 19:09:34]

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

[commit-bot: I haz the power, 2015-07-20 19:20:40]

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

[commit-bot: I haz the power, 2015-07-20 19:20:41]

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

[Kibeom Kim (inactive), 2015-07-20 19:46:11]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-07-20 19:46:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1170843002/#ps580001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-20 19:47:13]

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

[commit-bot: I haz the power, 2015-07-20 19:56:30]

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

[commit-bot: I haz the power, 2015-07-20 19:56:31]

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

[Kibeom Kim (inactive), 2015-07-20 20:19:22]

kkimlabs@chromium.org changed reviewers:
+ newt@chromium.org

[Kibeom Kim (inactive), 2015-07-20 20:19:23]

newt@: ptal ApiCompatibilityUtils.java (simple)

[newt (away), 2015-07-20 20:24:06]

https://codereview.chromium.org/1170843002/diff/600001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java (right):

https://codereview.chromium.org/1170843002/diff/600001/base/android/java/src/...
base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:66: public
static int getLayoutDirection(View view) {
You can use ViewCompat.getLayoutDirection() instead of adding this. (unless you
need to call this method from ui or content code, where ViewCompat can't be
used)

[Kibeom Kim (inactive), 2015-07-20 20:29:55]

https://codereview.chromium.org/1170843002/diff/600001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java (right):

https://codereview.chromium.org/1170843002/diff/600001/base/android/java/src/...
base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:66: public
static int getLayoutDirection(View view) {
On 2015/07/20 20:24:06, newt wrote:
> You can use ViewCompat.getLayoutDirection() instead of adding this. (unless
you
> need to call this method from ui or content code, where ViewCompat can't be
> used)

Done.

[Kibeom Kim (inactive), 2015-07-20 20:30:08]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-07-20 20:30:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1170843002/#ps620001
(title: "remove unnecessary ApiCompatibilityUtils#getLayoutDirection")

[commit-bot: I haz the power, 2015-07-20 20:30:28]

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

[commit-bot: I haz the power, 2015-07-20 21:54:51]

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

[commit-bot: I haz the power, 2015-07-20 21:54:52]

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

[Kibeom Kim (inactive), 2015-07-20 22:25:56]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-07-20 22:25:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1170843002/#ps640001
(title: "suppress findbug warnings for test")

[commit-bot: I haz the power, 2015-07-20 22:27:13]

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

[Kibeom Kim (inactive), 2015-07-21 01:02:29]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-07-21 01:02:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1170843002/#ps660001
(title: "fix ChromeShell tests")

[commit-bot: I haz the power, 2015-07-21 01:03:20]

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

[Kibeom Kim (inactive), 2015-07-21 01:03:23]

The CQ bit was unchecked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-07-21 01:10:44]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-07-21 01:10:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1170843002/#ps670001
(title: "ChromeShell test fix 2")

[commit-bot: I haz the power, 2015-07-21 01:11:07]

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

[commit-bot: I haz the power, 2015-07-21 02:16:52]

Committed patchset #35 (id:670001)

[commit-bot: I haz the power, 2015-07-21 02:18:52]

Patchset 35 (id:??) landed as
https://crrev.com/ee231168bb231ee117485ecd1e7cb16a1f632bae
Cr-Commit-Position: refs/heads/master@{#339598}

[Kibeom Kim (inactive), 2015-07-21 20:36:41]

A revert of this CL (patchset #35 id:670001) has been created in
https://codereview.chromium.org/1249793003/ by kkimlabs@chromium.org.

The reason for reverting is: Chrome Shell tests flaky.
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests

I think I know where I made a mistake and it should be a simple few line fix but
to make sure....