MD Bookmarks: Extract bookmark editing into a <bookmark-edit-dialog> element

MD Bookmarks: Extract bookmark editing into a <bookmarks-edit-dialog> element

This element will allow us to reuse editing markup/logic for adding new
bookmarks. The change also makes it easy to defer rendering of the dialog
until it is needed (using cr-lazy-render), which makes a small impact on
page load time.

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

Review-Url: https://codereview.chromium.org/2742583003
Cr-Commit-Position: refs/heads/master@{#458658}
Committed: https://chromium.googlesource.com/chromium/src/+/93582461cb4310f9ae81bd7fd3727917065f42c5

[tsergeant, 2017-03-09 04:55:23]

Description was changed from

==========
MD Bookmarks: Extract bookmark editing into a <bookmark-edit-dialog> element

This element will allow us to reuse editing markup/logic for adding new
bookmarks. The change also makes it easy to defer rendering of the dialog
until it is needed (using cr-lazy-render), which makes a small impact on
page load time.

BUG=692821
==========

to

==========
MD Bookmarks: Extract bookmark editing into a <bookmark-edit-dialog> element

This element will allow us to reuse editing markup/logic for adding new
bookmarks. The change also makes it easy to defer rendering of the dialog
until it is needed (using cr-lazy-render), which makes a small impact on
page load time.

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

[tsergeant, 2017-03-09 04:57:07]

Description was changed from

==========
MD Bookmarks: Extract bookmark editing into a <bookmark-edit-dialog> element

This element will allow us to reuse editing markup/logic for adding new
bookmarks. The change also makes it easy to defer rendering of the dialog
until it is needed (using cr-lazy-render), which makes a small impact on
page load time.

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

to

==========
MD Bookmarks: Extract bookmark editing into a <bookmarks-edit-dialog> element

This element will allow us to reuse editing markup/logic for adding new
bookmarks. The change also makes it easy to defer rendering of the dialog
until it is needed (using cr-lazy-render), which makes a small impact on
page load time.

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

[tsergeant, 2017-03-09 05:41:29]

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

[tsergeant, 2017-03-09 05:41:30]

PTAL (this is low priority though, so it doesn't matter if you don't get to it
right away)

I really like <bookmarks-edit-dialog>. It's self contained, does one thing well,
and is easy to test.

[tsergeant, 2017-03-09 05:41:41]

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

[commit-bot: I haz the power, 2017-03-09 05:41:59]

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-03-09 06:30:13]

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

[commit-bot: I haz the power, 2017-03-09 06:30:14]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-03-15 00:20:38]

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

[commit-bot: I haz the power, 2017-03-15 00:21:31]

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-03-15 01:55:53]

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

[commit-bot: I haz the power, 2017-03-15 01:55:54]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-03-15 03:42:06]

I've just updated this CL with some small changes.

[tsergeant, 2017-03-15 03:42:14]

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

[commit-bot: I haz the power, 2017-03-15 03:42:47]

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-03-15 03:53:07]

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

[commit-bot: I haz the power, 2017-03-15 03:53:07]

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

[tsergeant, 2017-03-15 04:03:33]

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

[commit-bot: I haz the power, 2017-03-15 04:04:03]

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-03-15 05:02:15]

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

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

Dry run: This issue passed the CQ dry run.

[calamity, 2017-03-20 05:04:51]

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

https://codereview.chromium.org/2742583003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/edit_dialog.js:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
Sooo last year.

