Copy and Cut option of the context menu is not working via keyboard in chrome://bookmarks

Copy and Cut option of the context menu is not working via keyboard in chrome://bookmarks

target is not getting passed properly to getSelectedBookmarkIds().
It should be either bmm.tree or bmm.list.
Changes done so that when the target is undefined then event target will be passed.

These changes require https://codereview.chromium.org/1058873009/.

BUG=478591
Committed: https://crrev.com/e0d1de3410ebeb274dca721c46aaba8a58d865f6
Cr-Commit-Position: refs/heads/master@{#325836}

[Deepak, 2015-04-20 05:39:38]

deepak.m1@samsung.com changed reviewers:
+ bauerb@chromium.org

[Deepak, 2015-04-20 05:39:38]

PTAL.
Thanks

[Bernhard Bauer, 2015-04-20 07:49:57]

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

https://codereview.chromium.org/1097873002/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:1317:
getSelectedBookmarkIds(target ||
Hm... Looking through this method (after
https://codereview.chromium.org/1058873009/ has landed), all uses of |target|
have |e.target| as a fallback if the former is null, right? In that case, why
don't we initialize |target| to |e.target| (together with the assertInstanceOf)
right away?

[Deepak, 2015-04-20 09:47:48]

On 2015/04/20 07:49:57, Bernhard Bauer wrote:
>
https://codereview.chromium.org/1097873002/diff/1/chrome/browser/resources/bo...
> File chrome/browser/resources/bookmark_manager/js/main.js (right):
> 
>
https://codereview.chromium.org/1097873002/diff/1/chrome/browser/resources/bo...
> chrome/browser/resources/bookmark_manager/js/main.js:1317:
> getSelectedBookmarkIds(target ||
> Hm... Looking through this method (after
> https://codereview.chromium.org/1058873009/ has landed), all uses of |target|
> have |e.target| as a fallback if the former is null, right? In that case, why
> don't we initialize |target| to |e.target| (together with the
assertInstanceOf)
> right away?

Humm..
I understand, make sense to me.
Changes done as suggested.
PTAL

[Bernhard Bauer, 2015-04-20 10:28:27]

https://codereview.chromium.org/1097873002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/bookmark_manager/js/main.js (left):

https://codereview.chromium.org/1097873002/diff/20001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/main.js:1355: editItem(bmm.tree);
You want to keep this, so that the tree is always what is being edited. What you
could do is swap those two cases, and assign `target = bmm.tree;` then fall
through, like below.

https://codereview.chromium.org/1097873002/diff/20001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/main.js:1364: target = bmm.tree;
You also want to keep this.

[Deepak, 2015-04-20 10:48:26]

Thanks for review.
Changes done as suggested.
PTAL

[Bernhard Bauer, 2015-04-20 12:47:32]

On 2015/04/20 12:39:22, Bernhard Bauer wrote:

LGTM I meant.

[Deepak, 2015-04-20 12:48:05]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-04-20 12:48:25]

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

[Deepak, 2015-04-20 12:56:37]

On 2015/04/20 12:47:32, Bernhard Bauer wrote:
> On 2015/04/20 12:39:22, Bernhard Bauer wrote:
> 
> LGTM I meant.

Thanks

[commit-bot: I haz the power, 2015-04-20 13:43:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-20 13:43:48]

Patchset 3 (id:??) landed as
https://crrev.com/e0d1de3410ebeb274dca721c46aaba8a58d865f6
Cr-Commit-Position: refs/heads/master@{#325836}