Add progressbar to animated doodle

Add progressbar to animated doodle

EnhancedBookmarkLoadingView is moved to widget folder in this CL so that
it could be used by ntp.

BUG=536210
Committed: https://crrev.com/56f0e6326bd0341df31ced7025fba3d500c1371f
Cr-Commit-Position: refs/heads/master@{#357414}

[Ian Wen, 2015-10-12 20:10:06]

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

[Ian Wen, 2015-10-12 20:10:06]

PTAL

[newt (away), 2015-10-13 05:25:00]

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
File chrome/android/java/res/layout/new_tab_page.xml (right):

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
chrome/android/java/res/layout/new_tab_page.xml:50:
<org.chromium.chrome.browser.ntp.LogoView
You could just make the LogoView extend FrameLayout, and avoid this extra
nesting

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
chrome/android/java/res/layout/new_tab_page.xml:60:
<org.chromium.chrome.browser.widget.LoadingView
don't you need to add a ProgressBar inside the LoadingView?

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
File chrome/android/java/src/org/chromium/chrome/browser/widget/LoadingView.java
(right):

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/widget/LoadingView.java:22:
public class LoadingView extends FrameLayout {
I think it makes more sense for this to extend ProgressBar, so that it's more
modular and requires only a single item in the view hierarchy to use. If you
need to put it inside a FrameLayout, fine, go for it, but it doesn't make sense
to tie the FrameLayout aspect to the delayed hide/show logic. That was
convenient for its use in EB, but it should be cleaned up now.

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/widget/LoadingView.java:91:
mStartTime + MINIMUM_ANIMATION_SHOW_TIME_MS - System.currentTimeMillis()));
FYI, this could do funky stuff on leap day or if the user's phone changes time
zones (e.g. the progress spinner could take an hour to hide). System.nanoTime()
should be used in cases like this to avoid those weird edge cases.

[Ian Wen, 2015-10-26 22:45:37]

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
File chrome/android/java/res/layout/new_tab_page.xml (right):

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
chrome/android/java/res/layout/new_tab_page.xml:50:
<org.chromium.chrome.browser.ntp.LogoView
On 2015/10/13 05:24:59, newt (OOO through 10-23) wrote:
> You could just make the LogoView extend FrameLayout, and avoid this extra
> nesting

Done.

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
chrome/android/java/res/layout/new_tab_page.xml:60:
<org.chromium.chrome.browser.widget.LoadingView
On 2015/10/13 05:24:59, newt (OOO through 10-23) wrote:
> don't you need to add a ProgressBar inside the LoadingView?

Done.

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
File chrome/android/java/src/org/chromium/chrome/browser/widget/LoadingView.java
(right):

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/widget/LoadingView.java:22:
public class LoadingView extends FrameLayout {
On 2015/10/13 05:25:00, newt (OOO through 10-23) wrote:
> I think it makes more sense for this to extend ProgressBar, so that it's more
> modular and requires only a single item in the view hierarchy to use. If you
> need to put it inside a FrameLayout, fine, go for it, but it doesn't make
sense
> to tie the FrameLayout aspect to the delayed hide/show logic. That was
> convenient for its use in EB, but it should be cleaned up now.

Done.

https://chromiumcodereview.appspot.com/1398423002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/widget/LoadingView.java:91:
mStartTime + MINIMUM_ANIMATION_SHOW_TIME_MS - System.currentTimeMillis()));
On 2015/10/13 05:25:00, newt (OOO through 10-23) wrote:
> FYI, this could do funky stuff on leap day or if the user's phone changes time
> zones (e.g. the progress spinner could take an hour to hide).
System.nanoTime()
> should be used in cases like this to avoid those weird edge cases.

Done.

[newt (away), 2015-10-27 02:26:03]

https://chromiumcodereview.appspot.com/1398423002/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java
(right):

https://chromiumcodereview.appspot.com/1398423002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:320: if
(!isPlaying) mLoadingView.showLoadingUI();
Won't this cause the loading view to be shown when you click on a regular
non-animated doodle? I think a better solution is to add a method
showLoadingView() to this class and have the NewTabPage call showLoadingView()
when it starts to download the logo.

https://chromiumcodereview.appspot.com/1398423002/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/widget/LoadingView.java
(right):

https://chromiumcodereview.appspot.com/1398423002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/widget/LoadingView.java:71:
setVisibility(GONE);
what's the reason for removing the call to setAlpha() in this method and the
call to setVisibility(GONE) below?

[Ian Wen, 2015-10-27 18:10:12]

https://chromiumcodereview.appspot.com/1398423002/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java
(right):

https://chromiumcodereview.appspot.com/1398423002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:320: if
(!isPlaying) mLoadingView.showLoadingUI();
On 2015/10/27 02:26:03, newt wrote:
> Won't this cause the loading view to be shown when you click on a regular
> non-animated doodle? I think a better solution is to add a method
> showLoadingView() to this class and have the NewTabPage call showLoadingView()
> when it starts to download the logo.

Done.

https://chromiumcodereview.appspot.com/1398423002/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/widget/LoadingView.java
(right):

https://chromiumcodereview.appspot.com/1398423002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/widget/LoadingView.java:71:
setVisibility(GONE);
On 2015/10/27 02:26:03, newt wrote:
> what's the reason for removing the call to setAlpha() in this method and the
> call to setVisibility(GONE) below?

It is because we need to wait for 500ms and then fade in the view. The previous
setVisibility() was called on the framelayout, and actually it does not show
anything.

[newt (away), 2015-10-30 21:43:12]

lgtm. Sorry I missed this earlier.

https://codereview.chromium.org/1398423002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/layout/eb_main_content.xml (right):

https://codereview.chromium.org/1398423002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/eb_main_content.xml:43: android:visibility="gone"
>
nit: since this doesn't contain any child views, just make this tag
self-closing, i.e. end with "/>" instead of
"</org.chromium.chrome.browser.widget.LoadingView>"

[Ian Wen, 2015-11-02 19:24:46]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-11-02 19:24:53]

The CQ bit was unchecked by ianwen@chromium.org

[Ian Wen, 2015-11-02 19:25:56]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-11-02 19:25:56]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1398423002/#ps80001
(title: "xml nit")

[commit-bot: I haz the power, 2015-11-02 19:28:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1398423002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1398423002/80001

[commit-bot: I haz the power, 2015-11-02 20:15:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-02 20:16:27]

Patchset 5 (id:??) landed as
https://crrev.com/56f0e6326bd0341df31ced7025fba3d500c1371f
Cr-Commit-Position: refs/heads/master@{#357414}