Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(26)

Issue 634523004: Allow copying of multiple bookmarks from within the bookmark manager. (Closed)

Created:
6 years, 2 months ago by hichris123
Modified:
6 years, 2 months ago
Reviewers:
sky
CC:
chromium-reviews, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Allow copying of multiple bookmarks from within the bookmark manager. BUG=57376 Committed: https://crrev.com/759a66d1d2d31d9fe4947b41b9395392ea80a220 Cr-Commit-Position: refs/heads/master@{#300553}

Patch Set 1 #

Total comments: 9

Patch Set 2 : Fixes, allows folders #

Patch Set 3 : Folders selected only have the name of it copied #

Total comments: 2

Patch Set 4 : Add unit tests #

Total comments: 1

Patch Set 5 : Attempt to fix unittest #

Total comments: 6

Patch Set 6 : Make unittest actually test changes #

Total comments: 1

Patch Set 7 : Fix compile errors, remove unittest not needed #

Patch Set 8 : Wrong title + reverse #

Patch Set 9 : Fix unused variable warning #

Unified diffs Side-by-side diffs Delta from patch set Stats (+109 lines, -0 lines) Patch
M AUTHORS View 1 chunk +1 line, -0 lines 0 comments Download
M components/bookmarks/browser/bookmark_node_data.cc View 1 2 3 4 5 1 chunk +16 lines, -0 lines 0 comments Download
M components/bookmarks/browser/bookmark_node_data_unittest.cc View 1 2 3 4 5 6 7 8 4 chunks +92 lines, -0 lines 0 comments Download

Messages

