Options: Disable the delete button when no profile is selected in personal settings.

Options: Disable the delete button when no profile is selected in personal settings.
According to this change, Delete button will be disabled when there are multiple profiles and none of the profiles is selected. If profile list is hidden i.e if there is only one default profile then Delete Button is also disabled.

BUG=100625
TEST='Delete...' button should be disabled when no profile is selected in chrome://settings/personal page.


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

[NaveenBobbili (Motorola), 2011-10-18 11:49:18]

Please review this patch.

[Evan Stade, 2011-10-18 16:18:57]

+Miranda

LGTM with comment addressed

http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
chrome/browser/resources/options/personal_options.js:66: !hasSingleProfile :
!hasSelection;
hasSingleProfile ? false : !hasSelection;

[Miranda Callahan, 2011-10-18 16:40:34]

This seems like the opposite of the correct change, but please correct me if I'm
missing something obvious.

http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
chrome/browser/resources/options/personal_options.js:66: !hasSingleProfile :
!hasSelection;
On 2011/10/18 16:18:57, Evan Stade wrote:
> hasSingleProfile ? false : !hasSelection;

This looks to me like it doesn't change the logic -- perhaps I'm missing
something obvious?

If hasSingleProfile is true, disabled will always be false in both cases.
If hasSingleProfile is false, disabled will be the opposite of hasSelection in
both cases.

Shouldn't the change be 

...disabled = !hasSelection || hasSingleProfile;

That is -- always disable the button if there's no selection, and additionally
always disable it if there's only a single profile (until
http://code.google.com/p/chromium/issues/detail?id=94586 is solved)?

http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
chrome/browser/resources/options/personal_options.js:229:
$('profiles-delete').disabled = !hasSingleProfile;
This seems like the opposite of what we should do. If there are multiple
profiles, we should not disable the delete button -- only if there is one
profile.

[Miranda Callahan, 2011-10-18 16:48:28]

On 2011/10/18 16:40:34, Miranda Callahan wrote:
> This seems like the opposite of the correct change, but please correct me if
I'm
> missing something obvious.
> 
>
http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
> File chrome/browser/resources/options/personal_options.js (right):
> 
>
http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
> chrome/browser/resources/options/personal_options.js:66: !hasSingleProfile :
> !hasSelection;
> On 2011/10/18 16:18:57, Evan Stade wrote:
> > hasSingleProfile ? false : !hasSelection;
> 
> This looks to me like it doesn't change the logic -- perhaps I'm missing
> something obvious?
> 
> If hasSingleProfile is true, disabled will always be false in both cases.
> If hasSingleProfile is false, disabled will be the opposite of hasSelection in
> both cases.
> 
> Shouldn't the change be 
> 
> ...disabled = !hasSelection || hasSingleProfile;
> 
> That is -- always disable the button if there's no selection, and additionally
> always disable it if there's only a single profile (until
> http://code.google.com/p/chromium/issues/detail?id=94586 is solved)?
> 
>
http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
> chrome/browser/resources/options/personal_options.js:229:
> $('profiles-delete').disabled = !hasSingleProfile;
> This seems like the opposite of what we should do. If there are multiple
> profiles, we should not disable the delete button -- only if there is one
> profile.

+binji, as he implemented the delete interface.

[binji, 2011-10-18 17:57:06]

http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
chrome/browser/resources/options/personal_options.js:66: !hasSingleProfile :
!hasSelection;
I agree w/ Miranda:

!hasSelection || hasSingleProfile

I'll switch it after the fix to crbug.com/94586 lands.

[Evan Stade, 2011-10-18 18:25:53]

ok, but the bug still exists. When I first load the page there is no selection
but the delete button is enabled (whereas edit is not). (the edit button is
disabled as expected)

On 2011/10/18 17:57:06, binji wrote:
>
http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
> File chrome/browser/resources/options/personal_options.js (right):
> 
>
http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
> chrome/browser/resources/options/personal_options.js:66: !hasSingleProfile :
> !hasSelection;
> I agree w/ Miranda:
> 
> !hasSelection || hasSingleProfile
> 
> I'll switch it after the fix to crbug.com/94586 lands.

