Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
437423005:
Show paste option on right click of bookmark bar if some URL is copied from outside browser or from… (Closed)
DescriptionShow paste option on right click of bookmark bar if some URL is copied from outside browser or from any page content
Paste option was not appearing when some URL is copied from outside browser
or web page content. But if some URL is copied from url bar the paste option
appears when right click is performed on bookmark bar. Added check in
CanPasteFromClipboard to handle this scenario.
BUG=400644
Committed: https://crrev.com/a54214a5ad96a46d6b06d43a841133d66172d49d
Cr-Commit-Position: refs/heads/master@{#294380}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Modified code as suggested. #
Total comments: 1
Patch Set 3 : Modified code as suggested. #
Total comments: 2
Patch Set 4 : Fixed indentation and incorporated other comments. #
Total comments: 3
Patch Set 5 : Refactored code as per suggestion #Patch Set 6 : few nits resolved #Patch Set 7 : Added unit tests #
Total comments: 1
Patch Set 8 : Modified code as per suggestion #Patch Set 9 : Code modification as per review comments. #
Total comments: 6
Patch Set 10 : Added suggested test case and changed fucntion name. #
Total comments: 5
Patch Set 11 : Created new test assertion #
Total comments: 2
Patch Set 12 : Modified code as per review comments. #Patch Set 13 : Fixing build breaks #Patch Set 14 : Fixing build breaks #Patch Set 15 : build breaks fix. #Patch Set 16 : Fixed build break. #
Messages
Total messages: 44 (3 generated)
PTAL
https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:125: bool BookmarkNodeData::ClipboardContainsURLs() { These functions don't need bookmarkNodeData at all. They should live somewhere else so that this code remains focused. https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_utils.cc:213: return BookmarkNodeData::WriteBookmarkToClipboardFromURL(); CanPaste shouldn't modify the clipboard.
https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... File components/bookmarks/browser/bookmark_node_data.cc (right): https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_node_data.cc:125: bool BookmarkNodeData::ClipboardContainsURLs() { On 2014/08/05 16:53:49, sky wrote: > These functions don't need bookmarkNodeData at all. They should live somewhere > else so that this code remains focused. Scott, would you be fine with bookmark_clipboard.*. I have always wanted to move some clipboard functions out of bookmark_utils.* into bookmark_clipboard.* anyway. Maybe now, it is time to start that move?
On 2014/08/05 16:53:49, sky wrote: > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > File components/bookmarks/browser/bookmark_node_data.cc (right): > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > components/bookmarks/browser/bookmark_node_data.cc:125: bool > BookmarkNodeData::ClipboardContainsURLs() { > These functions don't need bookmarkNodeData at all. They should live somewhere > else so that this code remains focused. > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > components/bookmarks/browser/bookmark_utils.cc:213: return > BookmarkNodeData::WriteBookmarkToClipboardFromURL(); > CanPaste shouldn't modify the clipboard. Is it fine to put same code(WriteBookmarkToClipboardFromURL implementation) inside PasteFromClipboard function? In that case we will not be modifying clipboard in CanPaste funtion.
On 2014/08/07 04:34:45, ankit wrote: > On 2014/08/05 16:53:49, sky wrote: > > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > > File components/bookmarks/browser/bookmark_node_data.cc (right): > > > > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > > components/bookmarks/browser/bookmark_node_data.cc:125: bool > > BookmarkNodeData::ClipboardContainsURLs() { > > These functions don't need bookmarkNodeData at all. They should live somewhere > > else so that this code remains focused. > > > > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > > components/bookmarks/browser/bookmark_utils.cc:213: return > > BookmarkNodeData::WriteBookmarkToClipboardFromURL(); > > CanPaste shouldn't modify the clipboard. > > Is it fine to put same code(WriteBookmarkToClipboardFromURL implementation) > inside PasteFromClipboard function? > In that case we will not be modifying clipboard in CanPaste funtion. @sky Please suggest if we can move same implementation to pasteFromClipboard function.
On 2014/08/08 15:28:27, ankit wrote: > On 2014/08/07 04:34:45, ankit wrote: > > On 2014/08/05 16:53:49, sky wrote: > > > > > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > > > File components/bookmarks/browser/bookmark_node_data.cc (right): > > > > > > > > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > > > components/bookmarks/browser/bookmark_node_data.cc:125: bool > > > BookmarkNodeData::ClipboardContainsURLs() { > > > These functions don't need bookmarkNodeData at all. They should live > somewhere > > > else so that this code remains focused. > > > > > > > > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > > > > > https://codereview.chromium.org/437423005/diff/1/components/bookmarks/browser... > > > components/bookmarks/browser/bookmark_utils.cc:213: return > > > BookmarkNodeData::WriteBookmarkToClipboardFromURL(); > > > CanPaste shouldn't modify the clipboard. > > > > Is it fine to put same code(WriteBookmarkToClipboardFromURL implementation) > > inside PasteFromClipboard function? > > In that case we will not be modifying clipboard in CanPaste funtion. > > @sky > Please suggest if we can move same implementation to pasteFromClipboard > function. @Sky PTAL new patch
https://codereview.chromium.org/437423005/diff/20001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_node_data.h (right): https://codereview.chromium.org/437423005/diff/20001/components/bookmarks/bro... components/bookmarks/browser/bookmark_node_data.h:103: static bool ClipboardContainsUrls(); This is only used by BookmarkBarView, and it's a one liner. So move the implementation where needed.
On 2014/08/12 16:39:53, sky wrote: > https://codereview.chromium.org/437423005/diff/20001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_node_data.h (right): > > https://codereview.chromium.org/437423005/diff/20001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_node_data.h:103: static bool > ClipboardContainsUrls(); > This is only used by BookmarkBarView, and it's a one liner. So move the > implementation where needed. @sky PTAL new patch. I have moved implementation of ClipboardContainsUrls where needed.
https://codereview.chromium.org/437423005/diff/40001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/437423005/diff/40001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:199: if (!bookmark_data.ReadFromClipboard(ui::CLIPBOARD_TYPE_COPY_PASTE)) Fix indentation here. https://codereview.chromium.org/437423005/diff/40001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:208: ui::Clipboard::GetUrlWFormatType(), ui::CLIPBOARD_TYPE_COPY_PASTE)) { I think this path should convert to a BookmarkNodeData so that you can share the code for pasting.
On 2014/08/13 16:48:35, sky wrote: > https://codereview.chromium.org/437423005/diff/40001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/437423005/diff/40001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:199: if > (!bookmark_data.ReadFromClipboard(ui::CLIPBOARD_TYPE_COPY_PASTE)) > Fix indentation here. > > https://codereview.chromium.org/437423005/diff/40001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:208: > ui::Clipboard::GetUrlWFormatType(), ui::CLIPBOARD_TYPE_COPY_PASTE)) { > I think this path should convert to a BookmarkNodeData so that you can share the > code for pasting. @sky PTAL new patch. I have fixed indentation and incorporated other comments.
On 2014/08/14 13:27:21, ankit wrote: > On 2014/08/13 16:48:35, sky wrote: > > > https://codereview.chromium.org/437423005/diff/40001/components/bookmarks/bro... > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > https://codereview.chromium.org/437423005/diff/40001/components/bookmarks/bro... > > components/bookmarks/browser/bookmark_utils.cc:199: if > > (!bookmark_data.ReadFromClipboard(ui::CLIPBOARD_TYPE_COPY_PASTE)) > > Fix indentation here. > > > > > https://codereview.chromium.org/437423005/diff/40001/components/bookmarks/bro... > > components/bookmarks/browser/bookmark_utils.cc:208: > > ui::Clipboard::GetUrlWFormatType(), ui::CLIPBOARD_TYPE_COPY_PASTE)) { > > I think this path should convert to a BookmarkNodeData so that you can share > the > > code for pasting. > > @sky > PTAL new patch. > I have fixed indentation and incorporated other comments. PTAL
https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:199: return; spacing is off. But I think this would be clearer if you move the !ReadFromClipboard to an else around 213ish. https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:225: if (ui::Clipboard::GetForCurrentThread()->IsFormatAvailable( 223-233 is nearly identical to 200-208, refactor to share.
On 2014/08/15 18:24:55, sky wrote: > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:199: return; > spacing is off. > But I think this would be clearer if you move the !ReadFromClipboard to an else > around 213ish. > > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:225: if > (ui::Clipboard::GetForCurrentThread()->IsFormatAvailable( > 223-233 is nearly identical to 200-208, refactor to share. PTAL new patch
PTAL new patch https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:199: return; On 2014/08/15 18:24:55, sky wrote: > spacing is off. > But I think this would be clearer if you move the !ReadFromClipboard to an else > around 213ish. Please correct if my understanding is not correct. First check should be whether clipboard has any bookmark or not. If I do as suggested in that case the order will be reversed.
On 2014/08/16 07:43:57, ankit wrote: > > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:199: return; > On 2014/08/15 18:24:55, sky wrote: > > spacing is off. > > But I think this would be clearer if you move the !ReadFromClipboard to an > else > > around 213ish. > > Please correct if my understanding is not correct. > First check should be whether clipboard has any bookmark or not. > If I do as suggested in that case the order will be reversed. PTAL new patch
On 2014/08/16 07:44:21, ankit wrote: > On 2014/08/16 07:43:57, ankit wrote: > > > > > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > > components/bookmarks/browser/bookmark_utils.cc:199: return; > > On 2014/08/15 18:24:55, sky wrote: > > > spacing is off. > > > But I think this would be clearer if you move the !ReadFromClipboard to an > > else > > > around 213ish. > > > > Please correct if my understanding is not correct. > > First check should be whether clipboard has any bookmark or not. > > If I do as suggested in that case the order will be reversed. > > PTAL new patch PTAL
On 2014/08/18 15:47:29, ankit wrote: > On 2014/08/16 07:44:21, ankit wrote: > > On 2014/08/16 07:43:57, ankit wrote: > > > > > > > > > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > > > > > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > > > components/bookmarks/browser/bookmark_utils.cc:199: return; > > > On 2014/08/15 18:24:55, sky wrote: > > > > spacing is off. > > > > But I think this would be clearer if you move the !ReadFromClipboard to an > > > else > > > > around 213ish. > > > > > > Please correct if my understanding is not correct. > > > First check should be whether clipboard has any bookmark or not. > > > If I do as suggested in that case the order will be reversed. > > > > PTAL new patch > > PTAL @sky PTAL new patch. I have refactored code as suggested.
On 2014/08/19 13:30:18, ankit wrote: > On 2014/08/18 15:47:29, ankit wrote: > > On 2014/08/16 07:44:21, ankit wrote: > > > On 2014/08/16 07:43:57, ankit wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > > > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/437423005/diff/60001/components/bookmarks/bro... > > > > components/bookmarks/browser/bookmark_utils.cc:199: return; > > > > On 2014/08/15 18:24:55, sky wrote: > > > > > spacing is off. > > > > > But I think this would be clearer if you move the !ReadFromClipboard to > an > > > > else > > > > > around 213ish. > > > > > > > > Please correct if my understanding is not correct. > > > > First check should be whether clipboard has any bookmark or not. > > > > If I do as suggested in that case the order will be reversed. > > > > > > PTAL new patch > > > > PTAL > > @sky > PTAL new patch. > I have refactored code as suggested. PTAL
@sky PTAL
On 2014/08/21 12:26:15, ankit wrote: > @sky > PTAL PTAL
https://codereview.chromium.org/437423005/diff/120001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.h (right): https://codereview.chromium.org/437423005/diff/120001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.h:154: bool ClipboardContainsValidUrl(base::string16&); I strongly urge you to read the style guide. Star with http://www.chromium.org/developers/coding-style and make sure to visit the c++ specific link. . Declaration and definition order should list argument names (and they should be the same). . Decleration and definition order need to be the same. Additionally declaration and definition order must be the same. . Only allowable references are const. If you need an out param, it should be a pointer. Why not make this return a URL? Having said all this, why both with this in the header? It is only used internally. It should be in an anonymous namespace in the .cc.
@sky PTAL
On 2014/08/27 13:28:27, ankit wrote: > @sky > PTAL @sky PTAL
https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:146: GURL ReadClipboardURL() { ReadClipboardURL -> GetUrlFromClipboard() https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:216: BookmarkNodeData bookmark(&node); Nuke this temporary, it's a bit confusingly named and you can inline on 217. https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:282: EXPECT_TRUE(CanPasteFromClipboard(model.get(), model->bookmark_bar_node())); You need to verify you get the right url here. Same thing below. I also want a test that if there is no url on the clipboard canpaste returns false.
@sky PTAL https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:146: GURL ReadClipboardURL() { On 2014/09/02 17:18:54, sky wrote: > ReadClipboardURL -> GetUrlFromClipboard() Done. https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:216: BookmarkNodeData bookmark(&node); On 2014/09/02 17:18:54, sky wrote: > Nuke this temporary, it's a bit confusingly named and you can inline on 217. Done. https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:282: EXPECT_TRUE(CanPasteFromClipboard(model.get(), model->bookmark_bar_node())); On 2014/09/02 17:18:54, sky wrote: > You need to verify you get the right url here. Same thing below. I also want a > test that if there is no url on the clipboard canpaste returns false. Verifying for right url below after pasting node. Added test case when no url is present on the clipboard, canpaste returns false.
On 2014/09/03 05:40:40, ankit wrote: > @sky > PTAL > > https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils.cc:146: GURL ReadClipboardURL() { > On 2014/09/02 17:18:54, sky wrote: > > ReadClipboardURL -> GetUrlFromClipboard() > > Done. > > https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils.cc:216: BookmarkNodeData > bookmark(&node); > On 2014/09/02 17:18:54, sky wrote: > > Nuke this temporary, it's a bit confusingly named and you can inline on 217. > > Done. > > https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_utils_unittest.cc (right): > > https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils_unittest.cc:282: > EXPECT_TRUE(CanPasteFromClipboard(model.get(), model->bookmark_bar_node())); > On 2014/09/02 17:18:54, sky wrote: > > You need to verify you get the right url here. Same thing below. I also want a > > test that if there is no url on the clipboard canpaste returns false. > Verifying for right url below after pasting node. Added test case when no url is > present on the clipboard, canpaste returns false. @sky PTAL
On 2014/09/04 15:40:42, ankit wrote: > On 2014/09/03 05:40:40, ankit wrote: > > @sky > > PTAL > > > > > https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... > > components/bookmarks/browser/bookmark_utils.cc:146: GURL ReadClipboardURL() { > > On 2014/09/02 17:18:54, sky wrote: > > > ReadClipboardURL -> GetUrlFromClipboard() > > > > Done. > > > > > https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... > > components/bookmarks/browser/bookmark_utils.cc:216: BookmarkNodeData > > bookmark(&node); > > On 2014/09/02 17:18:54, sky wrote: > > > Nuke this temporary, it's a bit confusingly named and you can inline on 217. > > > > Done. > > > > > https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... > > File components/bookmarks/browser/bookmark_utils_unittest.cc (right): > > > > > https://codereview.chromium.org/437423005/diff/160001/components/bookmarks/br... > > components/bookmarks/browser/bookmark_utils_unittest.cc:282: > > EXPECT_TRUE(CanPasteFromClipboard(model.get(), model->bookmark_bar_node())); > > On 2014/09/02 17:18:54, sky wrote: > > > You need to verify you get the right url here. Same thing below. I also want > a > > > test that if there is no url on the clipboard canpaste returns false. > > Verifying for right url below after pasting node. Added test case when no url > is > > present on the clipboard, canpaste returns false. > > @sky > PTAL PTAL
@sky PTAL
https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:145: // Returns URL if present in clipboard Returns the URL from the clipboard. If there is no URL an empty URL is returned. https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:276: // Write some valid url to the clipboard. Move your new assertions into their own test. Putting random assertions into CopyPasteMetaInfo in particular is wrong. https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:333: // Write some valid url to clipboard end with '.' https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:334: base::string16 url_text = ASCIIToUTF16("http://www.google.com/"); you've got two spaces here. And make url_text a const. https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:346: EXPECT_EQ(url_text,ASCIIToUTF16(folder->GetChild(1)->url().spec())); nit: space after ','
On 2014/09/08 16:26:19, sky wrote: > https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils.cc:145: // Returns URL if present in > clipboard > Returns the URL from the clipboard. If there is no URL an empty URL is returned. > > https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_utils_unittest.cc (right): > > https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils_unittest.cc:276: // Write some valid > url to the clipboard. > Move your new assertions into their own test. Putting random assertions into > CopyPasteMetaInfo in particular is wrong. > > https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils_unittest.cc:333: // Write some valid > url to clipboard > end with '.' > > https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils_unittest.cc:334: base::string16 > url_text = ASCIIToUTF16("http://www.google.com/"); > you've got two spaces here. And make url_text a const. > > https://codereview.chromium.org/437423005/diff/180001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils_unittest.cc:346: > EXPECT_EQ(url_text,ASCIIToUTF16(folder->GetChild(1)->url().spec())); > nit: space after ',' @sky PTAL new patch. I have incorporated review comments.
https://codereview.chromium.org/437423005/diff/200001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/437423005/diff/200001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:284: clipboard_writer.WriteText(ASCIIToUTF16("")); ASCIIToUTF16("")->base::string16()
@sky PTAL https://codereview.chromium.org/437423005/diff/200001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/437423005/diff/200001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:284: clipboard_writer.WriteText(ASCIIToUTF16("")); On 2014/09/09 15:52:29, sky wrote: > ASCIIToUTF16("")->base::string16() Done.
LGTM
On 2014/09/10 19:02:21, sky wrote: > LGTM Thanks.
The CQ bit was checked by ankit2.kumar@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/437423005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
The CQ bit was checked by ankit2.kumar@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/437423005/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as 76dace2f62ce9ad7348bb55c8011ac947ea75b0f
Message was sent while issue was closed.
On 2014/09/11 13:39:23, I haz the power (commit-bot) wrote: > Committed patchset #16 (id:300001) as 76dace2f62ce9ad7348bb55c8011ac947ea75b0f @sky Fixed build errors with following changes: Included clipboard header file as compilation was failing for |mac| and |android| os Added #if !defined(OS_IOS) in GetUrlFromClipboard as ui::Clipboard was throwing compile error. Removed ui::Clipboard::GetUrlWFormatType() as it is not available in clipboard_android.cc.
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/a54214a5ad96a46d6b06d43a841133d66172d49d Cr-Commit-Position: refs/heads/master@{#294380} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
