[sync] Display an error when sync settings aren't confirmed

[sync] Display an error when sync settings aren't confirmed

BUG=672225
R=skym@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2920853004
Cr-Commit-Position: refs/heads/master@{#478752}
Committed: https://chromium.googlesource.com/chromium/src/+/0c1d266856c65b2c4238cd7dae00ebcd4cc7cb01

[Patrick Noland, 2017-06-01 18:58:13]

Description was changed from

==========
[sync] Display an error when sync settings aren't confirmed

BUG=672225
R=skym@chromium.org
==========

to

==========
[sync] Display an error when sync settings aren't confirmed

BUG=672225
R=skym@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Patrick Noland, 2017-06-01 18:58:27]

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

[commit-bot: I haz the power, 2017-06-01 18:59:12]

Dry run: CQ is trying da patch.

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

[skym, 2017-06-01 19:48:44]

lgtm

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.cc (right):

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:340: if (!status_label->empty())
First, what a function. Combines figuring out your state, with setting UI. And
everyone had to deal with status_label and link_label to being null, maybe.

This check of status_label->empty() isn't guarded by if(status_label), right? So
this will deref nullptr, right?

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.h (right):

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.h:37: CONFIRM_SYNC_SETTINGS,  // User needs to
finish sync setup.
finish? not confirm?

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util_unittest.cc (right):

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util_unittest.cc:431: TEST_F(SyncUIUtilTest,
SyncSettingsConfirmationNeededTest) {
Can you make a unit test that has nullptr for the labels too?

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/ui/avatar_bu...
File chrome/browser/ui/avatar_button_error_controller.cc (right):

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/ui/avatar_bu...
chrome/browser/ui/avatar_button_error_controller.cc:98:
(!sync_service->IsFirstSetupComplete() &&
I've seen these two booleans checked before in this CL! Maybe the method in
sync_ui_util.cc should actually be in the PSS

[commit-bot: I haz the power, 2017-06-01 20:11:01]

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

[commit-bot: I haz the power, 2017-06-01 20:11:02]

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_...)

[Patrick Noland, 2017-06-01 21:57:58]

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

[commit-bot: I haz the power, 2017-06-01 21:59:18]

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, 2017-06-01 22:20:17]

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

[commit-bot: I haz the power, 2017-06-01 22:20:18]

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

[Patrick Noland, 2017-06-01 22:23:10]

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

[commit-bot: I haz the power, 2017-06-01 22:24:26]

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, 2017-06-01 23:26:43]

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

[commit-bot: I haz the power, 2017-06-01 23:26:44]

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_...)

[Patrick Noland, 2017-06-06 00:05:37]

Patchset #8 (id:140001) has been deleted

[Patrick Noland, 2017-06-06 00:05:40]

Patchset #7 (id:120001) has been deleted

[Patrick Noland, 2017-06-06 00:05:43]

Patchset #6 (id:100001) has been deleted

[Patrick Noland, 2017-06-06 00:05:47]

Patchset #5 (id:80001) has been deleted

[Patrick Noland, 2017-06-06 00:05:58]

Patchset #8 (id:220001) has been deleted

[Patrick Noland, 2017-06-06 00:06:00]

Patchset #9 (id:260001) has been deleted

[Patrick Noland, 2017-06-06 00:06:08]

Patchset #7 (id:200001) has been deleted

[Patrick Noland, 2017-06-06 00:06:25]

Patchset #11 (id:330001) has been deleted

[Patrick Noland, 2017-06-06 00:06:26]

Patchset #15 (id:430001) has been deleted

[Patrick Noland, 2017-06-06 00:06:35]

Patchset #9 (id:290001) has been deleted

[Patrick Noland, 2017-06-06 00:06:35]

Patchset #10 (id:310001) has been deleted

[Patrick Noland, 2017-06-06 00:06:41]

Patchset #12 (id:410001) has been deleted

[Patrick Noland, 2017-06-06 00:06:44]

Patchset #11 (id:390001) has been deleted

[Patrick Noland, 2017-06-06 00:06:47]

Patchset #10 (id:370001) has been deleted

[Patrick Noland, 2017-06-06 00:06:50]

Patchset #9 (id:350001) has been deleted

[Patrick Noland, 2017-06-06 00:07:00]

Patchset #6 (id:180001) has been deleted

[Patrick Noland, 2017-06-06 00:07:06]

