MD Bookmarks: Pull context menu into separate element

MD Bookmarks: Pull context menu into separate element

This creates a new element, <bookmarks-command-manager>, which is
responsible for showing the context menu, hiding commands which are
unavailable and responding to command clicks. Pulling out the context
menu like this will make it easier to reuse in multiple places, and
make it simpler to add keyboard shortcuts for menu items.

BUG=692827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2834493006
Cr-Commit-Position: refs/heads/master@{#469592}
Committed: https://chromium.googlesource.com/chromium/src/+/77365188782303a17f15571a0f253dcb5cc0e0b9

[tsergeant, 2017-04-21 03:53:14]

Description was changed from

==========
MD Bookmarks: Pull context menu into separate element, add keyboard shortcuts

This creates a new element, <bookmarks-command-manager>, which is
responsible for showing the context menu, hiding commands which are
unavailable, and responding to keyboard shortcuts.

This also makes it possible to delete multiple items, by shift-selecting
them and then selecting 'delete' from the right-click context menu.

BUG=692827
==========

to

==========
MD Bookmarks: Pull context menu into separate element, add keyboard shortcuts

This creates a new element, <bookmarks-command-manager>, which is
responsible for showing the context menu, hiding commands which are
unavailable, and responding to keyboard shortcuts.

This also makes it possible to delete multiple items, by shift-selecting
them and then selecting 'delete' from the right-click context menu.

BUG=692827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2017-04-21 04:25:17]

Description was changed from

==========
MD Bookmarks: Pull context menu into separate element, add keyboard shortcuts

This creates a new element, <bookmarks-command-manager>, which is
responsible for showing the context menu, hiding commands which are
unavailable, and responding to keyboard shortcuts.

This also makes it possible to delete multiple items, by shift-selecting
them and then selecting 'delete' from the right-click context menu.

BUG=692827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Bookmarks: Pull context menu into separate element

This creates a new element, <bookmarks-command-manager>, which is
responsible for showing the context menu, hiding commands which are
unavailable and responding to command clicks. Pulling out the context
menu like this will make it easier to reuse in multiple places, and
make it simpler to add keyboard shortcuts for menu items.

BUG=692827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2017-04-21 06:33:02]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 06:33:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tsergeant, 2017-04-21 06:33:25]

Description was changed from

==========
MD Bookmarks: Pull context menu into separate element

This creates a new element, <bookmarks-command-manager>, which is
responsible for showing the context menu, hiding commands which are
unavailable and responding to command clicks. Pulling out the context
menu like this will make it easier to reuse in multiple places, and
make it simpler to add keyboard shortcuts for menu items.

BUG=692827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Bookmarks: Pull context menu into separate element

This creates a new element, <bookmarks-command-manager>, which is
responsible for showing the context menu, hiding commands which are
unavailable and responding to command clicks. Pulling out the context
menu like this will make it easier to reuse in multiple places, and
make it simpler to add keyboard shortcuts for menu items.

BUG=692827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2017-04-21 06:33:33]

tsergeant@chromium.org changed reviewers:
+ calamity@chromium.org

[tsergeant, 2017-04-21 06:36:21]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 06:36:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 08:32:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 08:32:24]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-04-24 05:20:35]

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/command_manager.html (right):

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.html:11: </style>
nit: Unnecessary?

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.html:21:
hidden$="[[!canCopy(menuIds_)]]"
I'm conflicted about whether I want this to be canCopy or canExecute('copy').
When keyboard shortcuts come in, it'll need to be able to ask if certain things
are do-able, so it might be nicer to have something generic? Dunno, we can cross
that bridge when we get to it I suppose.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.html:30: </button>
I mean, looking at this, there's some part of me that wants to believe in a
dom-repeat solution, where given a bunch of commands to put in the menu, the
dropdown menu would generate and correctly figure out what to hide and show at
any given time.

This isn't terrible though, and is probably much simpler.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/command_manager.js (right):

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:35: * Display the
command context menu at the given |x|, |y| window
nit: at (|x|, |y|) in window co-ordinates.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:129: });
There's a lot of depressing options here.

How do you feel about:

Array.from(itemIds).some(function() { return predicate(nodes[id]); })

