[MD Bookmarks] Add import/export bookmarks HTML features

[MD Bookmarks] Add import/export bookmarks HTML features

BUG=692848
R=tsergeant@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2715523003
Cr-Commit-Position: refs/heads/master@{#452772}
Committed: https://chromium.googlesource.com/chromium/src/+/2f61efa6fcfabfcf520d3099969477531e4b6450

[rongjie, 2017-02-23 06:53:49]

Description was changed from

==========
WIP [MD Bookmarks] Add import/export bookmarks HTML features

BUG=692848
R=tsergeant@chromium.org
==========

to

==========
WIP [MD Bookmarks] Add import/export bookmarks HTML features

BUG=692848
R=tsergeant@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rongjie, 2017-02-23 07:05:13]

Export feature is complete.

Import feature still contains a bug: after importing, left sidebar is updated,
but the right rendered list of bookmarks is not updated, have to switch folders
to trigger update. Any idea?

[tsergeant, 2017-02-24 00:03:18]

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

https://codereview.chromium.org/2715523003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/store.js:335:
chrome.bookmarks.onRemoved.removeListener(this.onBookmarkRemoved_.bind(this));
The docs for importBegan say:

"Expensive observers should ignore onCreated updates until onImportEnded is
fired"

(https://developer.chrome.com/extensions/bookmarks#event-onImportBegan)

We currently don't have an onCreated listener, and I don't think we need to
pause onRemoved/onChanged listeners during the import.

So, I think it's okay to leave this listener blank, with a TODO to pause
onCreated once that event is used.

https://codereview.chromium.org/2715523003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/store.js:344:
this.setupStore_(results[0]);
I think you need to call

this.updateSelectedDisplay_()

here to make the list update.

We're currently in the middle of rewriting the store to reduce the need to do
some of these things (see: https://codereview.chromium.org/2704983002/), so I'm
not too concerned if this doesn't work perfectly right now -- so long we
implement it right in the new system.

[rongjie, 2017-02-24 00:19:42]

Description was changed from

==========
WIP [MD Bookmarks] Add import/export bookmarks HTML features

BUG=692848
R=tsergeant@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add import/export bookmarks HTML features

BUG=692848
R=tsergeant@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rongjie, 2017-02-24 00:20:47]

Update as you have suggested. Works now!

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

https://codereview.chromium.org/2715523003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/store.js:335:
chrome.bookmarks.onRemoved.removeListener(this.onBookmarkRemoved_.bind(this));
On 2017/02/24 00:03:18, tsergeant wrote:
> The docs for importBegan say:
> 
> "Expensive observers should ignore onCreated updates until onImportEnded is
> fired"
> 
> (https://developer.chrome.com/extensions/bookmarks#event-onImportBegan)
> 
> We currently don't have an onCreated listener, and I don't think we need to
> pause onRemoved/onChanged listeners during the import.
> 
> So, I think it's okay to leave this listener blank, with a TODO to pause
> onCreated once that event is used.

Done.

https://codereview.chromium.org/2715523003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/store.js:344:
this.setupStore_(results[0]);
On 2017/02/24 00:03:18, tsergeant wrote:
> I think you need to call
> 
> this.updateSelectedDisplay_()
> 
> here to make the list update.
> 
> We're currently in the middle of rewriting the store to reduce the need to do
> some of these things (see: https://codereview.chromium.org/2704983002/), so
I'm
> not too concerned if this doesn't work perfectly right now -- so long we
> implement it right in the new system.

Done.

[tsergeant, 2017-02-24 00:29:10]

lgtm

Thanks for contributing!

[rongjie, 2017-02-24 01:42:45]

The CQ bit was checked by loorongjie@gmail.com

[commit-bot: I haz the power, 2017-02-24 01:44:00]

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-24 01:55:28]

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

[commit-bot: I haz the power, 2017-02-24 01:55:29]

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

The closure failure indicates that we need to add a new dependency in the
closure configuration.

Add 	

'<(EXTERNS_GYP):chrome_extensions',

to the 'toolbar' 'dependencies' configuration at 

https://cs.chromium.org/chromium/src/chrome/browser/resources/md_bookmarks/co...

There's also instructions for running closure locally at
https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat...

[rongjie, 2017-02-24 04:58:54]

The CQ bit was checked by loorongjie@gmail.com

[rongjie, 2017-02-24 04:58:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2715523003/#ps40001
(title: "Update closure deps")

[commit-bot: I haz the power, 2017-02-24 04:59:13]

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-24 06:21:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487912334919800,
"parent_rev": "a3397e3e75e6338d188b713f36593c54c0b2b469", "commit_rev":
"2f61efa6fcfabfcf520d3099969477531e4b6450"}

[commit-bot: I haz the power, 2017-02-24 06:22:41]

Description was changed from

==========
[MD Bookmarks] Add import/export bookmarks HTML features

BUG=692848
R=tsergeant@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add import/export bookmarks HTML features

BUG=692848
R=tsergeant@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2715523003
Cr-Commit-Position: refs/heads/master@{#452772}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f61efa6fcfabfcf520d30999694...
==========

[commit-bot: I haz the power, 2017-02-24 06:22:42]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2f61efa6fcfabfcf520d30999694...