[binji, 2011-10-18 18:40:42]

That should be fixed by the second change. When setProfileViewSingle_ is called
with numProfiles > 1, the delete button will be disabled until the selection is
changed and profileList.onchanged is called.

It may be cleaner to call profileList.onchanged directly, however.

[Evan Stade, 2011-10-18 20:42:52]

On 2011/10/18 18:40:42, binji wrote:
> That should be fixed by the second change. When setProfileViewSingle_ is
called
> with numProfiles > 1, the delete button will be disabled until the selection
is
> changed and profileList.onchanged is called.
> 
> It may be cleaner to call profileList.onchanged directly, however.

rather than calling onchanged, use a helper function

[NaveenBobbili (Motorola), 2011-10-19 06:51:17]

Uploaded patchset based on review comments. I used helper function to
enable/disable edit and delete buttons in setProfiles_. Please review.

http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8339014/diff/3/chrome/browser/resources/option...
chrome/browser/resources/options/personal_options.js:66: !hasSingleProfile :
!hasSelection;
On 2011/10/18 16:18:57, Evan Stade wrote:
> hasSingleProfile ? false : !hasSelection;

Done.

[Miranda Callahan, 2011-10-19 12:42:23]

Please look at the comments carefully -- this logic is not correct. I know this
isn't well-documented right now, but the current state of the multi-profile
system is such that you cannot delete the last profile -- thus when the count is
1, the delete button should be disabled.

Ben (binji) has a CL in the works that will fix this bug -- and then we won't
need any special-casing for the situation in which there is only one profile --
we'll just need to see if a profile is selected.

But special-casing the "hasOneProfile" case and using it to always enable the
delete button is not correct now.

http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:66: false : !hasSelection;
This logic is incorrect. Please reread my comments carefully.

http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:255:
this.setDeleteButtonEnabled_(hasSingleProfile || hasSelection);
This logic is also wrong; for now, the delete button should *not* be enabled if
there is only one profile.

[NaveenBobbili (Motorola), 2011-10-19 13:01:08]

Corrected the logic as per requirement. Please review.

http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:66: false : !hasSelection;
On 2011/10/19 12:42:23, Miranda Callahan wrote:
> This logic is incorrect. Please reread my comments carefully.

Done.

http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:255:
this.setDeleteButtonEnabled_(hasSingleProfile || hasSelection);
On 2011/10/19 12:42:23, Miranda Callahan wrote:
> This logic is also wrong; for now, the delete button should *not* be enabled
if
> there is only one profile.

Done.

[Miranda Callahan, 2011-10-19 13:07:53]

Thanks, LGTM. Please wait for Evan to review the Javascript before committing.

On 2011/10/19 13:01:08, NaveenBobbili (Motorola) wrote:
> Corrected the logic as per requirement. Please review.
> 
>
http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
> File chrome/browser/resources/options/personal_options.js (right):
> 
>
http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:66: false :
!hasSelection;
> On 2011/10/19 12:42:23, Miranda Callahan wrote:
> > This logic is incorrect. Please reread my comments carefully.
> 
> Done.
> 
>
http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:255:
> this.setDeleteButtonEnabled_(hasSingleProfile || hasSelection);
> On 2011/10/19 12:42:23, Miranda Callahan wrote:
> > This logic is also wrong; for now, the delete button should *not* be enabled
> if
> > there is only one profile.
> 
> Done.

[NaveenBobbili (Motorola), 2011-10-19 13:10:16]

On 2011/10/19 13:07:53, Miranda Callahan wrote:
> Thanks, LGTM. Please wait for Evan to review the Javascript before committing.
> 
> On 2011/10/19 13:01:08, NaveenBobbili (Motorola) wrote:
> > Corrected the logic as per requirement. Please review.
> > 
> >
>
http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
> > File chrome/browser/resources/options/personal_options.js (right):
> > 
> >
>
http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
> > chrome/browser/resources/options/personal_options.js:66: false :
> !hasSelection;
> > On 2011/10/19 12:42:23, Miranda Callahan wrote:
> > > This logic is incorrect. Please reread my comments carefully.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
> > chrome/browser/resources/options/personal_options.js:255:
> > this.setDeleteButtonEnabled_(hasSingleProfile || hasSelection);
> > On 2011/10/19 12:42:23, Miranda Callahan wrote:
> > > This logic is also wrong; for now, the delete button should *not* be
enabled
> > if
> > > there is only one profile.
> > 
> > Done.

