Bookmark manager: Pass in explicit targets to the delete and rename commands.

Bookmark manager: Pass in explicit targets to the delete and rename commands.

To do this, we store the menu's contextElement when handling activation via keyboard. This matches the behavior for activation via mouse.

BUG=451395
Committed: https://crrev.com/98425643b426decf938bb8919524bebefdeb03b1
Cr-Commit-Position: refs/heads/master@{#325820}

[Bernhard Bauer, 2015-04-17 16:12:21]

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

[Bernhard Bauer, 2015-04-17 16:12:22]

Please review. Thanks!

[Dan Beam, 2015-04-17 19:26:33]

general concept lg to me, just a question re: undefined target

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:1359: editItem(target);
what happens if target is undefined?  seems entirely possible here...

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:1360: break;
nit: maybe:

  case 'edit-command':
    recordUserAction('Edit');
    // Fall through.

  case 'rename-folder-from-folders-menu-command':
    target = bmm.tree;
    // Fall through.

  case 'rename-folder-command':
    editItem(target || assertInstanceof(e.target, HTMLElement));
    break;

[Bernhard Bauer, 2015-04-17 21:46:51]

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:1359: editItem(target);
On 2015/04/17 19:26:33, Dan Beam wrote:
> what happens if target is undefined?  seems entirely possible here...

Yup, that's intended here. editItem() will fall back to using the currently
selected item (via document.activeElement), which matches the behavior of the
old editEditSelectedItem().

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:1360: break;
On 2015/04/17 19:26:33, Dan Beam wrote:
> nit: maybe:
> 
>   case 'edit-command':
>     recordUserAction('Edit');
>     // Fall through.
> 
>   case 'rename-folder-from-folders-menu-command':
>     target = bmm.tree;
>     // Fall through.
> 
>   case 'rename-folder-command':
>     editItem(target || assertInstanceof(e.target, HTMLElement));
>     break;

That won't work, because for edit we want to pass undefined (see my comment
above). What I do need to change though is to not record the user action for
rename-folder-from-folders-menu, at which point I might just directly call
editItem() and break in each of the cases. WDYT?

[Dan Beam, 2015-04-17 23:35:16]

lgtm

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:1359: editItem(target);
On 2015/04/17 21:46:51, Bernhard Bauer wrote:
> On 2015/04/17 19:26:33, Dan Beam wrote:
> > what happens if target is undefined?  seems entirely possible here...
> 
> Yup, that's intended here. editItem() will fall back to using the currently
> selected item (via document.activeElement), which matches the behavior of the
> old editEditSelectedItem().

ok, thanks for explaining

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:1360: break;
On 2015/04/17 21:46:51, Bernhard Bauer wrote:
> On 2015/04/17 19:26:33, Dan Beam wrote:
> > nit: maybe:
> > 
> >   case 'edit-command':
> >     recordUserAction('Edit');
> >     // Fall through.
> > 
> >   case 'rename-folder-from-folders-menu-command':
> >     target = bmm.tree;
> >     // Fall through.
> > 
> >   case 'rename-folder-command':
> >     editItem(target || assertInstanceof(e.target, HTMLElement));
> >     break;
> 
> That won't work, because for edit we want to pass undefined (see my comment
> above). What I do need to change though is to not record the user action for
> rename-folder-from-folders-menu, at which point I might just directly call
> editItem() and break in each of the cases. WDYT?

yeah, that's fine

[Bernhard Bauer, 2015-04-20 07:40:17]

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/1058873009/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:1360: break;
On 2015/04/17 23:35:16, Dan Beam wrote:
> On 2015/04/17 21:46:51, Bernhard Bauer wrote:
> > On 2015/04/17 19:26:33, Dan Beam wrote:
> > > nit: maybe:
> > > 
> > >   case 'edit-command':
> > >     recordUserAction('Edit');
> > >     // Fall through.
> > > 
> > >   case 'rename-folder-from-folders-menu-command':
> > >     target = bmm.tree;
> > >     // Fall through.
> > > 
> > >   case 'rename-folder-command':
> > >     editItem(target || assertInstanceof(e.target, HTMLElement));
> > >     break;
> > 
> > That won't work, because for edit we want to pass undefined (see my comment
> > above). What I do need to change though is to not record the user action for
> > rename-folder-from-folders-menu, at which point I might just directly call
> > editItem() and break in each of the cases. WDYT?
> 
> yeah, that's fine

Ok, done.

[Bernhard Bauer, 2015-04-20 07:40:20]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2015-04-20 07:40:21]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1058873009/#ps20001
(title: "review")

[commit-bot: I haz the power, 2015-04-20 08:02:00]

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

[commit-bot: I haz the power, 2015-04-20 09:29:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-20 09:29:59]

Patchset 2 (id:??) landed as
https://crrev.com/98425643b426decf938bb8919524bebefdeb03b1
Cr-Commit-Position: refs/heads/master@{#325820}