Patchset #6 (id:240001) has been deleted

[Patrick Noland, 2017-06-06 00:07:13]

Patchset #5 (id:160001) has been deleted

[Patrick Noland, 2017-06-06 21:50:30]

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

[commit-bot: I haz the power, 2017-06-06 21:50:59]

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, 2017-06-06 23:15:49]

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

[commit-bot: I haz the power, 2017-06-06 23:15:50]

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

[Patrick Noland, 2017-06-07 19:04:09]

Description was changed from

==========
[sync] Display an error when sync settings aren't confirmed

BUG=672225
R=skym@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[sync] Display an error when sync settings aren't confirmed

BUG=672225
R=skym@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Patrick Noland, 2017-06-07 19:04:09]

pnoland@chromium.org changed reviewers:
+ avi@chromium.org, sky@chromium.org, tommycli@chromium.org

[Patrick Noland, 2017-06-07 19:07:23]

+avi for browser/ui/cocoa/
+sky(Scott) for other browser/ui/
+tommycli for webui and resources

PTAL

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.cc (right):

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:340: if (!status_label->empty())
On 2017/06/01 19:48:44, skym wrote:
> First, what a function. Combines figuring out your state, with setting UI. And
> everyone had to deal with status_label and link_label to being null, maybe.
> 
> This check of status_label->empty() isn't guarded by if(status_label), right?
So
> this will deref nullptr, right?

Done.

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.h (right):

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.h:37: CONFIRM_SYNC_SETTINGS,  // User needs to
finish sync setup.
On 2017/06/01 19:48:44, skym wrote:
> finish? not confirm?

