MD Bookmarks: Add a confirmation dialog before opening many tabs

MD Bookmarks: Add a confirmation dialog before opening many tabs

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

Review-Url: https://codereview.chromium.org/2945513002
Cr-Commit-Position: refs/heads/master@{#482202}
Committed: https://chromium.googlesource.com/chromium/src/+/7fb9e13f4d35f2750367773271a4c8c6dcc622c7

[tsergeant, 2017-06-16 00:52:23]

Description was changed from

==========
MD Bookmarks: Add a confirmation dialog before opening many tabs

BUG=731486
==========

to

==========
MD Bookmarks: Add a confirmation dialog before opening many tabs

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

[tsergeant, 2017-06-16 00:53:40]

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

[commit-bot: I haz the power, 2017-06-16 00:53:55]

Dry run: CQ is trying da patch.

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

[tsergeant, 2017-06-16 01:05:50]

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

[commit-bot: I haz the power, 2017-06-16 01:06:06]

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-06-16 02:07:39]

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

[commit-bot: I haz the power, 2017-06-16 02:07:40]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tsergeant, 2017-06-16 04:04:55]

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

[tsergeant, 2017-06-16 06:41:31]

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

[commit-bot: I haz the power, 2017-06-16 06:41:42]

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-06-16 08:17:11]

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

[commit-bot: I haz the power, 2017-06-16 08:17:12]

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

[calamity, 2017-06-22 05:15:33]

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

https://codereview.chromium.org/2945513002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:339: var callback =
function() {
nit: openUrls?

https://codereview.chromium.org/2945513002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:357:
this.confirmOpenString_ =
Is this worth making a property? Can we just get the body of the dialog and set
it to this?

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

https://codereview.chromium.org/2945513002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/command_manager_test.js:21: var id = '3' +
i;
Oh boy.

https://codereview.chromium.org/2945513002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/command_manager_test.js:22:
bulkChildren.push(createItem(id, {url: 'http://' + id + '/'}));
nit: `http://${id}/`

https://codereview.chromium.org/2945513002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/command_manager_test.js:204: var dialog =
commandManager.$.openDialog.get();
nit: getIfExists?

[tsergeant, 2017-06-22 07:44:09]

Patchset #4 (id:60001) has been deleted

[tsergeant, 2017-06-22 07:46:29]

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

[commit-bot: I haz the power, 2017-06-22 07:46:50]

Dry run: CQ is trying da patch.

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

[tsergeant, 2017-06-22 07:47:31]

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

https://codereview.chromium.org/2945513002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:339: var callback =
function() {
On 2017/06/22 05:15:33, calamity wrote:
> nit: openUrls?

Changed it to openUrlsCallback, since I didn't really like having an openUrls()
function inside the openUrls_() function.

https://codereview.chromium.org/2945513002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:357:
this.confirmOpenString_ =
On 2017/06/22 05:15:33, calamity wrote:
> Is this worth making a property? Can we just get the body of the dialog and
set
> it to this?

Done.

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

https://codereview.chromium.org/2945513002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/command_manager_test.js:21: var id = '3' +
i;
On 2017/06/22 05:15:33, calamity wrote:
> Oh boy.

Acknowledged.

https://codereview.chromium.org/2945513002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/command_manager_test.js:22:
bulkChildren.push(createItem(id, {url: 'http://' + id + '/'}));
On 2017/06/22 05:15:33, calamity wrote:
> nit: `http://${id}/`

Done.

https://codereview.chromium.org/2945513002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/command_manager_test.js:204: var dialog =
commandManager.$.openDialog.get();
On 2017/06/22 05:15:33, calamity wrote:
> nit: getIfExists?

Done.

[commit-bot: I haz the power, 2017-06-22 09:33:10]

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

[commit-bot: I haz the power, 2017-06-22 09:33:11]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-06-26 05:19:50]

lgtm

[tsergeant, 2017-06-26 05:27:58]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-06-26 05:28:08]

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-06-26 06:35:22]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498454878903240,
"parent_rev": "9a7afd1047943bd54f2d28908deb81d32df01c3b", "commit_rev":
"7fb9e13f4d35f2750367773271a4c8c6dcc622c7"}

[commit-bot: I haz the power, 2017-06-26 06:35:33]

Description was changed from

==========
MD Bookmarks: Add a confirmation dialog before opening many tabs

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

to

==========
MD Bookmarks: Add a confirmation dialog before opening many tabs

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

Review-Url: https://codereview.chromium.org/2945513002
Cr-Commit-Position: refs/heads/master@{#482202}
Committed:
https://chromium.googlesource.com/chromium/src/+/7fb9e13f4d35f2750367773271a4...
==========

[commit-bot: I haz the power, 2017-06-26 06:35:34]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/7fb9e13f4d35f2750367773271a4...