Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
446003002:
Title: Same Bookmark url is getting pasted on the Bookmarkbar with same title. (Closed)
DescriptionSame Bookmark url is getting pasted on the Bookmarkbar with same title.
When I have bookmarked a page and copy paste that
Bookmark in the bookmark bar then bookmark is getting pasted
with the same title. Changes have been done so that when we
paste bookmark in the bookmark bar or in the folder that
already have that bookmark then new bookmark will get added
with modified title.
For example: "Autofill" bookmark is present in the BookmarkBar
and user copy it and pasted it again in the Bookmark Bar then
new entry will get added as "Autofill(1)", so that user
know it is copy of the existing Bookmark.
BUG=400682
R=sky@chromium.org
Committed: https://crrev.com/33164f770ae372ab34060bec75ef7a4bbfba1607
Cr-Commit-Position: refs/heads/master@{#299680}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Changes done as per reviewer comments. #
Total comments: 7
Patch Set 3 : Changes done as per comments and mentioned reason for functions usage. #
Total comments: 1
Patch Set 4 : changes as per review comments #Patch Set 5 : Changes as per review comments and test case added. #
Total comments: 11
Patch Set 6 : Changes as per review comments. #Patch Set 7 : Changing vector to hash_set as suggested. #
Total comments: 4
Patch Set 8 : Changes as per Review comments. #Patch Set 9 : removing changes from header file. #
Total comments: 10
Patch Set 10 : Changes as per review comments. #
Total comments: 8
Patch Set 11 : Changes as per review comments. #Patch Set 12 : TestCase update. #
Total comments: 4
Patch Set 13 : Changes as per review comments. #
Total comments: 2
Patch Set 14 : Changes as per review comments. #
Total comments: 2
Patch Set 15 : Changes as per review comments. #Patch Set 16 : changes as per review comments. #
Total comments: 2
Patch Set 17 : Addressed a nit. #Patch Set 18 : #Patch Set 19 : #
Messages
Total messages: 64 (8 generated)
PTAL
You only need a single reviewer for this. Also you don't need to write "Title:" and "Description:" in the CL description, these are inferred. Removing Brett from the reviewers list and updating the CL description. Thanks!
PTAL
Please review..
Please review this patch.
I'll let Scott judge the correctness of your algorithm. https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... File components/bookmarks/browser/bookmark_model.h (right): https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_model.h:172: const base::string16 title, const base::string16& https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_model.h:175: std::vector<base::string16>* titles); why you here titles here? https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_utils.cc:211: base::string16 newTitle = bookmark_data.elements[0].title; new_title but, pointing this style issues is getting repetitive. Please, take a moment to read http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml and http://www.chromium.org/developers/coding-style https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_utils.cc:223: newTitle = bookmark_data.elements[0].title + was this clang-formatted?
Changes done as per reviewer's comments. PTAL https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... File components/bookmarks/browser/bookmark_model.h (right): https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_model.h:172: const base::string16 title, On 2014/08/13 13:40:52, tfarina wrote: > const base::string16& Done. https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_model.h:175: std::vector<base::string16>* titles); On 2014/08/13 13:40:52, tfarina wrote: > why you here titles here? As I am making a vector that have titles of the nodes that satisfy my filter condition. https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_utils.cc:211: base::string16 newTitle = bookmark_data.elements[0].title; On 2014/08/13 13:40:52, tfarina wrote: > new_title > > but, pointing this style issues is getting repetitive. > > Please, take a moment to read > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml and > http://www.chromium.org/developers/coding-style Done. https://codereview.chromium.org/446003002/diff/1/components/bookmarks/browser... components/bookmarks/browser/bookmark_utils.cc:223: newTitle = bookmark_data.elements[0].title + On 2014/08/13 13:40:52, tfarina wrote: > was this clang-formatted? yes , I have used git cl format for making it formatted.
PTAL
https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_model.cc (right): https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... components/bookmarks/browser/bookmark_model.cc:510: void BookmarkModel::GetNodesByURLAndTitle( There is no reason for this in BookmarkModel. https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:207: model->IsBookmarked(bookmark_data.elements[0].url)) { It's not IsBookmarked you sure care about, rather if there is a bookmark in the same folder with the same name. Only then should change the name. Further the Copy name below needs to be localized. You should investigate what we do for downloaded files, it may be possible to use the same logic. And you'll need a test.
https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_model.cc (right): https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... components/bookmarks/browser/bookmark_model.cc:510: void BookmarkModel::GetNodesByURLAndTitle( On 2014/08/18 16:15:21, sky wrote: > There is no reason for this in BookmarkModel. we need this function so that I can make vector of titles and vector of the node pointers that have the same parent as the node that I have copied ,and their title is either same as the copied node or the starts with copied node title, As I am distinguished node by adding some suffix with the node title. I have made this function inline with the GetNodesByURL(), with some more parameters based on requirment.So that nodes under a parent are distinguished. If I am not doing this here then I have to do more processing in PasteFromClipboard(), after getting the nodelist.so It is better I will do that while making the node vector here itself. https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:207: model->IsBookmarked(bookmark_data.elements[0].url)) { On 2014/08/18 16:15:21, sky wrote: > It's not IsBookmarked you sure care about, rather if there is a bookmark in the > same folder with the same name. Only then should change the name. Further the > Copy name below needs to be localized. You should investigate what we do for > downloaded files, it may be possible to use the same logic. And you'll need a > test. IsBookmarked() will tell me weather a Bookmark with 'url' is present or not, But when user add a new bookmark that have the same url but different title then also it will return true, as it see's url but user see 'Title' of the Bookmark at the bookmark bar. Then I need to know which copy number user is going to make so I need all the nodes that are starting with copied bookmark title and have the same parent as the copied one. And I need to take care when the user have the same bookmark page marked in the bookmark bar with differet 'titles'. I understand the 'Copy-' localization. so I think it is better I just mention numbers.I have also checked while downloading we are making like test.html , test(1).html, test(2).html. In new patch I will follow same. Please review changes, I will upload the test case soon as my changes get approved.
https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_model.cc (right): https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... components/bookmarks/browser/bookmark_model.cc:510: void BookmarkModel::GetNodesByURLAndTitle( On 2014/08/19 11:42:19, deepak.m1 wrote: > On 2014/08/18 16:15:21, sky wrote: > > There is no reason for this in BookmarkModel. > > we need this function so that I can make vector of titles and vector of the node > pointers that have the same parent as the node that I have copied ,and their > title is either same as the copied node or the starts with copied node title, As > I am distinguished node by adding some suffix with the node title. > > I have made this function inline with the GetNodesByURL(), with some more > parameters based on requirment.So that nodes under a parent are distinguished. > > If I am not doing this here then I have to do more processing in > PasteFromClipboard(), after getting the nodelist.so It is better I will do that > while making the node vector here itself. This function is only useful in one place and doesn't need internals of the bookmarkmodel, it can get at all this via public api. There is no reason for it here. https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... components/bookmarks/browser/bookmark_model.cc:522: StartsWith((*i)->GetTitle(), title, false))) { The StartsWith logic makes no sense here. https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:207: model->IsBookmarked(bookmark_data.elements[0].url)) { On 2014/08/19 11:42:19, deepak.m1 wrote: > On 2014/08/18 16:15:21, sky wrote: > > It's not IsBookmarked you sure care about, rather if there is a bookmark in > the > > same folder with the same name. Only then should change the name. Further the > > Copy name below needs to be localized. You should investigate what we do for > > downloaded files, it may be possible to use the same logic. And you'll need a > > test. > > IsBookmarked() will tell me weather a Bookmark with 'url' is present or not, But > when user add a new bookmark that have the same url but different title then > also it will return true, as it see's url but user see 'Title' of the Bookmark > at the bookmark bar. > Then I need to know which copy number user is going to make so I need all the > nodes that are starting with copied bookmark title and have the same parent as > the copied one. > > And I need to take care when the user have the same bookmark page marked in the > bookmark bar with differet 'titles'. > > I understand the 'Copy-' localization. so I think it is better I just mention > numbers.I have also checked while downloading we are making like test.html , > test(1).html, test(2).html. > In new patch I will follow same. > > Please review changes, I will upload the test case soon as my changes get > approved. You should do the same as we do for downloads, and you should only consider duplicates if both the title and url match.
On 2014/08/19 16:38:31, sky wrote: > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_model.cc (right): > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_model.cc:510: void > BookmarkModel::GetNodesByURLAndTitle( > On 2014/08/19 11:42:19, deepak.m1 wrote: > > On 2014/08/18 16:15:21, sky wrote: > > > There is no reason for this in BookmarkModel. > > > > we need this function so that I can make vector of titles and vector of the > node > > pointers that have the same parent as the node that I have copied ,and their > > title is either same as the copied node or the starts with copied node title, > As > > I am distinguished node by adding some suffix with the node title. > > > > I have made this function inline with the GetNodesByURL(), with some more > > parameters based on requirment.So that nodes under a parent are distinguished. > > > > If I am not doing this here then I have to do more processing in > > PasteFromClipboard(), after getting the nodelist.so It is better I will do > that > > while making the node vector here itself. > > This function is only useful in one place and doesn't need internals of the > bookmarkmodel, it can get at all this via public api. There is no reason for it > here. > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_model.cc:522: StartsWith((*i)->GetTitle(), > title, false))) { > The StartsWith logic makes no sense here. > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:207: > model->IsBookmarked(bookmark_data.elements[0].url)) { > On 2014/08/19 11:42:19, deepak.m1 wrote: > > On 2014/08/18 16:15:21, sky wrote: > > > It's not IsBookmarked you sure care about, rather if there is a bookmark in > > the > > > same folder with the same name. Only then should change the name. Further > the > > > Copy name below needs to be localized. You should investigate what we do for > > > downloaded files, it may be possible to use the same logic. And you'll need > a > > > test. > > > > IsBookmarked() will tell me weather a Bookmark with 'url' is present or not, > But > > when user add a new bookmark that have the same url but different title then > > also it will return true, as it see's url but user see 'Title' of the Bookmark > > at the bookmark bar. > > Then I need to know which copy number user is going to make so I need all the > > nodes that are starting with copied bookmark title and have the same parent as > > the copied one. > > > > And I need to take care when the user have the same bookmark page marked in > the > > bookmark bar with differet 'titles'. > > > > I understand the 'Copy-' localization. so I think it is better I just mention > > numbers.I have also checked while downloading we are making like test.html , > > test(1).html, test(2).html. > > In new patch I will follow same. > > > > Please review changes, I will upload the test case soon as my changes get > > approved. > > You should do the same as we do for downloads, and you should only consider > duplicates if both the title and url match. Thanks sky for your time and comments. i)Can you please direct me about 'downloads' part, are you saying we should have same behavior as 'downloads' then while downloading same file we are getting file, file(1), file(2).. or if you are refering code part then please help me where should I look for downloads code. ii) I am alrteady considering duplicate only when url and title both are matching. please refer to below code. NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find(&tmp_node); 519 while (i != nodes_ordered_by_url_set_.end() && (*i)->url() == url) { 520 if ((parent == (*i)->parent()) && 521 (title == (*i)->GetTitle() || 522 StartsWith((*i)->GetTitle(), title, false))) { 523 titles->push_back((*i)->GetTitle()); 524 nodes->push_back(*i); 525 } 526 ++i; 527 } Here After getting all the node that matchs with the url, I am cheking for title title == (*i)->GetTitle(), that means considering both 'url' and 'title' and StartsWith((*i)->GetTitle(), title, false)) is used to know how many copies are present under the same parent, so that I can decide (subscript) for the making the next node. Please let me know your opinion for this.
On 2014/08/20 04:37:54, deepak.m1 wrote: > On 2014/08/19 16:38:31, sky wrote: > > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > > File components/bookmarks/browser/bookmark_model.cc (right): > > > > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > > components/bookmarks/browser/bookmark_model.cc:510: void > > BookmarkModel::GetNodesByURLAndTitle( > > On 2014/08/19 11:42:19, deepak.m1 wrote: > > > On 2014/08/18 16:15:21, sky wrote: > > > > There is no reason for this in BookmarkModel. > > > > > > we need this function so that I can make vector of titles and vector of the > > node > > > pointers that have the same parent as the node that I have copied ,and their > > > title is either same as the copied node or the starts with copied node > title, > > As > > > I am distinguished node by adding some suffix with the node title. > > > > > > I have made this function inline with the GetNodesByURL(), with some more > > > parameters based on requirment.So that nodes under a parent are > distinguished. > > > > > > If I am not doing this here then I have to do more processing in > > > PasteFromClipboard(), after getting the nodelist.so It is better I will do > > that > > > while making the node vector here itself. > > > > This function is only useful in one place and doesn't need internals of the > > bookmarkmodel, it can get at all this via public api. There is no reason for > it > > here. > > > > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > > components/bookmarks/browser/bookmark_model.cc:522: > StartsWith((*i)->GetTitle(), > > title, false))) { > > The StartsWith logic makes no sense here. > > > > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > > components/bookmarks/browser/bookmark_utils.cc:207: > > model->IsBookmarked(bookmark_data.elements[0].url)) { > > On 2014/08/19 11:42:19, deepak.m1 wrote: > > > On 2014/08/18 16:15:21, sky wrote: > > > > It's not IsBookmarked you sure care about, rather if there is a bookmark > in > > > the > > > > same folder with the same name. Only then should change the name. Further > > the > > > > Copy name below needs to be localized. You should investigate what we do > for > > > > downloaded files, it may be possible to use the same logic. And you'll > need > > a > > > > test. > > > > > > IsBookmarked() will tell me weather a Bookmark with 'url' is present or not, > > But > > > when user add a new bookmark that have the same url but different title then > > > also it will return true, as it see's url but user see 'Title' of the > Bookmark > > > at the bookmark bar. > > > Then I need to know which copy number user is going to make so I need all > the > > > nodes that are starting with copied bookmark title and have the same parent > as > > > the copied one. > > > > > > And I need to take care when the user have the same bookmark page marked in > > the > > > bookmark bar with differet 'titles'. > > > > > > I understand the 'Copy-' localization. so I think it is better I just > mention > > > numbers.I have also checked while downloading we are making like test.html , > > > test(1).html, test(2).html. > > > In new patch I will follow same. > > > > > > Please review changes, I will upload the test case soon as my changes get > > > approved. > > > > You should do the same as we do for downloads, and you should only consider > > duplicates if both the title and url match. > Thanks sky for your time and comments. i)Can you please direct me about 'downloads' part, are you saying we should have same behavior as 'downloads' then while downloading same file we are getting file, file(1), file(2).. or if you are refering code part then please help me where should I look for downloads code. ii) I am alrteady considering duplicate only when url and title both are matching. please refer to below code. NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find(&tmp_node); 519 while (i != nodes_ordered_by_url_set_.end() && (*i)->url() == url) { 520 if ((parent == (*i)->parent()) && 521 (title == (*i)->GetTitle() || 522 StartsWith((*i)->GetTitle(), title, false))) { 523 titles->push_back((*i)->GetTitle()); 524 nodes->push_back(*i); 525 } 526 ++i; 527 } Here After getting all the node that matchs with the url, I am cheking for title title == (*i)->GetTitle(), that means considering both 'url' and 'title' and StartsWith((*i)->GetTitle(), title, false)) is used to know how many copies are present under the same parent, so that I can decide (subscript) for the making the next node. Please let me know your opinion for this. @PTAL
deepak.m1@samsung.com changed reviewers: + finnur@chromium.org
deepak.m1@samsung.com changed reviewers: + brettw@chromium.org - finnur@chromium.org
On 2014/08/28 09:52:51, deepak.m1 wrote: > On 2014/08/20 04:37:54, deepak.m1 wrote: > > On 2014/08/19 16:38:31, sky wrote: > > > > > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > > > File components/bookmarks/browser/bookmark_model.cc (right): > > > > > > > > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > > > components/bookmarks/browser/bookmark_model.cc:510: void > > > BookmarkModel::GetNodesByURLAndTitle( > > > On 2014/08/19 11:42:19, deepak.m1 wrote: > > > > On 2014/08/18 16:15:21, sky wrote: > > > > > There is no reason for this in BookmarkModel. > > > > > > > > we need this function so that I can make vector of titles and vector of > the > > > node > > > > pointers that have the same parent as the node that I have copied ,and > their > > > > title is either same as the copied node or the starts with copied node > > title, > > > As > > > > I am distinguished node by adding some suffix with the node title. > > > > > > > > I have made this function inline with the GetNodesByURL(), with some more > > > > parameters based on requirment.So that nodes under a parent are > > distinguished. > > > > > > > > If I am not doing this here then I have to do more processing in > > > > PasteFromClipboard(), after getting the nodelist.so It is better I will do > > > that > > > > while making the node vector here itself. > > > > > > This function is only useful in one place and doesn't need internals of the > > > bookmarkmodel, it can get at all this via public api. There is no reason for > > it > > > here. > > > > > > > > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > > > components/bookmarks/browser/bookmark_model.cc:522: > > StartsWith((*i)->GetTitle(), > > > title, false))) { > > > The StartsWith logic makes no sense here. > > > > > > > > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > > > > > https://codereview.chromium.org/446003002/diff/20001/components/bookmarks/bro... > > > components/bookmarks/browser/bookmark_utils.cc:207: > > > model->IsBookmarked(bookmark_data.elements[0].url)) { > > > On 2014/08/19 11:42:19, deepak.m1 wrote: > > > > On 2014/08/18 16:15:21, sky wrote: > > > > > It's not IsBookmarked you sure care about, rather if there is a bookmark > > in > > > > the > > > > > same folder with the same name. Only then should change the name. > Further > > > the > > > > > Copy name below needs to be localized. You should investigate what we do > > for > > > > > downloaded files, it may be possible to use the same logic. And you'll > > need > > > a > > > > > test. > > > > > > > > IsBookmarked() will tell me weather a Bookmark with 'url' is present or > not, > > > But > > > > when user add a new bookmark that have the same url but different title > then > > > > also it will return true, as it see's url but user see 'Title' of the > > Bookmark > > > > at the bookmark bar. > > > > Then I need to know which copy number user is going to make so I need all > > the > > > > nodes that are starting with copied bookmark title and have the same > parent > > as > > > > the copied one. > > > > > > > > And I need to take care when the user have the same bookmark page marked > in > > > the > > > > bookmark bar with differet 'titles'. > > > > > > > > I understand the 'Copy-' localization. so I think it is better I just > > mention > > > > numbers.I have also checked while downloading we are making like test.html > , > > > > test(1).html, test(2).html. > > > > In new patch I will follow same. > > > > > > > > Please review changes, I will upload the test case soon as my changes get > > > > approved. > > > > > > You should do the same as we do for downloads, and you should only consider > > > duplicates if both the title and url match. > > > Thanks sky for your time and comments. > > i)Can you please direct me about 'downloads' part, > are you saying we should have same behavior as 'downloads' > then while downloading same file we are getting file, file(1), file(2).. > or if you are refering code part then please help me where should I look for > downloads code. > > ii) I am alrteady considering duplicate only when url and title both are > matching. > please refer to below code. > > NodesOrderedByURLSet::iterator i = > nodes_ordered_by_url_set_.find(&tmp_node); > 519 while (i != nodes_ordered_by_url_set_.end() && (*i)->url() == url) { > 520 if ((parent == (*i)->parent()) && > 521 (title == (*i)->GetTitle() || > 522 StartsWith((*i)->GetTitle(), title, false))) { > 523 titles->push_back((*i)->GetTitle()); > 524 nodes->push_back(*i); > 525 } > 526 ++i; > 527 } > Here After getting all the node that matchs with the url, I am cheking for > title > title == (*i)->GetTitle(), that means considering both 'url' and 'title' and > StartsWith((*i)->GetTitle(), title, false)) is used to know how many copies are > present under the same parent, so that I can decide (subscript) for the making > the next node. > > Please let me know your opinion for this. @PTAL
deepak.m1@samsung.com changed reviewers: + tfarina@chromium.org - brettw@chromium.org
PTAL
You need to add test coverage too. https://codereview.chromium.org/446003002/diff/40001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_model.h (right): https://codereview.chromium.org/446003002/diff/40001/components/bookmarks/bro... components/bookmarks/browser/bookmark_model.h:171: void GetNodesByURLAndTitle(const GURL& url, There is no need for this function on BookmarkModel. Iterate through the children of the node you care about in bookmark_utils.
On 2014/09/22 16:40:52, sky wrote: > You need to add test coverage too. > > https://codereview.chromium.org/446003002/diff/40001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_model.h (right): Funtion Removed, Thanks for Guidance. > https://codereview.chromium.org/446003002/diff/40001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_model.h:171: void > GetNodesByURLAndTitle(const GURL& url, > There is no need for this function on BookmarkModel. Iterate through the > children of the node you care about in bookmark_utils. GetNodesByURLAndTitle() Removed, Thanks for Guidance. I used same approach as suggested of traversing children. Test case added, and verified. PTAL.
On 2014/09/23 12:44:58, deepak.m1 wrote: > On 2014/09/22 16:40:52, sky wrote: > > You need to add test coverage too. > > https://codereview.chromium.org/446003002/diff/40001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_model.h (right): Funtion Removed, Thanks for Guidance. > https://codereview.chromium.org/446003002/diff/40001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_model.h:171: void > GetNodesByURLAndTitle(const GURL& url, > There is no need for this function on BookmarkModel. Iterate through the > children of the node you care about in bookmark_utils. GetNodesByURLAndTitle() Removed, Thanks for Guidance. I used same approach as suggested of traversing children. Test case added, and verified. PTAL.
https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:225: if (bookmark_data.elements.size() == 1 && Move this into a helper function. https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:227: std::vector<const BookmarkNode*> nodes; AFAICT you don't need this. https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:228: std::vector<base::string16> titles; Use a hashset. https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:229: std::vector<base::string16>::iterator it; Declare where needed. https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:247: new_title = bookmark_data.elements[0].title + base::UTF8ToUTF16("(") + Use StringPrintf.
On 2014/09/24 14:26:49, sky wrote: > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:225: if > (bookmark_data.elements.size() == 1 && > Move this into a helper function. Done. > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:227: std::vector<const > BookmarkNode*> nodes; > AFAICT you don't need this. Agreed. > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:228: std::vector<base::string16> > titles; > Use a hashset. For using HashSet, I have added third_party/WebKit/Source/wtf/HashSet.h header. But as HashSet internally have #include "wtf/DefaultAllocator.h" #include "wtf/HashTable.h" I am getting compilation error for HashSet usage in bookmark_utils.cc file. I tried to get HashSet usage in Bookmarks code for reference, but unable to find that code. so I am using vector here again. > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:229: > std::vector<base::string16>::iterator it; > Declare where needed. Done. > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:247: new_title = > bookmark_data.elements[0].title + base::UTF8ToUTF16("(") + > Use StringPrintf. Done. PTAL
PTAL https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:225: if (bookmark_data.elements.size() == 1 && On 2014/09/24 14:26:49, sky wrote: > Move this into a helper function. Done. https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:227: std::vector<const BookmarkNode*> nodes; On 2014/09/24 14:26:49, sky wrote: > AFAICT you don't need this. Acknowledged. https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:228: std::vector<base::string16> titles; On 2014/09/24 14:26:49, sky wrote: > Use a hashset. For using HashSet, I have added third_party/WebKit/Source/wtf/HashSet.h header. But as HashSet internally have #include "wtf/DefaultAllocator.h" #include "wtf/HashTable.h" I am getting compilation error for HashSet usage in bookmark_utils.cc file. I tried to get HashSet usage in Bookmarks code for reference, but unable to find that code. so I am using vector here again. https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:229: std::vector<base::string16>::iterator it; On 2014/09/24 14:26:49, sky wrote: > Declare where needed. Done. https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:247: new_title = bookmark_data.elements[0].title + base::UTF8ToUTF16("(") + On 2014/09/24 14:26:49, sky wrote: > Use StringPrintf. Done.
PTAL
https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... components/bookmarks/browser/bookmark_utils.cc:228: std::vector<base::string16> titles; On 2014/09/25 05:59:11, Deepak wrote: > On 2014/09/24 14:26:49, sky wrote: > > Use a hashset. > > For using HashSet, I have added > third_party/WebKit/Source/wtf/HashSet.h header. > But as HashSet internally have > #include "wtf/DefaultAllocator.h" > #include "wtf/HashTable.h" > > I am getting compilation error for HashSet usage in bookmark_utils.cc file. I > tried to get HashSet usage in Bookmarks code for reference, but unable to find > that code. so I am using vector here again. Don't use webkit types in chrome. Use the types in base.
On 2014/09/26 16:14:10, sky wrote: > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > components/bookmarks/browser/bookmark_utils.cc:228: std::vector<base::string16> > titles; > On 2014/09/25 05:59:11, Deepak wrote: > > On 2014/09/24 14:26:49, sky wrote: > > > Use a hashset. > > > > For using HashSet, I have added > > third_party/WebKit/Source/wtf/HashSet.h header. > > But as HashSet internally have > > #include "wtf/DefaultAllocator.h" > > #include "wtf/HashTable.h" > > > > I am getting compilation error for HashSet usage in bookmark_utils.cc file. I > > tried to get HashSet usage in Bookmarks code for reference, but unable to find > > that code. so I am using vector here again. > > Don't use webkit types in chrome. Use the types in base. I agree with you that we should not use Webkit types,But I does not found any HashSet in base. Please guide me what type of base, I should use in place of current vector usage. Thanks.
On 2014/09/27 08:45:54, Deepak wrote: > On 2014/09/26 16:14:10, sky wrote: > > > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > > components/bookmarks/browser/bookmark_utils.cc:228: > std::vector<base::string16> > > titles; > > On 2014/09/25 05:59:11, Deepak wrote: > > > On 2014/09/24 14:26:49, sky wrote: > > > > Use a hashset. > > > > > > For using HashSet, I have added > > > third_party/WebKit/Source/wtf/HashSet.h header. > > > But as HashSet internally have > > > #include "wtf/DefaultAllocator.h" > > > #include "wtf/HashTable.h" > > > > > > I am getting compilation error for HashSet usage in bookmark_utils.cc file. > I > > > tried to get HashSet usage in Bookmarks code for reference, but unable to > find > > > that code. so I am using vector here again. > > > > Don't use webkit types in chrome. Use the types in base. > I agree with you that we should not use Webkit types,But I does not found any HashSet in base. Please guide me what type of base, I should use in place of current vector usage. Thanks.
On 2014/09/29 15:02:21, Deepak wrote: > On 2014/09/27 08:45:54, Deepak wrote: > > On 2014/09/26 16:14:10, sky wrote: > > > > > > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > > > > > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > > > components/bookmarks/browser/bookmark_utils.cc:228: > > std::vector<base::string16> > > > titles; > > > On 2014/09/25 05:59:11, Deepak wrote: > > > > On 2014/09/24 14:26:49, sky wrote: > > > > > Use a hashset. > > > > > > > > For using HashSet, I have added > > > > third_party/WebKit/Source/wtf/HashSet.h header. > > > > But as HashSet internally have > > > > #include "wtf/DefaultAllocator.h" > > > > #include "wtf/HashTable.h" > > > > > > > > I am getting compilation error for HashSet usage in bookmark_utils.cc > file. > > I > > > > tried to get HashSet usage in Bookmarks code for reference, but unable to > > find > > > > that code. so I am using vector here again. > > > > > > Don't use webkit types in chrome. Use the types in base. > > > I agree with you that we should not use Webkit types,But I does not found any > HashSet in base. > Please guide me what type of base, I should use in place of current vector > usage. > Thanks. See base/containers/hash_tables.h
On 2014/09/29 17:59:00, sky wrote: > On 2014/09/29 15:02:21, Deepak wrote: > > On 2014/09/27 08:45:54, Deepak wrote: > > > On 2014/09/26 16:14:10, sky wrote: > > > > > > > > > > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > > > > File components/bookmarks/browser/bookmark_utils.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/446003002/diff/80001/components/bookmarks/bro... > > > > components/bookmarks/browser/bookmark_utils.cc:228: > > > std::vector<base::string16> > > > > titles; > > > > On 2014/09/25 05:59:11, Deepak wrote: > > > > > On 2014/09/24 14:26:49, sky wrote: > > > > > > Use a hashset. > > > > > > > > > > For using HashSet, I have added > > > > > third_party/WebKit/Source/wtf/HashSet.h header. > > > > > But as HashSet internally have > > > > > #include "wtf/DefaultAllocator.h" > > > > > #include "wtf/HashTable.h" > > > > > > > > > > I am getting compilation error for HashSet usage in bookmark_utils.cc > > file. > > > I > > > > > tried to get HashSet usage in Bookmarks code for reference, but unable > to > > > find > > > > > that code. so I am using vector here again. > > > > > > > > Don't use webkit types in chrome. Use the types in base. > > > > > I agree with you that we should not use Webkit types,But I does not found any > > HashSet in base. > > Please guide me what type of base, I should use in place of current vector > > usage. > > Thanks. > > See base/containers/hash_tables.h Thanks sky for guidance, I have done chnages with using hash_set. PTAL.
https://codereview.chromium.org/446003002/diff/120001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/120001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:209: BookmarkNodeData* bookmark_data) { BookmarkNodeData may contain any number of bookmarks, yet this only applies to a single bookmark. Make the function take the URL and title. https://codereview.chromium.org/446003002/diff/120001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.h (right): https://codereview.chromium.org/446003002/diff/120001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.h:59: void UpdateCopiedBookmarkTitle(BookmarkModel* model, Why is this in the header? AFAICT it's only needed in the implementation.
Thanks for review. I have done changes as suggested. PTAL https://codereview.chromium.org/446003002/diff/120001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/120001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:209: BookmarkNodeData* bookmark_data) { On 2014/09/30 15:45:16, sky wrote: > BookmarkNodeData may contain any number of bookmarks, yet this only applies to a > single bookmark. Make the function take the URL and title. Done. https://codereview.chromium.org/446003002/diff/120001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.h (right): https://codereview.chromium.org/446003002/diff/120001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.h:59: void UpdateCopiedBookmarkTitle(BookmarkModel* model, On 2014/09/30 15:45:16, sky wrote: > Why is this in the header? AFAICT it's only needed in the implementation. Done.
https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:213: const base::string16& title) { This should be a pointer and modified in place. https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:219: (new_title == node->GetTitle() || Doesn't the StartsWith cover this case too? https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:230: base::StringPrintf("%s(%lu)", base::UTF16ToUTF8(title).c_str(), i + 1)); Add a space here. https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:230: base::StringPrintf("%s(%lu)", base::UTF16ToUTF8(title).c_str(), i + 1)); All this conversion is ugly. Can you do just one conversion? https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:232: break; You can return here, right?
PTAL https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:213: const base::string16& title) { On 2014/10/01 19:28:26, sky wrote: > This should be a pointer and modified in place. Done. https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:219: (new_title == node->GetTitle() || On 2014/10/01 19:28:25, sky wrote: > Doesn't the StartsWith cover this case too? Done. https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:230: base::StringPrintf("%s(%lu)", base::UTF16ToUTF8(title).c_str(), i + 1)); On 2014/10/01 19:28:26, sky wrote: > Add a space here. Done. https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:230: base::StringPrintf("%s(%lu)", base::UTF16ToUTF8(title).c_str(), i + 1)); On 2014/10/01 19:28:26, sky wrote: > All this conversion is ugly. Can you do just one conversion? As I am creating a new title everytime by changing the subscript in the input title.and base::StringPrintf() return me std::string that is UTF8 and our new_title is UTF16, so base::UTF8ToUTF16 in base::UTF8ToUTF16(base::StringPrintf()) above I can not avoid. and StringPrintf() takes const char* as I need to change title to UTF8 format.so that %s can use that.It is also necessary for me. https://codereview.chromium.org/446003002/diff/160001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:232: break; On 2014/10/01 19:28:26, sky wrote: > You can return here, right? Done.
PTAL
On 2014/10/05 05:50:45, Deepak wrote: > PTAL PTAL
https://codereview.chromium.org/446003002/diff/180001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:207: // This function will update title for the bookmark with numeric subscript if // Updates |title| such that |url| and |title| pair are unique among the children // of |parent|. MakeTitleUnique() https://codereview.chromium.org/446003002/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:214: base::string16 new_title = *title; Keep variables close to where you need them. You should only need this at 227. https://codereview.chromium.org/446003002/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:223: if (titles.size() == 0 || titles.find(new_title) == titles.end()) Why bother with the size check (empty() is better here anyway). Doesn't the find cover the case you care about? https://codereview.chromium.org/446003002/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:227: new_title = base::UTF8ToUTF16(base::StringPrintf( This is still more verbose than you need.
PTAL https://codereview.chromium.org/446003002/diff/180001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:207: // This function will update title for the bookmark with numeric subscript if On 2014/10/07 16:21:23, sky wrote: > > // Updates |title| such that |url| and |title| pair are unique among the > children > // of |parent|. > MakeTitleUnique() Done. https://codereview.chromium.org/446003002/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:214: base::string16 new_title = *title; On 2014/10/07 16:21:23, sky wrote: > Keep variables close to where you need them. You should only need this at 227. Done. https://codereview.chromium.org/446003002/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:223: if (titles.size() == 0 || titles.find(new_title) == titles.end()) On 2014/10/07 16:21:23, sky wrote: > Why bother with the size check (empty() is better here anyway). Doesn't the find > cover the case you care about? Done. https://codereview.chromium.org/446003002/diff/180001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:227: new_title = base::UTF8ToUTF16(base::StringPrintf( On 2014/10/07 16:21:23, sky wrote: > This is still more verbose than you need. This verbose is due to way StringPrintf() works. As It takes utf8 data and our *title and new_title are utf16. and I need to take titles hash_set string16,as node->GetTitle() gives me string16. If we are making StringPrintf() statement short then conversion shift at other places.I think that will do more cluttering. please suggest if you have some other opinion for this.
https://codereview.chromium.org/446003002/diff/220001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/220001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:225: base::string16 new_title = *title; new_title should be moved into for loop. https://codereview.chromium.org/446003002/diff/220001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:228: "%s (%lu)", base::UTF16ToUTF8(*title).c_str(), i + 1)); const base::string16 new_title(*title + base::ASCIIToUTF16(base::StringPrintf("(%lu"), i + 1)));
On 2014/10/08 15:30:27, sky wrote: > https://codereview.chromium.org/446003002/diff/220001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_utils.cc (right): > > https://codereview.chromium.org/446003002/diff/220001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils.cc:225: base::string16 new_title = > *title; > new_title should be moved into for loop. > > https://codereview.chromium.org/446003002/diff/220001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils.cc:228: "%s (%lu)", > base::UTF16ToUTF8(*title).c_str(), i + 1)); > const base::string16 new_title(*title + > base::ASCIIToUTF16(base::StringPrintf("(%lu"), i + 1))); Thanks for review & suggestion in Stringprintf. Changes done. PTAL.
PTAL https://codereview.chromium.org/446003002/diff/220001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/220001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:225: base::string16 new_title = *title; On 2014/10/08 15:30:27, sky wrote: > new_title should be moved into for loop. Done. https://codereview.chromium.org/446003002/diff/220001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:228: "%s (%lu)", base::UTF16ToUTF8(*title).c_str(), i + 1)); On 2014/10/08 15:30:27, sky wrote: > const base::string16 new_title(*title + > base::ASCIIToUTF16(base::StringPrintf("(%lu"), i + 1))); Acknowledged.
https://codereview.chromium.org/446003002/diff/240001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/240001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:233: } Seems like there should be a NOTREACHED here, right?
NOTREACHED() added.Thanks PTAL. https://codereview.chromium.org/446003002/diff/240001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils.cc (right): https://codereview.chromium.org/446003002/diff/240001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils.cc:233: } On 2014/10/09 17:14:24, sky wrote: > Seems like there should be a NOTREACHED here, right? Done.
https://codereview.chromium.org/446003002/diff/280001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/446003002/diff/280001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:317: TEST_F(BookmarkUtilsTest, PasteSameBookmarkedURL) { Either add a description of what this test is verifying, or make the title reflect that. Right now the name is overly generic and doesn't indicate what this is asserting.
Test function name changed and its usage updated. PTAL https://codereview.chromium.org/446003002/diff/280001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/446003002/diff/280001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:317: TEST_F(BookmarkUtilsTest, PasteSameBookmarkedURL) { On 2014/10/10 17:32:45, sky wrote: > Either add a description of what this test is verifying, or make the title > reflect that. Right now the name is overly generic and doesn't indicate what > this is asserting. Done.
On 2014/10/11 06:32:39, Deepak wrote: Test function name changed and its usage updated. PTAL > https://codereview.chromium.org/446003002/diff/280001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_utils_unittest.cc (right): > > https://codereview.chromium.org/446003002/diff/280001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils_unittest.cc:317: > TEST_F(BookmarkUtilsTest, PasteSameBookmarkedURL) { > On 2014/10/10 17:32:45, sky wrote: > > Either add a description of what this test is verifying, or make the title > > reflect that. Right now the name is overly generic and doesn't indicate what > > this is asserting. > Done.
LGTM https://codereview.chromium.org/446003002/diff/350001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/446003002/diff/350001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:319: TEST_F(BookmarkUtilsTest, MakeTitleUniqueTest) { You don't need to duplicate Test. MakeTitleUnique is fine.
On 2014/10/14 19:22:33, sky wrote: > LGTM > > https://codereview.chromium.org/446003002/diff/350001/components/bookmarks/br... > File components/bookmarks/browser/bookmark_utils_unittest.cc (right): > > https://codereview.chromium.org/446003002/diff/350001/components/bookmarks/br... > components/bookmarks/browser/bookmark_utils_unittest.cc:319: > TEST_F(BookmarkUtilsTest, MakeTitleUniqueTest) { > You don't need to duplicate Test. MakeTitleUnique is fine. Changes done. Thanks for your time and review.
https://codereview.chromium.org/446003002/diff/350001/components/bookmarks/br... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/446003002/diff/350001/components/bookmarks/br... components/bookmarks/browser/bookmark_utils_unittest.cc:319: TEST_F(BookmarkUtilsTest, MakeTitleUniqueTest) { On 2014/10/14 19:22:33, sky wrote: > You don't need to duplicate Test. MakeTitleUnique is fine. Done.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/446003002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/446003002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/446003002/440001
Message was sent while issue was closed.
Committed patchset #19 (id:440001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/33164f770ae372ab34060bec75ef7a4bbfba1607 Cr-Commit-Position: refs/heads/master@{#299680}
Message was sent while issue was closed.
I need to do 2 changes to resolve build error's in build boat. 1) typecasting "i+1" to unsigned long. 2) As now copied node pasted under same have title with subscript (1) so done changes in test.js file. Thanks.
This patch already landed, right? On Wed, Oct 15, 2014 at 6:06 AM, <deepak.m1@samsung.com> wrote: > I need to do 2 changes to resolve build error's in build boat. > > 1) typecasting "i+1" to unsigned long. > 2) As now copied node pasted under same have title with subscript (1) so > done > changes in test.js file. > Thanks. > > https://codereview.chromium.org/446003002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Yes, Patch is landed. But As I should Inform you about build error changes so I updated in comments. Thanks |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
