Add a ProgressDialog during sign-in flow when GMS is updated.

Add a ProgressDialog during sign-in flow when GMS is updated.

Calls to GMS can be extremely slow after updating GMS, which can
result in an ANR error since we currently make calls to GMS on the UI
thread. This CL introduces a ProgressDialog that is shown while accounts
are fetched in the background, preventing ANR errors while letting the
user know that we're still waiting for GMS.

BUG=669522
Committed: https://crrev.com/3705bf2e4ba91d57815ac59af70654ef95286d45
Cr-Commit-Position: refs/heads/master@{#439108}

[estevenson, 2016-12-05 20:13:03]

estevenson@chromium.org changed reviewers:
+ bauerb@chromium.org

[estevenson, 2016-12-05 20:13:08]

ptal bauerb!

Can you think of a better way to figure out whether or not we should show the
progress bar? The way I've implemented it works fine if the user updates Play
Services and then navigates back to Chrome. If they wait a while before going
back to Chrome and the call to #getGoogleAccountNames takes while to finish
(unlikely but could happen), then they'll see "Waiting for Google Play Services
to finish updating" even though that isn't the cause of the delay.

[estevenson, 2016-12-05 20:13:19]

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

[commit-bot: I haz the power, 2016-12-05 20:13:49]

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

[Bernhard Bauer, 2016-12-06 17:09:34]

Generally, I think this is fine. If we're worried about that edge case, we could
change the dialog message to something that doesn't mention GMS.

https://codereview.chromium.org/2551063003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
(right):

https://codereview.chromium.org/2551063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:119:
private boolean mHasGooglePlayServicesUpdatedRecently;
We set this flag even before Google Play Services has been updated, so this
isn't always correct. Maybe call it mIsGooglePlayServicesOutOfDate? I guess
technically that would now be incorrect once Google PlayServices _has_ in fact
been updated, but I think I would rather have this reflect an old state and get
corrected later than having it reflect a state that isn't true yet.

https://codereview.chromium.org/2551063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:295:
private boolean canUseGooglePlayServices() {
The method name implies that it just returns a value, but it also has the side
effect of setting the flag... maybe call it checkGooglePlayServicesAvailable()?

[estevenson, 2016-12-06 23:43:44]

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

[commit-bot: I haz the power, 2016-12-06 23:45:10]

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

[estevenson, 2016-12-06 23:50:29]

ptal Bernhard!

I agree that leaving the GMS update message as-is may be best.

https://codereview.chromium.org/2551063003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
(right):

https://codereview.chromium.org/2551063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:119:
private boolean mHasGooglePlayServicesUpdatedRecently;
On 2016/12/06 17:09:34, Bernhard Bauer wrote:
> We set this flag even before Google Play Services has been updated, so this
> isn't always correct. Maybe call it mIsGooglePlayServicesOutOfDate? I guess
> technically that would now be incorrect once Google PlayServices _has_ in fact
> been updated, but I think I would rather have this reflect an old state and
get
> corrected later than having it reflect a state that isn't true yet.

Done.

https://codereview.chromium.org/2551063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:295:
private boolean canUseGooglePlayServices() {
On 2016/12/06 17:09:34, Bernhard Bauer wrote:
> The method name implies that it just returns a value, but it also has the side
> effect of setting the flag... maybe call it
checkGooglePlayServicesAvailable()?

Done.

[commit-bot: I haz the power, 2016-12-06 23:54:35]

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

[commit-bot: I haz the power, 2016-12-06 23:54: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...)

[Bernhard Bauer, 2016-12-07 10:07:13]

LGTM with some more nits I overlooked initially:

https://codereview.chromium.org/2551063003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
(right):

https://codereview.chromium.org/2551063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:234:
final AlertDialog updatingGmsDialog = new AlertDialog.Builder(getContext())
Could you create the dialog only if |mIsGooglePlayServicesOutOfDate| is true? I
think even if |updatingGmsDialog| is final, you could declare it outside of the
if, and then either assign null to it or a new dialog.

https://codereview.chromium.org/2551063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:241:
mIsGooglePlayServicesOutOfDate = false;
Could you move this to onResult()? It probably doesn't make a difference, but it
seems a bit more logical to me to reset it only when we have a result (because
then we know for sure that GMS is not out of date anymore).

[estevenson, 2016-12-07 15:48:35]

Thanks! Going to wait for UX approval before landing this
(https://crbug.com/669522#c18).

https://codereview.chromium.org/2551063003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
(right):

https://codereview.chromium.org/2551063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:234:
final AlertDialog updatingGmsDialog = new AlertDialog.Builder(getContext())
On 2016/12/07 10:07:12, Bernhard Bauer wrote:
> Could you create the dialog only if |mIsGooglePlayServicesOutOfDate| is true?
I
> think even if |updatingGmsDialog| is final, you could declare it outside of
the
> if, and then either assign null to it or a new dialog.

Done.

https://codereview.chromium.org/2551063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:241:
mIsGooglePlayServicesOutOfDate = false;
On 2016/12/07 10:07:12, Bernhard Bauer wrote:
> Could you move this to onResult()? It probably doesn't make a difference, but
it
> seems a bit more logical to me to reset it only when we have a result (because
> then we know for sure that GMS is not out of date anymore).

Done.

[estevenson, 2016-12-07 22:15:28]

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

[commit-bot: I haz the power, 2016-12-07 22:15:57]

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-12-07 22:59:00]

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

[commit-bot: I haz the power, 2016-12-07 22:59:01]

Dry run: This issue passed the CQ dry run.

[estevenson, 2016-12-15 20:27:21]

estevenson@chromium.org changed reviewers:
+ gogerald@chromium.org

[estevenson, 2016-12-15 20:27:22]

Hi Ganggui, can you take a look at
chrome/android/java/res/layout/updating_gms_progress_view.xml? Thanks!

[gogerald1, 2016-12-15 21:58:53]

lgtm with comments, but I and Bernhard as I remember do not own res files. You'd
better ask yusufo@ or tedchoc@ to review it.

https://codereview.chromium.org/2551063003/diff/140001/chrome/android/java/re...
File chrome/android/java/res/layout/updating_gms_progress_view.xml (right):

https://codereview.chromium.org/2551063003/diff/140001/chrome/android/java/re...
chrome/android/java/res/layout/updating_gms_progress_view.xml:8:
xmlns:chrome="http://schemas.android.com/apk/res-auto"
may no need "xmlns:chrome="http://schemas.android.com/apk/res-auto""

https://codereview.chromium.org/2551063003/diff/140001/chrome/android/java/re...
chrome/android/java/res/layout/updating_gms_progress_view.xml:22:
android:layout_width="wrap_content"
can you re-order these attributes, like layout related first (width, height,
gravity, margin, padding..), then view related (textsize, lineSpacing..., text)

[estevenson, 2016-12-15 22:13:23]

https://codereview.chromium.org/2551063003/diff/140001/chrome/android/java/re...
File chrome/android/java/res/layout/updating_gms_progress_view.xml (right):

https://codereview.chromium.org/2551063003/diff/140001/chrome/android/java/re...
chrome/android/java/res/layout/updating_gms_progress_view.xml:8:
xmlns:chrome="http://schemas.android.com/apk/res-auto"
On 2016/12/15 21:58:53, gogerald1 wrote:
> may no need "xmlns:chrome="http://schemas.android.com/apk/res-auto""

It's still needed. I think to use BoundedLinearLayout?

https://codereview.chromium.org/2551063003/diff/140001/chrome/android/java/re...
chrome/android/java/res/layout/updating_gms_progress_view.xml:22:
android:layout_width="wrap_content"
On 2016/12/15 21:58:53, gogerald1 wrote:
> can you re-order these attributes, like layout related first (width, height,
> gravity, margin, padding..), then view related (textsize, lineSpacing...,
text)

Done.

[estevenson, 2016-12-15 22:14:54]

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

[estevenson, 2016-12-15 22:14:55]

Hi Ted, can you do an OWNERS review for //chrome/android/java/res/ please?

[Ted C, 2016-12-15 23:52:07]

On 2016/12/15 22:14:55, estevenson wrote:
> Hi Ted, can you do an OWNERS review for //chrome/android/java/res/ please?

chrome/android/java/res - lgtm

[estevenson, 2016-12-16 14:32:23]

The CQ bit was checked by estevenson@chromium.org

[estevenson, 2016-12-16 14:32:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
gogerald@chromium.org
Link to the patchset: https://codereview.chromium.org/2551063003/#ps160001
(title: "Reorder attributes")

[commit-bot: I haz the power, 2016-12-16 14:32:41]

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-12-16 15:24:18]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481898743214880,
"parent_rev": "9a70d124033a7f8c32a15514395904bb34b6c571", "commit_rev":
"c2815621ec30d1440bd2fa2e3f80dc972d061b8c"}

[commit-bot: I haz the power, 2016-12-16 15:24:46]

Description was changed from

==========
Add a ProgressDialog during sign-in flow when GMS is updated.

Calls to GMS can be extremely slow after updating GMS, which can
result in an ANR error since we currently make calls to GMS on the UI
thread. This CL introduces a ProgressDialog that is shown while accounts
are fetched in the background, preventing ANR errors while letting the
user know that we're still waiting for GMS.

BUG=669522
==========

to

==========
Add a ProgressDialog during sign-in flow when GMS is updated.

Calls to GMS can be extremely slow after updating GMS, which can
result in an ANR error since we currently make calls to GMS on the UI
thread. This CL introduces a ProgressDialog that is shown while accounts
are fetched in the background, preventing ANR errors while letting the
user know that we're still waiting for GMS.

BUG=669522

Review-Url: https://codereview.chromium.org/2551063003
==========

[commit-bot: I haz the power, 2016-12-16 15:24:47]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-16 15:26:22]

Description was changed from

==========
Add a ProgressDialog during sign-in flow when GMS is updated.

Calls to GMS can be extremely slow after updating GMS, which can
result in an ANR error since we currently make calls to GMS on the UI
thread. This CL introduces a ProgressDialog that is shown while accounts
are fetched in the background, preventing ANR errors while letting the
user know that we're still waiting for GMS.

BUG=669522

Review-Url: https://codereview.chromium.org/2551063003
==========

to

==========
Add a ProgressDialog during sign-in flow when GMS is updated.

Calls to GMS can be extremely slow after updating GMS, which can
result in an ANR error since we currently make calls to GMS on the UI
thread. This CL introduces a ProgressDialog that is shown while accounts
are fetched in the background, preventing ANR errors while letting the
user know that we're still waiting for GMS.

BUG=669522

Committed: https://crrev.com/3705bf2e4ba91d57815ac59af70654ef95286d45
Cr-Commit-Position: refs/heads/master@{#439108}
==========

[commit-bot: I haz the power, 2016-12-16 15:26:23]

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