Added Sync-related controls to Preferences pane under "Personal Stuff".

Added Sync-related controls to Preferences pane under "Personal Stuff".

These controls are only visible when sync is enabled.  This isn't finished,
but it is enough to be usable.

Made RemoveViewFromView() handle top-most views.  Also added
RemoveGroupFromView().

BUG=23073
TEST=manual testing, trybots

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31775

[akalin, 2009-11-10 00:43:18]

+nick to review sync code.
+tvl to review cocoa code.

The trybots are barfing because of diffs in Preferences.xib, but the actual code
is ready for review.  Once the tree turns green I will redo the Preferences.xib
changes.

[akalin (wrong akalin), 2009-11-10 02:20:40]

Synced to head, trybots are happy now. :)

On Mon, Nov 9, 2009 at 4:43 PM,  <akalin@chromium.org> wrote:
> Reviewers: nick, TVL,
>
> Message:
> +nick to review sync code.
> +tvl to review cocoa code.
>
> The trybots are barfing because of diffs in Preferences.xib, but the actu=
al
> code
> is ready for review. =C2=A0Once the tree turns green I will redo the
> Preferences.xib
> changes.
>
> Description:
> Added Sync-related controls to Preferences pane under "Personal Stuff".
>
> These controls are only visible when sync is enabled. =C2=A0This isn't fi=
nished,
> but it is enough to be usable.
>
> Made RemoveViewFromView() handle top-most views. =C2=A0Also added
> RemoveGroupFromView().
>
> BUG=3D23073
> TEST=3Dmanual testing, trybots
>
> Please review this at http://codereview.chromium.org/380006
>
> Affected files:
> =C2=A0M chrome/app/nibs/Preferences.xib
> =C2=A0M chrome/browser/cocoa/preferences_window_controller.h
> =C2=A0M chrome/browser/cocoa/preferences_window_controller.mm
> =C2=A0M chrome/browser/cocoa/preferences_window_controller_unittest.mm
>
>
>



--=20
Frederick Akalin
Software Engineer

[TVL, 2009-11-10 02:32:36]

Comments so far, I need to patch this in, we might have to tweak the auto sizing
here because you update the label over time.

http://codereview.chromium.org/380006/diff/4001/4004
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/380006/diff/4001/4004#newcode254
Line 254: // Helper to remove a view and move everything below it up to take
over the
From a coordinate pov, it's down, the loop is this view to the top view.  And
when we resize the view, we simple move the top down also.

http://codereview.chromium.org/380006/diff/4001/4004#newcode286
Line 286: shrinkHeight = NSMaxY([view frame]) - NSMinY([toRemove frame]);
I believe this should be the Height.  MaxY is probably just working because
origin happens to be 0,0.  But the subviews are all local coordinates within the
view, so they don't know about the origin on the parent.

Actually, the space between two border and an item isn't always the space
between two items.  should this actually be the distance between MaxY of the
object below this one and MaxY of this object?

[akalin (wrong akalin), 2009-11-10 13:39:49]

I will address your initial comments, but if you're going to patch
this in, sync is disabled at compile time for OS X, so with this patch
alone the sync controls will never show up.  To make it show up, you
want to also patch in http://codereview.chromium.org/385005/show ,
which will enable sync by default.  To disable sync at run-time, you
pass in --disable-sync to the chromium command line.

On Mon, Nov 9, 2009 at 6:32 PM,  <thomasvl@chromium.org> wrote:
> Comments so far, I need to patch this in, we might have to tweak the auto
> sizing
> here because you update the label over time.
>
>
> http://codereview.chromium.org/380006/diff/4001/4004
> File chrome/browser/cocoa/preferences_window_controller.mm (right):
>
> http://codereview.chromium.org/380006/diff/4001/4004#newcode254
> Line 254: // Helper to remove a view and move everything below it up to
> take over the
> From a coordinate pov, it's down, the loop is this view to the top view.
> =C2=A0And when we resize the view, we simple move the top down also.
>
> http://codereview.chromium.org/380006/diff/4001/4004#newcode286
> Line 286: shrinkHeight =3D NSMaxY([view frame]) - NSMinY([toRemove
> frame]);
> I believe this should be the Height. =C2=A0MaxY is probably just working
> because origin happens to be 0,0. =C2=A0But the subviews are all local
> coordinates within the view, so they don't know about the origin on the
> parent.
>
> Actually, the space between two border and an item isn't always the
> space between two items. =C2=A0should this actually be the distance betwe=
en
> MaxY of the object below this one and MaxY of this object?
>
> http://codereview.chromium.org/380006
>



