[Payments] Add compareBoolean for consistence

Add compareBoolean for consistence

BUG=675686
Review-Url: https://codereview.chromium.org/2628463005
Cr-Commit-Position: refs/heads/master@{#442792}
Committed: https://chromium.googlesource.com/chromium/src/+/8455c9126224dade8593d544baaf830f1cd58acc

[gogerald1, 2017-01-10 19:28:05]

Description was changed from

==========
clean

BUG=
==========

to

==========
Add compareBoolean to consist with compareLong

BUG=2614193002
==========

[gogerald1, 2017-01-10 19:29:29]

Description was changed from

==========
Add compareBoolean to consist with compareLong

BUG=2614193002
==========

to

==========
Add compareBoolean for consistence

BUG=2614193002
==========

[gogerald1, 2017-01-10 19:29:47]

Description was changed from

==========
Add compareBoolean for consistence

BUG=2614193002
==========

to

==========
Add compareBoolean for consistence

BUG=675686
==========

[gogerald1, 2017-01-10 19:32:20]

gogerald@chromium.org changed reviewers:
+ agrieve@chromium.org, tedchoc@chromium.org

[gogerald1, 2017-01-10 19:32:21]

Hi tedchoc@ and agrieve@, PTAL,

[gogerald1, 2017-01-10 19:32:22]

Hi tedchoc@ and agrieve@, PTAL,

[Ted C, 2017-01-10 19:33:54]

lgtm

[agrieve, 2017-01-10 19:35:35]

lgtm /w nit

https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java (right):

https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:61: if
(Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
I think it'd be better to move the fact that this is KK+ to the java doc, and
just leave the else clause as the only implementation. That will result in less
code.

[gogerald1, 2017-01-10 19:58:27]

https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java (right):

https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:61: if
(Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
On 2017/01/10 19:35:35, agrieve wrote:
> I think it'd be better to move the fact that this is KK+ to the java doc, and
> just leave the else clause as the only implementation. That will result in
less
> code.

Not quite sure I understand this comment correctly. Do you mean only do "return
lhs == rhs ? 0 : lhs ? 1 : -1;" as the implementation since Boolean.compare does
the same thing in essential? 

I followed the pattern in compareLong for consistence and expect this interface
works for all versions as the other interfaces in this file.

[gogerald1, 2017-01-10 19:58:32]

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

[commit-bot: I haz the power, 2017-01-10 19:59:02]

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

[agrieve, 2017-01-10 20:53:21]

https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java (right):

https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:61: if
(Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
On 2017/01/10 19:58:27, gogerald1 wrote:
> On 2017/01/10 19:35:35, agrieve wrote:
> > I think it'd be better to move the fact that this is KK+ to the java doc,
and
> > just leave the else clause as the only implementation. That will result in
> less
> > code.
> 
> Not quite sure I understand this comment correctly. Do you mean only do
"return
> lhs == rhs ? 0 : lhs ? 1 : -1;" as the implementation since Boolean.compare
does
> the same thing in essential? 
> 
> I followed the pattern in compareLong for consistence and expect this
interface
> works for all versions as the other interfaces in this file.

Ah, didn't click "expand" to see this is a copy & paste as above, but that is
what I mean.
If you updated compareLong too, I think that'd be nice.

There's no value in using a runtime conditional to delegate to the system
implementation here. It's just more code and an extra branch to test.

[commit-bot: I haz the power, 2017-01-10 21:45:41]

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

[commit-bot: I haz the power, 2017-01-10 21:45:42]

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

[gogerald1, 2017-01-10 22:26:06]

Hi all, please take another look. I simplified the implementation according to
the comment since it is more efficient and the android implementation is less
likely to be changed.

https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java (right):

https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:61: if
(Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
On 2017/01/10 20:53:21, agrieve wrote:
> On 2017/01/10 19:58:27, gogerald1 wrote:
> > On 2017/01/10 19:35:35, agrieve wrote:
> > > I think it'd be better to move the fact that this is KK+ to the java doc,
> and
> > > just leave the else clause as the only implementation. That will result in
> > less
> > > code.
> > 
> > Not quite sure I understand this comment correctly. Do you mean only do
> "return
> > lhs == rhs ? 0 : lhs ? 1 : -1;" as the implementation since Boolean.compare
> does
> > the same thing in essential? 
> > 
> > I followed the pattern in compareLong for consistence and expect this
> interface
> > works for all versions as the other interfaces in this file.
> 
> Ah, didn't click "expand" to see this is a copy & paste as above, but that is
> what I mean.
> If you updated compareLong too, I think that'd be nice.
> 
> There's no value in using a runtime conditional to delegate to the system
> implementation here. It's just more code and an extra branch to test.

Done.

[gogerald1, 2017-01-10 22:26:14]

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

[commit-bot: I haz the power, 2017-01-10 22:27:08]

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

[Ted C, 2017-01-10 22:32:29]

On 2017/01/10 22:26:06, gogerald1 wrote:
> Hi all, please take another look. I simplified the implementation according to
> the comment since it is more efficient and the android implementation is less
> likely to be changed.
> 
>
https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
> File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java
(right):
> 
>
https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
> base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:61: if
> (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
> On 2017/01/10 20:53:21, agrieve wrote:
> > On 2017/01/10 19:58:27, gogerald1 wrote:
> > > On 2017/01/10 19:35:35, agrieve wrote:
> > > > I think it'd be better to move the fact that this is KK+ to the java
doc,
> > and
> > > > just leave the else clause as the only implementation. That will result
in
> > > less
> > > > code.
> > > 
> > > Not quite sure I understand this comment correctly. Do you mean only do
> > "return
> > > lhs == rhs ? 0 : lhs ? 1 : -1;" as the implementation since
Boolean.compare
> > does
> > > the same thing in essential? 
> > > 
> > > I followed the pattern in compareLong for consistence and expect this
> > interface
> > > works for all versions as the other interfaces in this file.
> > 
> > Ah, didn't click "expand" to see this is a copy & paste as above, but that
is
> > what I mean.
> > If you updated compareLong too, I think that'd be nice.
> > 
> > There's no value in using a runtime conditional to delegate to the system
> > implementation here. It's just more code and an extra branch to test.
> 
> Done.

slgtm

[commit-bot: I haz the power, 2017-01-11 00:22:39]

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

[commit-bot: I haz the power, 2017-01-11 00:22:40]

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

[agrieve, 2017-01-11 02:20:40]

On 2017/01/10 22:32:29, Ted C wrote:
> On 2017/01/10 22:26:06, gogerald1 wrote:
> > Hi all, please take another look. I simplified the implementation according
to
> > the comment since it is more efficient and the android implementation is
less
> > likely to be changed.
> > 
> >
>
https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
> > File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java
> (right):
> > 
> >
>
https://codereview.chromium.org/2628463005/diff/1/base/android/java/src/org/c...
> > base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:61: if
> > (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
> > On 2017/01/10 20:53:21, agrieve wrote:
> > > On 2017/01/10 19:58:27, gogerald1 wrote:
> > > > On 2017/01/10 19:35:35, agrieve wrote:
> > > > > I think it'd be better to move the fact that this is KK+ to the java
> doc,
> > > and
> > > > > just leave the else clause as the only implementation. That will
result
> in
> > > > less
> > > > > code.
> > > > 
> > > > Not quite sure I understand this comment correctly. Do you mean only do
> > > "return
> > > > lhs == rhs ? 0 : lhs ? 1 : -1;" as the implementation since
> Boolean.compare
> > > does
> > > > the same thing in essential? 
> > > > 
> > > > I followed the pattern in compareLong for consistence and expect this
> > > interface
> > > > works for all versions as the other interfaces in this file.
> > > 
> > > Ah, didn't click "expand" to see this is a copy & paste as above, but that
> is
> > > what I mean.
> > > If you updated compareLong too, I think that'd be nice.
> > > 
> > > There's no value in using a runtime conditional to delegate to the system
> > > implementation here. It's just more code and an extra branch to test.
> > 
> > Done.
> 
> slgtm

lgtm thanks!

[gogerald1, 2017-01-11 03:29:31]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2017-01-11 03:29:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2628463005/#ps20001
(title: "address comments")

[commit-bot: I haz the power, 2017-01-11 03:30:17]

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-01-11 04:11:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484105371797400,
"parent_rev": "4dbdb0dd364c7ac324c731004d1ccc69e3bdf2e2", "commit_rev":
"8455c9126224dade8593d544baaf830f1cd58acc"}

[commit-bot: I haz the power, 2017-01-11 04:11:40]

Description was changed from

==========
Add compareBoolean for consistence

BUG=675686
==========

to

==========
Add compareBoolean for consistence

BUG=675686

Review-Url: https://codereview.chromium.org/2628463005
Cr-Commit-Position: refs/heads/master@{#442792}
Committed:
https://chromium.googlesource.com/chromium/src/+/8455c9126224dade8593d544baaf...
==========

[commit-bot: I haz the power, 2017-01-11 04:11:42]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8455c9126224dade8593d544baaf...