Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(20)

Issue 2893833002: MD Bookmarks: Enable the delete button in the toolbar overlay (Closed)

Created:
3 years, 7 months ago by tsergeant
Modified:
3 years, 7 months ago
Reviewers:
calamity
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, vitalyp+closure_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, jlklein+watch-closure_chromium.org, dbeam+watch-closure_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

MD Bookmarks: Enable the delete button in the toolbar overlay The Delete button in the overlay which appears when selecting more than one item will now delete all selected items when clicked. BUG=692827 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2893833002 Cr-Commit-Position: refs/heads/master@{#473835} Committed: https://chromium.googlesource.com/chromium/src/+/62eb5de09e830c95b1d27688d2bac91a6390f0e7

Patch Set 1 #

Patch Set 2 : Reparent #

Total comments: 2

Patch Set 3 : Add a test #

Total comments: 5

Patch Set 4 : Split out file #

Total comments: 2

Patch Set 5 : Fix nit #

Messages

Total messages: 37 (27 generated)
tsergeant
PTAL. The actual change here is really small (hooray, code re-use!), but I couldn't figure ...
3 years, 7 months ago (2017-05-18 04:30:16 UTC) #7
calamity
https://codereview.chromium.org/2893833002/diff/40001/chrome/browser/resources/md_bookmarks/toolbar.js File chrome/browser/resources/md_bookmarks/toolbar.js (right): https://codereview.chromium.org/2893833002/diff/40001/chrome/browser/resources/md_bookmarks/toolbar.js#newcode106 chrome/browser/resources/md_bookmarks/toolbar.js:106: commandManager.handle(Command.DELETE, selection); I do think it's worth having a ...
3 years, 7 months ago (2017-05-19 03:54:19 UTC) #17
tsergeant
https://codereview.chromium.org/2893833002/diff/40001/chrome/browser/resources/md_bookmarks/toolbar.js File chrome/browser/resources/md_bookmarks/toolbar.js (right): https://codereview.chromium.org/2893833002/diff/40001/chrome/browser/resources/md_bookmarks/toolbar.js#newcode106 chrome/browser/resources/md_bookmarks/toolbar.js:106: commandManager.handle(Command.DELETE, selection); On 2017/05/19 03:54:19, calamity wrote: > I ...
3 years, 7 months ago (2017-05-22 23:51:25 UTC) #18
calamity
https://codereview.chromium.org/2893833002/diff/60001/chrome/test/data/webui/md_bookmarks/test_util.js File chrome/test/data/webui/md_bookmarks/test_util.js (right): https://codereview.chromium.org/2893833002/diff/60001/chrome/test/data/webui/md_bookmarks/test_util.js#newcode145 chrome/test/data/webui/md_bookmarks/test_util.js:145: function TestCommandManager() { Yeah, probably best to have this ...
3 years, 7 months ago (2017-05-23 03:42:56 UTC) #23
tsergeant
https://codereview.chromium.org/2893833002/diff/60001/chrome/test/data/webui/md_bookmarks/test_util.js File chrome/test/data/webui/md_bookmarks/test_util.js (right): https://codereview.chromium.org/2893833002/diff/60001/chrome/test/data/webui/md_bookmarks/test_util.js#newcode145 chrome/test/data/webui/md_bookmarks/test_util.js:145: function TestCommandManager() { On 2017/05/23 03:42:55, calamity wrote: > ...
3 years, 7 months ago (2017-05-23 04:11:37 UTC) #25
calamity
lgtm https://codereview.chromium.org/2893833002/diff/60001/chrome/test/data/webui/md_bookmarks/toolbar_test.js File chrome/test/data/webui/md_bookmarks/toolbar_test.js (right): https://codereview.chromium.org/2893833002/diff/60001/chrome/test/data/webui/md_bookmarks/toolbar_test.js#newcode26 chrome/test/data/webui/md_bookmarks/toolbar_test.js:26: commandManager = new TestCommandManager(); On 2017/05/23 04:11:36, tsergeant ...
3 years, 7 months ago (2017-05-23 05:34:19 UTC) #26
calamity
https://codereview.chromium.org/2893833002/diff/100001/chrome/test/data/webui/md_bookmarks/test_command_manager.js File chrome/test/data/webui/md_bookmarks/test_command_manager.js (right): https://codereview.chromium.org/2893833002/diff/100001/chrome/test/data/webui/md_bookmarks/test_command_manager.js#newcode25 chrome/test/data/webui/md_bookmarks/test_command_manager.js:25: assertDeepEquals(normalizeSet(lastCommandIds), ids); nit: Flip the assert args here and ...
3 years, 7 months ago (2017-05-23 05:46:03 UTC) #27
tsergeant
https://codereview.chromium.org/2893833002/diff/100001/chrome/test/data/webui/md_bookmarks/test_command_manager.js File chrome/test/data/webui/md_bookmarks/test_command_manager.js (right): https://codereview.chromium.org/2893833002/diff/100001/chrome/test/data/webui/md_bookmarks/test_command_manager.js#newcode25 chrome/test/data/webui/md_bookmarks/test_command_manager.js:25: assertDeepEquals(normalizeSet(lastCommandIds), ids); On 2017/05/23 05:46:03, calamity wrote: > nit: ...
3 years, 7 months ago (2017-05-23 06:50:42 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2893833002/120001
3 years, 7 months ago (2017-05-23 06:50:52 UTC) #34
commit-bot: I haz the power
3 years, 7 months ago (2017-05-23 08:17:16 UTC) #37
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/62eb5de09e830c95b1d27688d2ba...

Powered by Google App Engine
This is Rietveld 408576698