--=20
Frederick Akalin
Software Engineer

[TVL, 2009-11-10 13:42:50]

http://codereview.chromium.org/380006/diff/4001/4004
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/380006/diff/4001/4004#newcode1594
Line 1594: // TODO(akalin): Resize controls and re-layout when necessary.
actually, we don't want to resize/relayout, because you don't really want the
content moving on someone, they could be about to click and you could move
something out from under their mouse or move something under their mouse.

http://codereview.chromium.org/380006/diff/4001/4004#newcode1603
Line 1603: SyncStatusUIHelper::GetLabels(syncService_, &statusLabel,
&linkLabel);
what are all the string that could be used for status label?  as setup right
now, we're gonna auto size the status text to what ever string it initial gets,
but that might not fit the strings we need for all states in all languages.

http://codereview.chromium.org/380006/diff/4001/4004#newcode1605
Line 1605: buttonLabel =
l10n_util::GetString(IDS_SYNC_STOP_SYNCING_BUTTON_LABEL);
include app/l10n_util_mac.h and use l10n_util::GetNSStringWithFixup().  That
will directly give you the NSString you need and it also takes care of mapping
ellipses or windows accelerators that can be ui UI titles.

[TVL, 2009-11-10 13:43:33]

http://codereview.chromium.org/380006/diff/4001/4004
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/380006/diff/4001/4004#newcode1594
Line 1594: // TODO(akalin): Resize controls and re-layout when
necessary.
actually, we don't want to resize/relayout, because you don't really
want the content moving on someone, they could be about to click and you
could move something out from under their mouse or move something under
their mouse.

http://codereview.chromium.org/380006/diff/4001/4004#newcode1603
Line 1603: SyncStatusUIHelper::GetLabels(syncService_, &statusLabel,
&linkLabel);
what are all the string that could be used for status label?  as setup
right now, we're gonna auto size the status text to what ever string it
initial gets, but that might not fit the strings we need for all states
in all languages.

http://codereview.chromium.org/380006/diff/4001/4004#newcode1605
Line 1605: buttonLabel =
l10n_util::GetString(IDS_SYNC_STOP_SYNCING_BUTTON_LABEL);
include app/l10n_util_mac.h and use l10n_util::GetNSStringWithFixup().
That will directly give you the NSString you need and it also takes care
of mapping ellipses or windows accelerators that can be ui UI titles.

http://codereview.chromium.org/380006

[TVL, 2009-11-10 14:15:07]

http://codereview.chromium.org/380006/diff/4001/4004
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/380006/diff/4001/4004#newcode595
Line 595: kAutoSizeGroupBehaviorVerticalToFit,
Ok, looking at the UI, my suggestion is this:

Move the button above the text block (more mac like) (reorder things in the
IBArray also).
Have the text field tall enough for 2 lines of text.
Then make this group use kAutoSizeGroupBehaviorVerticalFirstToFit.  This will
make the button autosize but leave the text field alone so it should fit what is
needed in all languages.

[TVL, 2009-11-10 14:15:27]

http://codereview.chromium.org/380006/diff/4001/4004
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/380006/diff/4001/4004#newcode595
Line 595: kAutoSizeGroupBehaviorVerticalToFit,
Ok, looking at the UI, my suggestion is this:

Move the button above the text block (more mac like) (reorder things in
the IBArray also).
Have the text field tall enough for 2 lines of text.
Then make this group use kAutoSizeGroupBehaviorVerticalFirstToFit.  This
will make the button autosize but leave the text field alone so it
should fit what is needed in all languages.