https://codereview.chromium.org/2742583003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/edit_dialog.js:23: editNode:
function(editItem) {
I think I'd prefer showForNode. It's not obvious that editNode does a UI thing.

https://codereview.chromium.org/2742583003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/edit_dialog.js:58: },
Can has Press enter to save?

https://codereview.chromium.org/2742583003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/edit_dialog_test.js (right):

https://codereview.chromium.org/2742583003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/edit_dialog_test.js:7: var lastUpdate = {};
nit: rm obj creat.

[tsergeant, 2017-03-20 06:02:40]

Patchset #7 (id:120001) has been deleted

[tsergeant, 2017-03-20 06:03:18]

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

https://codereview.chromium.org/2742583003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/edit_dialog.js:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/03/20 05:04:50, calamity wrote:
> Sooo last year.

Done.

https://codereview.chromium.org/2742583003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/edit_dialog.js:23: editNode:
function(editItem) {
On 2017/03/20 05:04:50, calamity wrote:
> I think I'd prefer showForNode. It's not obvious that editNode does a UI
thing.

The difficulty with naming this is that there'll eventually be a way to add a
new bookmark using this element. The intention was that there'd be an addNode()
method to do that.

You're right, though, that this doesn't come across as interactive. I've changed
it to 'showEditDialog' (so we can have 'showAddDialog' later).

WDYT?

https://codereview.chromium.org/2742583003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/edit_dialog.js:58: },
On 2017/03/20 05:04:50, calamity wrote:
> Can has Press enter to save?

Yup, this constantly annoys me.

I've added a TODO, so we can keep this CL to a fairly straightforward move with
no behavior changes.

https://codereview.chromium.org/2742583003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/edit_dialog_test.js (right):

https://codereview.chromium.org/2742583003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/edit_dialog_test.js:7: var lastUpdate = {};
On 2017/03/20 05:04:50, calamity wrote:
> nit: rm obj creat.

Done.

[calamity, 2017-03-22 00:36:51]

lgtm.

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

https://codereview.chromium.org/2742583003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/edit_dialog.js:23: editNode:
function(editItem) {
On 2017/03/20 06:03:18, tsergeant wrote:
> On 2017/03/20 05:04:50, calamity wrote:
> > I think I'd prefer showForNode. It's not obvious that editNode does a UI
> thing.
> 
> The difficulty with naming this is that there'll eventually be a way to add a
> new bookmark using this element. The intention was that there'd be an
addNode()
> method to do that.
> 
> You're right, though, that this doesn't come across as interactive. I've
changed
> it to 'showEditDialog' (so we can have 'showAddDialog' later).
> 
> WDYT?

Oh, I didn't realize this would be getting the add functionality as well. Are we
keeping the strings for Cancel and Save? Is it worth changing the name of this
to add_edit_dialog? Can we change onSaveEditTap_ to onSaveButtonTap_?

https://codereview.chromium.org/2742583003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/edit_dialog.js:58: },
On 2017/03/20 06:03:18, tsergeant wrote:
> On 2017/03/20 05:04:50, calamity wrote:
> > Can has Press enter to save?
> 
> Yup, this constantly annoys me.
> 
> I've added a TODO, so we can keep this CL to a fairly straightforward move
with
> no behavior changes.

Acknowledged.

[tsergeant, 2017-03-22 00:53:23]

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

https://codereview.chromium.org/2742583003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/edit_dialog.js:23: editNode:
function(editItem) {
On 2017/03/22 00:36:50, calamity wrote:
> On 2017/03/20 06:03:18, tsergeant wrote:
> > On 2017/03/20 05:04:50, calamity wrote:
> > > I think I'd prefer showForNode. It's not obvious that editNode does a UI
> > thing.
> > 
> > The difficulty with naming this is that there'll eventually be a way to add
a
> > new bookmark using this element. The intention was that there'd be an
> addNode()
> > method to do that.
> > 
> > You're right, though, that this doesn't come across as interactive. I've
> changed
> > it to 'showEditDialog' (so we can have 'showAddDialog' later).
> > 
> > WDYT?
> 
> Oh, I didn't realize this would be getting the add functionality as well. Are
we
> keeping the strings for Cancel and Save? Is it worth changing the name of this
> to add_edit_dialog? Can we change onSaveEditTap_ to onSaveButtonTap_?

-> Cancel and Save are the button names for the native Add dialog, so I think we
can keep them
-> I wanted to keep the element name short-ish, so I'd prefer not to have both
verbs. Happy to revisit during the CL for Add
-> Done, renamed button event handlers.

[tsergeant, 2017-03-22 00:54:54]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2017-03-22 00:54:54]

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/2742583003/#ps180001
(title: "Rename event handlers")

[commit-bot: I haz the power, 2017-03-22 00:55:52]

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-03-22 02:57:24]

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

[commit-bot: I haz the power, 2017-03-22 02:57:26]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[tsergeant, 2017-03-22 05:47:28]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-03-22 05:48:06]

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

[tsergeant, 2017-03-22 06:56:32]

The CQ bit was unchecked by tsergeant@chromium.org

[tsergeant, 2017-03-22 06:56:39]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-03-22 07:02:52]

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-03-22 07:15:20]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490165799263410,
"parent_rev": "11a82c3aaabd39fc3a0981a6e66bf870cc46d5e8", "commit_rev":
"93582461cb4310f9ae81bd7fd3727917065f42c5"}

[commit-bot: I haz the power, 2017-03-22 07:16:07]

Description was changed from

==========
MD Bookmarks: Extract bookmark editing into a <bookmarks-edit-dialog> element

This element will allow us to reuse editing markup/logic for adding new
bookmarks. The change also makes it easy to defer rendering of the dialog
until it is needed (using cr-lazy-render), which makes a small impact on
page load time.

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

to

==========
MD Bookmarks: Extract bookmark editing into a <bookmarks-edit-dialog> element

This element will allow us to reuse editing markup/logic for adding new
bookmarks. The change also makes it easy to defer rendering of the dialog
until it is needed (using cr-lazy-render), which makes a small impact on
page load time.

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

Review-Url: https://codereview.chromium.org/2742583003
Cr-Commit-Position: refs/heads/master@{#458658}
Committed:
https://chromium.googlesource.com/chromium/src/+/93582461cb4310f9ae81bd7fd372...
==========

[commit-bot: I haz the power, 2017-03-22 07:16:10]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/93582461cb4310f9ae81bd7fd372...