Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26894002:
Experimental bookmark model based on tags. (Closed)
|
Created:
7 years, 2 months ago by noyau (Ping after 24h) Modified:
7 years, 2 months ago CC:
chromium-reviews, tfarina, browser-components-watch_chromium.org, Mike Wittman Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionExperimental bookmark model based on tags.
This CL introduces a BookmarTagModel class. BookmarTagModel provides
a way to access and manipulate bookmarks in a non-hierarchical way.
BookmarkTagModel view the bookmarks as a flat list, and each one can be
marked with a collection of tags (tags are simply strings).
BookmarkTagModel converts on demand the data from an existing
BookmarkModel to its view of the world by considering all the titles of all
the ancestors as tags. This view is frozen on an individual bookmarks when
the BookmarkTagModel performs a change on the tags of this bookmarks.
The Bookmark's meta info is used for storage.
An observer may be attached to a BookmarkTagModel to observe relevant
events.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229104
Patch Set 1 #
Total comments: 16
Patch Set 2 : Fix memory leak and Android build failures. #Patch Set 3 : Remove pragmas: Win compiler doesn't like them. #Patch Set 4 : s/tab/tag #
Total comments: 58
Patch Set 5 : Feedback. #Patch Set 6 : Adding TODOs. #Patch Set 7 : Fixing trybot failures. #
Total comments: 10
Patch Set 8 : More fixes. #
Messages
Total messages: 27 (0 generated)
Probably should add sky (bookmarks) and rlarocque (bookmarks sync) for comments on approach https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... File chrome/browser/bookmarks/bookmark_tag_model.cc (right): https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.cc:55: if (bookmark_model_) Can this ever fail? Seems like the c-tor guarantees you have one. https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.cc:70: DCHECK(bookmark_model_); These DCHECKS all seem unnecessary for reasons above https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.cc:141: const BookmarkNode* bookmark = bookmark_model_->AddURL( I'm pretty sure the underlying BookmarkModel will still fire notifications here. https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... File chrome/browser/bookmarks/bookmark_tag_model.h (right): https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.h:14: // BookmarTabModel provides a way to access an manipulate bookmarks in a non "BookmarkTagModel" .. "and" https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.h:33: // LAST_USED_TIME_BOOKMARK_ORDERING // TODO(noyau): Maybe provide last used Seems fine to omit for now. We'll add if needed https://codereview.chromium.org/26894002/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/26894002/diff/1/chrome/chrome_browser.gypi#ne... chrome/chrome_browser.gypi:295: 'browser/bookmarks/bookmark_tag_model.cc', Condition this on the |enabled_enhanced_bookmarks| gyp variable to avoid overhead. Actually, I see that it's never constructed. What's your plan there? Will the existing BookmarkModel factory conditionally construct this? Does it wrap the BookmarkModel and cause it to be loaded?
General bot fixing, plus Yaron feedback. https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... File chrome/browser/bookmarks/bookmark_tag_model.cc (right): https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.cc:55: if (bookmark_model_) On 2013/10/11 10:50:41, Yaron wrote: > Can this ever fail? Seems like the c-tor guarantees you have one. The bookmark model notifies its observer when it is going away, and I do clear the bookmark_model_ instance variable then. See BookmarkModelBeingDeleted() further down. https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.cc:70: DCHECK(bookmark_model_); On 2013/10/11 10:50:41, Yaron wrote: > These DCHECKS all seem unnecessary for reasons above These DCHECK are necessary for reasons above :) https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.cc:141: const BookmarkNode* bookmark = bookmark_model_->AddURL( On 2013/10/11 10:50:41, Yaron wrote: > I'm pretty sure the underlying BookmarkModel will still fire notifications here. Yes, it will, and should. This class relies on the notifications to update its internal caches. This is just inhibiting the BookmarkTagModel notifications to avoid sending two notifications, one that the node was created and second that the tags where changed. https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.cc:208: bookmarks.insert(subset.begin(), subset.end()); GCC on Android fail to compile this insert() with a very unclear error message. https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.cc:316: base::Value* result = serializer.Deserialize(&error_code, &error_message); Memory leak fixed (Thanks memory bot!) https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... File chrome/browser/bookmarks/bookmark_tag_model.h (right): https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.h:14: // BookmarTabModel provides a way to access an manipulate bookmarks in a non On 2013/10/11 10:50:41, Yaron wrote: > "BookmarkTagModel" .. "and" Done. https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.h:33: // LAST_USED_TIME_BOOKMARK_ORDERING // TODO(noyau): Maybe provide last used On 2013/10/11 10:50:41, Yaron wrote: > Seems fine to omit for now. We'll add if needed Done. https://codereview.chromium.org/26894002/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/26894002/diff/1/chrome/chrome_browser.gypi#ne... chrome/chrome_browser.gypi:295: 'browser/bookmarks/bookmark_tag_model.cc', On 2013/10/11 10:50:41, Yaron wrote: > Condition this on the |enabled_enhanced_bookmarks| gyp variable to avoid > overhead. Well, it's compiled but removed at link time as nobody uses it yet. Conditioning it on the |enabled_enhanced_bookmarks| will mean that this could be easily broken by a change to BookmarkManager, and that the unit tests will not be run by the bots. > >Actually, I see that it's never constructed. What's your plan there? > Will the existing BookmarkModel factory conditionally construct this? Does it > wrap the BookmarkModel and cause it to be loaded? I was thinking that for now the UI will simply create one of those as needed, passing the BookmarkModel from the factory. We could always make a new factory later to manage a singleton.
On Fri, Oct 11, 2013 at 9:09 AM, <noyau@chromium.org> wrote: > Reviewers: Yaron, > > Message: > General bot fixing, plus Yaron feedback. > > > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > File chrome/browser/bookmarks/bookmark_tag_model.cc (right): > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > chrome/browser/bookmarks/bookmark_tag_model.cc:55: if (bookmark_model_) > On 2013/10/11 10:50:41, Yaron wrote: >> >> Can this ever fail? Seems like the c-tor guarantees you have one. > > The bookmark model notifies its observer when it is going away, and I do > clear the bookmark_model_ instance variable then. See > BookmarkModelBeingDeleted() further down. > > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > chrome/browser/bookmarks/bookmark_tag_model.cc:70: > DCHECK(bookmark_model_); > On 2013/10/11 10:50:41, Yaron wrote: >> >> These DCHECKS all seem unnecessary for reasons above > > These DCHECK are necessary for reasons above :) > > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > chrome/browser/bookmarks/bookmark_tag_model.cc:141: const BookmarkNode* > bookmark = bookmark_model_->AddURL( > On 2013/10/11 10:50:41, Yaron wrote: >> >> I'm pretty sure the underlying BookmarkModel will still fire > > notifications here. > Yes, it will, and should. This class relies on the notifications to > update its internal caches. This is just inhibiting the BookmarkTagModel > notifications to avoid sending two notifications, one that the node was > created and second that the tags where changed. > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > chrome/browser/bookmarks/bookmark_tag_model.cc:208: > bookmarks.insert(subset.begin(), subset.end()); > GCC on Android fail to compile this insert() with a very unclear error > message. > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > chrome/browser/bookmarks/bookmark_tag_model.cc:316: base::Value* result > = serializer.Deserialize(&error_code, &error_message); > Memory leak fixed (Thanks memory bot!) > > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > File chrome/browser/bookmarks/bookmark_tag_model.h (right): > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > chrome/browser/bookmarks/bookmark_tag_model.h:14: // BookmarTabModel > provides a way to access an manipulate bookmarks in a non > On 2013/10/11 10:50:41, Yaron wrote: >> >> "BookmarkTagModel" .. "and" > > > Done. > > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > chrome/browser/bookmarks/bookmark_tag_model.h:33: // > LAST_USED_TIME_BOOKMARK_ORDERING // TODO(noyau): Maybe provide last used > On 2013/10/11 10:50:41, Yaron wrote: >> >> Seems fine to omit for now. We'll add if needed > > > Done. > > > https://codereview.chromium.org/26894002/diff/1/chrome/chrome_browser.gypi > File chrome/chrome_browser.gypi (right): > > https://codereview.chromium.org/26894002/diff/1/chrome/chrome_browser.gypi#ne... > chrome/chrome_browser.gypi:295: > 'browser/bookmarks/bookmark_tag_model.cc', > On 2013/10/11 10:50:41, Yaron wrote: >> >> Condition this on the |enabled_enhanced_bookmarks| gyp variable to > > avoid >> >> overhead. > > > Well, it's compiled but removed at link time as nobody uses it yet. > Conditioning it on the |enabled_enhanced_bookmarks| will mean that this > could be easily broken by a change to BookmarkManager, and that the unit > tests will not be run by the bots. > > > >> Actually, I see that it's never constructed. What's your plan there? >> Will the existing BookmarkModel factory conditionally construct this? > > Does it >> >> wrap the BookmarkModel and cause it to be loaded? > > > I was thinking that for now the UI will simply create one of those as > needed, passing the BookmarkModel from the factory. We could always make > a new factory later to manage a singleton. > > Description: > Experimental bookmark model based on tags. > > This CL introduces a BookmarTabModel class. This class provides a way to > access and manipulate bookmarks in a non hierarchical way. It converts on > demand the data from an existing BookmarkModel instance to its warped > view of the world. > > The underline BookmarkModel is used for storage of the tag data. > > BookmarkTabModel presents the bookmarks as a flat list, and each one can > be marked with tags. The API allow to retrieve all the bookmarks for a > given set of tags and all the tags associated to a specific bookmark. > > In a fashion similar to the BookmarkModel an observer may be attached to a > BookmarkTabModel to observe relevant events. > You wrote TabModel all over the CL description when you meant Tag* So this is experimental, but definitive? You guys are going to implement a tag system to support google.com/bookmarks? Is that the idea? Could you have a bug filed for this? > BUG=None > > Please review this at https://codereview.chromium.org/26894002/ > > SVN Base: http://git.chromium.org/chromium/src.git@master > > Affected files (+1459, -0 lines): > M chrome/browser/bookmarks/bookmark_model.cc > M chrome/browser/bookmarks/bookmark_model_observer.h > A chrome/browser/bookmarks/bookmark_tag_model.h > A chrome/browser/bookmarks/bookmark_tag_model.cc > A chrome/browser/bookmarks/bookmark_tag_model_observer.h > A chrome/browser/bookmarks/bookmark_tag_model_unittest.cc > M chrome/chrome_browser.gypi > M chrome/chrome_tests_unit.gypi > > -- Thiago To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/11 12:48:34, tfarina wrote: > You wrote TabModel all over the CL description when you meant Tag* > Ooops, thanks. And I did that in the code as well. I guess I wrote too much tab code... > So this is experimental, but definitive? You guys are going to > implement a tag system to support google.com/bookmarks? Is that the > idea? Could you have a bug filed for this? > It's completely experimental. We're doing UI research trying to figure out what's possible.
+sky, +rlarocque Sky for the general approach, rlarocque for the viability of syncing the bookmark metainfo.
One question before I review. Is it possible to do all this in an extension so that we don't need to muck with internals yet?
On Fri, Oct 11, 2013 at 1:28 PM, <sky@chromium.org> wrote: > One question before I review. Is it possible to do all this in an extension > so > that we don't need to muck with internals yet? > I'd be happy (very) if that would be possible. Thanks! > https://codereview.chromium.org/26894002/ -- Thiago To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/11 16:28:17, sky wrote: > One question before I review. Is it possible to do all this in an extension so > that we don't need to muck with internals yet? Unfortunately no, we need to share this between mobile platforms. No extensions there.
From the sync point of view, your model looks pretty good. I like the idea of storing the set of tags inside the bookmark. It might allow us to do a graceful migration from one model to the other without significant data loss or needing to modify data on the server. I was a bit worried about the case where two identical bookmarks live in the same folder, but it looks like that's handled correctly. You might want to add a test for that case anyway. I'd recommend also adding some tests for bookmarks and folders with the name "", though it looks like you've handled those cases correctly, too. https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... File chrome/browser/bookmarks/bookmark_tag_model.cc (right): https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.cc:288: #pragma mark Private methods. What does this do?
Can you only build this on the platforms we care about now? https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_model.cc (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_model.cc:499: FOR_EACH_OBSERVER(BookmarkModelObserver, observers_, Two places that I don't believe track meta data yet: undo and copy/paste (drag and drop use the same code). Since this is experimental, TODOs are fine for now. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_model.cc:500: OnWillChangeBookmarkMetaInfo(this, node)); Can you only notify if something actually changes? To make this easy BookmarkModel could get the metadata as a string, create a new string with the contents (effectively what SetMetaInfo() does now). If the two are the same early out, otherwise notify observers, apply the change (simple string swap), notify again. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_model_observer.h (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_model_observer.h:60: // Invoked before the metainfo of a node is changed. Clarify that nothing may have changed here and below. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_tag_model.cc (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:36: bool operator()(BookmarkTag a, BookmarkTag b) { const BookmarkTag& ? https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:139: inhibit_change_notifications_ = true; Use Autoreset. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:159: const std::set<BookmarkTag>& tags, const BookmarkNode* bookmark) { nit: wrap https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:207: const std::set<const BookmarkNode*> subset(tag_to_bookmarks_[*it]); const std::<...>& subset https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:208: for (std::set<const BookmarkNode*>::const_iterator tag_it = subset.begin(); bookmarks.insert(subset.begin(), subset.end()) https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:238: const BookmarkTag& tag, BookmarkOrdering ordering) { nit: one line per param, and indent 2 more. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:262: std::set<BookmarkTag> subset(bookmark_to_tags_[*it]); const std::set...& https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:301: std::string encoded; Could you extract this into a function and put in namespace above? That way easier to see it doesn't modify 'this' in anyway. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:359: bookmark_model_->SetNodeMetaInfo(bookmark, TAG_KEY, encoded); Does encoded result in an empty string if tags is empty? If not, could you make it. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:359: bookmark_model_->SetNodeMetaInfo(bookmark, TAG_KEY, encoded); Can you also add a comment that internal mappings are updated on notification that meta data changed. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:406: void BookmarkTagModel::Loaded(BookmarkModel* model, bool ids_reassigned) { Make order match header. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:408: }; nit: no ; https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:416: observers_.Clear(); Why do you clear the observers here? https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:492: // Invoked when the title or url of a node changes. Might this effect the tags of all desendants if node is a folder? https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:512: const BookmarkNode* node) { nit: alignment here (and many other places below). https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_tag_model.h (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:4: #ifndef CHROME_BROWSER_BOOKMARKS_BOOKMARK_TAG_MODEL_H_ nit: newline between 3/4. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:14: // BookmarTagModel provides a way to access and manipulate bookmarks in a non nit: non-hierarchical https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:16: // BookmarkModel to its warped view of the world. It also uses the BookmarkModel HAH! Please document better what 'warped view of the world' means. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:28: enum BookmarkOrdering { Name enums as document in component: "enum values should start with the name of the type, i.e. PAGE_TRANSITION_LINK in the content::PageTransition enum" https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:33: }; Is there a reason you baked sorting into these functions too? Seems like it would be nice to return a set<BookmarkNode*> and have sorting done externally. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:55: void BeginExtensiveChanges(); Can you hide these behind a scoper object that is a friend? https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:111: std::set<BookmarkTag> AllTagsForBookmark(const BookmarkNode* bookmark); GetTagsForBookmark? https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:116: const std::set<BookmarkTag>& tags, BookmarkOrdering ordering); nit: when you wrap, one line per param. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:177: void ReplaceTagsOnBookmark(const std::set<BookmarkTag>& tags, nit: SetTagsOnBookmark ? https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:207: }; DISALLOW... https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_tag_model_unittest.cc (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model_unittest.cc:40: enum ObserverCounts { Same nit about naming. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model_unittest.cc:74: nit: no newline here. https://codereview.chromium.org/26894002/diff/64001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/26894002/diff/64001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:295: 'browser/bookmarks/bookmark_tag_model.cc', nit: sort.
https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_tag_model.h (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: No (c). Here and in every new file (also check the year). https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_tag_model_unittest.cc (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model_unittest.cc:144: }; nit: Sometimes there is a semi colon, sometimes not.
Thanks for the reviews. PTAL. I've used my flying time to add the requested tests and fix all the feedback so far on BookmarkTagModel. I still have a couple of fixes to do on Bookmark model: - Adding TODOs for copy/paste not preserving the MetaInfo - Preventing the metaInfoChanged notification to be send if there are no changes. I'll do that tomorrow, as I'm too jet-lagged now to continue to code :) As for limiting this to "the platform we care about" we would only be removing Android, and only for a short time, I'm not sure it's worth it. https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... File chrome/browser/bookmarks/bookmark_tag_model.cc (right): https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... chrome/browser/bookmarks/bookmark_tag_model.cc:288: #pragma mark Private methods. On 2013/10/11 18:06:25, rlarocque wrote: > What does this do? Mac tools read those and use the text as a separator in the code outline. It's just a hint to the IDE. But the windows compiler generate a warning on unknown pragma, and the build treats warnings as errors, so I removed them all. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_model_observer.h (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_model_observer.h:60: // Invoked before the metainfo of a node is changed. On 2013/10/11 21:57:41, sky wrote: > Clarify that nothing may have changed here and below. I'll be changing the notification so it is only send when something actually changes. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_tag_model.cc (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:36: bool operator()(BookmarkTag a, BookmarkTag b) { On 2013/10/11 21:57:41, sky wrote: > const BookmarkTag& ? Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:139: inhibit_change_notifications_ = true; On 2013/10/11 21:57:41, sky wrote: > Use Autoreset. Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:159: const std::set<BookmarkTag>& tags, const BookmarkNode* bookmark) { On 2013/10/11 21:57:41, sky wrote: > nit: wrap Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:207: const std::set<const BookmarkNode*> subset(tag_to_bookmarks_[*it]); On 2013/10/11 21:57:41, sky wrote: > const std::<...>& subset Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:208: for (std::set<const BookmarkNode*>::const_iterator tag_it = subset.begin(); On 2013/10/11 21:57:41, sky wrote: > bookmarks.insert(subset.begin(), subset.end()) I've tried this code (see patch 1) but it failed to compile with GCC on android, hence the fallback to a loop. Anyway, this code was incorrect as it was doing an union of all the tags, where what I intended was an intersection. (sorry, the diff is hard to follow as I reordered the code to match the header). https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:238: const BookmarkTag& tag, BookmarkOrdering ordering) { On 2013/10/11 21:57:41, sky wrote: > nit: one line per param, and indent 2 more. Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:262: std::set<BookmarkTag> subset(bookmark_to_tags_[*it]); On 2013/10/11 21:57:41, sky wrote: > const std::set...& Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:301: std::string encoded; On 2013/10/11 21:57:41, sky wrote: > Could you extract this into a function and put in namespace above? That way > easier to see it doesn't modify 'this' in anyway. Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:359: bookmark_model_->SetNodeMetaInfo(bookmark, TAG_KEY, encoded); On 2013/10/11 21:57:41, sky wrote: > Can you also add a comment that internal mappings are updated on notification > that meta data changed. Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.cc:359: bookmark_model_->SetNodeMetaInfo(bookmark, TAG_KEY, encoded); On 2013/10/11 21:57:41, sky wrote: > Does encoded result in an empty string if tags is empty? If not, could you > make it. Actually no, if this is turned into an empty string the bookmark will revert to the default value, aka the title of all the parent bookmarks. Added a comment to explain this behaviour. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_tag_model.h (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/10/14 09:18:17, lpromero-g wrote: > nit: No (c). Here and in every new file (also check the year). Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:4: #ifndef CHROME_BROWSER_BOOKMARKS_BOOKMARK_TAG_MODEL_H_ On 2013/10/11 21:57:41, sky wrote: > nit: newline between 3/4. Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:14: // BookmarTagModel provides a way to access and manipulate bookmarks in a non On 2013/10/11 21:57:41, sky wrote: > nit: non-hierarchical Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:16: // BookmarkModel to its warped view of the world. It also uses the BookmarkModel On 2013/10/11 21:57:41, sky wrote: > HAH! Please document better what 'warped view of the world' means. Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:28: enum BookmarkOrdering { On 2013/10/11 21:57:41, sky wrote: > Name enums as document in component: "enum values should start with the name of > the type, i.e. PAGE_TRANSITION_LINK in the content::PageTransition enum" Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:33: }; On 2013/10/11 21:57:41, sky wrote: > Is there a reason you baked sorting into these functions too? Seems like it > would be nice to return a set<BookmarkNode*> and have sorting done externally. I've removed the ordering for everything, including all those enums. The only place I kept an ordering is in tagsRelatedToTags where the tags are now always sorted by relevance. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:55: void BeginExtensiveChanges(); On 2013/10/11 21:57:41, sky wrote: > Can you hide these behind a scoper object that is a friend? Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:111: std::set<BookmarkTag> AllTagsForBookmark(const BookmarkNode* bookmark); On 2013/10/11 21:57:41, sky wrote: > GetTagsForBookmark? Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:177: void ReplaceTagsOnBookmark(const std::set<BookmarkTag>& tags, On 2013/10/11 21:57:41, sky wrote: > nit: SetTagsOnBookmark ? Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model.h:207: }; On 2013/10/11 21:57:41, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_tag_model_unittest.cc (right): https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model_unittest.cc:40: enum ObserverCounts { On 2013/10/11 21:57:41, sky wrote: > Same nit about naming. Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model_unittest.cc:74: On 2013/10/11 21:57:41, sky wrote: > nit: no newline here. Done. https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_tag_model_unittest.cc:144: }; On 2013/10/14 09:18:17, lpromero-g wrote: > nit: Sometimes there is a semi colon, sometimes not. Done. https://codereview.chromium.org/26894002/diff/64001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/26894002/diff/64001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:295: 'browser/bookmarks/bookmark_tag_model.cc', On 2013/10/11 21:57:41, sky wrote: > nit: sort. Done.
On Mon, Oct 14, 2013 at 4:59 PM, <noyau@chromium.org> wrote: > Thanks for the reviews. PTAL. > > I've used my flying time to add the requested tests and fix all the feedback > so > far on BookmarkTagModel. > > I still have a couple of fixes to do on Bookmark model: > > - Adding TODOs for copy/paste not preserving the MetaInfo > - Preventing the metaInfoChanged notification to be send if there are no > changes. > > I'll do that tomorrow, as I'm too jet-lagged now to continue to code :) > > As for limiting this to "the platform we care about" we would only be > removing > Android, and only for a short time, I'm not sure it's worth it. I was under the impression the experimentation was going to happen first on the mobile side? I must be misinformed. Let me know when you wrap up the changes. Alternatively if you want to do the metaInfoChanged thing subsequently, I'm fine with that too, just add a TODO and let me know. -Scott > > > > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > File chrome/browser/bookmarks/bookmark_tag_model.cc (right): > > https://codereview.chromium.org/26894002/diff/1/chrome/browser/bookmarks/book... > chrome/browser/bookmarks/bookmark_tag_model.cc:288: #pragma mark Private > methods. > On 2013/10/11 18:06:25, rlarocque wrote: >> >> What does this do? > > > Mac tools read those and use the text as a separator in the code > outline. It's just a hint to the IDE. But the windows compiler generate > a warning on unknown pragma, and the build treats warnings as errors, so > I removed them all. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > File chrome/browser/bookmarks/bookmark_model_observer.h (right): > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_model_observer.h:60: // Invoked before > the metainfo of a node is changed. > On 2013/10/11 21:57:41, sky wrote: >> >> Clarify that nothing may have changed here and below. > > I'll be changing the notification so it is only send when something > actually changes. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > File chrome/browser/bookmarks/bookmark_tag_model.cc (right): > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.cc:36: bool > operator()(BookmarkTag a, BookmarkTag b) { > On 2013/10/11 21:57:41, sky wrote: >> >> const BookmarkTag& ? > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.cc:139: > inhibit_change_notifications_ = true; > On 2013/10/11 21:57:41, sky wrote: >> >> Use Autoreset. > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.cc:159: const > std::set<BookmarkTag>& tags, const BookmarkNode* bookmark) { > On 2013/10/11 21:57:41, sky wrote: >> >> nit: wrap > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.cc:207: const std::set<const > BookmarkNode*> subset(tag_to_bookmarks_[*it]); > On 2013/10/11 21:57:41, sky wrote: >> >> const std::<...>& subset > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.cc:208: for (std::set<const > BookmarkNode*>::const_iterator tag_it = subset.begin(); > On 2013/10/11 21:57:41, sky wrote: >> >> bookmarks.insert(subset.begin(), subset.end()) > > I've tried this code (see patch 1) but it failed to compile with GCC on > android, hence the fallback to a loop. > > Anyway, this code was incorrect as it was doing an union of all the > tags, where what I intended was an intersection. (sorry, the diff is > hard to follow as I reordered the code to match the header). > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.cc:238: const BookmarkTag& > tag, BookmarkOrdering ordering) { > On 2013/10/11 21:57:41, sky wrote: >> >> nit: one line per param, and indent 2 more. > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.cc:262: > std::set<BookmarkTag> subset(bookmark_to_tags_[*it]); > On 2013/10/11 21:57:41, sky wrote: >> >> const std::set...& > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.cc:301: std::string encoded; > On 2013/10/11 21:57:41, sky wrote: >> >> Could you extract this into a function and put in namespace above? > > That way >> >> easier to see it doesn't modify 'this' in anyway. > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.cc:359: > bookmark_model_->SetNodeMetaInfo(bookmark, TAG_KEY, encoded); > On 2013/10/11 21:57:41, sky wrote: >> >> Can you also add a comment that internal mappings are updated on > > notification >> >> that meta data changed. > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.cc:359: > bookmark_model_->SetNodeMetaInfo(bookmark, TAG_KEY, encoded); > On 2013/10/11 21:57:41, sky wrote: >> >> Does encoded result in an empty string if tags is empty? If not, could > > you >> >> make it. > > Actually no, if this is turned into an empty string the bookmark will > revert to the default value, aka the title of all the parent bookmarks. > Added a comment to explain this behaviour. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > File chrome/browser/bookmarks/bookmark_tag_model.h (right): > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.h:1: // Copyright (c) 2013 > The Chromium Authors. All rights reserved. > On 2013/10/14 09:18:17, lpromero-g wrote: >> >> nit: No (c). Here and in every new file (also check the year). > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.h:4: #ifndef > CHROME_BROWSER_BOOKMARKS_BOOKMARK_TAG_MODEL_H_ > On 2013/10/11 21:57:41, sky wrote: >> >> nit: newline between 3/4. > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.h:14: // BookmarTagModel > provides a way to access and manipulate bookmarks in a non > On 2013/10/11 21:57:41, sky wrote: >> >> nit: non-hierarchical > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.h:16: // BookmarkModel to > its warped view of the world. It also uses the BookmarkModel > On 2013/10/11 21:57:41, sky wrote: >> >> HAH! Please document better what 'warped view of the world' means. > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.h:28: enum BookmarkOrdering > { > On 2013/10/11 21:57:41, sky wrote: >> >> Name enums as document in component: "enum values should start with > > the name of >> >> the type, i.e. PAGE_TRANSITION_LINK in the content::PageTransition > > enum" > > Done. > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.h:33: }; > > On 2013/10/11 21:57:41, sky wrote: >> >> Is there a reason you baked sorting into these functions too? Seems > > like it >> >> would be nice to return a set<BookmarkNode*> and have sorting done > > externally. > I've removed the ordering for everything, including all those enums. The > only place I kept an ordering is in tagsRelatedToTags where the tags are > now always sorted by relevance. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.h:55: void > BeginExtensiveChanges(); > On 2013/10/11 21:57:41, sky wrote: >> >> Can you hide these behind a scoper object that is a friend? > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.h:111: std::set<BookmarkTag> > AllTagsForBookmark(const BookmarkNode* bookmark); > On 2013/10/11 21:57:41, sky wrote: >> >> GetTagsForBookmark? > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.h:177: void > ReplaceTagsOnBookmark(const std::set<BookmarkTag>& tags, > On 2013/10/11 21:57:41, sky wrote: >> >> nit: SetTagsOnBookmark ? > > > Done. > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model.h:207: }; > On 2013/10/11 21:57:41, sky wrote: >> >> DISALLOW... > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > File chrome/browser/bookmarks/bookmark_tag_model_unittest.cc (right): > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model_unittest.cc:40: enum > ObserverCounts { > On 2013/10/11 21:57:41, sky wrote: >> >> Same nit about naming. > > > Done. > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model_unittest.cc:74: > On 2013/10/11 21:57:41, sky wrote: >> >> nit: no newline here. > > > Done. > > https://codereview.chromium.org/26894002/diff/64001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_tag_model_unittest.cc:144: }; > > On 2013/10/14 09:18:17, lpromero-g wrote: >> >> nit: Sometimes there is a semi colon, sometimes not. > > > Done. > > > https://codereview.chromium.org/26894002/diff/64001/chrome/chrome_browser.gypi > File chrome/chrome_browser.gypi (right): > > https://codereview.chromium.org/26894002/diff/64001/chrome/chrome_browser.gyp... > chrome/chrome_browser.gypi:295: > 'browser/bookmarks/bookmark_tag_model.cc', > On 2013/10/11 21:57:41, sky wrote: >> >> nit: sort. > > > Done. > > https://codereview.chromium.org/26894002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
sky: Our desire is to experiment simultaneously on both desktop and mobile and the mobile aspect is what necessitates this not being an extension
Added the requested TODOs.
https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... File chrome/browser/bookmarks/bookmark_tag_model.cc (right): https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_tag_model.cc:39: const BookmarkNode* bookmark) { nit: seems like you can fit all on one line. https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_tag_model.cc:314: }; nit: no ; https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_tag_model.cc:316: // Invoked from the destructor of the BookmarkModel. Are these comments really helpful? We generally don't have such comments. https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_tag_model.cc:380: const BookmarkNode* parent, nit: indentation off. https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_tag_model.cc:402: // A folder title changed. This may change the tags on all the descendants Do you need to remove node too as iteration doesn't include node? If not, how is the mapping for the node itself updated?
PTAL. https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... File chrome/browser/bookmarks/bookmark_tag_model.cc (right): https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_tag_model.cc:39: const BookmarkNode* bookmark) { On 2013/10/16 13:46:19, sky wrote: > nit: seems like you can fit all on one line. Done. https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_tag_model.cc:314: }; On 2013/10/16 13:46:19, sky wrote: > nit: no ; Done. https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_tag_model.cc:316: // Invoked from the destructor of the BookmarkModel. On 2013/10/16 13:46:19, sky wrote: > Are these comments really helpful? We generally don't have such comments. You are right, those were duplicated from the BookmarkModelObserver header anyway. Removed them all. https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_tag_model.cc:380: const BookmarkNode* parent, On 2013/10/16 13:46:19, sky wrote: > nit: indentation off. Fixed all of them. https://codereview.chromium.org/26894002/diff/146001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_tag_model.cc:402: // A folder title changed. This may change the tags on all the descendants On 2013/10/16 13:46:19, sky wrote: > Do you need to remove node too as iteration doesn't include node? If not, how is > the mapping for the node itself updated? There is no mapping kept for folder nodes, RemoveBookmark and LoadBookmark are no-op on folders. But this is unclear. Instead I made the two methods DCHECK that the node is an url and fixed the code at the caller sites for clarity. While doing this I noticed two bugs that I also fixed: - No notifications were send for those tag modifications - Moving a folder was not triggering the same behavior. I added tests to check and fixed both.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noyau@chromium.org/26894002/167001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noyau@chromium.org/26894002/167001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noyau@chromium.org/26894002/167001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noyau@chromium.org/26894002/167001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noyau@chromium.org/26894002/167001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noyau@chromium.org/26894002/167001
Message was sent while issue was closed.
Change committed as 229104 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