http://codereview.chromium.org/380006

[ncarter (slow), 2009-11-10 19:05:38]

The calls to ProfileSyncService look good.

http://codereview.chromium.org/380006/diff/4001/4004
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/380006/diff/4001/4004#newcode254
Line 254: // Helper to remove a view and move everything below it up to take
over the
On 2009/11/10 02:32:36, TVL wrote:
> From a coordinate pov, it's down, the loop is this view to the top view.  And
> when we resize the view, we simple move the top down also.

Moving things "towards the top" would be an unambiguous construction.

http://codereview.chromium.org/380006/diff/4001/4004#newcode273
Line 273: // Now cycle over the views above it moving them down.
You're still saying "down" here (in the comment as well as in the variable name)

http://codereview.chromium.org/380006/diff/4001/4004#newcode286
Line 286: shrinkHeight = NSMaxY([view frame]) - NSMinY([toRemove frame]);
On 2009/11/10 02:32:36, TVL wrote:
> I believe this should be the Height.  MaxY is probably just working because
> origin happens to be 0,0.  But the subviews are all local coordinates within
the
> view, so they don't know about the origin on the parent.
> 
> Actually, the space between two border and an item isn't always the space
> between two items.  should this actually be the distance between MaxY of the
> object below this one and MaxY of this object?

What if there's only one object (no next or prev)?

If it were possible to account for the border/padding explicitly, rather than by
measuring the distances between this object and the next, then this logic would
get simpler.  Is that possible to do robustly?

http://codereview.chromium.org/380006/diff/4001/4004#newcode1603
Line 1603: SyncStatusUIHelper::GetLabels(syncService_, &statusLabel,
&linkLabel);
On 2009/11/10 13:42:50, TVL wrote:
> what are all the string that could be used for status label?  as setup right
> now, we're gonna auto size the status text to what ever string it initial
gets,
> but that might not fit the strings we need for all states in all languages.

At least one of the error messages contains a username, so it might not be
possible to compute a max size statically.  Another thing to consider is that
the error messages are considerably more verbose than the common case "you are
synced" non-error message.

http://codereview.chromium.org/380006/diff/4001/4004#newcode1605
Line 1605: buttonLabel =
l10n_util::GetString(IDS_SYNC_STOP_SYNCING_BUTTON_LABEL);
On 2009/11/10 13:42:50, TVL wrote:
> include app/l10n_util_mac.h and use l10n_util::GetNSStringWithFixup().  That
> will directly give you the NSString you need and it also takes care of mapping
> ellipses or windows accelerators that can be ui UI titles.

Would the fixup be necessary on the label text as well?

[ncarter (slow), 2009-11-10 19:05:58]

The calls to ProfileSyncService look good.


http://codereview.chromium.org/380006/diff/4001/4004
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/380006/diff/4001/4004#newcode254
Line 254: // Helper to remove a view and move everything below it up to
take over the
On 2009/11/10 02:32:36, TVL wrote:
>  From a coordinate pov, it's down, the loop is this view to the top
view.  And
> when we resize the view, we simple move the top down also.

Moving things "towards the top" would be an unambiguous construction.

http://codereview.chromium.org/380006/diff/4001/4004#newcode273
Line 273: // Now cycle over the views above it moving them down.
You're still saying "down" here (in the comment as well as in the
variable name)

http://codereview.chromium.org/380006/diff/4001/4004#newcode286
Line 286: shrinkHeight = NSMaxY([view frame]) - NSMinY([toRemove
frame]);
On 2009/11/10 02:32:36, TVL wrote:
> I believe this should be the Height.  MaxY is probably just working
because
> origin happens to be 0,0.  But the subviews are all local coordinates
within the
> view, so they don't know about the origin on the parent.

> Actually, the space between two border and an item isn't always the
space
> between two items.  should this actually be the distance between MaxY
of the
> object below this one and MaxY of this object?

What if there's only one object (no next or prev)?

