📰 Add a spinner to indicate content is loading

[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/7zp8HVi6BKL2VRucA

BUG=624338
Committed: https://crrev.com/a15325bc18fbc76ed1d535a352cf3a9eda4c2098
Cr-Commit-Position: refs/heads/master@{#409192}

[dgn, 2016-07-27 17:26:58]

Description was changed from

==========
[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/F87X6syhxZi53qaa8

BUG=627488
==========

to

==========
[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/F87X6syhxZi53qaa8

BUG=627488
==========

[dgn, 2016-07-27 17:26:58]

dgn@chromium.org changed reviewers:
+ bauerb@chromium.org, mvanouwerkerk@chromium.org

[dgn, 2016-07-27 17:31:22]

dgn@chromium.org changed reviewers:
+ tedchoc@chromium.org

[dgn, 2016-07-27 17:31:23]

tedchoc@chromium.org: Please review changes in 3p/android_swipe_refresh

bauerb@, mvanouwerkerk@: Please review changes in c/b/ntp/

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java:5:
package org.chromium.chrome.browser.ntp.cards;
Should this go in chrome.widgets instead?

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:210:
super(0, 0, 0);
a bit of a hack, because we currently assume that StatusListItem will be
displayed as cards. I could add an intermediate parent class but I don't think
it's worth it.

[PEConn, 2016-07-27 19:21:39]

peconn@chromium.org changed reviewers:
+ peconn@chromium.org

[PEConn, 2016-07-27 19:21:40]

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java:29:
public LoadingIndicatorView(Context context) {
nit: javadoc on public methods.

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java:40:
mProgressDrawable.setBackgroundColor(asInt(R.color.ntp_material_design_bg));
Can this part be done in XML? It seems to be just configuration.

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:189:
if (viewType == NewTabPageListItem.VIEW_TYPE_ABOVE_THE_FOLD) {
Is there any reason this whole thing isn't a switch statement?

[Ted C, 2016-07-27 21:28:06]

On 2016/07/27 19:21:40, PEConn1 wrote:
>
https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java
> (right):
> 
>
https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java:29:
> public LoadingIndicatorView(Context context) {
> nit: javadoc on public methods.
> 
>
https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java:40:
> mProgressDrawable.setBackgroundColor(asInt(R.color.ntp_material_design_bg));
> Can this part be done in XML? It seems to be just configuration.
> 
>
https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
> (right):
> 
>
https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:189:
> if (viewType == NewTabPageListItem.VIEW_TYPE_ABOVE_THE_FOLD) {
> Is there any reason this whole thing isn't a switch statement?

android_swipe_refresh - lgtm

[dgn, 2016-07-28 08:59:56]

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java:40:
mProgressDrawable.setBackgroundColor(asInt(R.color.ntp_material_design_bg));
On 2016/07/27 19:21:39, PEConn1 wrote:
> Can this part be done in XML? It seems to be just configuration.

The existing constructor for MaterialProgressDrawable is not compatible with
instantiation from XML files, and half of the setters here are custom anyway.

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:189:
if (viewType == NewTabPageListItem.VIEW_TYPE_ABOVE_THE_FOLD) {
On 2016/07/27 19:21:40, PEConn1 wrote:
> Is there any reason this whole thing isn't a switch statement?

I think the code from the documentation had that form and we just kept it.

[Bernhard Bauer, 2016-07-28 09:02:57]

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java:58:
postDelayed(mShowSpinnerRunnable, 500);
Extract the 500 to a constant?

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java:61:
private int asInt(@ColorRes int colorId) {
getColorAsInt()?

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:325:
((StatusListItem.LoadingViewHolder) holder).hide();
It might be worth making this a method on NewTabPageViewHolder with an empty
default implementation.

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageListItem.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageListItem.java:55:
public static final int VIEW_TYPE_LOADING = 6;
Javadoc?

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:155:
if (lastCard == null) lastCard = findLoadingIndicator();
Instead of adding an additional method, could we change findLastCard() to do
what we really want? (Which presumably is returning the last item before the
spacing?)

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:220:
// No action. Should never be called.
assert false then? Also, how do we ensure this?

[dgn, 2016-07-29 09:54:59]

Description was changed from

==========
[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/F87X6syhxZi53qaa8

BUG=627488
==========

to

==========
[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/F87X6syhxZi53qaa8

BUG=624338
==========

[dgn, 2016-08-01 16:53:11]

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

[commit-bot: I haz the power, 2016-08-01 16:53:43]

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

[commit-bot: I haz the power, 2016-08-01 16:55:32]

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

[commit-bot: I haz the power, 2016-08-01 16:55:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgn, 2016-08-01 17:06:25]

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

[dgn, 2016-08-01 17:06:39]

PTAL

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java:58:
postDelayed(mShowSpinnerRunnable, 500);
On 2016/07/28 09:02:56, Bernhard Bauer wrote:
> Extract the 500 to a constant?

Done.

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/LoadingIndicatorView.java:61:
private int asInt(@ColorRes int colorId) {
On 2016/07/28 09:02:56, Bernhard Bauer wrote:
> getColorAsInt()?

Done.

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:325:
((StatusListItem.LoadingViewHolder) holder).hide();
On 2016/07/28 09:02:56, Bernhard Bauer wrote:
> It might be worth making this a method on NewTabPageViewHolder with an empty
> default implementation.

Not needed anymore.

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:155:
if (lastCard == null) lastCard = findLoadingIndicator();
On 2016/07/28 09:02:56, Bernhard Bauer wrote:
> Instead of adding an additional method, could we change findLastCard() to do
> what we really want? (Which presumably is returning the last item before the
> spacing?)

Done.

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2191593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:220:
// No action. Should never be called.
On 2016/07/28 09:02:56, Bernhard Bauer wrote:
> assert false then? Also, how do we ensure this?

Made an independent list item instead.

[commit-bot: I haz the power, 2016-08-01 17:06:41]

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

[commit-bot: I haz the power, 2016-08-01 17:08:35]

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

[commit-bot: I haz the power, 2016-08-01 17:08:35]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgn, 2016-08-01 17:25:37]

Description was changed from

==========
[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/F87X6syhxZi53qaa8

BUG=624338
==========

to

==========
[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/7zp8HVi6BKL2VRucA

BUG=624338
==========

[Bernhard Bauer, 2016-08-02 09:59:36]

lgtm

[dgn, 2016-08-02 11:46:42]

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

[commit-bot: I haz the power, 2016-08-02 11:46:50]

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

[commit-bot: I haz the power, 2016-08-02 12:53:11]

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

[commit-bot: I haz the power, 2016-08-02 12:53:12]

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

[dgn, 2016-08-02 13:42:11]

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

[commit-bot: I haz the power, 2016-08-02 13:42:24]

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

[commit-bot: I haz the power, 2016-08-02 14:48:15]

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

[commit-bot: I haz the power, 2016-08-02 14:48:16]

Dry run: This issue passed the CQ dry run.

[dgn, 2016-08-02 15:02:37]

Thanks, submitting.

[dgn, 2016-08-02 15:02:44]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-08-02 15:02:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2191593002/#ps80001
(title: "fix unit tests, add an extra one")

[commit-bot: I haz the power, 2016-08-02 15:03:00]

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

[commit-bot: I haz the power, 2016-08-02 15:06:44]

Description was changed from

==========
[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/7zp8HVi6BKL2VRucA

BUG=624338
==========

to

==========
[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/7zp8HVi6BKL2VRucA

BUG=624338
==========

[commit-bot: I haz the power, 2016-08-02 15:06:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-02 15:09:28]

Description was changed from

==========
[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/7zp8HVi6BKL2VRucA

BUG=624338
==========

to

==========
[NTP Client] Add a spinner to indicate content is loading

The spinner currently only shows during the initialization phase, before
we the sync status gets confirmed.

Preview: https://goo.gl/photos/7zp8HVi6BKL2VRucA

BUG=624338

Committed: https://crrev.com/a15325bc18fbc76ed1d535a352cf3a9eda4c2098
Cr-Commit-Position: refs/heads/master@{#409192}
==========

[commit-bot: I haz the power, 2016-08-02 15:09:29]

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