When we create a new folder in the BookmarkEditor, it is useful to be able to...

When we create a new folder in the BookmarkEditor, it is useful to be able to delete said folder within the BookmarkEditor. This patch gives the option to delete newly created folders that have not been added to the BookmarkModel (by clicking OK). This way the user isn't blindly deleting folders without seeing their contents. The BookmarkManager exists for this reason.

BUG=7261
TEST=unittest

[pierre.lafayette, 2009-08-06 04:07:15]

http://code.google.com/p/chromium/issues/detail?id=7261

[sky, 2009-08-06 22:38:02]

Two things that concern me with this patch:

1. I don't like that delete only works for newly added folders. This
seems wrong. It should either always work, or never work.
2. Allowing users to delete from this view is risky as they can't see
all the contents.

As such, I'm inclined to think we shouldn't do anything. I'm adding
Glen and Ben for their thoughts.

  -Scott

On Wed, Aug 5, 2009 at 9:07 PM, <pierre.lafayette@gmail.com> wrote:
> Reviewers: sky, Ben Goodger,
>
> Message:
> http://code.google.com/p/chromium/issues/detail?id=3D7261
>
> Description:
> When we create a new folder in the BookmarkEditor, it is useful to be
> able to delete said folder within the BookmarkEditor. This patch gives
> the option to delete newly created folders that have not been added to
> the BookmarkModel (by clicking OK). This way the user isn't blindly
> deleting folders without seeing their contents. The BookmarkManager
> exists for this reason.
>
> BUG=3D7261
> TEST=3Dunittest
>
>
> Please review this at http://codereview.chromium.org/165031
>
> SVN Base: http://src.chromium.org/svn/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 chrome/browser/views/bookmark_editor_view.h
> =A0M =A0 =A0 chrome/browser/views/bookmark_editor_view.cc
> =A0M =A0 =A0 chrome/browser/views/bookmark_editor_view_unittest.cc
>
>
>

[Glen Murphy, 2009-08-06 23:27:17]

(For anyone watching who was confused as I was, this is for the properties
dialog for a bookmark, not for the bookmark *manager*).

I agree with Scott - the editor isn't perfect, and has suckiness when dealing
with folders, but I'd rather not introduce magic, especially given how rare this
problem is is.

If anything, I'd allow 'delete' on all folders, but with a confirm asking "This
folder contains 291 bookmarks, are you sure?".

> 1. I don't like that delete only works for newly added folders. This
> seems wrong. It should either always work, or never work.
> 2. Allowing users to delete from this view is risky as they can't see
> all the contents.
> 
> As such, I'm inclined to think we shouldn't do anything. I'm adding
> Glen and Ben for their thoughts.
> 
>   -Scott
> 
> On Wed, Aug 5, 2009 at 9:07 PM, <pierre.lafayette@gmail.com> wrote:
> > Reviewers: sky, Ben Goodger,
> >
> > Message:
> > http://code.google.com/p/chromium/issues/detail?id=3D7261
> >
> > Description:
> > When we create a new folder in the BookmarkEditor, it is useful to be
> > able to delete said folder within the BookmarkEditor. This patch gives
> > the option to delete newly created folders that have not been added to
> > the BookmarkModel (by clicking OK). This way the user isn't blindly
> > deleting folders without seeing their contents. The BookmarkManager
> > exists for this reason.
> >
> > BUG=3D7261
> > TEST=3Dunittest
> >
> >
> > Please review this at http://codereview.chromium.org/165031
> >
> > SVN Base: http://src.chromium.org/svn/trunk/src/
> >
> > Affected files:
> > =A0M =A0 =A0 chrome/browser/views/bookmark_editor_view.h
> > =A0M =A0 =A0 chrome/browser/views/bookmark_editor_view.cc
> > =A0M =A0 =A0 chrome/browser/views/bookmark_editor_view_unittest.cc
> >
> >
> >

[tfarina (gmail-do not use), 2009-08-07 01:03:50]

