MD History: Remove bookmark when clicking bookmark star

MD History: Remove bookmark when clicking bookmark star

On current history page, history items with bookmarks will show a bookmark
star, and it's unclickable.

Remove the url's bookmark when clicking bookmark star.

BUG=618580
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/064a34e7e54d2458dc56e76e6c3e343c622f327b
Cr-Commit-Position: refs/heads/master@{#403419}

[lshang, 2016-06-24 04:40:04]

Description was changed from

==========
MD History: Remove bookmark when clicking bookmark star

On current history page, history items with bookmarks will show a bookmark
star, and it's unclickable.

Remove the url's bookmark when clicking bookmark star.

BUG=618580
==========

to

==========
MD History: Remove bookmark when clicking bookmark star

On current history page, history items with bookmarks will show a bookmark
star, and it's unclickable.

Remove the url's bookmark when clicking bookmark star.

BUG=618580
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[lshang, 2016-06-24 05:43:34]

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

[lshang, 2016-06-24 05:43:34]

PTAL thanks!

[lshang, 2016-06-24 07:09:21]

Patchset #2 (id:20001) has been deleted

[calamity, 2016-06-29 07:36:05]

It would be good to have a test for this. Looks good otherwise.

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:49:
chrome.send('removeBookmark', [this.item.url]);
Consider moving this into a method in browser_service.js. We want to consolidate
all chrome.send calls there.

Opinions tsergeant?

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:56: * Remove bookmark star
of an url's all history items.
nit: Remove bookmark star for history items with matching URLs.

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:67: if
(this.historyData_[i].url == url) {
nit: No braces needed for single line if.

[tsergeant, 2016-06-30 01:41:11]

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.html:146: <paper-icon-button
icon="cr:star" id="bookmark-star"
We use paper-icon-button-light instead of paper-icon-button inside
<history-item>. See below (#menu-button) for an example of how to do this.

The light version is a little bit faster, which adds up when we create 50ish
<history-item> during page load.

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:49:
chrome.send('removeBookmark', [this.item.url]);
On 2016/06/29 07:36:05, calamity wrote:
> Consider moving this into a method in browser_service.js. We want to
consolidate
> all chrome.send calls there.
> 
> Opinions tsergeant?

Sure, sgtm.

[lshang, 2016-07-01 00:52:11]

Patchset #4 (id:80001) has been deleted

[lshang, 2016-07-01 00:54:39]

Patchset #3 (id:60001) has been deleted

[lshang, 2016-07-01 01:15:16]

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.html:146: <paper-icon-button
icon="cr:star" id="bookmark-star"
On 2016/06/30 01:41:10, tsergeant wrote:
> We use paper-icon-button-light instead of paper-icon-button inside
> <history-item>. See below (#menu-button) for an example of how to do this.
> 
> The light version is a little bit faster, which adds up when we create 50ish
> <history-item> during page load.

Done.

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:49:
chrome.send('removeBookmark', [this.item.url]);
On 2016/06/29 07:36:05, calamity wrote:
> Consider moving this into a method in browser_service.js. We want to
consolidate
> all chrome.send calls there.
> 
> Opinions tsergeant?

Done.

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:56: * Remove bookmark star
of an url's all history items.
On 2016/06/29 07:36:05, calamity wrote:
> nit: Remove bookmark star for history items with matching URLs.

Done.

https://codereview.chromium.org/2097003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:67: if
(this.historyData_[i].url == url) {
On 2016/06/29 07:36:05, calamity wrote:
> nit: No braces needed for single line if.

Done.

[tsergeant, 2016-07-01 04:27:10]

lgtm with a nit

https://codereview.chromium.org/2097003002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_item_test.js (right):

https://codereview.chromium.org/2097003002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_item_test.js:90: return flush();
Nit: You shouldn't need this line here.

return flush() is only needed if you're chaining another block after it.

[lshang, 2016-07-01 06:11:53]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-07-01 06:11:54]

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/2097003002/#ps120001
(title: "minor change")

[commit-bot: I haz the power, 2016-07-01 06:12:02]

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

[lshang, 2016-07-01 06:12:07]

https://codereview.chromium.org/2097003002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_item_test.js (right):

https://codereview.chromium.org/2097003002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_item_test.js:90: return flush();
On 2016/07/01 04:27:10, tsergeant wrote:
> Nit: You shouldn't need this line here.
> 
> return flush() is only needed if you're chaining another block after it.

Done.

[commit-bot: I haz the power, 2016-07-01 07:14:44]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-07-01 07:14:49]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-01 07:16:25]

Description was changed from

==========
MD History: Remove bookmark when clicking bookmark star

On current history page, history items with bookmarks will show a bookmark
star, and it's unclickable.

Remove the url's bookmark when clicking bookmark star.

BUG=618580
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD History: Remove bookmark when clicking bookmark star

On current history page, history items with bookmarks will show a bookmark
star, and it's unclickable.

Remove the url's bookmark when clicking bookmark star.

BUG=618580
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/064a34e7e54d2458dc56e76e6c3e343c622f327b
Cr-Commit-Position: refs/heads/master@{#403419}
==========

[commit-bot: I haz the power, 2016-07-01 07:16:26]

Patchset 4 (id:??) landed as
https://crrev.com/064a34e7e54d2458dc56e76e6c3e343c622f327b
Cr-Commit-Position: refs/heads/master@{#403419}