Allow AccountManagerHelper.updateCredentials callback param to be null

Allow AccountManagerHelper.updateCredentials callback param to be null

Passing null to AccountManagerHelper.updateCredentials would cause
NullPointerException, which led to creation of unnecessary callbacks,
for example in SyncCustomizationFragment. This CL fixes it.

BUG=NONE

Review-Url: https://codereview.chromium.org/2740873004
Cr-Commit-Position: refs/heads/master@{#456057}
Committed: https://chromium.googlesource.com/chromium/src/+/ea4cd6e84d00c433d05fda8a30156e704a654f3c

[bsazonov, 2017-03-09 19:40:20]

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

[commit-bot: I haz the power, 2017-03-09 19:40:50]

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

[bsazonov, 2017-03-09 19:44:54]

bsazonov@chromium.org changed reviewers:
+ bauerb@chromium.org, msarda@chromium.org

[bsazonov, 2017-03-09 19:44:55]

Mihai, Bernhard, please take a look at this simple CL.

[Bernhard Bauer, 2017-03-09 19:55:48]

Wait, if the only current user of updateCredentials doesn't even use the
callback, why don't we get rid of it completely?

[bsazonov, 2017-03-09 20:04:51]

On 2017/03/09 19:55:48, Bernhard Bauer wrote:
> Wait, if the only current user of updateCredentials doesn't even use the
> callback, why don't we get rid of it completely?

Because I will need this callback to fix the bug that I still have to file
(sorry, keep forgetting about it). The bug is related to indefinite delay before
sync state is updated after updating credentials.

[commit-bot: I haz the power, 2017-03-09 20:19:59]

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

[commit-bot: I haz the power, 2017-03-09 20:20:00]

Dry run: This issue passed the CQ dry run.

[msarda, 2017-03-10 12:49:09]

lgtm

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
File
components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java
(right):

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java:187:
}
I think it would be good to log the errors in update credentials even when the
callback is null. So remove the realCallback variable and just return here if
callback is null.

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
File
components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/MockAccountManager.java:210:
if (callback != null) {
We prefer early-returns in this cases as code is easier to read (this is also
good in general to avoid a lot of indent that lead to lines that have more than
100 characters):
if (callback == null)
  return;

ThreadUtils.post...

[bsazonov, 2017-03-10 13:24:42]

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

[commit-bot: I haz the power, 2017-03-10 13:24:53]

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

[bsazonov, 2017-03-10 13:34:12]

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
File
components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java
(right):

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java:187:
}
On 2017/03/10 12:49:09, msarda wrote:
> I think it would be good to log the errors in update credentials even when the
> callback is null. So remove the realCallback variable and just return here if
> callback is null.

Done.

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
File
components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/MockAccountManager.java:210:
if (callback != null) {
On 2017/03/10 12:49:09, msarda wrote:
> We prefer early-returns in this cases as code is easier to read (this is also
> good in general to avoid a lot of indent that lead to lines that have more
than
> 100 characters):
> if (callback == null)
>   return;
> 
> ThreadUtils.post...

Java style requires braces event when "then" statement is one-liner, so using
return here would have increased line count. Since line length is not an issue
in this case I would prefer leaving it as is.

[Bernhard Bauer, 2017-03-10 13:44:19]

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
File
components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/MockAccountManager.java:210:
if (callback != null) {
On 2017/03/10 13:34:12, bsazonov wrote:
> On 2017/03/10 12:49:09, msarda wrote:
> > We prefer early-returns in this cases as code is easier to read (this is
also
> > good in general to avoid a lot of indent that lead to lines that have more
> than
> > 100 characters):
> > if (callback == null)
> >   return;
> > 
> > ThreadUtils.post...
> 
> Java style requires braces event when "then" statement is one-liner, so using
> return here would have increased line count. Since line length is not an issue
> in this case I would prefer leaving it as is.

No, it doesn't. Chrome on Android follows the Android style guide, which states
that putting both the if-statement and the body on a single line without braces
is allowed if it fits
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java....).
Also, the number of lines tends to be less of a concern than unnecessary
indentation (it's not only that it takes away horizontal space, but nested
if-statements would otherwise lead to a "leaning tower" of indentation).

[bsazonov, 2017-03-10 14:08:19]

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

[commit-bot: I haz the power, 2017-03-10 14:08:21]

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

[bsazonov, 2017-03-10 14:20:02]

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
File
components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/2740873004/diff/1/components/signin/core/brow...
components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/MockAccountManager.java:210:
if (callback != null) {
On 2017/03/10 13:44:19, Bernhard Bauer wrote:
> On 2017/03/10 13:34:12, bsazonov wrote:
> > On 2017/03/10 12:49:09, msarda wrote:
> > > We prefer early-returns in this cases as code is easier to read (this is
> also
> > > good in general to avoid a lot of indent that lead to lines that have more
> > than
> > > 100 characters):
> > > if (callback == null)
> > >   return;
> > > 
> > > ThreadUtils.post...
> > 
> > Java style requires braces event when "then" statement is one-liner, so
using
> > return here would have increased line count. Since line length is not an
issue
> > in this case I would prefer leaving it as is.
> 
> No, it doesn't. Chrome on Android follows the Android style guide, which
states
> that putting both the if-statement and the body on a single line without
braces
> is allowed if it fits
>
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java....).
> Also, the number of lines tends to be less of a concern than unnecessary
> indentation (it's not only that it takes away horizontal space, but nested
> if-statements would otherwise lead to a "leaning tower" of indentation).

The Style Guide allows both one-line and braced if in this case, so it's up to
the developer (and the code consistency). I think that returns on separate lines
are easier to spot (and the rest of the file uses braced ifs anyway).

Done: replaced with braced if in the beginning of the function.

[Bernhard Bauer, 2017-03-10 14:40:53]

lgtm

[commit-bot: I haz the power, 2017-03-10 14:44:07]

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

[commit-bot: I haz the power, 2017-03-10 14:44:08]

Dry run: This issue passed the CQ dry run.

[bsazonov, 2017-03-10 14:45:23]

The CQ bit was checked by bsazonov@chromium.org

[bsazonov, 2017-03-10 14:45:23]

The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org
Link to the patchset: https://codereview.chromium.org/2740873004/#ps40001
(title: "Addressed comments 2")

[commit-bot: I haz the power, 2017-03-10 14:45:40]

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, 2017-03-10 14:49:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489157123220430,
"parent_rev": "b4db19ff1260511b90e14cde6ddf251a1c77cfa9", "commit_rev":
"ea4cd6e84d00c433d05fda8a30156e704a654f3c"}

[commit-bot: I haz the power, 2017-03-10 14:50:20]

Description was changed from

==========
Allow AccountManagerHelper.updateCredentials callback param to be null

Passing null to AccountManagerHelper.updateCredentials would cause
NullPointerException, which led to creation of unnecessary callbacks,
for example in SyncCustomizationFragment. This CL fixes it.

BUG=NONE
==========

to

==========
Allow AccountManagerHelper.updateCredentials callback param to be null

Passing null to AccountManagerHelper.updateCredentials would cause
NullPointerException, which led to creation of unnecessary callbacks,
for example in SyncCustomizationFragment. This CL fixes it.

BUG=NONE

Review-Url: https://codereview.chromium.org/2740873004
Cr-Commit-Position: refs/heads/master@{#456057}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea4cd6e84d00c433d05fda8a3015...
==========

[commit-bot: I haz the power, 2017-03-10 14:50:21]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ea4cd6e84d00c433d05fda8a3015...