Sure. Will wait.

[Miranda Callahan, 2011-10-19 14:14:39]

Changed CL description to reflect current state of the patch.

On 2011/10/19 13:10:16, NaveenBobbili (Motorola) wrote:
> On 2011/10/19 13:07:53, Miranda Callahan wrote:
> > Thanks, LGTM. Please wait for Evan to review the Javascript before
committing.
> > 
> > On 2011/10/19 13:01:08, NaveenBobbili (Motorola) wrote:
> > > Corrected the logic as per requirement. Please review.
> > > 
> > >
> >
>
http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
> > > File chrome/browser/resources/options/personal_options.js (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
> > > chrome/browser/resources/options/personal_options.js:66: false :
> > !hasSelection;
> > > On 2011/10/19 12:42:23, Miranda Callahan wrote:
> > > > This logic is incorrect. Please reread my comments carefully.
> > > 
> > > Done.
> > > 
> > >
> >
>
http://codereview.chromium.org/8339014/diff/4002/chrome/browser/resources/opt...
> > > chrome/browser/resources/options/personal_options.js:255:
> > > this.setDeleteButtonEnabled_(hasSingleProfile || hasSelection);
> > > On 2011/10/19 12:42:23, Miranda Callahan wrote:
> > > > This logic is also wrong; for now, the delete button should *not* be
> enabled
> > > if
> > > > there is only one profile.
> > > 
> > > Done.
> 
> Sure. Will wait.

[binji, 2011-10-19 22:07:11]

http://codereview.chromium.org/8339014/diff/7003/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8339014/diff/7003/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:255:
this.setEditButtonEnabled_(hasSelection);
Create a helper function consisting of these four lines, and call it here and in
profilesList.onchange above.

http://codereview.chromium.org/8339014/diff/7003/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:258:
setDeleteButtonEnabled_: function(enabled) {
Please remove this helper function. The similar functions below are used solely
for communication to C++.

http://codereview.chromium.org/8339014/diff/7003/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:262: setEditButtonEnabled_:
function(enabled) {
Please remove this helper function.

[NaveenBobbili (Motorola), 2011-10-20 05:46:14]

Fixed review comments. Please review.

http://codereview.chromium.org/8339014/diff/7003/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8339014/diff/7003/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:255:
this.setEditButtonEnabled_(hasSelection);
On 2011/10/19 22:07:11, binji wrote:
> Create a helper function consisting of these four lines, and call it here and
in
> profilesList.onchange above.

Done.

http://codereview.chromium.org/8339014/diff/7003/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:258:
setDeleteButtonEnabled_: function(enabled) {
On 2011/10/19 22:07:11, binji wrote:
> Please remove this helper function. The similar functions below are used
solely
> for communication to C++.

Done.

http://codereview.chromium.org/8339014/diff/7003/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:262: setEditButtonEnabled_:
function(enabled) {
On 2011/10/19 22:07:11, binji wrote:
> Please remove this helper function.

Done.

[binji, 2011-10-20 22:49:33]

LGTM, thanks!

[Evan Stade, 2011-10-21 01:13:07]

http://codereview.chromium.org/8339014/diff/4005/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8339014/diff/4005/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:60: profilesList.onchange =
function(event) {
this should be

profilesList.onchange = this.setProfileViewButtonsStatus_;

[NaveenBobbili (Motorola), 2011-10-22 17:34:35]

Fixed review comments. Please review.

http://codereview.chromium.org/8339014/diff/4005/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8339014/diff/4005/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:60: profilesList.onchange =
function(event) {
On 2011/10/21 01:13:07, Evan Stade wrote:
> this should be
> 
> profilesList.onchange = this.setProfileViewButtonsStatus_;

Done.

[sail, 2011-10-24 20:06:39]

Looks like this change will also fix bug 100609.

This should be merged to M16 since it's a fairly visible issue.

[NaveenBobbili (Motorola), 2011-10-25 06:38:10]

On 2011/10/24 20:06:39, sail wrote:
> Looks like this change will also fix bug 100609.
> 
> This should be merged to M16 since it's a fairly visible issue.

Can you please let me know if there changes can be committed?

[NaveenBobbili (Motorola), 2011-10-25 06:38:25]

On 2011/10/25 06:38:10, NaveenBobbili (Motorola) wrote:
> On 2011/10/24 20:06:39, sail wrote:
> > Looks like this change will also fix bug 100609.
> > 
> > This should be merged to M16 since it's a fairly visible issue.
> 
> Can you please let me know if these changes can be committed?

[commit-bot: I haz the power, 2011-10-27 14:58:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qghc36@motorola.com/8339014/15001

[commit-bot: I haz the power, 2011-10-27 14:58:24]

Can't apply patch for file chrome/browser/resources/options/personal_options.js.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/options/personal_options.js
Hunk #1 FAILED at 57.
Hunk #2 succeeded at 221 (offset 5 lines).
Hunk #3 succeeded at 265 (offset 5 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/resources/options/personal_options.js.rej

[Miranda Callahan, 2011-10-27 15:00:33]

On 2011/10/25 06:38:25, NaveenBobbili (Motorola) wrote:
> On 2011/10/25 06:38:10, NaveenBobbili (Motorola) wrote:
> > On 2011/10/24 20:06:39, sail wrote:
> > > Looks like this change will also fix bug 100609.
> > > 
> > > This should be merged to M16 since it's a fairly visible issue.
> > 
> > Can you please let me know if these changes can be committed?

This is good to go -- hit the commit button. I will make sure this is merged to
M16.

[Miranda Callahan, 2011-10-27 19:42:44]

On 2011/10/27 15:00:33, Miranda Callahan wrote:
> On 2011/10/25 06:38:25, NaveenBobbili (Motorola) wrote:
> > On 2011/10/25 06:38:10, NaveenBobbili (Motorola) wrote:
> > > On 2011/10/24 20:06:39, sail wrote:
> > > > Looks like this change will also fix bug 100609.
> > > > 
> > > > This should be merged to M16 since it's a fairly visible issue.
> > > 
> > > Can you please let me know if these changes can be committed?
> 
> This is good to go -- hit the commit button. I will make sure this is merged
to
> M16.

I'll resolve the conflicts and commit.

[Miranda Callahan, 2011-10-27 21:33:20]

On 2011/10/27 19:42:44, Miranda Callahan wrote:
> On 2011/10/27 15:00:33, Miranda Callahan wrote:
> > On 2011/10/25 06:38:25, NaveenBobbili (Motorola) wrote:
> > > On 2011/10/25 06:38:10, NaveenBobbili (Motorola) wrote:
> > > > On 2011/10/24 20:06:39, sail wrote:
> > > > > Looks like this change will also fix bug 100609.
> > > > > 
> > > > > This should be merged to M16 since it's a fairly visible issue.
> > > > 
> > > > Can you please let me know if these changes can be committed?
> > 
> > This is good to go -- hit the commit button. I will make sure this is merged
> to
> > M16.
> 
> I'll resolve the conflicts and commit.

I have patch http://codereview.chromium.org/8343032/ going through the trybots,
which resolves the conflicts in this patch. I'll commit it when the trybots show
green.

[NaveenBobbili (Motorola), 2011-10-30 02:22:19]

Fixed merge conflicts and committing this change as
http://codereview.chromium.org/8343032/ is not yet committed.

[commit-bot: I haz the power, 2011-10-30 02:22:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qghc36@motorola.com/8339014/25002

[commit-bot: I haz the power, 2011-10-30 03:33:03]

Change committed as 107894