[MD Bookmarks] Add edit folder.

[MD Bookmarks] Add edit folder.

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

Review-Url: https://codereview.chromium.org/2666033002
Cr-Commit-Position: refs/heads/master@{#448556}
Committed: https://chromium.googlesource.com/chromium/src/+/1dc1289009fc9dab98f5f840ff342ecaa1238c98

[angelayang, 2017-01-31 06:07:58]

Description was changed from

==========
[MD Bookmarks] Add edit folder.

BUG=658980
==========

to

==========
[MD Bookmarks] Add edit folder.

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

[angelayang, 2017-01-31 06:10:48]

angelayang@google.com changed reviewers:
+ calamity@chromium.org, jiaxi@google.com, tsergeant@chromium.org

[angelayang, 2017-01-31 06:10:49]

Cl that adds in the missing folder editor for the folder items.
Take a look thankyou.

[angelayang, 2017-01-31 06:59:39]

On 2017/01/31 06:10:49, angelayang wrote:
> Cl that adds in the missing folder editor for the folder items.
> Take a look thankyou.

Would you guys mind if i slipped removing the separator lines into this cl too
as a patch?

[jiaxi, 2017-02-01 00:31:23]

One change per CL would be nice.

https://codereview.chromium.org/2666033002/diff/1/chrome/app/bookmarks_string...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/2666033002/diff/1/chrome/app/bookmarks_string...
chrome/app/bookmarks_strings.grdp:331: Edit Folder
Rename Folder.

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pag...

https://codereview.chromium.org/2666033002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2666033002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.js:91: },
Hmm... I'm a bit confused here. Where is this function getting used?

[angelayang, 2017-02-01 02:47:34]

I fixed the issues with the cl. have a look- thanks :)

https://codereview.chromium.org/2666033002/diff/1/chrome/app/bookmarks_string...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/2666033002/diff/1/chrome/app/bookmarks_string...
chrome/app/bookmarks_strings.grdp:331: Edit Folder
On 2017/02/01 00:31:23, jiaxi wrote:
> Rename Folder.
> 
>
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pag...

Thanks, missed that.

https://codereview.chromium.org/2666033002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2666033002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.js:91: },
On 2017/02/01 00:31:23, jiaxi wrote:
> Hmm... I'm a bit confused here. Where is this function getting used?

Oops yes sorry i forgot to commit where i did use it. It's supposed to be used
in the modal.

[jiaxi, 2017-02-01 03:47:42]

lgtm

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

Generally looks good.

One thing is that Folders should have a different menu to Items, with 'Rename'
instead of 'Edit'.

Do you plan to do this? Doesn't necessarily need to be this CL.

https://codereview.chromium.org/2666033002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2666033002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:35: // TODO(jiaxi): change these
dummy click event handlers later.
Can you get rid of this TODO while you're here? It doesn't apply anymore.

