Surface sync error icon if sync is off may be unintentional

Surface sync error icon if sync is off may be unintentional

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
Committed: https://crrev.com/93018d6d6d5fc9bd3d54addc498d296187af6423
Cr-Commit-Position: refs/heads/master@{#398097}

[gogerald1, 2016-06-02 17:36:25]

Patchset #1 (id:1) has been deleted

[gogerald1, 2016-06-02 17:49:22]

Patchset #1 (id:20001) has been deleted

[gogerald1, 2016-06-02 17:52:07]

Description was changed from

==========
remove redundant headers


surface sync error icon

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

to

==========
Surface sync error icon if sync is off unintentionally

This CL is an update of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

[gogerald1, 2016-06-02 17:55:13]

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

[gogerald1, 2016-06-02 17:55:40]

Description was changed from

==========
Surface sync error icon if sync is off unintentionally

This CL is an update of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

to

==========
Surface sync error icon if sync is off unintentionally

This CL is an strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

[commit-bot: I haz the power, 2016-06-02 17:55:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032043002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2032043002/40001

[gogerald1, 2016-06-02 17:55:49]

Description was changed from

==========
Surface sync error icon if sync is off unintentionally

This CL is an strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

to

==========
Surface sync error icon if sync is off unintentionally

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

[gogerald1, 2016-06-02 17:57:05]

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

[gogerald1, 2016-06-02 17:57:06]

Hey, PTAL,

[commit-bot: I haz the power, 2016-06-02 21:04:44]

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

[commit-bot: I haz the power, 2016-06-02 21:04:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bernhard Bauer, 2016-06-03 08:14:51]

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

[Bernhard Bauer, 2016-06-03 08:14:52]

+Max for sync

https://codereview.chromium.org/2032043002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java
(right):

https://codereview.chromium.org/2032043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:38:
if (AndroidSyncSettings.isSyncEnabled(getContext()) && ProfileSyncService.get()
!= null
Why don't you move AndroidSyncSettings.isSyncEnabled() into
isSyncOffUnintentionally() as well?

[gogerald1, 2016-06-03 14:19:23]

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

[gogerald1, 2016-06-03 14:19:31]

https://codereview.chromium.org/2032043002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java
(right):

https://codereview.chromium.org/2032043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncPreference.java:38:
if (AndroidSyncSettings.isSyncEnabled(getContext()) && ProfileSyncService.get()
!= null
On 2016/06/03 08:14:52, Bernhard Bauer wrote:
> Why don't you move AndroidSyncSettings.isSyncEnabled() into
> isSyncOffUnintentionally() as well?

Basically, I want keep ProfileSyncService.java self-contained, it either calls
into C++ side or only depend on itself. But here I could remove
AndroidSyncSettings.isSyncEnabled() check since we only care about whether sync
is abnormal.

[commit-bot: I haz the power, 2016-06-03 14:19:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032043002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2032043002/60001

[commit-bot: I haz the power, 2016-06-03 15:06:26]

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

[commit-bot: I haz the power, 2016-06-03 15:06:26]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-06-03 16:16:02]

LGTM.

It might be worth checking with Eli though that we don't want to show the error
if Sync has been disabled in Android settings -- just to be absolutely sure :)

[gogerald1, 2016-06-03 17:54:11]

Yes, we should show sync error icon in that case after confirm with Eli :). I've
updated the CL. PTAL.

[gogerald1, 2016-06-03 17:54:17]

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

[commit-bot: I haz the power, 2016-06-03 17:54:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032043002/80001

[commit-bot: I haz the power, 2016-06-03 18:44:36]

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

[commit-bot: I haz the power, 2016-06-03 18:44:37]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-06-03 19:04:56]

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SignInPreference.java
(right):

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SignInPreference.java:97:
if (!AndroidSyncSettings.isMasterSyncEnabled(getContext())
Why are you checking master sync? If that's off, the user intends for sync to be
off.

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:381:
public boolean isSyncOffUnintentionally() {
Random helper functions are not allowed here. This class is for bindings to the
native ProfileSyncService only. I suggest making this a package-private static
method on SyncPreference.

[gogerald1, 2016-06-03 19:32:24]

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SignInPreference.java
(right):

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SignInPreference.java:97:
if (!AndroidSyncSettings.isMasterSyncEnabled(getContext())
On 2016/06/03 19:04:55, maxbogue wrote:
> Why are you checking master sync? If that's off, the user intends for sync to
be
> off.

This is by requirement after talking to PM ewald@. User's may not know disable
sync on device will disable sync in chrome.

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:381:
public boolean isSyncOffUnintentionally() {
On 2016/06/03 19:04:56, maxbogue wrote:
> Random helper functions are not allowed here. This class is for bindings to
the
> native ProfileSyncService only. I suggest making this a package-private static
> method on SyncPreference.

Yeah, it looks not ideal to add it here, but at least it's self-contained. This
function needs to be available in SignInPreference, define it in SyncPreference
looks even more strange to me. Does this make sense? Otherwise I'd like to keep
two copies of them or a new file SyncUtil.java in the same directory of
ProfileSyncService.java.

[maxbogue, 2016-06-03 20:09:24]

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:381:
public boolean isSyncOffUnintentionally() {
On 2016/06/03 19:32:24, gogerald1 wrote:
> On 2016/06/03 19:04:56, maxbogue wrote:
> > Random helper functions are not allowed here. This class is for bindings to
> the
> > native ProfileSyncService only. I suggest making this a package-private
static
> > method on SyncPreference.
> 
> Yeah, it looks not ideal to add it here, but at least it's self-contained.
This
> function needs to be available in SignInPreference, define it in
SyncPreference
> looks even more strange to me. Does this make sense? Otherwise I'd like to
keep
> two copies of them or a new file SyncUtil.java in the same directory of
> ProfileSyncService.java. 

But it's not self-contained here, it's available to all the code. This is a
condition that's only used for those preferences. There's no reason it needs to
be available outside of them. Why does it seem strange to define it on one and
use it from the other?

[Bernhard Bauer, 2016-06-06 08:25:45]

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/2032043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:381:
public boolean isSyncOffUnintentionally() {
On 2016/06/03 19:32:24, gogerald1 wrote:
> On 2016/06/03 19:04:56, maxbogue wrote:
> > Random helper functions are not allowed here. This class is for bindings to
> the
> > native ProfileSyncService only. I suggest making this a package-private
static
> > method on SyncPreference.
> 
> Yeah, it looks not ideal to add it here, but at least it's self-contained.
This
> function needs to be available in SignInPreference, define it in
SyncPreference
> looks even more strange to me. Does this make sense? Otherwise I'd like to
keep
> two copies of them or a new file SyncUtil.java in the same directory of
> ProfileSyncService.java. 

FWIW, given that SigninPreference does show information about the state of Sync,
I don't think it's unreasonable to have it call a public static method on
SyncPreference.

[gogerald1, 2016-06-06 13:56:39]

Thanks,
 
The thing makes me feel strange is that Preference is the basic preference UI
building block, it looks like 'View' to me even though it is a subclass of
Object. So define a static function in a Preference (SyncPreference) which
checks the status of ProfileSyncService.java which is not part of it, and then
call this function from another Preference (SigninPreference) looks not good.
For 'self-contained', I mean it's implementation doesn't depend on other
services :). It looks like we add a new help interface in
ProfileSyncService.java for UI, like these functions
https://cs.chromium.org/chromium/src/chrome/browser/sync/profile_sync_service....

Anyway, I do not insist on this if you guys think it make more sense to define
it in SyncPreference instead of ProfileSyncService.java.

[gogerald1, 2016-06-06 15:34:21]

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

[commit-bot: I haz the power, 2016-06-06 15:34:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032043002/100001

[gogerald1, 2016-06-06 15:35:49]

Description was changed from

==========
Surface sync error icon if sync is off unintentionally

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

to

==========
Surface sync error icon if sync is off may unintentionally

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

[gogerald1, 2016-06-06 15:36:15]

Description was changed from

==========
Surface sync error icon if sync is off may unintentionally

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

to

==========
Surface sync error icon if sync is off might unintentionally

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

[gogerald1, 2016-06-06 15:37:01]

Description was changed from

==========
Surface sync error icon if sync is off might unintentionally

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

to

==========
Surface sync error icon if sync is off may be unintentional

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

[commit-bot: I haz the power, 2016-06-06 16:36:04]

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

[commit-bot: I haz the power, 2016-06-06 16:36:06]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-06-06 17:19:12]

lgtm

[gogerald1, 2016-06-06 19:36:02]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2016-06-06 19:36:03]

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

[commit-bot: I haz the power, 2016-06-06 19:36:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032043002/100001

[commit-bot: I haz the power, 2016-06-06 19:41:31]

Description was changed from

==========
Surface sync error icon if sync is off may be unintentional

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

to

==========
Surface sync error icon if sync is off may be unintentional

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

[commit-bot: I haz the power, 2016-06-06 19:41:33]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-06-06 19:44:19]

Description was changed from

==========
Surface sync error icon if sync is off may be unintentional

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695
==========

to

==========
Surface sync error icon if sync is off may be unintentional

This CL is a strength of https://codereview.chromium.org/1952853005/.

BUG=607695

Committed: https://crrev.com/93018d6d6d5fc9bd3d54addc498d296187af6423
Cr-Commit-Position: refs/heads/master@{#398097}
==========

[commit-bot: I haz the power, 2016-06-06 19:44:20]

Patchset 4 (id:??) landed as
https://crrev.com/93018d6d6d5fc9bd3d54addc498d296187af6423
Cr-Commit-Position: refs/heads/master@{#398097}