Done.

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util_unittest.cc (right):

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util_unittest.cc:431: TEST_F(SyncUIUtilTest,
SyncSettingsConfirmationNeededTest) {
On 2017/06/01 19:48:44, skym wrote:
> Can you make a unit test that has nullptr for the labels too?

Alas, GetStatusLabels DCHECKS that link_label and status_label are non-null,
even though GetStatusInfo doesn't require that. GetStatusInfo is internal to
sync_ui_utils so there's not a good way to pass nullptr without doing a lot of
refactoring. And if I was refactoring this, I would change it so much that this
change wouldn't apply any longer.

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/ui/avatar_bu...
File chrome/browser/ui/avatar_button_error_controller.cc (right):

https://codereview.chromium.org/2920853004/diff/1/chrome/browser/ui/avatar_bu...
chrome/browser/ui/avatar_button_error_controller.cc:98:
(!sync_service->IsFirstSetupComplete() &&
On 2017/06/01 19:48:44, skym wrote:
> I've seen these two booleans checked before in this CL! Maybe the method in
> sync_ui_util.cc should actually be in the PSS

Done.

[Avi (use Gerrit), 2017-06-07 19:24:39]

Those Cocoa files lgtm

[sky, 2017-06-07 20:25:04]

https://codereview.chromium.org/2920853004/diff/510001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/2920853004/diff/510001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:181: views::LabelButton*
sync_error_settings_unconfirmed_button_;
Don't you need to initialize this to null?

[Patrick Noland, 2017-06-08 17:38:41]

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

[commit-bot: I haz the power, 2017-06-08 17:39:09]

Dry run: CQ is trying da patch.

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

[Patrick Noland, 2017-06-08 18:47:02]

PTAL

https://codereview.chromium.org/2920853004/diff/510001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/2920853004/diff/510001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:181: views::LabelButton*
sync_error_settings_unconfirmed_button_;
On 2017/06/07 20:25:04, sky wrote:
> Don't you need to initialize this to null?

Good catch. It looks like this is done in ResetView, which is first called from
the constructor. Done.

[commit-bot: I haz the power, 2017-06-08 20:43:51]

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

[commit-bot: I haz the power, 2017-06-08 20:43:52]

Dry run: This issue passed the CQ dry run.

[sky, 2017-06-08 21:23:02]

LGTM

[Patrick Noland, 2017-06-12 18:35:12]

tommycli@, friendly ping. PTAL?

[tommycli, 2017-06-12 18:41:01]

lgtm. apologies for the delay here.

https://codereview.chromium.org/2920853004/diff/530001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/sync_browser_proxy.js
(right):

https://codereview.chromium.org/2920853004/diff/530001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/sync_browser_proxy.js:43:
CONFIRM_SYNC_SETTINGS: "confirmSyncSettings",
nit: We use single quotes in JS for strings. Unfortunate that the comment
doesn't fit. Maybe on top would be better than on bottom?

Run 'git cl format --js' afterwards too to see if it has an opinion.

[Patrick Noland, 2017-06-12 18:44:30]

https://codereview.chromium.org/2920853004/diff/530001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/sync_browser_proxy.js
(right):

https://codereview.chromium.org/2920853004/diff/530001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/sync_browser_proxy.js:43:
CONFIRM_SYNC_SETTINGS: "confirmSyncSettings",
On 2017/06/12 18:41:01, tommycli wrote:
> nit: We use single quotes in JS for strings. Unfortunate that the comment
> doesn't fit. Maybe on top would be better than on bottom?
> 
> Run 'git cl format --js' afterwards too to see if it has an opinion.

Done.

[tommycli, 2017-06-12 18:47:00]

https://codereview.chromium.org/2920853004/diff/550001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/sync_browser_proxy.js
(right):

https://codereview.chromium.org/2920853004/diff/550001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/sync_browser_proxy.js:45:
CONFIRM_SYNC_SETTINGS: 'confirmSyncSettings',
Nit again: Looking above at SIGNOUT_AND_SIGNIN, it appears that git cl format
--js does indeed have an opinion. Maybe we should just follow up.

[tommycli, 2017-06-12 18:47:23]

https://codereview.chromium.org/2920853004/diff/550001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/sync_browser_proxy.js
(right):

https://codereview.chromium.org/2920853004/diff/550001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/sync_browser_proxy.js:45:
CONFIRM_SYNC_SETTINGS: 'confirmSyncSettings',
On 2017/06/12 18:47:00, tommycli wrote:
> Nit again: Looking above at SIGNOUT_AND_SIGNIN, it appears that git cl format
> --js does indeed have an opinion. Maybe we should just follow up.

*follow that.

[Patrick Noland, 2017-06-12 18:55:54]

https://codereview.chromium.org/2920853004/diff/550001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/sync_browser_proxy.js
(right):

https://codereview.chromium.org/2920853004/diff/550001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/sync_browser_proxy.js:45:
CONFIRM_SYNC_SETTINGS: 'confirmSyncSettings',
On 2017/06/12 18:47:23, tommycli wrote:
> On 2017/06/12 18:47:00, tommycli wrote:
> > Nit again: Looking above at SIGNOUT_AND_SIGNIN, it appears that git cl
format
> > --js does indeed have an opinion. Maybe we should just follow up.
> 
> *follow that.

Done.

[Patrick Noland, 2017-06-12 19:10:08]

The CQ bit was checked by pnoland@chromium.org

[Patrick Noland, 2017-06-12 19:10:09]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org,
avi@chromium.org, tommycli@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2920853004/#ps570001
(title: "Reformat, remove browser_options change")

[commit-bot: I haz the power, 2017-06-12 19:10:57]

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-06-12 20:49:32]

CQ is committing da patch.

Bot data: {"patchset_id": 570001, "attempt_start_ts": 1497294608778060,
"parent_rev": "3fc5cf5d90d28fb3d4beb5c47004f8406f2d050c", "commit_rev":
"0c1d266856c65b2c4238cd7dae00ebcd4cc7cb01"}

[commit-bot: I haz the power, 2017-06-12 20:49:43]

Description was changed from

==========
[sync] Display an error when sync settings aren't confirmed

BUG=672225
R=skym@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[sync] Display an error when sync settings aren't confirmed

BUG=672225
R=skym@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2920853004
Cr-Commit-Position: refs/heads/master@{#478752}
Committed:
https://chromium.googlesource.com/chromium/src/+/0c1d266856c65b2c4238cd7dae00...
==========

[commit-bot: I haz the power, 2017-06-12 20:49:46]

Committed patchset #12 (id:570001) as
https://chromium.googlesource.com/chromium/src/+/0c1d266856c65b2c4238cd7dae00...

[Patrick Noland, 2017-09-07 00:16:41]

Patchset #4 (id:60001) has been deleted

[Patrick Noland, 2017-09-07 00:16:52]

Patchset #3 (id:40001) has been deleted

[Patrick Noland, 2017-09-07 00:17:02]

Patchset #5 (id:470001) has been deleted

[Patrick Noland, 2017-09-07 00:17:12]

Patchset #3 (id:270001) has been deleted