If it were possible to account for the border/padding explicitly, rather
than by measuring the distances between this object and the next, then
this logic would get simpler.  Is that possible to do robustly?

http://codereview.chromium.org/380006/diff/4001/4004#newcode1603
Line 1603: SyncStatusUIHelper::GetLabels(syncService_, &statusLabel,
&linkLabel);
On 2009/11/10 13:42:50, TVL wrote:
> what are all the string that could be used for status label?  as setup
right
> now, we're gonna auto size the status text to what ever string it
initial gets,
> but that might not fit the strings we need for all states in all
languages.

At least one of the error messages contains a username, so it might not
be possible to compute a max size statically.  Another thing to consider
is that the error messages are considerably more verbose than the common
case "you are synced" non-error message.

http://codereview.chromium.org/380006/diff/4001/4004#newcode1605
Line 1605: buttonLabel =
l10n_util::GetString(IDS_SYNC_STOP_SYNCING_BUTTON_LABEL);
On 2009/11/10 13:42:50, TVL wrote:
> include app/l10n_util_mac.h and use l10n_util::GetNSStringWithFixup().
  That
> will directly give you the NSString you need and it also takes care of
mapping
> ellipses or windows accelerators that can be ui UI titles.

Would the fixup be necessary on the label text as well?

http://codereview.chromium.org/380006

[TVL, 2009-11-10 19:54:32]

On 2009/11/10 19:05:58, nick wrote:
> The calls to ProfileSyncService look good.
> 
> 
> http://codereview.chromium.org/380006/diff/4001/4004
> File chrome/browser/cocoa/preferences_window_controller.mm (right):
> 
> http://codereview.chromium.org/380006/diff/4001/4004#newcode254
> Line 254: // Helper to remove a view and move everything below it up to
> take over the
> On 2009/11/10 02:32:36, TVL wrote:
> >  From a coordinate pov, it's down, the loop is this view to the top
> view.  And
> > when we resize the view, we simple move the top down also.
> 
> Moving things "towards the top" would be an unambiguous construction.
> 
> http://codereview.chromium.org/380006/diff/4001/4004#newcode273
> Line 273: // Now cycle over the views above it moving them down.
> You're still saying "down" here (in the comment as well as in the
> variable name)
> 
> http://codereview.chromium.org/380006/diff/4001/4004#newcode286
> Line 286: shrinkHeight = NSMaxY([view frame]) - NSMinY([toRemove
> frame]);
> On 2009/11/10 02:32:36, TVL wrote:
> > I believe this should be the Height.  MaxY is probably just working
> because
> > origin happens to be 0,0.  But the subviews are all local coordinates
> within the
> > view, so they don't know about the origin on the parent.
> 
> > Actually, the space between two border and an item isn't always the
> space
> > between two items.  should this actually be the distance between MaxY
> of the
> > object below this one and MaxY of this object?
> 
> What if there's only one object (no next or prev)?
> 
> If it were possible to account for the border/padding explicitly, rather
> than by measuring the distances between this object and the next, then
> this logic would get simpler.  Is that possible to do robustly?
> 

As much as possible, I try to figure out the values from the controls at
runtime, this way spacing can be adjusted in the xib instead of there and
constants in the code.

> http://codereview.chromium.org/380006/diff/4001/4004#newcode1603
> Line 1603: SyncStatusUIHelper::GetLabels(syncService_, &statusLabel,
> &linkLabel);
> On 2009/11/10 13:42:50, TVL wrote:
> > what are all the string that could be used for status label?  as setup
> right
> > now, we're gonna auto size the status text to what ever string it
> initial gets,
> > but that might not fit the strings we need for all states in all
> languages.
> 
> At least one of the error messages contains a username, so it might not
> be possible to compute a max size statically.  Another thing to consider
> is that the error messages are considerably more verbose than the common
> case "you are synced" non-error message.
> 

Right. I filed a bug to revisit the UI in general, anytime you have this much
dynamic in a UI like this, it gets really hard to handle.  Since an error
message can come in while the UI is on screen, you don't really want to resize
things and run the risk of resizing a control while a user might be clicking. 
So we're sorta stuck making things big enough for the worse case, which makes
lot of things look suboptimal.

