Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
155560:
Don't use ID generation logic always. Only reassign IDs... (Closed)
|
Created:
11 years, 5 months ago by Munjal (Google) Modified:
9 years ago CC:
chromium-reviews_googlegroups.com, Ben Goodger (Google) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionDon't use ID generation logic always. Only reassign IDs
when checksums differ or if IDs are missing and do that
simply by assigning the next maximum id.
Add a unit test.
BUG=16357
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20779
Patch Set 1 #
Total comments: 28
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 4
Patch Set 4 : '' #
Total comments: 1
Messages
Total messages: 11 (0 generated)
http://codereview.chromium.org/155560/diff/1/3 File chrome/browser/bookmarks/bookmark_codec.cc (right): http://codereview.chromium.org/155560/diff/1/3#newcode264 Line 264: void BookmarkCodec::ReassignIDs(BookmarkNode* bb_node, As not having ids is really an error condition and nothing something we need to recover elegantly from, just reset all the ids and don't try and be smart.
http://codereview.chromium.org/155560/diff/1/3 File chrome/browser/bookmarks/bookmark_codec.cc (right): http://codereview.chromium.org/155560/diff/1/3#newcode76 Line 76: if (ids_missing_ || computed_checksum() != stored_checksum()) { If success is false, do we want to ever enter this code? http://codereview.chromium.org/155560/diff/1/3#newcode185 Line 185: int64 id = 0; If the id is missing, it will stay 0, which I think triggers a DCHECK below. http://codereview.chromium.org/155560/diff/1/3#newcode186 Line 186: if (value.GetString(kIdKey, &id_string)) { You can combine these two tests into one and get rid of the conditional altogether: ids_missing_ |= !value.GetString(...) || !StringToInt64(...); http://codereview.chromium.org/155560/diff/1/3#newcode193 Line 193: if (id > maximum_id_) likewise maximum_id_ = std::max(maximum_id_, id); http://codereview.chromium.org/155560/diff/1/3#newcode198 Line 198: title = std::wstring(); You don't need this assignment. GetString doesn't affect title if it fails. http://codereview.chromium.org/155560/diff/1/3#newcode241 Line 241: DCHECK(id != 0); here's the DCHECK http://codereview.chromium.org/155560/diff/1/3#newcode265 Line 265: BookmarkNode* other_node) { whitespace http://codereview.chromium.org/155560/diff/1/3#newcode266 Line 266: std::set<int64> assigned_ids; I think you should add 0 to the already assigned ids since it's the magic "root node". It appears that DecodeNode will now let nodes with an id of 0 through sometimes, so it's important that all of them get caught as dups. http://codereview.chromium.org/155560/diff/1/3#newcode279 Line 279: std::set<int64> *assigned_ids, * to the left http://codereview.chromium.org/155560/diff/1/3#newcode289 Line 289: if (node_id > maximum_id_) maximum_id_ == std::max(... http://codereview.chromium.org/155560/diff/1/3#newcode296 Line 296: CheckIDs(node->GetChild(i), assigned_ids, reassign_list); One worry about this kind of recursive algorithm is stack depth. Any thoughts on protecting ourselves against a super deep structure? These things could be constructed by code (via extensions), so it's not outlandish to think that this could happen (although in theory you'd hope this should only happen if someone modified the local disk and messed up the checksum). http://codereview.chromium.org/155560/diff/1/4 File chrome/browser/bookmarks/bookmark_codec.h (right): http://codereview.chromium.org/155560/diff/1/4#newcode145 Line 145: // Whehter or not IDs were missing for some bookmark nodes during decoding. typo: Whether http://codereview.chromium.org/155560/diff/1/2 File chrome/browser/bookmarks/bookmark_codec_unittest.cc (right): http://codereview.chromium.org/155560/diff/1/2#newcode219 Line 219: // The test depends on existence of multiple children under bookmark bar, so newline above comment
http://codereview.chromium.org/155560/diff/1/3 File chrome/browser/bookmarks/bookmark_codec.cc (right): http://codereview.chromium.org/155560/diff/1/3#newcode76 Line 76: if (ids_missing_ || computed_checksum() != stored_checksum()) { On 2009/07/15 16:13:32, Erik Kay wrote: > If success is false, do we want to ever enter this code? It seems like we don't check the return value of Decode in BookmarkStorage class. So I think it's better to do this regardless of success since if IDs are duplicate, we could crash somewhere. http://codereview.chromium.org/155560/diff/1/3#newcode185 Line 185: int64 id = 0; On 2009/07/15 16:13:32, Erik Kay wrote: > If the id is missing, it will stay 0, which I think triggers a DCHECK below. Good catch. Saved me investigation of some unit test failures :). http://codereview.chromium.org/155560/diff/1/3#newcode186 Line 186: if (value.GetString(kIdKey, &id_string)) { On 2009/07/15 16:13:32, Erik Kay wrote: > You can combine these two tests into one and get rid of the conditional > altogether: > ids_missing_ |= !value.GetString(...) || !StringToInt64(...); > I thought about doing that but then thought that short-circuiting makes it harder to understand that it actually works despite short-circuting. So I thought it's better to be more explicit. That's also why I haven't combined the two if conditions. http://codereview.chromium.org/155560/diff/1/3#newcode193 Line 193: if (id > maximum_id_) On 2009/07/15 16:13:32, Erik Kay wrote: > likewise maximum_id_ = std::max(maximum_id_, id); Done. http://codereview.chromium.org/155560/diff/1/3#newcode198 Line 198: title = std::wstring(); On 2009/07/15 16:13:32, Erik Kay wrote: > You don't need this assignment. GetString doesn't affect title if it fails. Done. http://codereview.chromium.org/155560/diff/1/3#newcode241 Line 241: DCHECK(id != 0); On 2009/07/15 16:13:32, Erik Kay wrote: > here's the DCHECK Done. http://codereview.chromium.org/155560/diff/1/3#newcode264 Line 264: void BookmarkCodec::ReassignIDs(BookmarkNode* bb_node, On 2009/07/15 15:47:34, sky wrote: > As not having ids is really an error condition and nothing something we need to > recover elegantly from, just reset all the ids and don't try and be smart. I thought about that. But I thought it's better to try to not change as many IDs as we can. Otherwise, everytime a user changes something in the file that's not even an ID we will end up reassigning all IDs. That doesn't sound very good to me. Unless we had different checksums for IDs and other content, it's probably better to not change IDs if we don't ahve to. For example in cases when user has changed a bunch of stuff in the file but not IDs. In short, persistent IDs lose their meaning in such cases. http://codereview.chromium.org/155560/diff/1/3#newcode265 Line 265: BookmarkNode* other_node) { On 2009/07/15 16:13:32, Erik Kay wrote: > whitespace Done. http://codereview.chromium.org/155560/diff/1/3#newcode266 Line 266: std::set<int64> assigned_ids; On 2009/07/15 16:13:32, Erik Kay wrote: > I think you should add 0 to the already assigned ids since it's the magic "root > node". It appears that DecodeNode will now let nodes with an id of 0 through > sometimes, so it's important that all of them get caught as dups. Done. Very good catch. http://codereview.chromium.org/155560/diff/1/3#newcode279 Line 279: std::set<int64> *assigned_ids, On 2009/07/15 16:13:32, Erik Kay wrote: > * to the left Done. Switching between two projects that use different conventions introduces such errors. http://codereview.chromium.org/155560/diff/1/3#newcode289 Line 289: if (node_id > maximum_id_) On 2009/07/15 16:13:32, Erik Kay wrote: > maximum_id_ == std::max(... Done. http://codereview.chromium.org/155560/diff/1/3#newcode296 Line 296: CheckIDs(node->GetChild(i), assigned_ids, reassign_list); On 2009/07/15 16:13:32, Erik Kay wrote: > One worry about this kind of recursive algorithm is stack depth. Any thoughts > on protecting ourselves against a super deep structure? These things could be > constructed by code (via extensions), so it's not outlandish to think that this > could happen (although in theory you'd hope this should only happen if someone > modified the local disk and messed up the checksum). Most of the bookmark code currently uses recursion. For example decoding uses recursion. There are so many places that we will have the same issue that it's something to consider independently of this CL because even if I use an explicit stack, we will get stack overflow in Decode for example. Does that sound okay? Does tha http://codereview.chromium.org/155560/diff/1/4 File chrome/browser/bookmarks/bookmark_codec.h (right): http://codereview.chromium.org/155560/diff/1/4#newcode145 Line 145: // Whehter or not IDs were missing for some bookmark nodes during decoding. On 2009/07/15 16:13:32, Erik Kay wrote: > typo: Whether Done. http://codereview.chromium.org/155560/diff/1/2 File chrome/browser/bookmarks/bookmark_codec_unittest.cc (right): http://codereview.chromium.org/155560/diff/1/2#newcode219 Line 219: // The test depends on existence of multiple children under bookmark bar, so On 2009/07/15 16:13:32, Erik Kay wrote: > newline above comment Done.
Again, I don't think many users are going to manually change their file. If the user manually changed their file you can't assume any of the ids map to what they were. In that case you should just completely punt and do the easy thing. -Scott On Wed, Jul 15, 2009 at 11:21 AM, <munjal@chromium.org> wrote: > > http://codereview.chromium.org/155560/diff/1/3 > File chrome/browser/bookmarks/bookmark_codec.cc (right): > > http://codereview.chromium.org/155560/diff/1/3#newcode76 > Line 76: if (ids_missing_ || computed_checksum() !=3D stored_checksum()) = { > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> If success is false, do we want to ever enter this code? > > It seems like we don't check the return value of Decode in > BookmarkStorage class. So I think it's better to do this regardless of > success since if IDs are duplicate, we could crash somewhere. > > http://codereview.chromium.org/155560/diff/1/3#newcode185 > Line 185: int64 id =3D 0; > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> If the id is missing, it will stay 0, which I think triggers a DCHECK > > below. > > Good catch. Saved me investigation of some unit test failures :). > > http://codereview.chromium.org/155560/diff/1/3#newcode186 > Line 186: if (value.GetString(kIdKey, &id_string)) { > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> You can combine these two tests into one and get rid of the > > conditional >> >> altogether: >> ids_missing_ |=3D !value.GetString(...) || !StringToInt64(...); > > > I thought about doing that but then thought that short-circuiting makes > it harder to understand that it actually works despite short-circuting. > So I thought it's better to be more explicit. That's also why I haven't > combined the two if conditions. > > http://codereview.chromium.org/155560/diff/1/3#newcode193 > Line 193: if (id > maximum_id_) > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> likewise maximum_id_ =3D std::max(maximum_id_, id); > > Done. > > http://codereview.chromium.org/155560/diff/1/3#newcode198 > Line 198: title =3D std::wstring(); > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> You don't need this assignment. =A0GetString doesn't affect title if it > > fails. > > Done. > > http://codereview.chromium.org/155560/diff/1/3#newcode241 > Line 241: DCHECK(id !=3D 0); > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> here's the DCHECK > > Done. > > http://codereview.chromium.org/155560/diff/1/3#newcode264 > Line 264: void BookmarkCodec::ReassignIDs(BookmarkNode* bb_node, > On 2009/07/15 15:47:34, sky wrote: >> >> As not having ids is really an error condition and nothing something > > we need to >> >> recover elegantly from, just reset all the ids and don't try and be > > smart. > > I thought about that. But I thought it's better to try to not change as > many IDs as we can. Otherwise, everytime a user changes something in the > file that's not even an ID we will end up reassigning all IDs. That > doesn't sound very good to me. Unless we had different checksums for IDs > and other content, it's probably better to not change IDs if we don't > ahve to. For example in cases when user has changed a bunch of stuff in > the file but not IDs. > > In short, persistent IDs lose their meaning in such cases. > > http://codereview.chromium.org/155560/diff/1/3#newcode265 > Line 265: BookmarkNode* other_node) { > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> whitespace > > Done. > > http://codereview.chromium.org/155560/diff/1/3#newcode266 > Line 266: std::set<int64> assigned_ids; > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> I think you should add 0 to the already assigned ids since it's the > > magic "root >> >> node". =A0It appears that DecodeNode will now let nodes with an id of 0 > > through >> >> sometimes, so it's important that all of them get caught as dups. > > Done. Very good catch. > > http://codereview.chromium.org/155560/diff/1/3#newcode279 > Line 279: std::set<int64> *assigned_ids, > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> * to the left > > Done. Switching between two projects that use different conventions > introduces such errors. > > http://codereview.chromium.org/155560/diff/1/3#newcode289 > Line 289: if (node_id > maximum_id_) > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> maximum_id_ =3D=3D std::max(... > > Done. > > http://codereview.chromium.org/155560/diff/1/3#newcode296 > Line 296: CheckIDs(node->GetChild(i), assigned_ids, reassign_list); > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> One worry about this kind of recursive algorithm is stack depth. =A0Any > > thoughts >> >> on protecting ourselves against a super deep structure? =A0These things > > could be >> >> constructed by code (via extensions), so it's not outlandish to think > > that this >> >> could happen (although in theory you'd hope this should only happen if > > someone >> >> modified the local disk and messed up the checksum). > > Most of the bookmark code currently uses recursion. For example decoding > uses recursion. There are so many places that we will have the same > issue that it's something to consider independently of this CL because > even if I use an explicit stack, we will get stack overflow in Decode > for example. > > Does that sound okay? > > Does tha > > http://codereview.chromium.org/155560/diff/1/4 > File chrome/browser/bookmarks/bookmark_codec.h (right): > > http://codereview.chromium.org/155560/diff/1/4#newcode145 > Line 145: // Whehter or not IDs were missing for some bookmark nodes > during decoding. > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> typo: Whether > > Done. > > http://codereview.chromium.org/155560/diff/1/2 > File chrome/browser/bookmarks/bookmark_codec_unittest.cc (right): > > http://codereview.chromium.org/155560/diff/1/2#newcode219 > Line 219: // The test depends on existence of multiple children under > bookmark bar, so > On 2009/07/15 16:13:32, Erik Kay wrote: >> >> newline above comment > > Done. > > http://codereview.chromium.org/155560 >
Done. Can you take a quick look again? On Wed, Jul 15, 2009 at 11:34 AM, Scott Violet <sky@chromium.org> wrote: > Again, I don't think many users are going to manually change their > file. If the user manually changed their file you can't assume any of > the ids map to what they were. In that case you should just completely > punt and do the easy thing. > > -Scott > > On Wed, Jul 15, 2009 at 11:21 AM, <munjal@chromium.org> wrote: > > > > http://codereview.chromium.org/155560/diff/1/3 > > File chrome/browser/bookmarks/bookmark_codec.cc (right): > > > > http://codereview.chromium.org/155560/diff/1/3#newcode76 > > Line 76: if (ids_missing_ || computed_checksum() != stored_checksum()) { > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> If success is false, do we want to ever enter this code? > > > > It seems like we don't check the return value of Decode in > > BookmarkStorage class. So I think it's better to do this regardless of > > success since if IDs are duplicate, we could crash somewhere. > > > > http://codereview.chromium.org/155560/diff/1/3#newcode185 > > Line 185: int64 id = 0; > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> If the id is missing, it will stay 0, which I think triggers a DCHECK > > > > below. > > > > Good catch. Saved me investigation of some unit test failures :). > > > > http://codereview.chromium.org/155560/diff/1/3#newcode186 > > Line 186: if (value.GetString(kIdKey, &id_string)) { > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> You can combine these two tests into one and get rid of the > > > > conditional > >> > >> altogether: > >> ids_missing_ |= !value.GetString(...) || !StringToInt64(...); > > > > > > I thought about doing that but then thought that short-circuiting makes > > it harder to understand that it actually works despite short-circuting. > > So I thought it's better to be more explicit. That's also why I haven't > > combined the two if conditions. > > > > http://codereview.chromium.org/155560/diff/1/3#newcode193 > > Line 193: if (id > maximum_id_) > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> likewise maximum_id_ = std::max(maximum_id_, id); > > > > Done. > > > > http://codereview.chromium.org/155560/diff/1/3#newcode198 > > Line 198: title = std::wstring(); > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> You don't need this assignment. GetString doesn't affect title if it > > > > fails. > > > > Done. > > > > http://codereview.chromium.org/155560/diff/1/3#newcode241 > > Line 241: DCHECK(id != 0); > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> here's the DCHECK > > > > Done. > > > > http://codereview.chromium.org/155560/diff/1/3#newcode264 > > Line 264: void BookmarkCodec::ReassignIDs(BookmarkNode* bb_node, > > On 2009/07/15 15:47:34, sky wrote: > >> > >> As not having ids is really an error condition and nothing something > > > > we need to > >> > >> recover elegantly from, just reset all the ids and don't try and be > > > > smart. > > > > I thought about that. But I thought it's better to try to not change as > > many IDs as we can. Otherwise, everytime a user changes something in the > > file that's not even an ID we will end up reassigning all IDs. That > > doesn't sound very good to me. Unless we had different checksums for IDs > > and other content, it's probably better to not change IDs if we don't > > ahve to. For example in cases when user has changed a bunch of stuff in > > the file but not IDs. > > > > In short, persistent IDs lose their meaning in such cases. > > > > http://codereview.chromium.org/155560/diff/1/3#newcode265 > > Line 265: BookmarkNode* other_node) { > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> whitespace > > > > Done. > > > > http://codereview.chromium.org/155560/diff/1/3#newcode266 > > Line 266: std::set<int64> assigned_ids; > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> I think you should add 0 to the already assigned ids since it's the > > > > magic "root > >> > >> node". It appears that DecodeNode will now let nodes with an id of 0 > > > > through > >> > >> sometimes, so it's important that all of them get caught as dups. > > > > Done. Very good catch. > > > > http://codereview.chromium.org/155560/diff/1/3#newcode279 > > Line 279: std::set<int64> *assigned_ids, > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> * to the left > > > > Done. Switching between two projects that use different conventions > > introduces such errors. > > > > http://codereview.chromium.org/155560/diff/1/3#newcode289 > > Line 289: if (node_id > maximum_id_) > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> maximum_id_ == std::max(... > > > > Done. > > > > http://codereview.chromium.org/155560/diff/1/3#newcode296 > > Line 296: CheckIDs(node->GetChild(i), assigned_ids, reassign_list); > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> One worry about this kind of recursive algorithm is stack depth. Any > > > > thoughts > >> > >> on protecting ourselves against a super deep structure? These things > > > > could be > >> > >> constructed by code (via extensions), so it's not outlandish to think > > > > that this > >> > >> could happen (although in theory you'd hope this should only happen if > > > > someone > >> > >> modified the local disk and messed up the checksum). > > > > Most of the bookmark code currently uses recursion. For example decoding > > uses recursion. There are so many places that we will have the same > > issue that it's something to consider independently of this CL because > > even if I use an explicit stack, we will get stack overflow in Decode > > for example. > > > > Does that sound okay? > > > > Does tha > > > > http://codereview.chromium.org/155560/diff/1/4 > > File chrome/browser/bookmarks/bookmark_codec.h (right): > > > > http://codereview.chromium.org/155560/diff/1/4#newcode145 > > Line 145: // Whehter or not IDs were missing for some bookmark nodes > > during decoding. > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> typo: Whether > > > > Done. > > > > http://codereview.chromium.org/155560/diff/1/2 > > File chrome/browser/bookmarks/bookmark_codec_unittest.cc (right): > > > > http://codereview.chromium.org/155560/diff/1/2#newcode219 > > Line 219: // The test depends on existence of multiple children under > > bookmark bar, so > > On 2009/07/15 16:13:32, Erik Kay wrote: > >> > >> newline above comment > > > > Done. > > > > http://codereview.chromium.org/155560 > > >
LGTM with the following changes. http://codereview.chromium.org/155560/diff/20/22 File chrome/browser/bookmarks/bookmark_codec.cc (right): http://codereview.chromium.org/155560/diff/20/22#newcode185 Line 185: if (value.GetString(kIdKey, &id_string)) { can we make this a one liner: if (!value.GetString(kIdKey, &id_string)) || !StringToInt64(id_string, &id)) { ids_missing_ = true; } http://codereview.chromium.org/155560/diff/20/22#newcode271 Line 271: for (int i = 0; i < node->GetChildCount(); ++i) { nit: no {}
http://codereview.chromium.org/155560/diff/20/22 File chrome/browser/bookmarks/bookmark_codec.cc (right): http://codereview.chromium.org/155560/diff/20/22#newcode185 Line 185: if (value.GetString(kIdKey, &id_string)) { On 2009/07/15 19:27:38, sky wrote: > can we make this a one liner: > > if (!value.GetString(kIdKey, &id_string)) || > !StringToInt64(id_string, &id)) { > ids_missing_ = true; > } Done. I didn't do that intially for the same reason I told Erik about that it's harder to understand that it works even with short-circuiting. But it doesn't seem that tough to understand. http://codereview.chromium.org/155560/diff/20/22#newcode271 Line 271: for (int i = 0; i < node->GetChildCount(); ++i) { On 2009/07/15 19:27:38, sky wrote: > nit: no {} Done.
LGTM
depending on Scott's reply here, LGTM http://codereview.chromium.org/155560/diff/27/1008 File chrome/browser/bookmarks/bookmark_codec.cc (right): http://codereview.chromium.org/155560/diff/27/1008#newcode259 Line 259: ReassignIDsHelper(bb_node); Are there (or should there be) any assumptions about the ids of bb_node and other_node? At initial creation time, these will have an id of 1 and 2 respectively, but in this new code that isn't true. Scott, do you think this is an issue?
On Wed, Jul 15, 2009 at 12:44 PM, <erikkay@chromium.org> wrote: > depending on Scott's reply here, LGTM > > > > http://codereview.chromium.org/155560/diff/27/1008 > File chrome/browser/bookmarks/bookmark_codec.cc (right): > > http://codereview.chromium.org/155560/diff/27/1008#newcode259 > Line 259: ReassignIDsHelper(bb_node); > Are there (or should there be) any assumptions about the ids of bb_node > and other_node? =A0At initial creation time, these will have an id of 1 > and 2 respectively, but in this new code that isn't true. =A0Scott, do yo= u > think this is an issue? That assumption was removed a long time ago. Changing the id of the bookmark/other node shouldn't cause problems. -Scott |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