Total messages: 44 (4 generated)
hichris123
I updated vanlam's patch for this issue. I left out the WriteHyperlink() part since writing ...
6 years, 2 months ago (2014-10-07 20:40:24 UTC) #2
sky
https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser/bookmark_node_data.cc File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser/bookmark_node_data.cc#newcode181 components/bookmarks/browser/bookmark_node_data.cc:181: See style guide for handling of else. Basically you ...
6 years, 2 months ago (2014-10-07 21:58:57 UTC) #3
hichris123
https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser/bookmark_node_data.cc File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser/bookmark_node_data.cc#newcode184 components/bookmarks/browser/bookmark_node_data.cc:184: bool all_urls = true; On 2014/10/07 21:58:57, sky wrote: ...
6 years, 2 months ago (2014-10-07 22:35:55 UTC) #4
sky
Not sure what you mean here. Element is used for either a url or folder. ...
6 years, 2 months ago (2014-10-07 23:11:10 UTC) #5
hichris123
Fixed all of these things. The one thing I had a question on: When this ...
6 years, 2 months ago (2014-10-08 01:04:29 UTC) #6
sky
I think we should just paste the name of the folder and not attempt to ...
6 years, 2 months ago (2014-10-08 03:01:01 UTC) #7
hichris123
On 2014/10/08 03:01:01, sky wrote: > I think we should just paste the name of ...
6 years, 2 months ago (2014-10-08 21:07:48 UTC) #8
sky
You're going to need to add test coverage here too. https://codereview.chromium.org/634523004/diff/40001/components/bookmarks/browser/bookmark_node_data.cc File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/634523004/diff/40001/components/bookmarks/browser/bookmark_node_data.cc#newcode189 ...
6 years, 2 months ago (2014-10-09 16:59:06 UTC) #9
hichris123
On 2014/10/09 16:59:06, sky wrote: > You're going to need to add test coverage here ...
6 years, 2 months ago (2014-10-09 19:53:53 UTC) #10
hichris123
On 2014/10/09 19:53:53, hichris123 wrote: > On 2014/10/09 16:59:06, sky wrote: > > You're going ...
6 years, 2 months ago (2014-10-09 20:27:06 UTC) #11
hichris123
On 2014/10/09 20:27:06, hichris123 wrote: > On 2014/10/09 19:53:53, hichris123 wrote: > > On 2014/10/09 ...
6 years, 2 months ago (2014-10-09 20:36:08 UTC) #12
hichris123
On 2014/10/09 20:36:08, hichris123 wrote: > Sorry for the mass of emails, but I'm finding ...
6 years, 2 months ago (2014-10-13 20:31:01 UTC) #13
sky
The tests there use OSExchangeData directly. You should be able to do the same. -Scott ...
6 years, 2 months ago (2014-10-14 13:18:15 UTC) #14
hichris123
On 2014/10/14 13:18:15, sky wrote: > The tests there use OSExchangeData directly. You should be ...
6 years, 2 months ago (2014-10-14 22:51:57 UTC) #15
sky
On 2014/10/14 22:51:57, hichris123 wrote: > On 2014/10/14 13:18:15, sky wrote: > > The tests ...
6 years, 2 months ago (2014-10-15 00:08:10 UTC) #16
hichris123
On 2014/10/15 00:08:10, sky wrote: > Sorry, yes, you should definitely call into WriteToClipboard. And ...
6 years, 2 months ago (2014-10-15 16:06:49 UTC) #17
sky
That only makes sense if the underlying platform provides a format with more than one ...
6 years, 2 months ago (2014-10-15 19:09:26 UTC) #18
hichris123
Alright, added my attempt at unit tests. I built these at home, and they all ...
6 years, 2 months ago (2014-10-16 00:46:58 UTC) #19
sky
https://codereview.chromium.org/634523004/diff/110001/components/bookmarks/browser/bookmark_node_data_unittest.cc File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): https://codereview.chromium.org/634523004/diff/110001/components/bookmarks/browser/bookmark_node_data_unittest.cc#newcode272 components/bookmarks/browser/bookmark_node_data_unittest.cc:272: TEST_F(BookmarkNodeDataTest, WriteToClipboardURL) { Does this inject a fake clipboard ...
6 years, 2 months ago (2014-10-16 16:04:06 UTC) #20
hichris123
On 2014/10/16 16:04:06, sky wrote: > https://codereview.chromium.org/634523004/diff/110001/components/bookmarks/browser/bookmark_node_data_unittest.cc > File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): > > https://codereview.chromium.org/634523004/diff/110001/components/bookmarks/browser/bookmark_node_data_unittest.cc#newcode272 > ...
6 years, 2 months ago (2014-10-16 20:12:03 UTC) #21
hichris123
Okay, I fixed the build errors -- turns out that the clipboard needs a message ...
6 years, 2 months ago (2014-10-18 21:50:55 UTC) #22
sky
Does this write to the system clip board?
6 years, 2 months ago (2014-10-20 16:42:32 UTC) #23
hichris123
On 2014/10/20 16:42:32, sky wrote: > Does this write to the system clip board? Yeah. ...
6 years, 2 months ago (2014-10-20 16:45:29 UTC) #24
sky
I actually think these tests write directly to the system clipboard. But it seems we ...
6 years, 2 months ago (2014-10-20 20:37:12 UTC) #25
hichris123
https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/browser/bookmark_node_data_unittest.cc File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/browser/bookmark_node_data_unittest.cc#newcode362 components/bookmarks/browser/bookmark_node_data_unittest.cc:362: TEST_F(BookmarkNodeDataTest, WriteToClipboardFolderAndURL) { On 2014/10/20 20:37:11, sky wrote: > ...
6 years, 2 months ago (2014-10-20 20:41:12 UTC) #26
sky
But you're not looking at the text content, which is what you're test is writing. ...
6 years, 2 months ago (2014-10-20 22:20:43 UTC) #27
hichris123
On 2014/10/20 22:20:43, sky wrote: > But you're not looking at the text content, which ...
6 years, 2 months ago (2014-10-20 22:32:26 UTC) #28
sky
And please, just worry about testing your functionality in this patch. I'm totally fine adding ...
6 years, 2 months ago (2014-10-20 23:02:22 UTC) #29
hichris123
Okay, this should work now -- please run a trybot to make sure it actually ...
6 years, 2 months ago (2014-10-20 23:12:59 UTC) #30
sky
https://codereview.chromium.org/634523004/diff/150001/components/bookmarks/browser/bookmark_node_data_unittest.cc File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): https://codereview.chromium.org/634523004/diff/150001/components/bookmarks/browser/bookmark_node_data_unittest.cc#newcode353 components/bookmarks/browser/bookmark_node_data_unittest.cc:353: TEST_F(BookmarkNodeDataTest, WriteToClipboardFolderAndURL) { This is the only test that ...
6 years, 2 months ago (2014-10-21 00:07:17 UTC) #31
hichris123
On 2014/10/21 00:07:17, sky wrote: > https://codereview.chromium.org/634523004/diff/150001/components/bookmarks/browser/bookmark_node_data_unittest.cc > File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): > > https://codereview.chromium.org/634523004/diff/150001/components/bookmarks/browser/bookmark_node_data_unittest.cc#newcode353 > ...
6 years, 2 months ago (2014-10-21 00:24:32 UTC) #32
hichris123
On 2014/10/21 00:24:32, hichris123 wrote: > No, all but WriteToClipboardURL hit my code. I'll remove ...
6 years, 2 months ago (2014-10-21 00:38:32 UTC) #33
sky
Thanks, this looks good. I don't see that you signed the cla ( http://www.chromium.org/developers/contributing-code ) ...
6 years, 2 months ago (2014-10-21 03:17:11 UTC) #34
hichris123
On 2014/10/21 03:17:11, sky wrote: > Thanks, this looks good. I don't see that you ...
6 years, 2 months ago (2014-10-21 03:20:06 UTC) #35
sky
LGTM
6 years, 2 months ago (2014-10-21 18:08:15 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/634523004/190001
6 years, 2 months ago (2014-10-21 18:09:14 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_swarming/builds/26004)
6 years, 2 months ago (2014-10-21 18:44:02 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/634523004/210001
6 years, 2 months ago (2014-10-21 19:43:15 UTC) #42
commit-bot: I haz the power
Committed patchset #9 (id:210001)
6 years, 2 months ago (2014-10-21 20:52:18 UTC) #43
commit-bot: I haz the power
6 years, 2 months ago (2014-10-21 20:53:05 UTC) #44
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/759a66d1d2d31d9fe4947b41b9395392ea80a220
Cr-Commit-Position: refs/heads/master@{#300553}

Powered by Google App Engine
This is Rietveld 408576698