Edit function for bookmarks.

Edit function for bookmarks.

This cl enables the edit bookmarks function in the dropdown menu.

BUG=

[jiaxi, 2016-12-16 03:48:11]

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/list.html:62: </dialog>
The close button on the top right corner doesn't fit the spec, so is the fonts.
I probably shouldn't modify stuff inside cr-dialog. Let me know if I need to
change it.

[angelayang, 2016-12-16 04:00:09]

Just a rename of the listener in the store.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:95: handleBookmarkChanged_:
function(id, changeInfo) {
Rename function: bookmarkTitleChanged, i think Chris said on mine he didn't like
"handle" in the function names haha.

[angelayang, 2016-12-16 04:00:22]

On 2016/12/16 04:00:09, angelayang wrote:
> Just a rename of the listener in the store.
> 
>
https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
> File chrome/browser/resources/md_bookmarks/store.js (right):
> 
>
https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
> chrome/browser/resources/md_bookmarks/store.js:95: handleBookmarkChanged_:
> function(id, changeInfo) {
> Rename function: bookmarkTitleChanged, i think Chris said on mine he didn't
like
> "handle" in the function names haha.

lgtm

[jiaxi, 2016-12-16 04:02:54]

ฅ(•ㅅ•❀)ฅ

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:95: handleBookmarkChanged_:
function(id, changeInfo) {
On 2016/12/16 04:00:09, angelayang wrote:
> Rename function: bookmarkTitleChanged, i think Chris said on mine he didn't
like
> "handle" in the function names haha. 

Hmmm actually this one and the handleRemovec_ function above are all names from
the old bmm.js...Let's get some comments form Chris and Tim.

[calamity, 2016-12-16 05:20:25]

Round 1. TGIF time.

https://codereview.appspot.com/318100043/diff/20001/chrome/app/bookmarks_stri...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/app/bookmarks_stri...
chrome/app/bookmarks_strings.grdp:329: </message>
Just use IDS_CANCEL for this.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/list.html:57: <paper-button
id="saveButton" class="save-button"
id="save-button"

Also, why can't this use class="action-button"?

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/list.js:62: {'title':
this.menuItem_.title, 'url': this.menuItem_.url},
nit: trailing comma after url and then format again.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/list.js:68: cancel_: function() {
this.$['edit-bookmark'].cancel(); },
Try to keep all these button tap handlers named consistently.

onSaveEditTap_ and onCancelEditTap_ I think.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/shared_style.html (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/shared_style.html:54: }
These are crazy funky paddings. Try vertical-align instead. Are these paddings
actually necessary at all for the right measurements? Settings doesn't specify
it in its shared css at all.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:42:
chrome.bookmarks.onRemoved.addListener(this.handleRemoved_.bind(this));
Comment above this line: // Attach bookmarks API listeners.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:83: * @param {!Object} removeInfo
The information about removed.
Remove the description at the end of this line. It's not super useful.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:93: * @param {!Object} changeInfo
The information about how the node changed.
Same here.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:95: handleBookmarkChanged_:
function(id, changeInfo) {
On 2016/12/16 04:02:54, jiaxi wrote:
> On 2016/12/16 04:00:09, angelayang wrote:
> > Rename function: bookmarkTitleChanged, i think Chris said on mine he didn't
> like
> > "handle" in the function names haha. 
> 
> Hmmm actually this one and the handleRemovec_ function above are all names
from
> the old bmm.js...Let's get some comments form Chris and Tim.

This is for all bookmark changes, not just the title.

onBookmarkRemoved and onBookmarkChanged pls. Also, because this store.js is
going to get sicknasty large, we're going to want to keep these event handlers
under one big ol' heading.

Tim, what do you think of copying the break-style from say
app_list_presenter_delegate.cc:74?

Probably just need sections for private methods and bookmarks API event handlers
for now.

Also, I'm gonna be honest, before we land into chrome, there's gonna be one big
ol' review where I'll more than likely ask you both to change a bunch more
names. I'm fairly certain I haven't been adequately meticulous or consistent in
my reviews.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:98:
this.notifyPath(this.idToNodeMap_[id].path + '.title');
Why not this.set(...path + 'title', changeInfo.title)?

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:102:
this.notifyPath(this.idToNodeMap_[id].path + '.url');
And here.

Also, single line ifs and fors don't need braces.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/ui/webui/m...
File chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:28:
IDS_BOOKMARK_AX_BUBBLE_PAGE_BOOKMARK);
I would be very careful about this one. It's an accessibility message, not an
actual displayed string. This is one that we'll actually probably want a new
string.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:42:
source->AddLocalizedString("name", IDS_MD_BOOKMARK_MANAGER_NAME);
IDS_BOOKMARK_MANAGER_NAME_INPUT_PLACE_HOLDER?

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:43:
source->AddLocalizedString("saveEdit", IDS_MD_BOOKMARK_MANAGER_SAVE_BUTTON);
Can probably get away with IDS_SAVE here.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:47:
source->AddLocalizedString("url", IDS_MD_BOOKMARK_MANAGER_URL);
IDS_BOOKMARK_MANAGER_URL_INPUT_PLACE_HOLDER?

url and name are too ambiguous as string names.

[jiaxi, 2016-12-19 23:52:01]

https://codereview.appspot.com/318100043/diff/20001/chrome/app/bookmarks_stri...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/app/bookmarks_stri...
chrome/app/bookmarks_strings.grdp:329: </message>
On 2016/12/16 05:20:24, calamity wrote:
> Just use IDS_CANCEL for this.

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/app/bookmarks_stri...
chrome/app/bookmarks_strings.grdp:368: </message>
Should we wait for a patch to remove the &amp for this part?

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/list.html:57: <paper-button
id="saveButton" class="save-button"
On 2016/12/16 05:20:24, calamity wrote:
> id="save-button"
> 
> Also, why can't this use class="action-button"?

The action-button which is used elsewhere have multiple usages. So I thought it
might not be a proper name. I've changed it to action button

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/list.js:62: {'title':
this.menuItem_.title, 'url': this.menuItem_.url},
On 2016/12/16 05:20:24, calamity wrote:
> nit: trailing comma after url and then format again.

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/list.js:68: cancel_: function() {
this.$['edit-bookmark'].cancel(); },
On 2016/12/16 05:20:24, calamity wrote:
> Try to keep all these button tap handlers named consistently.
> 
> onSaveEditTap_ and onCancelEditTap_ I think.

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/shared_style.html (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/shared_style.html:54: }
On 2016/12/16 05:20:24, calamity wrote:
> These are crazy funky paddings. Try vertical-align instead. Are these paddings
> actually necessary at all for the right measurements? Settings doesn't specify
> it in its shared css at all.

vertical-align doesn't give the right padding (it should be 8.5px 16px, v-a
gives 9.1px 7.41px) In setting's spec[1], the dialog's button doesn't have
specified padding. But in bookmark's spec it does for some reason...?

[1]https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pages/03-Settings/specs#%2FSPEC-settings_dialogs.png%3Fz=width

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:42:
chrome.bookmarks.onRemoved.addListener(this.handleRemoved_.bind(this));
On 2016/12/16 05:20:24, calamity wrote:
> Comment above this line: // Attach bookmarks API listeners.

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:83: * @param {!Object} removeInfo
The information about removed.
On 2016/12/16 05:20:24, calamity wrote:
> Remove the description at the end of this line. It's not super useful.

It's been removed on this(https://codereview.appspot.com/316020043/) patch which
has this function in it. This patch is a child of the remove bookmark function
since they use the same data flow.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:93: * @param {!Object} changeInfo
The information about how the node changed.
On 2016/12/16 05:20:24, calamity wrote:
> Same here.

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:95: handleBookmarkChanged_:
function(id, changeInfo) {
On 2016/12/16 05:20:24, calamity wrote:
> On 2016/12/16 04:02:54, jiaxi wrote:
> > On 2016/12/16 04:00:09, angelayang wrote:
> > > Rename function: bookmarkTitleChanged, i think Chris said on mine he
didn't
> > like
> > > "handle" in the function names haha. 
> > 
> > Hmmm actually this one and the handleRemovec_ function above are all names
> from
> > the old bmm.js...Let's get some comments form Chris and Tim.
> 
> This is for all bookmark changes, not just the title.
> 
> onBookmarkRemoved and onBookmarkChanged pls. Also, because this store.js is
> going to get sicknasty large, we're going to want to keep these event handlers
> under one big ol' heading.
> 
> Tim, what do you think of copying the break-style from say
> app_list_presenter_delegate.cc:74?
> 
> Probably just need sections for private methods and bookmarks API event
handlers
> for now.
> 
> Also, I'm gonna be honest, before we land into chrome, there's gonna be one
big
> ol' review where I'll more than likely ask you both to change a bunch more
> names. I'm fairly certain I haven't been adequately meticulous or consistent
in
> my reviews.

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:98:
this.notifyPath(this.idToNodeMap_[id].path + '.title');
On 2016/12/16 05:20:24, calamity wrote:
> Why not this.set(...path + 'title', changeInfo.title)?

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:102:
this.notifyPath(this.idToNodeMap_[id].path + '.url');
On 2016/12/16 05:20:24, calamity wrote:
> And here.
> 
> Also, single line ifs and fors don't need braces.

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/ui/webui/m...
File chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:28:
IDS_BOOKMARK_AX_BUBBLE_PAGE_BOOKMARK);
On 2016/12/16 05:20:24, calamity wrote:
> I would be very careful about this one. It's an accessibility message, not an
> actual displayed string. This is one that we'll actually probably want a new
> string.

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:42:
source->AddLocalizedString("name", IDS_MD_BOOKMARK_MANAGER_NAME);
On 2016/12/16 05:20:24, calamity wrote:
> IDS_BOOKMARK_MANAGER_NAME_INPUT_PLACE_HOLDER?

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:43:
source->AddLocalizedString("saveEdit", IDS_MD_BOOKMARK_MANAGER_SAVE_BUTTON);
On 2016/12/16 05:20:24, calamity wrote:
> Can probably get away with IDS_SAVE here.

Done.

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:47:
source->AddLocalizedString("url", IDS_MD_BOOKMARK_MANAGER_URL);
On 2016/12/16 05:20:24, calamity wrote:
> IDS_BOOKMARK_MANAGER_URL_INPUT_PLACE_HOLDER?
> 
> url and name are too ambiguous as string names.

Done.

[jiaxi, 2016-12-20 03:05:04]

https://codereview.appspot.com/318100043/diff/20001/chrome/app/bookmarks_stri...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/app/bookmarks_stri...
chrome/app/bookmarks_strings.grdp:368: </message>
On 2016/12/19 23:52:00, jiaxi wrote:
> Should we wait for a patch to remove the &amp for this part?

These are all new strings(we have `Name:`, `URL:`, but no `Name`, `URL`

[calamity, 2016-12-20 03:54:54]

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/list.html:30: }
Sorry, I wasn't clear. If you add

<link rel="import" href="chrome://resources/cr_elements/shared_style_css.html">

and make :13

<style include="shared-style cr-shared-style">

then you'll have the action-button and cancel-button classes. In fact, at the
moment, cancel-button isn't even defined anywhere.

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:42: // Attach bookmarks API
listeners
Newline above this.

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:44:
chrome.bookmarks.onChanged.addListener(
Formatter should unwrap this.

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:123: /**
Check the previous patchset for a comment about sections here. Try not to mark
things as Done unless they're fully done and require no follow-up. If something
is half-done, explain what got done and why the other half isn't done.

https://codereview.appspot.com/318100043/diff/80001/chrome/app/bookmarks_stri...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.appspot.com/318100043/diff/80001/chrome/app/bookmarks_stri...
chrome/app/bookmarks_strings.grdp:330: <message
name="IDS_MD_BOOKMARK_MANAGER_EDIT_TITLE" desc="Title of the bookmark edit
page">
This isn't used.

https://codereview.appspot.com/318100043/diff/80001/chrome/browser/ui/webui/m...
File chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc (right):

https://codereview.appspot.com/318100043/diff/80001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:38:
AddLocalizedString("editBookmarkTitle", IDS_BOOKMARK_AX_BUBBLE_PAGE_BOOKMARK);
Use IDS_BOOKMARK_EDITOR_TITLE. Also go change it to be Sentence case instead of
Title Case.

https://codereview.appspot.com/318100043/diff/80001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:53:
IDS_MD_BOOKMARK_MANAGER_NAME_INPUT_PLACE_HOLDER);
As discussed, use pre-existing strings.

https://codereview.appspot.com/318100043/diff/80001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:58:
AddLocalizedString("urlInputPlaceHolder",
These names still don't describe the purpose of these strings very well. In
particular, they're not placeholders, we're just repurposing the placeholder
strings for input titles.

Use editDialogNameInput and editDialogUrlInput.

[jiaxi, 2016-12-20 07:06:54]

I've also added a test to this one

https://codereview.appspot.com/318100043/diff/80001/chrome/app/bookmarks_stri...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.appspot.com/318100043/diff/80001/chrome/app/bookmarks_stri...
chrome/app/bookmarks_strings.grdp:330: <message
name="IDS_MD_BOOKMARK_MANAGER_EDIT_TITLE" desc="Title of the bookmark edit
page">
On 2016/12/20 03:54:53, calamity wrote:
> This isn't used.

Done.

https://codereview.appspot.com/318100043/diff/80001/chrome/browser/ui/webui/m...
File chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc (right):

https://codereview.appspot.com/318100043/diff/80001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:38:
AddLocalizedString("editBookmarkTitle", IDS_BOOKMARK_AX_BUBBLE_PAGE_BOOKMARK);
On 2016/12/20 03:54:53, calamity wrote:
> Use IDS_BOOKMARK_EDITOR_TITLE. Also go change it to be Sentence case instead
of
> Title Case.

Done.

https://codereview.appspot.com/318100043/diff/80001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:53:
IDS_MD_BOOKMARK_MANAGER_NAME_INPUT_PLACE_HOLDER);
On 2016/12/20 03:54:53, calamity wrote:
> As discussed, use pre-existing strings.

Done.

https://codereview.appspot.com/318100043/diff/80001/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc:58:
AddLocalizedString("urlInputPlaceHolder",
On 2016/12/20 03:54:53, calamity wrote:
> These names still don't describe the purpose of these strings very well. In
> particular, they're not placeholders, we're just repurposing the placeholder
> strings for input titles.
> 
> Use editDialogNameInput and editDialogUrlInput.

Done.

[tsergeant, 2016-12-20 23:02:13]

There are a bunch of comments still in patchset 4 that it looks like you've
missed.

I'll take a closer look once they're done.

[jiaxi, 2016-12-20 23:51:47]

On 2016/12/20 23:02:13, tsergeant wrote:
> There are a bunch of comments still in patchset 4 that it looks like you've
> missed.
> 
> I'll take a closer look once they're done.

Oooooops total missed it. I've fixed problems there. ;)

[jiaxi, 2016-12-20 23:51:55]

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/list.html:30: }
On 2016/12/20 03:54:53, calamity wrote:
> Sorry, I wasn't clear. If you add
> 
> <link rel="import"
href="chrome://resources/cr_elements/shared_style_css.html">
> 
> and make :13
> 
> <style include="shared-style cr-shared-style">
> 
> then you'll have the action-button and cancel-button classes. In fact, at the
> moment, cancel-button isn't even defined anywhere.

Done.

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:42: // Attach bookmarks API
listeners
On 2016/12/20 03:54:53, calamity wrote:
> Newline above this.

Done.

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:44:
chrome.bookmarks.onChanged.addListener(
On 2016/12/20 03:54:53, calamity wrote:
> Formatter should unwrap this.

The formatter seems just left it here...

https://codereview.appspot.com/318100043/diff/60001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/store.js:123: /**
On 2016/12/20 03:54:53, calamity wrote:
> Check the previous patchset for a comment about sections here. Try not to mark
> things as Done unless they're fully done and require no follow-up. If
something
> is half-done, explain what got done and why the other half isn't done.

Done.

[jiaxi, 2016-12-22 04:55:09]

The merge is done for this CL as well :)

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
chrome/browser/resources/md_bookmarks/store.js:125: if (bookmarkNode.children) {
This early return will get changed in
this(https://codereview.appspot.com/315240043/) patch.

[calamity, 2017-01-03 05:30:48]

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
chrome/browser/resources/md_bookmarks/list.html:27: </style>
Undo.

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
chrome/browser/resources/md_bookmarks/list.js:61: function() {});
nit: remove the last argument here since it's optional.

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
chrome/browser/resources/md_bookmarks/store.js:43: // Attach bookmarks API
listeners
nit: Punctuation.

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
chrome/browser/resources/md_bookmarks/store.js:66: // bookmark-store, private:
bookmarks-store. And elsewhere.

https://codereview.appspot.com/318100043/diff/220001/chrome/test/data/webui/m...
File chrome/test/data/webui/md_bookmarks/store_test.js (right):

https://codereview.appspot.com/318100043/diff/220001/chrome/test/data/webui/m...
chrome/test/data/webui/md_bookmarks/store_test.js:202: // rootNode gets
notified.
This doesn't actually check the notification? This only ensures that the value
is actually getting set (which is trivial since store.idToNodeMap_['4'] is the
same object as this).

Just remove these lines. They add no value.

It would be good to check the right notifications get sent, but I'm not quite
sure how to do that. Any ideas Tim?

https://codereview.appspot.com/318100043/diff/220001/chrome/test/data/webui/m...
chrome/test/data/webui/md_bookmarks/store_test.js:219: };
This object seems a bit pointless. If you want to save some duplication, just
add var title and var url at the top of the test and use them everywhere in
here.

Inline this object on the next line.

[tsergeant, 2017-01-04 04:20:20]

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/shared_style.html (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/shared_style.html:54: }
On 2016/12/19 23:52:00, jiaxi wrote:
> On 2016/12/16 05:20:24, calamity wrote:
> > These are crazy funky paddings. Try vertical-align instead. Are these
paddings
> > actually necessary at all for the right measurements? Settings doesn't
specify
> > it in its shared css at all.
> 
> vertical-align doesn't give the right padding (it should be 8.5px 16px, v-a
> gives 9.1px 7.41px) In setting's spec[1], the dialog's button doesn't have
> specified padding. But in bookmark's spec it does for some reason...?
> 
>
[1]https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pages/03-Settings/specs#%2FSPEC-settings_dialogs.png%3Fz=width

In this case, it's more important for us to be consistent with other webui than
to match the exact pixel values from the spec.

This is because the cr-dialog and action-button styles are shared in many
different places. Even if the styles of cr-dialog don't match the spec (and they
don't!), it's okay because:

* Everywhere is consistent in being wrong
* When someone comes along to fix it, they can fix everyone at once, without
worrying about whether the bookmarks page will still look right because it went
and did something different.

(Also: There are some specs for the action buttons on the 'buttons' page of the
settings spec)

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
chrome/browser/resources/md_bookmarks/list.html:39: <dialog is="cr-dialog"
id="edit-bookmark">
Style guide is to use camelCase for ids so that you can do

this.$.editBookmark

rather than

this.$['edit-bookmark']

(History doesn't do this, since it predates this rule)

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
chrome/browser/resources/md_bookmarks/list.html:60: <div id="bookmarks-card">
Should probably change this ID while you're here.

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
File chrome/browser/resources/md_bookmarks/shared_style.html (right):

https://codereview.appspot.com/318100043/diff/220001/chrome/browser/resources...
chrome/browser/resources/md_bookmarks/shared_style.html:9: .button-container {
None of this CSS should be needed, since it's already part of cr-dialog.

https://codereview.appspot.com/318100043/diff/220001/chrome/test/data/webui/m...
File chrome/test/data/webui/md_bookmarks/store_test.js (right):

https://codereview.appspot.com/318100043/diff/220001/chrome/test/data/webui/m...
chrome/test/data/webui/md_bookmarks/store_test.js:202: // rootNode gets
notified.
On 2017/01/03 05:30:47, calamity wrote:
> This doesn't actually check the notification? This only ensures that the value
> is actually getting set (which is trivial since store.idToNodeMap_['4'] is the
> same object as this).
> 
> Just remove these lines. They add no value.
> 
> It would be good to check the right notifications get sent, but I'm not quite
> sure how to do that. Any ideas Tim?

If the application code uses `this.set`, then the test will check that you've at
least got the right paths set up.

To test databinding, you'd need to do an 'integration' test, with a whole
<bookmarks-app> set up. Then, when a change is made in the <bookmarks-store> you
can check that it's reflected to the DOM in the sidebar or list.

[tsergeant, 2017-01-04 05:00:04]

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/md_bookmarks/shared_style.html (right):

https://codereview.appspot.com/318100043/diff/20001/chrome/browser/resources/...
chrome/browser/resources/md_bookmarks/shared_style.html:54: }
On 2017/01/04 04:20:19, tsergeant wrote:
> On 2016/12/19 23:52:00, jiaxi wrote:
> > On 2016/12/16 05:20:24, calamity wrote:
> > > These are crazy funky paddings. Try vertical-align instead. Are these
> paddings
> > > actually necessary at all for the right measurements? Settings doesn't
> specify
> > > it in its shared css at all.
> > 
> > vertical-align doesn't give the right padding (it should be 8.5px 16px, v-a
> > gives 9.1px 7.41px) In setting's spec[1], the dialog's button doesn't have
> > specified padding. But in bookmark's spec it does for some reason...?
> > 
> >
>
[1]https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pages/03-Settings/specs#%2FSPEC-settings_dialogs.png%3Fz=width
> 
> In this case, it's more important for us to be consistent with other webui
than
> to match the exact pixel values from the spec.
> 
> This is because the cr-dialog and action-button styles are shared in many
> different places. Even if the styles of cr-dialog don't match the spec (and
they
> don't!), it's okay because:
> 
> * Everywhere is consistent in being wrong
> * When someone comes along to fix it, they can fix everyone at once, without
> worrying about whether the bookmarks page will still look right because it
went
> and did something different.
> 
> (Also: There are some specs for the action buttons on the 'buttons' page of
the
> settings spec)

To be clear: Get rid of the custom padding, leave the height and margin lines.
This matches what settings does.