|
|
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. |
DescriptionAllow 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 #
Messages
Total messages: 44 (4 generated)
hichris123@gmail.com changed reviewers: + sky@chromium.org
I updated vanlam's patch for this issue. I left out the WriteHyperlink() part since writing the rich text part was removed (https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar... and https://codereview.chromium.org/25894003).
https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:181: See style guide for handling of else. Basically you want: } else { https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:182: // We have more than one element This comment isn't quite right. https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:184: bool all_urls = true; What's the rationale for this logic? Why ignore folders? https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:192: if (all_urls == true) { No == true here, just if (all_urls)
https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:184: bool all_urls = true; On 2014/10/07 21:58:57, sky wrote: > What's the rationale for this logic? Why ignore folders? I just tried not including folders, and it wouldn't copy & paste if a folder was included. Is a folder not an Element? If it isn't, I could just loop through all bookmarks in a folder, and copy that to the clipboard. Would that work?
Not sure what you mean here. Element is used for either a url or folder. -Scott On Tue, Oct 7, 2014 at 3:35 PM, <hichris123@gmail.com> wrote: > > https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... > File components/bookmarks/browser/bookmark_node_data.cc (right): > > https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... > components/bookmarks/browser/bookmark_node_data.cc:184: bool all_urls = > true; > On 2014/10/07 21:58:57, sky wrote: >> >> What's the rationale for this logic? Why ignore folders? > > > I just tried not including folders, and it wouldn't copy & paste if a > folder was included. Is a folder not an Element? If it isn't, I could > just loop through all bookmarks in a folder, and copy that to the > clipboard. Would that work? > > https://codereview.chromium.org/634523004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Fixed all of these things. The one thing I had a question on: When this goes through a folder, it can't deal with a folder inside of a folder. Would it be best just to create a separate method to handle folders, and then have that method recurse if it finds a folder inside of a folder? https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:181: On 2014/10/07 21:58:57, sky wrote: > See style guide for handling of else. Basically you want: > } else { Done. https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:182: // We have more than one element On 2014/10/07 21:58:57, sky wrote: > This comment isn't quite right. Done. https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:184: bool all_urls = true; On 2014/10/07 22:35:55, hichris123 wrote: > On 2014/10/07 21:58:57, sky wrote: > > What's the rationale for this logic? Why ignore folders? > > I just tried not including folders, and it wouldn't copy & paste if a folder was > included. Is a folder not an Element? If it isn't, I could just loop through all > bookmarks in a folder, and copy that to the clipboard. Would that work? Done. https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:192: if (all_urls == true) { On 2014/10/07 21:58:57, sky wrote: > No == true here, just if (all_urls) Done.
I think we should just paste the name of the folder and not attempt to recurse. -Scott On Tue, Oct 7, 2014 at 6:04 PM, <hichris123@gmail.com> wrote: > Fixed all of these things. > > The one thing I had a question on: > > When this goes through a folder, it can't deal with a folder inside of a > folder. > Would it be best just to create a separate method to handle folders, and > then > have that method recurse if it finds a folder inside of a folder? > > > https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... > File components/bookmarks/browser/bookmark_node_data.cc (right): > > https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... > components/bookmarks/browser/bookmark_node_data.cc:181: > On 2014/10/07 21:58:57, sky wrote: >> >> See style guide for handling of else. Basically you want: >> } else { > > > Done. > > https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... > components/bookmarks/browser/bookmark_node_data.cc:182: // We have more > than one element > On 2014/10/07 21:58:57, sky wrote: >> >> This comment isn't quite right. > > > Done. > > https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... > components/bookmarks/browser/bookmark_node_data.cc:184: bool all_urls = > true; > On 2014/10/07 22:35:55, hichris123 wrote: >> >> On 2014/10/07 21:58:57, sky wrote: >> > What's the rationale for this logic? Why ignore folders? > > >> I just tried not including folders, and it wouldn't copy & paste if a > > folder was >> >> included. Is a folder not an Element? If it isn't, I could just loop > > through all >> >> bookmarks in a folder, and copy that to the clipboard. Would that > > work? > > Done. > > https://codereview.chromium.org/634523004/diff/1/components/bookmarks/browser... > components/bookmarks/browser/bookmark_node_data.cc:192: if (all_urls == > true) { > On 2014/10/07 21:58:57, sky wrote: >> >> No == true here, just if (all_urls) > > > Done. > > https://codereview.chromium.org/634523004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/08 03:01:01, sky wrote: > I think we should just paste the name of the folder and not attempt to recurse. > > -Scott Alright, uploaded a new patch with that behavior.
You're going to need to add test coverage here too. https://codereview.chromium.org/634523004/diff/40001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/634523004/diff/40001/components/bookmarks/bro... components/bookmarks/browser/bookmark_node_data.cc:189: i++; Why the i++ here?
On 2014/10/09 16:59:06, sky wrote: > You're going to need to add test coverage here too. Hmm, looks like the place for this would be components/bookmarks/test. Doing a quick lookthough, I'm not seeing anything there related to copying bookmarks to the clipboard. Maybe I'm missing it, but either way: what file should I implement it in, and what should be the test? > https://codereview.chromium.org/634523004/diff/40001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_node_data.cc (right): > > https://codereview.chromium.org/634523004/diff/40001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_node_data.cc:189: i++; > Why the i++ here? It was to advance the for loop. Looking at it now, it seems like an easier way would just be to have the if(!elements[i].is_url){}, take out the i++, and add the rest of the code specifically to a URL under an else{}.
On 2014/10/09 19:53:53, hichris123 wrote: > On 2014/10/09 16:59:06, sky wrote: > > You're going to need to add test coverage here too. > > Hmm, looks like the place for this would be components/bookmarks/test. Doing a > quick lookthough, I'm not seeing anything there related to copying bookmarks to > the clipboard. Maybe I'm missing it, but either way: what file should I > implement it in, and what should be the test? Ah, nevermind, I found bookmark_node_data_unittest.cc. Wonder why it's in brower/ and not test/...
On 2014/10/09 20:27:06, hichris123 wrote: > On 2014/10/09 19:53:53, hichris123 wrote: > > On 2014/10/09 16:59:06, sky wrote: > > > You're going to need to add test coverage here too. > > > > Hmm, looks like the place for this would be components/bookmarks/test. Doing a > > quick lookthough, I'm not seeing anything there related to copying bookmarks > to > > the clipboard. Maybe I'm missing it, but either way: what file should I > > implement it in, and what should be the test? > > Ah, nevermind, I found bookmark_node_data_unittest.cc. Wonder why it's in > brower/ and not test/... Sorry for the mass of emails, but I'm finding this to be a little odd: bookmark_node_data_unittest.cc contains no uses of WriteToClipboard. Is that intentional? Or should I be adding test cases with WriteToClipboard?
On 2014/10/09 20:36:08, hichris123 wrote: > Sorry for the mass of emails, but I'm finding this to be a little odd: > bookmark_node_data_unittest.cc contains no uses of WriteToClipboard. Is that > intentional? Or should I be adding test cases with WriteToClipboard? Did you reach a conclusion on this, Scott?
The tests there use OSExchangeData directly. You should be able to do the same. -Scott On Mon, Oct 13, 2014 at 1:31 PM, <hichris123@gmail.com> wrote: > On 2014/10/09 20:36:08, hichris123 wrote: >> >> Sorry for the mass of emails, but I'm finding this to be a little odd: >> bookmark_node_data_unittest.cc contains no uses of WriteToClipboard. Is >> that >> intentional? Or should I be adding test cases with WriteToClipboard? > > > Did you reach a conclusion on this, Scott? > > https://codereview.chromium.org/634523004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/14 13:18:15, sky wrote: > The tests there use OSExchangeData directly. You should be able to do the same. > > -Scott I'm confused, why do I need to create a test *not* using the method that I touched? And besides, the other methods appear to already be testing for multiple bookmarks (they have a folder, folder with children, and folder and another URL test case). What should I be adding?
On 2014/10/14 22:51:57, hichris123 wrote: > On 2014/10/14 13:18:15, sky wrote: > > The tests there use OSExchangeData directly. You should be able to do the > same. > > > > -Scott > > I'm confused, why do I need to create a test *not* using the method that I > touched? And besides, the other methods appear to already be testing for > multiple bookmarks (they have a folder, folder with children, and folder and > another URL test case). What should I be adding? Sorry, yes, you should definitely call into WriteToClipboard. And it may very well be we don't have coverage of WriteToCLipboard yet.
On 2014/10/15 00:08:10, sky wrote: > Sorry, yes, you should definitely call into WriteToClipboard. And it may very > well be we don't have coverage of WriteToCLipboard yet. Okay, great, I'll write some tests using WriteToClipboard. Quick question I had: when looking through the tests, I noticed that the titles of the bookmarks were copied to the clipboard, too, via WriteBookmark. I never implemented a method like this for multiple bookmarks; do you think it is important to implement a WriteBookmarks? Or just call WriteBookmark on each iteration? Or simply not use WriteBookmark(s) at all?
That only makes sense if the underlying platform provides a format with more than one bookmark at a time. I'm not sure they do. On Wed, Oct 15, 2014 at 9:06 AM, <hichris123@gmail.com> wrote: > On 2014/10/15 00:08:10, sky wrote: >> >> Sorry, yes, you should definitely call into WriteToClipboard. And it may >> very >> well be we don't have coverage of WriteToCLipboard yet. > > > Okay, great, I'll write some tests using WriteToClipboard. > > Quick question I had: when looking through the tests, I noticed that the > titles > of the bookmarks were copied to the clipboard, too, via WriteBookmark. I > never > implemented a method like this for multiple bookmarks; do you think it is > important to implement a WriteBookmarks? Or just call WriteBookmark on each > iteration? Or simply not use WriteBookmark(s) at all? > > > https://codereview.chromium.org/634523004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Alright, added my attempt at unit tests. I built these at home, and they all passed, so it *should* work. https://codereview.chromium.org/634523004/diff/40001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/634523004/diff/40001/components/bookmarks/bro... components/bookmarks/browser/bookmark_node_data.cc:189: i++; On 2014/10/09 16:59:06, sky wrote: > Why the i++ here? Done.
https://codereview.chromium.org/634523004/diff/110001/components/bookmarks/br... File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): https://codereview.chromium.org/634523004/diff/110001/components/bookmarks/br... components/bookmarks/browser/bookmark_node_data_unittest.cc:272: TEST_F(BookmarkNodeDataTest, WriteToClipboardURL) { Does this inject a fake clipboard so that the test isn't mutating the system clipboard?
On 2014/10/16 16:04:06, sky wrote: > https://codereview.chromium.org/634523004/diff/110001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): > > https://codereview.chromium.org/634523004/diff/110001/components/bookmarks/br... > components/bookmarks/browser/bookmark_node_data_unittest.cc:272: > TEST_F(BookmarkNodeDataTest, WriteToClipboardURL) { > Does this inject a fake clipboard so that the test isn't mutating the system > clipboard? No, that explains the failures. How do I inject a fake clipboard?
Okay, I fixed the build errors -- turns out that the clipboard needs a message queue. It should be fine to not inject a fake clipboard, as bookmark_utils_unittest just does it like I have it right now.
Does this write to the system clip board?
On 2014/10/20 16:42:32, sky wrote: > Does this write to the system clip board? Yeah. And this is exactly what other unittests do (https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar... and https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar...).
I actually think these tests write directly to the system clipboard. But it seems we do that in other places too. https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... components/bookmarks/browser/bookmark_node_data.cc:188: text += i == 0 ? title : base::ASCIIToUTF16("\n") + title; nit: move the adding \n before the if. https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... components/bookmarks/browser/bookmark_node_data_unittest.cc:281: // Write to clipboard nit: this comment here and in other places don't provide any value and can be nuked. https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... components/bookmarks/browser/bookmark_node_data_unittest.cc:362: TEST_F(BookmarkNodeDataTest, WriteToClipboardFolderAndURL) { Do any of these tests actually verify the code you're adding. AFAICT they all verify existing code and would be better served in a separate cl.
https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... components/bookmarks/browser/bookmark_node_data_unittest.cc:362: TEST_F(BookmarkNodeDataTest, WriteToClipboardFolderAndURL) { On 2014/10/20 20:37:11, sky wrote: > Do any of these tests actually verify the code you're adding. AFAICT they all > verify existing code and would be better served in a separate cl. Only WriteToClipboardURL should be verifying existing code. The rest deal with situations with folders or more than one URL and passing that to WriteToClipboard, which is what I modified WriteToClipboard to handle.
But you're not looking at the text content, which is what you're test is writing. -Scott On Mon, Oct 20, 2014 at 1:41 PM, <hichris123@gmail.com> wrote: > > https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_node_data_unittest.cc > (right): > > https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... > components/bookmarks/browser/bookmark_node_data_unittest.cc:362: > TEST_F(BookmarkNodeDataTest, WriteToClipboardFolderAndURL) { > On 2014/10/20 20:37:11, sky wrote: >> >> Do any of these tests actually verify the code you're adding. AFAICT > > they all >> >> verify existing code and would be better served in a separate cl. > > > Only WriteToClipboardURL should be verifying existing code. The rest > deal with situations with folders or more than one URL and passing that > to WriteToClipboard, which is what I modified WriteToClipboard to > handle. > > https://codereview.chromium.org/634523004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/20 22:20:43, sky wrote: > But you're not looking at the text content, which is what you're test > is writing. > > -Scott Ah, hm, true. I guess I am overreaching a bit in these tests -- which makes the assumption that pasting it in will generate the same results. So, I guess I'll switch to just comparing the text outputs. Should have this uploaded in a bit.
And please, just worry about testing your functionality in this patch. I'm totally fine adding more coverage, but better to do in a separate patch. On Mon, Oct 20, 2014 at 3:32 PM, <hichris123@gmail.com> wrote: > On 2014/10/20 22:20:43, sky wrote: >> >> But you're not looking at the text content, which is what you're test >> is writing. > > >> -Scott > > Ah, hm, true. I guess I am overreaching a bit in these tests -- which makes > the > assumption that pasting it in will generate the same results. So, I guess > I'll > switch to just comparing the text outputs. Should have this uploaded in a > bit. > > > https://codereview.chromium.org/634523004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Okay, this should work now -- please run a trybot to make sure it actually works. https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... components/bookmarks/browser/bookmark_node_data.cc:188: text += i == 0 ? title : base::ASCIIToUTF16("\n") + title; On 2014/10/20 20:37:11, sky wrote: > nit: move the adding \n before the if. Done. https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): https://codereview.chromium.org/634523004/diff/130001/components/bookmarks/br... components/bookmarks/browser/bookmark_node_data_unittest.cc:281: // Write to clipboard On 2014/10/20 20:37:12, sky wrote: > nit: this comment here and in other places don't provide any value and can be > nuked. Done.
https://codereview.chromium.org/634523004/diff/150001/components/bookmarks/br... File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): https://codereview.chromium.org/634523004/diff/150001/components/bookmarks/br... components/bookmarks/browser/bookmark_node_data_unittest.cc:353: TEST_F(BookmarkNodeDataTest, WriteToClipboardFolderAndURL) { This is the only test that hits your code, right? Please move the rest to a separate patch.
On 2014/10/21 00:07:17, sky wrote: > https://codereview.chromium.org/634523004/diff/150001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_node_data_unittest.cc (right): > > https://codereview.chromium.org/634523004/diff/150001/components/bookmarks/br... > components/bookmarks/browser/bookmark_node_data_unittest.cc:353: > TEST_F(BookmarkNodeDataTest, WriteToClipboardFolderAndURL) { > This is the only test that hits your code, right? Please move the rest to a > separate patch. No, all but WriteToClipboardURL hit my code. I'll remove that from this patch.
On 2014/10/21 00:24:32, hichris123 wrote: > No, all but WriteToClipboardURL hit my code. I'll remove that from this patch. Okay, uploaded a patch without WriteToClipboardURL & fixed compilation errors.
Thanks, this looks good. I don't see that you signed the cla ( http://www.chromium.org/developers/contributing-code ) . Did you?
On 2014/10/21 03:17:11, sky wrote: > Thanks, this looks good. I don't see that you signed the cla ( > http://www.chromium.org/developers/contributing-code ) . Did you? I just double checked -- yeah, I did. It's under Chris Nardi, signed on July 10th.
LGTM
The CQ bit was checked by sky@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/634523004/190001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by hichris123@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/634523004/210001
Message was sent while issue was closed.
Committed patchset #9 (id:210001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/759a66d1d2d31d9fe4947b41b9395392ea80a220 Cr-Commit-Position: refs/heads/master@{#300553} |