On 2009/08/06 23:27:17, Glen Murphy wrote:
> (For anyone watching who was confused as I was, this is for the properties
> dialog for a bookmark, not for the bookmark *manager*).
> 
> I agree with Scott - the editor isn't perfect, and has suckiness when dealing
> with folders, but I'd rather not introduce magic, especially given how rare
this
> problem is is.
> 
> If anything, I'd allow 'delete' on all folders, but with a confirm asking
"This
> folder contains 291 bookmarks, are you sure?".
For this particulary thing, he can use the ConfirmMessageBoxDialog class (see
passwords_page_view.cc). 
> 
> > 1. I don't like that delete only works for newly added folders. This
> > seems wrong. It should either always work, or never work.
> > 2. Allowing users to delete from this view is risky as they can't see
> > all the contents.
> > 
> > As such, I'm inclined to think we shouldn't do anything. I'm adding
> > Glen and Ben for their thoughts.
> > 
> >   -Scott
> > 
> > On Wed, Aug 5, 2009 at 9:07 PM, <pierre.lafayette@gmail.com> wrote:
> > > Reviewers: sky, Ben Goodger,
> > >
> > > Message:
> > > http://code.google.com/p/chromium/issues/detail?id=3D7261
> > >
> > > Description:
> > > When we create a new folder in the BookmarkEditor, it is useful to be
> > > able to delete said folder within the BookmarkEditor. This patch gives
> > > the option to delete newly created folders that have not been added to
> > > the BookmarkModel (by clicking OK). This way the user isn't blindly
> > > deleting folders without seeing their contents. The BookmarkManager
> > > exists for this reason.
> > >
> > > BUG=3D7261
> > > TEST=3Dunittest
> > >
> > >
> > > Please review this at http://codereview.chromium.org/165031
> > >
> > > SVN Base: http://src.chromium.org/svn/trunk/src/
> > >
> > > Affected files:
> > > =A0M =A0 =A0 chrome/browser/views/bookmark_editor_view.h
> > > =A0M =A0 =A0 chrome/browser/views/bookmark_editor_view.cc
> > > =A0M =A0 =A0 chrome/browser/views/bookmark_editor_view_unittest.cc
> > >
> > >
> > >

[sky, 2009-08-07 15:36:47]

I've closed out the bug. Sorry for not doing so sooner Pierre.

  -Scott

On Thu, Aug 6, 2009 at 4:27 PM, <glen@chromium.org> wrote:
> (For anyone watching who was confused as I was, this is for the
> properties dialog for a bookmark, not for the bookmark *manager*).
>
> I agree with Scott - the editor isn't perfect, and has suckiness when
> dealing with folders, but I'd rather not introduce magic, especially
> given how rare this problem is is.
>
> If anything, I'd allow 'delete' on all folders, but with a confirm
> asking "This folder contains 291 bookmarks, are you sure?".
>
>> 1. I don't like that delete only works for newly added folders. This
>> seems wrong. It should either always work, or never work.
>> 2. Allowing users to delete from this view is risky as they can't see
>> all the contents.
>
>> As such, I'm inclined to think we shouldn't do anything. I'm adding
>> Glen and Ben for their thoughts.
>
>> =A0 -Scott
>
>> On Wed, Aug 5, 2009 at 9:07 PM, <pierre.lafayette@gmail.com> wrote:
>> > Reviewers: sky, Ben Goodger,
>> >
>> > Message:
>> > http://code.google.com/p/chromium/issues/detail?id=3D3D7261
>> >
>> > Description:
>> > When we create a new folder in the BookmarkEditor, it is useful to
>
> be
>>
>> > able to delete said folder within the BookmarkEditor. This patch
>
> gives
>>
>> > the option to delete newly created folders that have not been added
>
> to
>>
>> > the BookmarkModel (by clicking OK). This way the user isn't blindly
>> > deleting folders without seeing their contents. The BookmarkManager
>> > exists for this reason.
>> >
>> > BUG=3D3D7261
>> > TEST=3D3Dunittest
>> >
>> >
>> > Please review this at http://codereview.chromium.org/165031
>> >
>> > SVN Base: http://src.chromium.org/svn/trunk/src/
>> >
>> > Affected files:
>> > =3DA0M =3DA0 =3DA0 chrome/browser/views/bookmark_editor_view.h
>> > =3DA0M =3DA0 =3DA0 chrome/browser/views/bookmark_editor_view.cc
>> > =3DA0M =3DA0 =3DA0 chrome/browser/views/bookmark_editor_view_unittest.=
cc
>> >
>> >
>> >
>
>
>
> http://codereview.chromium.org/165031
>