Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19287013:
Bookmark Undo service for multiple level undo/redo of bookmarks. (Closed)
|
Created:
7 years, 5 months ago by Tom Cassiotis Modified:
5 years, 10 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionBookmark Undo service for multiple level undo/redo of bookmarks.
Added BookmarkUndoService that is owned by the profile to provide
multi-level undo and redo for bookmark changes.
This is a staged commit of a larger feature.
The code is not invoked anywhere other than the provided tests so
there should be no impact on the application at this time.
Further integration will continue in future commits.
BUG=126092
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221943
Patch Set 1 : #
Total comments: 20
Patch Set 2 : #
Total comments: 4
Patch Set 3 : Incorporates changes from other CLs. #
Total comments: 14
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #
Total comments: 5
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 1
Messages
Total messages: 27 (1 generated)
sky@ please take a look. Note: I had originally placed bookmark_undo_* files under c/b/bookmarks/ however there is a DEPS rule set in bookmarks for the move of the Bookmarks to components. I would imagine that that bookmark undo would remain under browser and so I moved it under c/b/undo/.
https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.cc:19: BookmarkNode* AsMutable(const BookmarkNode* node) { There should be no reason for this. If there is, it means something is wrong. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.cc:27: class BookmarkUndoOperation : public UndoOperation { The style guide indicates you should separate declaration/definition even for classes entirely in a .cc. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.cc:42: }; DISALLOW... and newline between 39/40 https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.cc:57: int64 parent_id_; Use const where you can in all these classes. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... File chrome/browser/undo/bookmark_undo_service.h (right): https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.h:29: class BookmarkIdMap { I think I like notifying existing BookmarkUndoOperations of id changes. They can then update indices immediately. That way there is no long lived map that is never cleared around growing unbounded. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.h:60: explicit BookmarkUndoService(BookmarkModel* model); Can you nuke this and use a TestingProfile? https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.h:63: UndoManager* GetUndoManager() { return &undo_manager_; } undo_manager() https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.h:66: friend class ::BookmarkUndoOperation; I'm not a fan of this. I would rather see BookmarkUndoOperation take pointers to the model and IdMap. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/undo_m... File chrome/browser/undo/undo_manager.cc (right): https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/undo_m... chrome/browser/undo/undo_manager.cc:54: void UndoManager::AddUndoOperation(scoped_ptr<UndoOperation> operation) { Can you add a max count to this class (you can create a separate patch for it). There is no reason for the undo stack to grow unbounded.
Please take a look. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.cc:19: BookmarkNode* AsMutable(const BookmarkNode* node) { On 2013/07/18 14:35:41, sky wrote: > There should be no reason for this. If there is, it means something is wrong. AsMutable is used in one place to reorder the children in BookmarkReorderOperation::Undo. The need for AsMutable is due to the BookmarkNodes being reinserted in TreeNode::SetChildren and require a non-const pointer (tree_node_model.h). We either have to remove the const-ness in either Undo/ReorderChildren/SetChildren or be able to retrieve non-const BookmarkNodes from BookmarkModel::GetNodeByID. BookmarkReorderOperation::Undo - const currently removed here BookmarkModel::ReorderChildren - the vector would need to be rebuilt TreeNode::SetChildren - TreeNode currently doesn't do any const-removing casts and should not be introduced here Changing or adding a version of GetNodeByID that returns a non-const BookmarkNode* would be my preference. However I am weary of making this sort of change as the BookmarkModel does not provide any public methods that return non-const BookmarkNode* and all public functions take const BookmarkNode*. The consistency makes me feel that it was no accident and rather by design. I have been curious about the approach as it seems odd to me. Functions obviously modifying the nodes take const pointers (eg BookmarkModel::SetTitle, SetURL). Is this done because in most cases const pointers will do and so AsMutable does not need to proliferate into other code? Whatever the reason it eliminates a nice benefit of const-ness which is to have the compiler inform you that the object that you want to be const may be modified by a function. In the end, I'm asking for guidance to resolve this point. :) https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.cc:27: class BookmarkUndoOperation : public UndoOperation { On 2013/07/18 14:35:41, sky wrote: > The style guide indicates you should separate declaration/definition even for > classes entirely in a .cc. Done. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.cc:42: }; On 2013/07/18 14:35:41, sky wrote: > DISALLOW... and newline between 39/40 The reason I did not include a the DISALLOW_ line is that the class should not be instantiated. With the change BookmarkUndoOperation contains a pure virtual function and so the class cannot be instantiated. Is a DISALLOW_ still needed? https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.cc:57: int64 parent_id_; On 2013/07/18 14:35:41, sky wrote: > Use const where you can in all these classes. I have not seen any parameters or functions that I could make const. Most of parameters are POD and there aren't that many functions. Fore example, Undo() could be const however it is part of the UndoObject interface and I am not confident that all implementations will be able to adhere to const-ness. GetForProfile requires a non-const Profile so I'm restricted in making the Profile const. Am I missing something? https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... File chrome/browser/undo/bookmark_undo_service.h (right): https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.h:29: class BookmarkIdMap { On 2013/07/18 14:35:41, sky wrote: > I think I like notifying existing BookmarkUndoOperations of id changes. They can > then update indices immediately. That way there is no long lived map that is > never cleared around growing unbounded. Since the UndoOperation and UndoService should know nothing about bookmarks and bookmark ids, this change took a bit of effort. To accomplish this I added UndoManager::ForEachUndoOperation that takes a functor and executes this on all UndoGroups and UndoOperations. It would have been cleaner if C++11 features but <functions> is not available to us at this time. The examples I saw of functors in the code did not try to use this for isolation, the closest would be the event dispatchers. I hope that the approach is acceptable. Done. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.h:60: explicit BookmarkUndoService(BookmarkModel* model); On 2013/07/18 14:35:41, sky wrote: > Can you nuke this and use a TestingProfile? TestingProfile simplified the code. However, I am getting warnings about temporary paths. I did find that other tests with that use the TestingProfile also report the same, so it is not my use of it but it may be specific to my machine. [656:9700:0724/091848:510020121:WARNING:file_util_win.cc(293)] Failed to get temporary file name in C:\Users\USERNAME\AppData\Local\Temp\scoped_dir656_2526: The directory name is invalid. Should I do more digging? Done. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.h:63: UndoManager* GetUndoManager() { return &undo_manager_; } On 2013/07/18 14:35:41, sky wrote: > undo_manager() Done. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.h:66: friend class ::BookmarkUndoOperation; On 2013/07/18 14:35:41, sky wrote: > I'm not a fan of this. I would rather see BookmarkUndoOperation take pointers to > the model and IdMap. The removal of the bookmark id map made this easier. Done. https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/undo_m... File chrome/browser/undo/undo_manager.cc (right): https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/undo_m... chrome/browser/undo/undo_manager.cc:54: void UndoManager::AddUndoOperation(scoped_ptr<UndoOperation> operation) { On 2013/07/18 14:35:41, sky wrote: > Can you add a max count to this class (you can create a separate patch for it). > There is no reason for the undo stack to grow unbounded. Good idea! I'll get a quick patch for this change after this CL.
https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... File chrome/browser/undo/bookmark_undo_service.h (right): https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.h:29: class BookmarkIdMap { On 2013/07/24 15:37:56, Tom Cassiotis wrote: > On 2013/07/18 14:35:41, sky wrote: > > I think I like notifying existing BookmarkUndoOperations of id changes. They > can > > then update indices immediately. That way there is no long lived map that is > > never cleared around growing unbounded. > > Since the UndoOperation and UndoService should know nothing about bookmarks and > bookmark ids, this change took a bit of effort. > > To accomplish this I added UndoManager::ForEachUndoOperation that takes a > functor and executes this on all UndoGroups and UndoOperations. It would have > been cleaner if C++11 features but <functions> is not available to us at this > time. > > The examples I saw of functors in the code did not try to use this for > isolation, the closest would be the event dispatchers. I hope that the approach > is acceptable. > > Done. Alternatively, I can add a new function in UndoOperation that takes a new base class 'UndoOperationModifier'. In this way I can change the functor to derive from UndoObjectModifier and pass that along. It would eliminate the need for a template function and thus removing the inline functions in UndoManager. I think this is a more understandable approach, what do you think?
https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/8001/chrome/browser/undo/bookma... chrome/browser/undo/bookmark_undo_service.cc:19: BookmarkNode* AsMutable(const BookmarkNode* node) { On 2013/07/24 15:37:56, Tom Cassiotis wrote: > On 2013/07/18 14:35:41, sky wrote: > > There should be no reason for this. If there is, it means something is wrong. > > AsMutable is used in one place to reorder the children in > BookmarkReorderOperation::Undo. > > The need for AsMutable is due to the BookmarkNodes being reinserted in > TreeNode::SetChildren and require a non-const pointer (tree_node_model.h). > > We either have to remove the const-ness in either > Undo/ReorderChildren/SetChildren or be able to retrieve non-const BookmarkNodes > from BookmarkModel::GetNodeByID. > > BookmarkReorderOperation::Undo - const currently removed here > > BookmarkModel::ReorderChildren - the vector would need to be rebuilt > > TreeNode::SetChildren - TreeNode currently doesn't do any const-removing casts > and should not be introduced here > > Changing or adding a version of GetNodeByID that returns a non-const > BookmarkNode* would be my preference. > > However I am weary of making this sort of change as the BookmarkModel does not > provide any public methods that return non-const BookmarkNode* and all public > functions take const BookmarkNode*. The consistency makes me feel that it was no > accident and rather by design. > > I have been curious about the approach as it seems odd to me. Functions > obviously modifying the nodes take const pointers (eg BookmarkModel::SetTitle, > SetURL). Is this done because in most cases const pointers will do and so > AsMutable does not need to proliferate into other code? > > Whatever the reason it eliminates a nice benefit of const-ness which is to have > the compiler inform you that the object that you want to be const may be > modified by a function. > > In the end, I'm asking for guidance to resolve this point. :) I think BookmarkModel::ReorderChildren should take a vector of const BookmarkNodes. The implementation can cast away the const'ness (as it does for all the other methods). Sound good?
Can you pull the undomanager chagnes into a separate cl with some tests? https://codereview.chromium.org/19287013/diff/29001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/29001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:25: class RenumberBookmarkIdFunctor { Please add a description. https://codereview.chromium.org/19287013/diff/29001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:44: virtual void RenumberBookmarkId(int64 old_id, int64 new_id) = 0; Description here too. In general you want descriptions for all non-obvious classes methods and members.
Scott please take a look. https://codereview.chromium.org/19287013/diff/29001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/29001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:25: class RenumberBookmarkIdFunctor { On 2013/07/26 15:35:42, sky wrote: > Please add a description. Code removed. https://codereview.chromium.org/19287013/diff/29001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:44: virtual void RenumberBookmarkId(int64 old_id, int64 new_id) = 0; On 2013/07/26 15:35:42, sky wrote: > Description here too. In general you want descriptions for all non-obvious > classes methods and members. Done.
https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:30: virtual void RenumberBookmarkId(int64 old_id, int64 new_id) = 0; Wouldn't BookmarkIdChanged be a better name for this? https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:114: void GetNewBookmarkIds(const BookmarkNodeData::Element& element, This doesn't actually Get anything. Maybe this should be named UpdateBookmarkIds https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:119: int old_index_; const? https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:141: bookmark_utils::CloneBookmarkNode(model, One thing I'm not sure about is meta info. Do we need to persist that too? https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:206: if (original_bookmark_.elements[0].is_url) Should this reset last modified too? https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:330: if (parent_id_ == old_id) Doesn't this need to check ordered_bookmarks_ ?
Scott welcome back :) https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:30: virtual void RenumberBookmarkId(int64 old_id, int64 new_id) = 0; On 2013/08/06 22:04:07, sky (OOO back on the 19th) wrote: > Wouldn't BookmarkIdChanged be a better name for this? I did use the 'verb' and 'event' form of the function purposely. BookmarkUndoOperation has smaller set of responsibilities and BookmarkUndoService knows about them so I thought the 'verb' form would be best. While in BookmarkUndoService I used OnBookmarkRenumbered to reflect that the class may have to do any number of things and it may be called from somewhere where it does not have much knowledge of the class. In the end, it was a "feels right" kind of decision. I do not have any concern changing it if you have a strong preference for the alternative naming. https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:114: void GetNewBookmarkIds(const BookmarkNodeData::Element& element, On 2013/08/06 22:04:07, sky (OOO back on the 19th) wrote: > This doesn't actually Get anything. Maybe this should be named UpdateBookmarkIds Done. https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:119: int old_index_; On 2013/08/06 22:04:07, sky (OOO back on the 19th) wrote: > const? Done. https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:141: bookmark_utils::CloneBookmarkNode(model, On 2013/08/06 22:04:07, sky (OOO back on the 19th) wrote: > One thing I'm not sure about is meta info. Do we need to persist that too? Currently the only use for the meta info is for versioning for sync. I cannot imagine a problem with syncing if we were to restore the version number, although that sync code is tricky and there may be some edge cases that I have not considered. On the other hand, not restoring the meta info works and is simpler. The undo of a delete is treated exactly like a new bookmark being created which is a basic sync operation. (Also, this is also how the current BMM "Undo Delete" behaves). The risk is if the meta info is used for other purposes which would require that it be persisted, but currently meta info is not saved to disk/synced (BookmarkChangeProcessor::CreateBookmarkNode) so if that change were to happen it would need to be dealt with at that time. https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:206: if (original_bookmark_.elements[0].is_url) On 2013/08/06 22:04:07, sky (OOO back on the 19th) wrote: > Should this reset last modified too? Did you mean date_folder_modified_? If that property is not restored there will be some subtle ordering when search results. This point brought to mind that date_added_ is also not restored. That is also not used extensively and the behavior of the current BMM undo delete does not restore the date_added_ property however I think that it should. So I think it would be best to restore both properties however I would like some advice on this. I am re-using BookmarkNodeData to store the bookmark information for delete and modification changes. This also lets me use bookmark_utils::CloneBookmarkNode to restore deleted bookmarks. I am unsure if it would be best to modify BookmarkNodeData and CloneBookmarkNode to take into account date_folder_modified_ and date_added_ or have the the bookmark undo system have it's own versions of this. The sync system also has a version of bookmark info that it uses to persist and transfer the data and if we were to have an undo structure that would make three ways to store bookmark data. https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:330: if (parent_id_ == old_id) On 2013/08/06 22:04:07, sky (OOO back on the 19th) wrote: > Doesn't this need to check ordered_bookmarks_ ? Done.
https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:206: if (original_bookmark_.elements[0].is_url) On 2013/08/19 13:47:00, Tom Cassiotis wrote: > On 2013/08/06 22:04:07, sky (OOO back on the 19th) wrote: > > Should this reset last modified too? > > Did you mean date_folder_modified_? > > If that property is not restored there will be some subtle ordering when search > results. > > This point brought to mind that date_added_ is also not restored. That is also > not used extensively and the behavior of the current BMM undo delete does not > restore the date_added_ property however I think that it should. > > So I think it would be best to restore both properties however I would like some > advice on this. I am re-using BookmarkNodeData to store the bookmark information > for delete and modification changes. This also lets me use > bookmark_utils::CloneBookmarkNode to restore deleted bookmarks. > > I am unsure if it would be best to modify BookmarkNodeData and CloneBookmarkNode > to take into account date_folder_modified_ and date_added_ or have the the > bookmark undo system have it's own versions of this. > > The sync system also has a version of bookmark info that it uses to persist and > transfer the data and if we were to have an undo structure that would make three > ways to store bookmark data. We don't want pasting to restore dates, only undo. Seems fine to make BookmarkNodeData track date_last_modified, but only the undo code would actually restore it.
sky@ this point was addressed in https://codereview.chromium.org/22855011/ https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/51002/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:206: if (original_bookmark_.elements[0].is_url) On 2013/08/19 15:38:00, sky wrote: > On 2013/08/19 13:47:00, Tom Cassiotis wrote: > > On 2013/08/06 22:04:07, sky (OOO back on the 19th) wrote: > > > Should this reset last modified too? > > > > Did you mean date_folder_modified_? > > > > If that property is not restored there will be some subtle ordering when > search > > results. > > > > This point brought to mind that date_added_ is also not restored. That is also > > not used extensively and the behavior of the current BMM undo delete does not > > restore the date_added_ property however I think that it should. > > > > So I think it would be best to restore both properties however I would like > some > > advice on this. I am re-using BookmarkNodeData to store the bookmark > information > > for delete and modification changes. This also lets me use > > bookmark_utils::CloneBookmarkNode to restore deleted bookmarks. > > > > I am unsure if it would be best to modify BookmarkNodeData and > CloneBookmarkNode > > to take into account date_folder_modified_ and date_added_ or have the the > > bookmark undo system have it's own versions of this. > > > > The sync system also has a version of bookmark info that it uses to persist > and > > transfer the data and if we were to have an undo structure that would make > three > > ways to store bookmark data. > > We don't want pasting to restore dates, only undo. Seems fine to make > BookmarkNodeData track date_last_modified, but only the undo code would actually > restore it. Done.
https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:205: model->SetURL(node, original_bookmark_.elements[0].url); nit: spacing is off. https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.h (right): https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.h:34: void OnBookmarkRenumbered(int64 old_id, int64 new_id); Can you make this private? Maybe move it into its own interface and pass said interface to each undo operation when created? https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service_test.cc (right): https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service_test.cc:8: #include "chrome/browser/undo/bookmark_undo_service.h" nit: this should be your first include (test files have same order as .cc files).
Back from some r and r. Scott PTAL. https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:205: model->SetURL(node, original_bookmark_.elements[0].url); On 2013/08/27 22:24:18, sky wrote: > nit: spacing is off. Done. https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.h (right): https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.h:34: void OnBookmarkRenumbered(int64 old_id, int64 new_id); On 2013/08/27 22:24:18, sky wrote: > Can you make this private? Maybe move it into its own interface and pass said > interface to each undo operation when created? I did this by defining BookmarkRenumberObserver. BookmarkUndoService and BookmarkUndoOperation are now renumber observers. BookmarkRemoveOperation notifies BookmarkUndoService when a bookmark is renumbers which in response notifies all BookmarkUndoOperation instances. Is this what you had in mind? https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service_test.cc (right): https://codereview.chromium.org/19287013/diff/62001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service_test.cc:8: #include "chrome/browser/undo/bookmark_undo_service.h" On 2013/08/27 22:24:18, sky wrote: > nit: this should be your first include (test files have same order as .cc > files). Done. https://codereview.chromium.org/19287013/diff/76001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/76001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:45: ) const { I spent a non-trivial amount of time to figure out how to format this line. Is this okay?
LGTM with the following changes https://codereview.chromium.org/19287013/diff/76001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_renumber_observer.h (right): https://codereview.chromium.org/19287013/diff/76001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_renumber_observer.h:10: // Should be called when a bookmark id has been renumbered so that any Should be called -> Called (or invoked if you prefer) https://codereview.chromium.org/19287013/diff/76001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/76001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:45: ) const { On 2013/09/04 13:44:43, Tom Cassiotis wrote: > I spent a non-trivial amount of time to figure out how to format this line. Is > this okay? HA! For the record the style guide is here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml . At one time the style guide don't let const dangle. It no longer says that though, so I think either of the following are fine: BookmarkRenumberObserver* BookmarkUndoOperation::GetUndoRenumberObserver() const { BookmarkRenumberObserver* BookmarkUndoOperation::GetUndoRenumberObserver() const { I would go for the latter though as the former looks bizarre.
https://codereview.chromium.org/19287013/diff/76001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_renumber_observer.h (right): https://codereview.chromium.org/19287013/diff/76001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_renumber_observer.h:10: // Should be called when a bookmark id has been renumbered so that any On 2013/09/04 21:52:18, sky wrote: > Should be called -> Called (or invoked if you prefer) Done. https://codereview.chromium.org/19287013/diff/76001/chrome/browser/undo/bookm... File chrome/browser/undo/bookmark_undo_service.cc (right): https://codereview.chromium.org/19287013/diff/76001/chrome/browser/undo/bookm... chrome/browser/undo/bookmark_undo_service.cc:45: ) const { On 2013/09/04 21:52:18, sky wrote: > On 2013/09/04 13:44:43, Tom Cassiotis wrote: > > I spent a non-trivial amount of time to figure out how to format this line. > Is > > this okay? > > HA! For the record the style guide is here: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml . At one time the > style guide don't let const dangle. It no longer says that though, so I think > either of the following are fine: > > BookmarkRenumberObserver* BookmarkUndoOperation::GetUndoRenumberObserver() > const { > > BookmarkRenumberObserver* > BookmarkUndoOperation::GetUndoRenumberObserver() const { > > I would go for the latter though as the former looks bizarre. I did look at the style guide to figure out the formatting for this line (and looked the Chromium specific pages just in case). In order to satisfy the following clause makes this tricky: - The return type is always on the same line as the function name. In any case, since you gave me an option I'll take the dangling const. :) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/19287013/89001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/19287013/101001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/19287013/133001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/19287013/137001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/19287013/83001
Message was sent while issue was closed.
Change committed as 221943
Message was sent while issue was closed.
tfarina@chromium.org changed reviewers: + tfarina@chromium.org
Message was sent while issue was closed.
Not sure why chromium-reviews was not copied on this CL. https://chromiumcodereview.appspot.com/19287013/diff/83001/chrome/browser/und... File chrome/browser/undo/bookmark_undo_service.cc (right): https://chromiumcodereview.appspot.com/19287013/diff/83001/chrome/browser/und... chrome/browser/undo/bookmark_undo_service.cc:188: original_bookmark_(node) { Why do you wrap |node| in BookmarkNodeData? If there is no good reason, I think we could just store it as a pointer to BookmarkNode, no? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