https://codereview.chromium.org/2834493006/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/command_manager_test.js (right):

https://codereview.chromium.org/2834493006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/command_manager_test.js:25: test('canCopy
only single URLs', function() {
nit: single URL items

[tsergeant, 2017-04-24 07:24:12]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 07:24:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tsergeant, 2017-04-24 07:26:39]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 07:26:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tsergeant, 2017-04-24 07:28:23]

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/command_manager.html (right):

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.html:11: </style>
On 2017/04/24 05:20:35, calamity wrote:
> nit: Unnecessary?

Oops, done.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.html:21:
hidden$="[[!canCopy(menuIds_)]]"
On 2017/04/24 05:20:35, calamity wrote:
> I'm conflicted about whether I want this to be canCopy or canExecute('copy').
> When keyboard shortcuts come in, it'll need to be able to ask if certain
things
> are do-able, so it might be nicer to have something generic? Dunno, we can
cross
> that bridge when we get to it I suppose.

I actually used to have it like canExecute('copy'). I'm not too attached to
either method, but I think I find this one nicer to read.

My intention for keyboard shortcuts is that there'll be a list of registered
commands with their canExecute/handle functions which we can use to figure that
stuff out.

I've sent you a short design doc I was working on for this -- feel free to make
suggestions about the design there.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.html:30: </button>
On 2017/04/24 05:20:35, calamity wrote:
> I mean, looking at this, there's some part of me that wants to believe in a
> dom-repeat solution, where given a bunch of commands to put in the menu, the
> dropdown menu would generate and correctly figure out what to hide and show at
> any given time.
> 
> This isn't terrible though, and is probably much simpler.

The biggest thing that gets in the way of the dom-repeat solution is that we're
going to have a <hr> separator in the middle of the context menu. Doing it this
way is much simpler, even though it's pretty verbose.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/command_manager.js (right):

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:35: * Display the
command context menu at the given |x|, |y| window
On 2017/04/24 05:20:35, calamity wrote:
> nit: at (|x|, |y|) in window co-ordinates.

Done.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:129: });
On 2017/04/24 05:20:35, calamity wrote:
> There's a lot of depressing options here.
> 
> How do you feel about:
> 
> Array.from(itemIds).some(function() { return predicate(nodes[id]); })

Done. Set is a really barebones type -- I wish it had some of those helper
functions on it.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/constants.js (right):

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/constants.js:22: var Command = {
Oops, this was a remnant of an earlier version of this CL.

https://codereview.chromium.org/2834493006/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/command_manager_test.js (right):

https://codereview.chromium.org/2834493006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/command_manager_test.js:25: test('canCopy
only single URLs', function() {
On 2017/04/24 05:20:35, calamity wrote:
> nit: single URL items

Done.

[commit-bot: I haz the power, 2017-04-24 08:45:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 08:45:56]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-04-26 03:43:44]

lgtm.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/command_manager.html (right):

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.html:21:
hidden$="[[!canCopy(menuIds_)]]"
On 2017/04/24 07:28:23, tsergeant wrote:
> On 2017/04/24 05:20:35, calamity wrote:
> > I'm conflicted about whether I want this to be canCopy or
canExecute('copy').
> > When keyboard shortcuts come in, it'll need to be able to ask if certain
> things
> > are do-able, so it might be nicer to have something generic? Dunno, we can
> cross
> > that bridge when we get to it I suppose.
> 
> I actually used to have it like canExecute('copy'). I'm not too attached to
> either method, but I think I find this one nicer to read.
> 
> My intention for keyboard shortcuts is that there'll be a list of registered
> commands with their canExecute/handle functions which we can use to figure
that
> stuff out.
> 
> I've sent you a short design doc I was working on for this -- feel free to
make
> suggestions about the design there.

Yeah, just saw it this morning. Sorry.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.html:30: </button>
On 2017/04/24 07:28:23, tsergeant wrote:
> On 2017/04/24 05:20:35, calamity wrote:
> > I mean, looking at this, there's some part of me that wants to believe in a
> > dom-repeat solution, where given a bunch of commands to put in the menu, the
> > dropdown menu would generate and correctly figure out what to hide and show
at
> > any given time.
> > 
> > This isn't terrible though, and is probably much simpler.
> 
> The biggest thing that gets in the way of the dom-repeat solution is that
we're
> going to have a <hr> separator in the middle of the context menu. Doing it
this
> way is much simpler, even though it's pretty verbose.