https://codereview.chromium.org/2666033002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:68:
chrome.bookmarks.update(this.menuItem_.id, {
I think you need to make it so that 'url' isn't sent for folder renames. I just
tried running chrome.bookmarks.update in devtools to change the URL of a folder,
and it completely crashed my browser, which is something we'd like to avoid.

[angelayang, 2017-02-01 07:19:34]

I fixed the folder's menu and a few other issues. take a look thanks.

https://codereview.chromium.org/2666033002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2666033002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:35: // TODO(jiaxi): change these
dummy click event handlers later.
On 2017/02/01 06:04:21, tsergeant wrote:
> Can you get rid of this TODO while you're here? It doesn't apply anymore.

Done.

https://codereview.chromium.org/2666033002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:68:
chrome.bookmarks.update(this.menuItem_.id, {
On 2017/02/01 06:04:21, tsergeant wrote:
> I think you need to make it so that 'url' isn't sent for folder renames. I
just
> tried running chrome.bookmarks.update in devtools to change the URL of a
folder,
> and it completely crashed my browser, which is something we'd like to avoid.

Done.

[tsergeant, 2017-02-01 23:46:48]

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

https://codereview.chromium.org/2666033002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:66: onSaveEditTap_: function() {
You can do this with a single call to chrome.bookmarks:

var edit = {'title': this.menuItem_.title};
if (this.menuItem_.url)
  edit['url'] = this.menuItem_.url;
chrome.bookmarks.update(this.menuItem_.id, edit);

https://codereview.chromium.org/2666033002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:93: getMenuActionLabel_:
function() {
Should have 'edit' in the function name since it's just for that menu item.
Maybe:

getEditActionLabel_()

[angelayang, 2017-02-02 00:57:08]

Made fixes to style~ thankyou :)

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

https://codereview.chromium.org/2666033002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:66: onSaveEditTap_: function() {
On 2017/02/01 23:46:48, tsergeant wrote:
> You can do this with a single call to chrome.bookmarks:
> 
> var edit = {'title': this.menuItem_.title};
> if (this.menuItem_.url)
>   edit['url'] = this.menuItem_.url;
> chrome.bookmarks.update(this.menuItem_.id, edit);
perfect thanks

https://codereview.chromium.org/2666033002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:93: getMenuActionLabel_:
function() {
On 2017/02/01 23:46:48, tsergeant wrote:
> Should have 'edit' in the function name since it's just for that menu item.
> Maybe:
> 
> getEditActionLabel_()

Done.

[tsergeant, 2017-02-02 02:24:28]

lgtm

https://codereview.chromium.org/2666033002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc (right):

https://codereview.chromium.org/2666033002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:43:
AddLocalizedString(source, "renameFolderTitle",
Nit: Sort this one alphabetically.

[calamity, 2017-02-02 04:40:53]

This could do with a simple test, just to ensure the field is hidden correctly.

https://codereview.chromium.org/2666033002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2666033002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:17: hideUrlInput_: Boolean,
Can we get rid of this property? Or at least change it to isFolder or the like?

[angelayang, 2017-02-02 05:04:56]

Removed the hideURL property and added a unit test
Thanks :)

[angelayang, 2017-02-02 05:29:49]

On 2017/02/02 05:04:56, angelayang wrote:
> Removed the hideURL property and added a unit test
> Thanks :)

Sorry I have to add in a small UI test for list.

[angelayang, 2017-02-02 05:32:47]

Patchset #6 (id:100001) has been deleted

[angelayang, 2017-02-02 05:33:52]

Okay, everything is ready, take a look :)

https://codereview.chromium.org/2666033002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2666033002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:17: hideUrlInput_: Boolean,
On 2017/02/02 04:40:53, calamity wrote:
> Can we get rid of this property? Or at least change it to isFolder or the
like?

Done.

[angelayang, 2017-02-02 05:34:49]

Patchset #6 (id:110001) has been deleted

[calamity, 2017-02-02 06:49:13]

https://codereview.chromium.org/2666033002/diff/130001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/list_test.js (right):

https://codereview.chromium.org/2666033002/diff/130001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/list_test.js:24: });
Also mock out chrome.bookmarks.update and make sure the right things get given
to it for folders and for URLs for an edit.

[angelayang, 2017-02-03 02:26:24]

Last bit of testing added, thank you.

https://codereview.chromium.org/2666033002/diff/130001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/list_test.js (right):

https://codereview.chromium.org/2666033002/diff/130001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/list_test.js:24: });
On 2017/02/02 06:49:13, calamity wrote:
> Also mock out chrome.bookmarks.update and make sure the right things get given
> to it for folders and for URLs for an edit.

Done.

[angelayang, 2017-02-03 02:27:41]

Patchset #7 (id:150001) has been deleted

[calamity, 2017-02-03 03:45:24]

https://codereview.chromium.org/2666033002/diff/170001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/list_test.js (right):

https://codereview.chromium.org/2666033002/diff/170001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/list_test.js:47: list.onSaveEditTap_();
If you're gonna open the actual dialog, seems a shame to not actually
MockInteractions.tap('#save-button')

[angelayang, 2017-02-06 00:49:39]

Fixes made, take a look :)

https://codereview.chromium.org/2666033002/diff/170001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/list_test.js (right):

https://codereview.chromium.org/2666033002/diff/170001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/list_test.js:47: list.onSaveEditTap_();
On 2017/02/03 03:45:24, calamity wrote:
> If you're gonna open the actual dialog, seems a shame to not actually
> MockInteractions.tap('#save-button')

Sounds good

[calamity, 2017-02-06 03:23:20]

lgtm

[angelayang, 2017-02-06 05:31:03]

The CQ bit was checked by angelayang@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 05:31:15]

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

[angelayang, 2017-02-06 05:54:57]

The CQ bit was unchecked by angelayang@google.com

[angelayang, 2017-02-07 05:25:56]

The CQ bit was checked by angelayang@google.com

[angelayang, 2017-02-07 05:25:56]

The patchset sent to the CQ was uploaded after l-g-t-m from jiaxi@google.com,
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2666033002/#ps190001
(title: "Test with mockinteractions in list test.")

[commit-bot: I haz the power, 2017-02-07 05:26: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-02-07 05:33:17]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1486445156503010,
"parent_rev": "efed310cc305f0d89f9d1aa49fb5e0d4ec9ff2cd", "commit_rev":
"8d6aab87dd9b90db33b8c4492e6b7be17008c892"}

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

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1486445156503010,
"parent_rev": "7032e2079c84274cc432a3ad2a09e96da8fcd162", "commit_rev":
"1dc1289009fc9dab98f5f840ff342ecaa1238c98"}

[commit-bot: I haz the power, 2017-02-07 05:40:59]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1486445156503010,
"parent_rev": "7032e2079c84274cc432a3ad2a09e96da8fcd162", "commit_rev":
"1dc1289009fc9dab98f5f840ff342ecaa1238c98"}

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

Description was changed from

==========
[MD Bookmarks] Add edit folder.

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

to

==========
[MD Bookmarks] Add edit folder.

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

Review-Url: https://codereview.chromium.org/2666033002
Cr-Commit-Position: refs/heads/master@{#448556}
Committed:
https://chromium.googlesource.com/chromium/src/+/1dc1289009fc9dab98f5f840ff34...
==========

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

Committed patchset #8 (id:190001) as
https://chromium.googlesource.com/chromium/src/+/1dc1289009fc9dab98f5f840ff34...