Description was changed from ========== MD Bookmarks: Enable the delete button in the toolbar overlay ...
3 years, 7 months ago
(2017-05-18 04:07:47 UTC)
#1
Description was changed from
==========
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
==========
to
==========
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
==========
tsergeant
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
3 years, 7 months ago
(2017-05-18 04:10:05 UTC)
#2
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/270842) android_clang_dbg_recipe on ...
3 years, 7 months ago
(2017-05-18 04:16:38 UTC)
#5
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
PTAL.
The actual change here is really small (hooray, code re-use!), but I couldn't
figure out any particularly good way to test it. Calling the 'onDeleteSelected'
method and checking that it calls the right thing in CommandManager is not
great.
Do you have any ideas? Do you want me to go ahead and write that test, or should
I just leave it. For what it's worth, somewhere down the line it will become
possible for canExecute(Command.DELETE) to be false, at which point we'll have
some more interesting and testable logic in Toolbar to deal with that.
tsergeant
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
3 years, 7 months ago
(2017-05-18 04:30:26 UTC)
#8
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/440738) linux_chromium_rel_ng on ...
3 years, 7 months ago
(2017-05-18 04:36:30 UTC)
#11
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/448237)
3 years, 7 months ago
(2017-05-18 08:25:51 UTC)
#16
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
https://codereview.chromium.org/2893833002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/toolbar.js (right):
https://codereview.chromium.org/2893833002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toolbar.js:106:
commandManager.handle(Command.DELETE, selection);
I do think it's worth having a test _somewhere_ that ensures that this button is
hooked up to the correct functionality.
I don't know if mocking our commandManager like we did for TestStore is overkill
or not, but you can at least locally replace commandManager.handle in a test and
check that the right arguments come through on-tap.
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
https://codereview.chromium.org/2893833002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/toolbar.js (right):
https://codereview.chromium.org/2893833002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toolbar.js:106:
commandManager.handle(Command.DELETE, selection);
On 2017/05/19 03:54:19, calamity wrote:
> I do think it's worth having a test _somewhere_ that ensures that this button
is
> hooked up to the correct functionality.
>
> I don't know if mocking our commandManager like we did for TestStore is
overkill
> or not, but you can at least locally replace commandManager.handle in a test
and
> check that the right arguments come through on-tap.
Okay, I've added a test which will hopefully fulfill the goal of breaking
annoyingly if the button stops working.
I've pulled out a simple TestCommandManager using the existing method to check
if commands fire. I've just stuck it in test_util for now, but I guess I can
move it if you prefer.
tsergeant
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
3 years, 7 months ago
(2017-05-22 23:51:29 UTC)
#19
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/451649)
3 years, 7 months ago
(2017-05-23 01:12:47 UTC)
#22
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
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
lgtm
https://codereview.chromium.org/2893833002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/toolbar_test.js (right):
https://codereview.chromium.org/2893833002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/toolbar_test.js:26: commandManager = new
TestCommandManager();
On 2017/05/23 04:11:36, tsergeant wrote:
> On 2017/05/23 03:42:55, calamity wrote:
> > Does this also need to get assigned to bookmarks.CommandManager.instance_?
It
> > might be worth putting it in the TestCommandManager() constructor, or at
least
> a
> > method (e.g HijackSingleton()).
> >
> > Same with TestStore.
>
> CommandManager is a bit weird, because it takes over the singleton for itself
> during attached:
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/md_bookmarks/co...
>
> That means that appending it to the body (and removing it during cleanup) will
> automatically take care of the singleton.
Oh yeah, I totally forgot about that subtlety.
>
> However, I don't think we want to do this in the constructor automatically, in
> case the test is doing something careful with the body.
>
> Do you agree? Do you still want me to change TestStore anyway?
We can defer this I think.
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
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495522235427490, "parent_rev": "d48794b9728276c1183b0c56f8cb7afe71b7bca4", "commit_rev": "62eb5de09e830c95b1d27688d2bac91a6390f0e7"}
3 years, 7 months ago
(2017-05-23 08:16:59 UTC)
#35
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495522235427490,
"parent_rev": "d48794b9728276c1183b0c56f8cb7afe71b7bca4", "commit_rev":
"62eb5de09e830c95b1d27688d2bac91a6390f0e7"}
commit-bot: I haz the power
Description was changed from ========== MD Bookmarks: Enable the delete button in the toolbar overlay ...
3 years, 7 months ago
(2017-05-23 08:17:15 UTC)
#36
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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/+/62eb5de09e830c95b1d27688d2ba...
==========
commit-bot: I haz the power
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/62eb5de09e830c95b1d27688d2bac91a6390f0e7
3 years, 7 months ago
(2017-05-23 08:17:16 UTC)
#37
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
Base URL:
Comments: 9