Fair enough.

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/command_manager.js (right):

https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:129: });
On 2017/04/24 07:28:23, tsergeant wrote:
> On 2017/04/24 05:20:35, calamity wrote:
> > There's a lot of depressing options here.
> > 
> > How do you feel about:
> > 
> > Array.from(itemIds).some(function() { return predicate(nodes[id]); })
> 
> Done. Set is a really barebones type -- I wish it had some of those helper
> functions on it.

Yeah =(

[tsergeant, 2017-04-26 05:49:04]

I think you're right: using common functions and a Command enum seems to be the
better way to go.

I've updated this CL with the changes (can you please take another quick look?),
and I'll update the design doc now

[tsergeant, 2017-04-26 05:49:06]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 05:49:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 07:04:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 07:04:37]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-04-27 03:42:04]

https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/command_manager.js (right):

https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/command_manager.js:68: return !!node.url;
Wait, I should be able to copy multiple items, which may or may not be folders,
right?.

[tsergeant, 2017-04-27 04:08:37]

https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/command_manager.js (right):

https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/command_manager.js:68: return !!node.url;
On 2017/04/27 03:42:04, calamity wrote:
> Wait, I should be able to copy multiple items, which may or may not be
folders,
> right?.

You can in the current BMM.

However, the new spec only shows "Copy URL" as in the menu for items (not for
folders), and the new string implies that you're only expected to do one at a
time. I interpreted this to mean we are meant to change the behavior, but I can
check with Alan if you'd like.

[calamity, 2017-04-28 04:04:46]

On 2017/04/27 04:08:37, tsergeant wrote:
>
https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resourc...
> File chrome/browser/resources/md_bookmarks/command_manager.js (right):
> 
>
https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/md_bookmarks/command_manager.js:68: return
!!node.url;
> On 2017/04/27 03:42:04, calamity wrote:
> > Wait, I should be able to copy multiple items, which may or may not be
> folders,
> > right?.
> 
> You can in the current BMM.
> 
> However, the new spec only shows "Copy URL" as in the menu for items (not for
> folders), and the new string implies that you're only expected to do one at a
> time. I interpreted this to mean we are meant to change the behavior, but I
can
> check with Alan if you'd like.

Yes pls. I don't think we should kill this feature without some explicit
thought. It's not very hard too leave in either.

[tsergeant, 2017-05-05 02:44:50]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 02:45:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 02:48:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 02:48:51]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tsergeant, 2017-05-05 03:01:31]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2017-05-05 03:01:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
calamity@chromium.org
Link to the patchset: https://codereview.chromium.org/2834493006/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-05 03:02:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 04:02:36]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493953291012330,
"parent_rev": "28d7f9d69d553325b112a7a2db2c69bef5422178", "commit_rev":
"77365188782303a17f15571a0f253dcb5cc0e0b9"}

[commit-bot: I haz the power, 2017-05-05 04:04:00]

Description was changed from

==========
MD Bookmarks: Pull context menu into separate element

This creates a new element, <bookmarks-command-manager>, which is
responsible for showing the context menu, hiding commands which are
unavailable and responding to command clicks. Pulling out the context
menu like this will make it easier to reuse in multiple places, and
make it simpler to add keyboard shortcuts for menu items.

BUG=692827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Bookmarks: Pull context menu into separate element

This creates a new element, <bookmarks-command-manager>, which is
responsible for showing the context menu, hiding commands which are
unavailable and responding to command clicks. Pulling out the context
menu like this will make it easier to reuse in multiple places, and
make it simpler to add keyboard shortcuts for menu items.

BUG=692827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2834493006
Cr-Commit-Position: refs/heads/master@{#469592}
Committed:
https://chromium.googlesource.com/chromium/src/+/77365188782303a17f15571a0f25...
==========

[commit-bot: I haz the power, 2017-05-05 04:04:02]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/77365188782303a17f15571a0f25...