> http://codereview.chromium.org/380006/diff/4001/4004#newcode1605
> Line 1605: buttonLabel =
> l10n_util::GetString(IDS_SYNC_STOP_SYNCING_BUTTON_LABEL);
> On 2009/11/10 13:42:50, TVL wrote:
> > include app/l10n_util_mac.h and use l10n_util::GetNSStringWithFixup().
>   That
> > will directly give you the NSString you need and it also takes care of
> mapping
> > ellipses or windows accelerators that can be ui UI titles.
> 
> Would the fixup be necessary on the label text as well?
> 

They can probably skip it since the accelerators are for actionable controls.

[TVL, 2009-11-10 19:54:53]

On 2009/11/10 19:05:58, nick wrote:
> The calls to ProfileSyncService look good.


> http://codereview.chromium.org/380006/diff/4001/4004
> File chrome/browser/cocoa/preferences_window_controller.mm (right):

> http://codereview.chromium.org/380006/diff/4001/4004#newcode254
> Line 254: // Helper to remove a view and move everything below it up to
> take over the
> On 2009/11/10 02:32:36, TVL wrote:
> >  From a coordinate pov, it's down, the loop is this view to the top
> view.  And
> > when we resize the view, we simple move the top down also.

> Moving things "towards the top" would be an unambiguous construction.

> http://codereview.chromium.org/380006/diff/4001/4004#newcode273
> Line 273: // Now cycle over the views above it moving them down.
> You're still saying "down" here (in the comment as well as in the
> variable name)

> http://codereview.chromium.org/380006/diff/4001/4004#newcode286
> Line 286: shrinkHeight = NSMaxY([view frame]) - NSMinY([toRemove
> frame]);
> On 2009/11/10 02:32:36, TVL wrote:
> > I believe this should be the Height.  MaxY is probably just working
> because
> > origin happens to be 0,0.  But the subviews are all local coordinates
> within the
> > view, so they don't know about the origin on the parent.

> > Actually, the space between two border and an item isn't always the
> space
> > between two items.  should this actually be the distance between MaxY
> of the
> > object below this one and MaxY of this object?

> What if there's only one object (no next or prev)?

> If it were possible to account for the border/padding explicitly, rather
> than by measuring the distances between this object and the next, then
> this logic would get simpler.  Is that possible to do robustly?


As much as possible, I try to figure out the values from the controls at
runtime, this way spacing can be adjusted in the xib instead of there and
constants in the code.

> http://codereview.chromium.org/380006/diff/4001/4004#newcode1603
> Line 1603: SyncStatusUIHelper::GetLabels(syncService_, &statusLabel,
> &linkLabel);
> On 2009/11/10 13:42:50, TVL wrote:
> > what are all the string that could be used for status label?  as setup
> right
> > now, we're gonna auto size the status text to what ever string it
> initial gets,
> > but that might not fit the strings we need for all states in all
> languages.

> At least one of the error messages contains a username, so it might not
> be possible to compute a max size statically.  Another thing to consider
> is that the error messages are considerably more verbose than the common
> case "you are synced" non-error message.


Right. I filed a bug to revisit the UI in general, anytime you have this  
much
dynamic in a UI like this, it gets really hard to handle.  Since an error
message can come in while the UI is on screen, you don't really want to  
resize
things and run the risk of resizing a control while a user might be  
clicking.
So we're sorta stuck making things big enough for the worse case, which  
makes
lot of things look suboptimal.

> http://codereview.chromium.org/380006/diff/4001/4004#newcode1605
> Line 1605: buttonLabel =
> l10n_util::GetString(IDS_SYNC_STOP_SYNCING_BUTTON_LABEL);
> On 2009/11/10 13:42:50, TVL wrote:
> > include app/l10n_util_mac.h and use l10n_util::GetNSStringWithFixup().
>    That
> > will directly give you the NSString you need and it also takes care of
> mapping
> > ellipses or windows accelerators that can be ui UI titles.

> Would the fixup be necessary on the label text as well?


They can probably skip it since the accelerators are for actionable  
controls.


http://codereview.chromium.org/380006

[akalin (wrong akalin), 2009-11-11 03:24:09]

On Mon, Nov 9, 2009 at 6:32 PM,  <thomasvl@chromium.org> wrote:
> http://codereview.chromium.org/380006/diff/4001/4004
> File chrome/browser/cocoa/preferences_window_controller.mm (right):
>
> http://codereview.chromium.org/380006/diff/4001/4004#newcode254
> Line 254: // Helper to remove a view and move everything below it up to
> take over the
> From a coordinate pov, it's down, the loop is this view to the top view.
> =C2=A0And when we resize the view, we simple move the top down also.

Ah.  Reverted comment changes.

> http://codereview.chromium.org/380006/diff/4001/4004#newcode286
> Actually, the space between two border and an item isn't always the
> space between two items. =C2=A0should this actually be the distance betwe=
en
> MaxY of the object below this one and MaxY of this object?

Yeah, I considered that and didn't do it because there's the chance
that the top edge of the 2nd-top item (according to min-y) is higher.
But I fixed that by simply doing max(0, _).

On Tue, Nov 10, 2009 at 5:42 AM,  <thomasvl@chromium.org> wrote:
>
> http://codereview.chromium.org/380006/diff/4001/4004
> File chrome/browser/cocoa/preferences_window_controller.mm (right):
>
> http://codereview.chromium.org/380006/diff/4001/4004#newcode1594
> Line 1594: // TODO(akalin): Resize controls and re-layout when
> necessary.
> actually, we don't want to resize/relayout, because you don't really
> want the content moving on someone, they could be about to click and you
> could move something out from under their mouse or move something under
> their mouse.

Removed TODO.

> http://codereview.chromium.org/380006/diff/4001/4004#newcode1603
> Line 1603: SyncStatusUIHelper::GetLabels(syncService_, &statusLabel,
> &linkLabel);
> what are all the string that could be used for status label? =C2=A0as set=
up
> right now, we're gonna auto size the status text to what ever string it
> initial gets, but that might not fit the strings we need for all states
> in all languages.

There are quite a few.  I have yet to figure out how to easily test
this, though.  Can we push it to a future CL?

> http://codereview.chromium.org/380006/diff/4001/4004#newcode1605
> Line 1605: buttonLabel =3D
> l10n_util::GetString(IDS_SYNC_STOP_SYNCING_BUTTON_LABEL);
> include app/l10n_util_mac.h and use l10n_util::GetNSStringWithFixup().
> That will directly give you the NSString you need and it also takes care
> of mapping ellipses or windows accelerators that can be ui UI titles.

Done.

> What if there's only one object (no next or prev)?

I think doing nothing is fine in this case.  We never call the
function with only one object, anyway.

Please take another look.

--=20
Frederick Akalin
Software Engineer

[akalin (wrong akalin), 2009-11-11 03:29:38]

On Tuesday, November 10, 2009,  <thomasvl@chromium.org> wrote:
> http://codereview.chromium.org/380006/diff/4001/4004
> File chrome/browser/cocoa/preferences_window_controller.mm (right):
>
> http://codereview.chromium.org/380006/diff/4001/4004#newcode595
> Line 595: kAutoSizeGroupBehaviorVerticalToFit,
> Ok, looking at the UI, my suggestion is this:
>
> Move the button above the text block (more mac like) (reorder things in
> the IBArray also).
> Have the text field tall enough for 2 lines of text.
> Then make this group use kAutoSizeGroupBehaviorVerticalFirstToFit. =C2=A0=
This
> will make the button autosize but leave the text field alone so it
> should fit what is needed in all languages.
>
> http://codereview.chromium.org/380006

Forgot to mention, did this too.

--=20
Frederick Akalin
Software Engineer

[TVL, 2009-11-11 13:42:01]

lgtm

[TVL, 2009-11-11 13:42:20]

lgtm

http://codereview.chromium.org/380006