Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
476573004:
Set version field when changes are made to enhanced bookmarks. (Closed)
|
Created:
6 years, 4 months ago by Rune Fevang Modified:
6 years, 3 months ago Base URL:
https://chromium.googlesource.com/chromium/src@master Project:
chromium Visibility:
Public. |
DescriptionSet version field when changes are made to enhanced bookmarks.
Moves meta info helper functions into a new class EnhancedBookmarkModel,
that writes the version field whenever other fields are modified.
BUG=404227
Committed: https://crrev.com/a1bf3af9b614208729f14af603abd5e6b5bae698
Cr-Commit-Position: refs/heads/master@{#294028}
Patch Set 1 #Patch Set 2 : #
Total comments: 20
Patch Set 3 : Forwarding methods for move and create #Patch Set 4 : Merge with ImageService CL #
Total comments: 19
Patch Set 5 : userEdit + remove initialize #Patch Set 6 : Added flags TODO #
Total comments: 29
Patch Set 7 : #Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Patch Set 10 : #Messages
Total messages: 52 (5 generated)
yfriedman@chromium.org changed reviewers: + noyau@chromium.org
+noyau who wrote the original code. didn't get to this today but will look tomorrow
yfriedman@chromium.org changed reviewers: + mcolbert@chromium.org
https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:19: explicit EnhancedBookmarkModel(BookmarkModel* bookmark_model); I don't see the value of making this a class. If you make this a class, then it needs to be a keyed service, with the proper dependencies set up so its memory management is clear. I found it way simpler to keep all this file as functions. https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:22: void Initialize(const std::string& version); Having both a constructor and an Initialize is always iffy. Epecially to set a version that should be private to the code: it will not change unless the code changes as well. Instead of an Initialize method I'd set the version in a constant in the .cc file.
https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:19: explicit EnhancedBookmarkModel(BookmarkModel* bookmark_model); On 2014/08/26 11:17:47, noyau wrote: > I don't see the value of making this a class. If you make this a class, then it > needs to be a keyed service, with the proper dependencies set up so its memory > management is clear. > > I found it way simpler to keep all this file as functions. WIth the current state, I agree with Eric. As this continues to evolve it might make more sense to be a class. I agree also that this would be a KeyedService
https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:19: explicit EnhancedBookmarkModel(BookmarkModel* bookmark_model); On 2014/08/26 16:55:22, Yaron wrote: > On 2014/08/26 11:17:47, noyau wrote: > > I don't see the value of making this a class. If you make this a class, then > it > > needs to be a keyed service, with the proper dependencies set up so its memory > > management is clear. > > > > I found it way simpler to keep all this file as functions. > > WIth the current state, I agree with Eric. As this continues to evolve it might > make more sense to be a class. I agree also that this would be a KeyedService The plan was to make it a KeyedService in a follow-up CL where I provide an extension function to call Initialize. https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:22: void Initialize(const std::string& version); On 2014/08/26 11:17:47, noyau wrote: > Having both a constructor and an Initialize is always iffy. Epecially to set a > version that should be private to the code: it will not change unless the code > changes as well. > > Instead of an Initialize method I'd set the version in a constant in the .cc > file. It needs to be settable if we're going to be calling this from the extension. More than that, it needs to be changable, as an updated extension needs to set a new version. My thought was to have initialize set the version (but not do the rest of the initialization) if the class is already initialized, as there's no reasonable way for the extension to tell if the enhanced bookmark model is already initialized or not.
I'll be OOO on Thursday/Friday but Eric's approval would be sufficient. High-level: I'm fine with this as a temporary step (with slight modification), but I don't understand the function removal. https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (left): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.cc:200: bool SetAllImagesForBookmark(BookmarkModel* bookmark_model, Where did this code? Presumably it's used by the mobile client. https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:19: explicit EnhancedBookmarkModel(BookmarkModel* bookmark_model); On 2014/08/26 18:47:52, Rune Fevang wrote: > On 2014/08/26 16:55:22, Yaron wrote: > > On 2014/08/26 11:17:47, noyau wrote: > > > I don't see the value of making this a class. If you make this a class, then > > it > > > needs to be a keyed service, with the proper dependencies set up so its > memory > > > management is clear. > > > > > > I found it way simpler to keep all this file as functions. > > > > WIth the current state, I agree with Eric. As this continues to evolve it > might > > make more sense to be a class. I agree also that this would be a KeyedService > > The plan was to make it a KeyedService in a follow-up CL where I provide an > extension function to call Initialize. Ok. That seems fine then. It's tricky though in this current phase because the mobile clients which are using the static functions now need to manage the lifetime themselves. I suppose for the time being there's no harm in each view allocating its own instance as the overhead is low and there isn't centralized state. https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:22: void Initialize(const std::string& version); On 2014/08/26 18:47:52, Rune Fevang wrote: > On 2014/08/26 11:17:47, noyau wrote: > > Having both a constructor and an Initialize is always iffy. Epecially to set a > > version that should be private to the code: it will not change unless the > code > > changes as well. > > > > Instead of an Initialize method I'd set the version in a constant in the .cc > > file. > > It needs to be settable if we're going to be calling this from the extension. > More than that, it needs to be changable, as an updated extension needs to set a > new version. My thought was to have initialize set the version (but not do the > rest of the initialization) if the class is already initialized, as there's no > reasonable way for the extension to tell if the enhanced bookmark model is > already initialized or not. Well, according to the spec, for mobile clients this can be computed by this code and not by the client. I think you can do that for all clients, and just have an optional override from the extension. Do you really need the |initialized_| member? https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:74: BookmarkModel* bookmark_model_; Without this being a keyed service, we run the risk of this being a dangling pointer but it's workable for the time being if each view allocates this as I mentioned previously.
https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (left): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.cc:200: bool SetAllImagesForBookmark(BookmarkModel* bookmark_model, On 2014/08/27 23:32:36, Yaron wrote: > Where did this code? Presumably it's used by the mobile client. I removed it because the comment said it was used for tests and it was only used in one test, that did the same thing a different test did without using it anyway. If it's actually used in mobile test code I could put it back, though it probably belongs in a test-only utils file in that case? https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:19: explicit EnhancedBookmarkModel(BookmarkModel* bookmark_model); On 2014/08/27 23:32:36, Yaron wrote: > On 2014/08/26 18:47:52, Rune Fevang wrote: > > On 2014/08/26 16:55:22, Yaron wrote: > > > On 2014/08/26 11:17:47, noyau wrote: > > > > I don't see the value of making this a class. If you make this a class, > then > > > it > > > > needs to be a keyed service, with the proper dependencies set up so its > > memory > > > > management is clear. > > > > > > > > I found it way simpler to keep all this file as functions. > > > > > > WIth the current state, I agree with Eric. As this continues to evolve it > > might > > > make more sense to be a class. I agree also that this would be a > KeyedService > > > > The plan was to make it a KeyedService in a follow-up CL where I provide an > > extension function to call Initialize. > > Ok. That seems fine then. It's tricky though in this current phase because the > mobile clients which are using the static functions now need to manage the > lifetime themselves. I suppose for the time being there's no harm in each view > allocating its own instance as the overhead is low and there isn't centralized > state. I didn't do it straight away because I wasn't sure how long it would take me to write the factory class and test it and I wanted something usable/reviewable out more quickly. https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:22: void Initialize(const std::string& version); On 2014/08/27 23:32:36, Yaron wrote: > On 2014/08/26 18:47:52, Rune Fevang wrote: > > On 2014/08/26 11:17:47, noyau wrote: > > > Having both a constructor and an Initialize is always iffy. Epecially to set > a > > > version that should be private to the code: it will not change unless the > > code > > > changes as well. > > > > > > Instead of an Initialize method I'd set the version in a constant in the .cc > > > file. > > > > It needs to be settable if we're going to be calling this from the extension. > > More than that, it needs to be changable, as an updated extension needs to set > a > > new version. My thought was to have initialize set the version (but not do the > > rest of the initialization) if the class is already initialized, as there's no > > reasonable way for the extension to tell if the enhanced bookmark model is > > already initialized or not. > > Well, according to the spec, for mobile clients this can be computed by this > code and not by the client. I think you can do that for all clients, and just > have an optional override from the extension. Do you really need the > |initialized_| member? There's no need for the initialized_ member yet, but there will be once the initialization code gets more complicated. I figured it might as well go in right away to make sure clients don't call functions without setting the version. https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:74: BookmarkModel* bookmark_model_; On 2014/08/27 23:32:36, Yaron wrote: > Without this being a keyed service, we run the risk of this being a dangling > pointer but it's workable for the time being if each view allocates this as I > mentioned previously. Acknowledged.
https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (left): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.cc:200: bool SetAllImagesForBookmark(BookmarkModel* bookmark_model, On 2014/08/27 23:52:22, Rune Fevang wrote: > On 2014/08/27 23:32:36, Yaron wrote: > > Where did this code? Presumably it's used by the mobile client. > > I removed it because the comment said it was used for tests and it was only used > in one test, that did the same thing a different test did without using it > anyway. If it's actually used in mobile test code I could put it back, though it > probably belongs in a test-only utils file in that case? From a quick grep, it appears to be used from internal test code. https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:22: void Initialize(const std::string& version); On 2014/08/27 23:52:22, Rune Fevang wrote: > On 2014/08/27 23:32:36, Yaron wrote: > > On 2014/08/26 18:47:52, Rune Fevang wrote: > > > On 2014/08/26 11:17:47, noyau wrote: > > > > Having both a constructor and an Initialize is always iffy. Epecially to > set > > a > > > > version that should be private to the code: it will not change unless the > > > code > > > > changes as well. > > > > > > > > Instead of an Initialize method I'd set the version in a constant in the > .cc > > > > file. > > > > > > It needs to be settable if we're going to be calling this from the > extension. > > > More than that, it needs to be changable, as an updated extension needs to > set > > a > > > new version. My thought was to have initialize set the version (but not do > the > > > rest of the initialization) if the class is already initialized, as there's > no > > > reasonable way for the extension to tell if the enhanced bookmark model is > > > already initialized or not. > > > > Well, according to the spec, for mobile clients this can be computed by this > > code and not by the client. I think you can do that for all clients, and just > > have an optional override from the extension. Do you really need the > > |initialized_| member? > > There's no need for the initialized_ member yet, but there will be once the > initialization code gets more complicated. I figured it might as well go in > right away to make sure clients don't call functions without setting the > version. If the initialization is handled by this class on construction, you don't have that issue. I think that's what Eric's asking for and I think it makes sense. The way I'm thinking of this (and I believe how we should think about it going forward) is that the smarts needs to be here and can't be in the clients or they'll never be consistent. Is there a harm in having the initial upgrade/initialization handled in this code, and then subsequent writes from the extension would have the extension id as part of the version? The reason I want this is because that's what mobile clients would do.
https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:22: void Initialize(const std::string& version); On 2014/08/28 00:11:49, Yaron_OOO_8.28-8.29 wrote: > On 2014/08/27 23:52:22, Rune Fevang wrote: > > On 2014/08/27 23:32:36, Yaron wrote: > > > On 2014/08/26 18:47:52, Rune Fevang wrote: > > > > On 2014/08/26 11:17:47, noyau wrote: > > > > > Having both a constructor and an Initialize is always iffy. Epecially to > > set > > > a > > > > > version that should be private to the code: it will not change unless > the > > > > code > > > > > changes as well. > > > > > > > > > > Instead of an Initialize method I'd set the version in a constant in the > > .cc > > > > > file. > > > > > > > > It needs to be settable if we're going to be calling this from the > > extension. > > > > More than that, it needs to be changable, as an updated extension needs to > > set > > > a > > > > new version. My thought was to have initialize set the version (but not do > > the > > > > rest of the initialization) if the class is already initialized, as > there's > > no > > > > reasonable way for the extension to tell if the enhanced bookmark model is > > > > already initialized or not. > > > > > > Well, according to the spec, for mobile clients this can be computed by this > > > code and not by the client. I think you can do that for all clients, and > just > > > have an optional override from the extension. Do you really need the > > > |initialized_| member? > > > > There's no need for the initialized_ member yet, but there will be once the > > initialization code gets more complicated. I figured it might as well go in > > right away to make sure clients don't call functions without setting the > > version. > > If the initialization is handled by this class on construction, you don't have > that issue. I think that's what Eric's asking for and I think it makes sense. > > The way I'm thinking of this (and I believe how we should think about it going > forward) is that the smarts needs to be here and can't be in the clients or > they'll never be consistent. Is there a harm in having the initial > upgrade/initialization handled in this code, and then subsequent writes from the > extension would have the extension id as part of the version? The reason I want > this is because that's what mobile clients would do. I agree this is where the smarts should be. I'd like to move to a future where any interaction with the bookmark model, both from the mobile clients and the extension go through this class. I'm not sure I understand what you're saying about the extension and version writing, the extension id would remain the same through extension upgrades, and the extension needs some way to tell this class what version string to write.
Thinking a bit more about this, how is this class going to deal with the upgrade path? Existing client will need to be upgraded to the new versioning, not just the new bookmarks? How do you plan to manage the transition? https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:22: void Initialize(const std::string& version); On 2014/08/26 18:47:52, Rune Fevang wrote: > On 2014/08/26 11:17:47, noyau wrote: > > Having both a constructor and an Initialize is always iffy. Epecially to set a > > version that should be private to the code: it will not change unless the > code > > changes as well. > > > > Instead of an Initialize method I'd set the version in a constant in the .cc > > file. > > It needs to be settable if we're going to be calling this from the extension. > More than that, it needs to be changable, as an updated extension needs to set a > new version. My thought was to have initialize set the version (but not do the > rest of the initialization) if the class is already initialized, as there's no > reasonable way for the extension to tell if the enhanced bookmark model is > already initialized or not. The way it is done in a component is usually to have an abstract method to return the version and have the chrome side subclass the KeyedService and implement the function. But in this case it is probably easier to not bother with initilaize at all and have the version as an argument in the constructor, with the factory, on the chrome side, being responsible for passing it in at creation time.
https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (left): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.cc:200: bool SetAllImagesForBookmark(BookmarkModel* bookmark_model, On 2014/08/28 00:11:49, Yaron_OOO_8.28-8.29 wrote: > On 2014/08/27 23:52:22, Rune Fevang wrote: > > On 2014/08/27 23:32:36, Yaron wrote: > > > Where did this code? Presumably it's used by the mobile client. > > > > I removed it because the comment said it was used for tests and it was only > used > > in one test, that did the same thing a different test did without using it > > anyway. If it's actually used in mobile test code I could put it back, though > it > > probably belongs in a test-only utils file in that case? > > From a quick grep, it appears to be used from internal test code. OK, re-added it as a static function. https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:19: explicit EnhancedBookmarkModel(BookmarkModel* bookmark_model); On 2014/08/27 23:52:22, Rune Fevang wrote: > On 2014/08/27 23:32:36, Yaron wrote: > > On 2014/08/26 18:47:52, Rune Fevang wrote: > > > On 2014/08/26 16:55:22, Yaron wrote: > > > > On 2014/08/26 11:17:47, noyau wrote: > > > > > I don't see the value of making this a class. If you make this a class, > > then > > > > it > > > > > needs to be a keyed service, with the proper dependencies set up so its > > > memory > > > > > management is clear. > > > > > > > > > > I found it way simpler to keep all this file as functions. > > > > > > > > WIth the current state, I agree with Eric. As this continues to evolve it > > > might > > > > make more sense to be a class. I agree also that this would be a > > KeyedService > > > > > > The plan was to make it a KeyedService in a follow-up CL where I provide an > > > extension function to call Initialize. > > > > Ok. That seems fine then. It's tricky though in this current phase because the > > mobile clients which are using the static functions now need to manage the > > lifetime themselves. I suppose for the time being there's no harm in each view > > allocating its own instance as the overhead is low and there isn't centralized > > state. > > I didn't do it straight away because I wasn't sure how long it would take me to > write the factory class and test it and I wanted something usable/reviewable out > more quickly. Made it a KeyedService like we talked about. https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:22: void Initialize(const std::string& version); On 2014/08/28 08:36:53, noyau wrote: > On 2014/08/26 18:47:52, Rune Fevang wrote: > > On 2014/08/26 11:17:47, noyau wrote: > > > Having both a constructor and an Initialize is always iffy. Epecially to set > a > > > version that should be private to the code: it will not change unless the > > code > > > changes as well. > > > > > > Instead of an Initialize method I'd set the version in a constant in the .cc > > > file. > > > > It needs to be settable if we're going to be calling this from the extension. > > More than that, it needs to be changable, as an updated extension needs to set > a > > new version. My thought was to have initialize set the version (but not do the > > rest of the initialization) if the class is already initialized, as there's no > > reasonable way for the extension to tell if the enhanced bookmark model is > > already initialized or not. > > The way it is done in a component is usually to have an abstract method to > return the version and have the chrome side subclass the KeyedService and > implement the function. But in this case it is probably easier to not bother > with initilaize at all and have the version as an argument in the constructor, > with the factory, on the chrome side, being responsible for passing it in at > creation time. I think I understand what you're saying now. Basically you want initialization to happen whenever some function requests the enhanced bookmark model the first time, instead of being an explicit step? I'm not sure if that will work while we're still transitioning to where we don't need the bookmark model. For the extension at least, it's important (or will be when initialize is implemented properly) that it's called before anything is read from the bookmark model. Since we need to explicitly call initialize anyway (and may do so again on extension reload/update), it seems like a good place to set the version to me.
On 2014/08/29 01:10:10, Rune Fevang wrote: > https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... > File components/enhanced_bookmarks/enhanced_bookmark_model.cc (left): > > https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... > components/enhanced_bookmarks/enhanced_bookmark_model.cc:200: bool > SetAllImagesForBookmark(BookmarkModel* bookmark_model, > On 2014/08/28 00:11:49, Yaron_OOO_8.28-8.29 wrote: > > On 2014/08/27 23:52:22, Rune Fevang wrote: > > > On 2014/08/27 23:32:36, Yaron wrote: > > > > Where did this code? Presumably it's used by the mobile client. > > > > > > I removed it because the comment said it was used for tests and it was only > > used > > > in one test, that did the same thing a different test did without using it > > > anyway. If it's actually used in mobile test code I could put it back, > though > > it > > > probably belongs in a test-only utils file in that case? > > > > From a quick grep, it appears to be used from internal test code. > > OK, re-added it as a static function. > > https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... > File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): > > https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... > components/enhanced_bookmarks/enhanced_bookmark_model.h:19: explicit > EnhancedBookmarkModel(BookmarkModel* bookmark_model); > On 2014/08/27 23:52:22, Rune Fevang wrote: > > On 2014/08/27 23:32:36, Yaron wrote: > > > On 2014/08/26 18:47:52, Rune Fevang wrote: > > > > On 2014/08/26 16:55:22, Yaron wrote: > > > > > On 2014/08/26 11:17:47, noyau wrote: > > > > > > I don't see the value of making this a class. If you make this a > class, > > > then > > > > > it > > > > > > needs to be a keyed service, with the proper dependencies set up so > its > > > > memory > > > > > > management is clear. > > > > > > > > > > > > I found it way simpler to keep all this file as functions. > > > > > > > > > > WIth the current state, I agree with Eric. As this continues to evolve > it > > > > might > > > > > make more sense to be a class. I agree also that this would be a > > > KeyedService > > > > > > > > The plan was to make it a KeyedService in a follow-up CL where I provide > an > > > > extension function to call Initialize. > > > > > > Ok. That seems fine then. It's tricky though in this current phase because > the > > > mobile clients which are using the static functions now need to manage the > > > lifetime themselves. I suppose for the time being there's no harm in each > view > > > allocating its own instance as the overhead is low and there isn't > centralized > > > state. > > > > I didn't do it straight away because I wasn't sure how long it would take me > to > > write the factory class and test it and I wanted something usable/reviewable > out > > more quickly. > > Made it a KeyedService like we talked about. > > https://codereview.chromium.org/476573004/diff/20001/components/enhanced_book... > components/enhanced_bookmarks/enhanced_bookmark_model.h:22: void > Initialize(const std::string& version); > On 2014/08/28 08:36:53, noyau wrote: > > On 2014/08/26 18:47:52, Rune Fevang wrote: > > > On 2014/08/26 11:17:47, noyau wrote: > > > > Having both a constructor and an Initialize is always iffy. Epecially to > set > > a > > > > version that should be private to the code: it will not change unless the > > > code > > > > changes as well. > > > > > > > > Instead of an Initialize method I'd set the version in a constant in the > .cc > > > > file. > > > > > > It needs to be settable if we're going to be calling this from the > extension. > > > More than that, it needs to be changable, as an updated extension needs to > set > > a > > > new version. My thought was to have initialize set the version (but not do > the > > > rest of the initialization) if the class is already initialized, as there's > no > > > reasonable way for the extension to tell if the enhanced bookmark model is > > > already initialized or not. > > > > The way it is done in a component is usually to have an abstract method to > > return the version and have the chrome side subclass the KeyedService and > > implement the function. But in this case it is probably easier to not bother > > with initilaize at all and have the version as an argument in the constructor, > > with the factory, on the chrome side, being responsible for passing it in at > > creation time. > > I think I understand what you're saying now. Basically you want initialization > to happen whenever some function requests the enhanced bookmark model the first > time, instead of being an explicit step? I'm not sure if that will work while > we're still transitioning to where we don't need the bookmark model. For the > extension at least, it's important (or will be when initialize is implemented > properly) that it's called before anything is read from the bookmark model. > Since we need to explicitly call initialize anyway (and may do so again on > extension reload/update), it seems like a good place to set the version to me. You can have a custom BrowserContextKeyedServiceFactory which guarantees that the init is done before any accessors. It would pass the version info there. See for example how DomDistillerService is init'd: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dom...
> You can have a custom BrowserContextKeyedServiceFactory which guarantees that > the init is done before any accessors. It would pass the version info there. See > for example how DomDistillerService is init'd: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dom... How does the service factory know what version to use? I guess I don't really see how you are supposed to use this from an extension API if the version/initialization is done in the constructor. Do you set a "hey, in case asking you for the enhanced bookmark model requires creating it, here's the version to use" variable on it? Or maybe I'm just misunderstanding the requirements, and its fine to use a different version for the fields changed by the initialization code and those changed by later calls. mcolbert@?
On 2014/09/03 02:13:18, Rune Fevang wrote: > [...] > How does the service factory know what version to use? I guess I don't really > see how you are supposed to use this from an extension API if the > version/initialization is done in the constructor. Do you set a "hey, in case > asking you for the enhanced bookmark model requires creating it, here's the > version to use" variable on it? > The service factory will be in Chrome, and can simply use chrome::VersionInfo to get at a version.
On 2014/09/03 07:34:46, noyau1 wrote: > On 2014/09/03 02:13:18, Rune Fevang wrote: > > [...] > > How does the service factory know what version to use? I guess I don't really > > see how you are supposed to use this from an extension API if the > > version/initialization is done in the constructor. Do you set a "hey, in case > > asking you for the enhanced bookmark model requires creating it, here's the > > version to use" variable on it? > > > The service factory will be in Chrome, and can simply use chrome::VersionInfo to > get at a version. The version to set is not Chrome's version, but the version of the extension that calls into the API. At least that's my understanding.
On 2014/09/03 08:30:49, Rune Fevang wrote: > On 2014/09/03 07:34:46, noyau1 wrote: > > On 2014/09/03 02:13:18, Rune Fevang wrote: > > > [...] > > > How does the service factory know what version to use? I guess I don't > really > > > see how you are supposed to use this from an extension API if the > > > version/initialization is done in the constructor. Do you set a "hey, in > case > > > asking you for the enhanced bookmark model requires creating it, here's the > > > version to use" variable on it? > > > > > The service factory will be in Chrome, and can simply use chrome::VersionInfo > to > > get at a version. > > The version to set is not Chrome's version, but the version of the extension > that calls into the API. At least that's my understanding. But why use the extension version for the initial write? My point previously was that using Chrome's version should be good enough. Since the upgrade would be done on first load and by built-in chrome code (i.e. not extension) it's actually a fine version to use. You can later set the version from the extension so we know what mutations came from there (if it's truly important). As I said previously, mobile clients have to use the built in code (no extensions) so it's not clear why using chrome's version wouldn't work
On 2014/09/03 18:54:53, Yaron wrote: > On 2014/09/03 08:30:49, Rune Fevang wrote: > > On 2014/09/03 07:34:46, noyau1 wrote: > > > On 2014/09/03 02:13:18, Rune Fevang wrote: > > > > [...] > > > > How does the service factory know what version to use? I guess I don't > > really > > > > see how you are supposed to use this from an extension API if the > > > > version/initialization is done in the constructor. Do you set a "hey, in > > case > > > > asking you for the enhanced bookmark model requires creating it, here's > the > > > > version to use" variable on it? > > > > > > > The service factory will be in Chrome, and can simply use > chrome::VersionInfo > > to > > > get at a version. > > > > The version to set is not Chrome's version, but the version of the extension > > that calls into the API. At least that's my understanding. > > But why use the extension version for the initial write? My point previously was > that using Chrome's version should be good enough. Since the upgrade would be > done on first load and by built-in chrome code (i.e. not extension) it's > actually a fine version to use. You can later set the version from the extension > so we know what mutations came from there (if it's truly important). As I said > previously, mobile clients have to use the built in code (no extensions) so it's > not clear why using chrome's version wouldn't work As far as I understood, the motivation for writing a version field was to figure out which version of the client requested various writes. The main use case that triggered this was a desire to see which writes were caused by the leaked extension version. In order to accomplish this all writes needs to set the version of the code requesting the changes, not the version of the code actually doing the writing.
> As far as I understood, the motivation for writing a version field was to figure > out which version of the client requested various writes. The main use case that > triggered this was a desire to see which writes were caused by the leaked > extension version. In order to accomplish this all writes needs to set the > version of the code requesting the changes, not the version of the code actually > doing the writing. Since stars.version is not synced between clients (the server strips it out during the GetUpdates call) we can just use the Chrome version until something overwrites it, like a CRX. Provided the version identifier identifies the platform, i.e. Bling, Clank or Desktop. We just need to make sure if there are clients that are going to live on top of this interface, like the CRX, that they can override this value. Does that make sense? If we can do that, we should be good to go.
https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.cc:122: std::string EnhancedBookmarkModel::SetRemoteIdForNode( Hah, you guys have to use the std:: prefix in Chrome, eh? https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.cc:129: random_id << "ebc_"; Nit: These should be top-level constants. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.cc:135: random_id << base::RandUint64() << base::RandUint64(); Base the first rand on time so that you are not in the same place on a pseudo-random number sequence. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:23: // bookmarks. I'm assuming everything around stars.userEdits will be handled to keep the last modified timestamp correct? https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:87: // node. Returns the empty string if the node doesn't have one set. This should ONLY be used for debugging. The server will not send back the last version info and thus it is the version last touched by the client. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:88: std::string GetVersionForNode(const BookmarkNode* node); Clients should be agnostic to this and this should be only used for debugging purposes. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:90: // Used for testing, simulates the process that creates the thumnails. Will Nit: s/thumnails/thumbnails https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:92: // expects valid or empty urls. Returns true if the metainfo is successfully s/expects/Expects https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:103: Add TODO to add method for setting and getting the flags.
Discussed a bit with Mark offline. Changed the code to take a version string in the constructor, and added a method to append a changable suffix to it. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.cc:122: std::string EnhancedBookmarkModel::SetRemoteIdForNode( On 2014/09/03 23:31:16, Mark wrote: > Hah, you guys have to use the std:: prefix in Chrome, eh? Acknowledged. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.cc:129: random_id << "ebc_"; On 2014/09/03 23:31:16, Mark wrote: > Nit: These should be top-level constants. Done. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.cc:135: random_id << base::RandUint64() << base::RandUint64(); On 2014/09/03 23:31:16, Mark wrote: > Base the first rand on time so that you are not in the same place on a > pseudo-random number sequence. This function has a comment stating it is safe to use for crypto purposes, so I believe this is fine as is. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:23: // bookmarks. On 2014/09/03 23:31:16, Mark wrote: > I'm assuming everything around stars.userEdits will be handled to keep the last > modified timestamp correct? Added stars.userEdit handling. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:87: // node. Returns the empty string if the node doesn't have one set. On 2014/09/03 23:31:16, Mark wrote: > This should ONLY be used for debugging. The server will not send back the last > version info and thus it is the version last touched by the client. Moved the function to the test class. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:88: std::string GetVersionForNode(const BookmarkNode* node); On 2014/09/03 23:31:16, Mark wrote: > Clients should be agnostic to this and this should be only used for debugging > purposes. Acknowledged. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:90: // Used for testing, simulates the process that creates the thumnails. Will On 2014/09/03 23:31:16, Mark wrote: > Nit: s/thumnails/thumbnails Done. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:92: // expects valid or empty urls. Returns true if the metainfo is successfully On 2014/09/03 23:31:16, Mark wrote: > s/expects/Expects Done. https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:103: On 2014/09/03 23:31:16, Mark wrote: > Add TODO to add method for setting and getting the flags. Getting and setting which flags?
https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... components/enhanced_bookmarks/enhanced_bookmark_model.h:103: On 2014/09/04 01:03:13, Rune Fevang wrote: > On 2014/09/03 23:31:16, Mark wrote: > > Add TODO to add method for setting and getting the flags. > > Getting and setting which flags? stars.flag. This could be handled by generic SetNodeMetaInfo, but I was figuring something more might be better.
On 2014/09/04 01:52:10, Mark wrote: > https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... > File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): > > https://codereview.chromium.org/476573004/diff/60001/components/enhanced_book... > components/enhanced_bookmarks/enhanced_bookmark_model.h:103: > On 2014/09/04 01:03:13, Rune Fevang wrote: > > On 2014/09/03 23:31:16, Mark wrote: > > > Add TODO to add method for setting and getting the flags. > > > > Getting and setting which flags? > > stars.flag. This could be handled by generic SetNodeMetaInfo, but I was > figuring something more might be better. Ok, added TODO.
https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/bookmark_image_service.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/bookmark_image_service.cc:202: bookmark, image_url, size.width(), size.height(), false); what does stars.userEdit mean? This can be as a result of the user adding a new bookmark. What happens if this is set incorrectly? https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:257: meta_info.insert(old_meta_info->begin(), old_meta_info->end()); Some of these fields are quite long. Isn't this a speculative copy of the entire map for every write to meta info?
https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/bookmark_image_service.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/bookmark_image_service.cc:202: bookmark, image_url, size.width(), size.height(), false); On 2014/09/05 03:16:53, Yaron wrote: > what does stars.userEdit mean? This can be as a result of the user adding a new > bookmark. What happens if this is set incorrectly? I don't understand the purpose of this field myself, hopefully Mark can answer. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:257: meta_info.insert(old_meta_info->begin(), old_meta_info->end()); On 2014/09/05 03:16:53, Yaron wrote: > Some of these fields are quite long. Isn't this a speculative copy of the entire > map for every write to meta info? It is, however if the fields are set one at a time, sync might send partial writes to the server. If the fields are long enough that this is an issue that seems wrong.
https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:257: meta_info.insert(old_meta_info->begin(), old_meta_info->end()); On 2014/09/05 03:40:26, Rune Fevang wrote: > On 2014/09/05 03:16:53, Yaron wrote: > > Some of these fields are quite long. Isn't this a speculative copy of the > entire > > map for every write to meta info? > > It is, however if the fields are set one at a time, sync might send partial > writes to the server. If the fields are long enough that this is an issue that > seems wrong. Hmm. That's a little concerning re: sync. It's not clear from the API that this could be an issue but I see that creation uses a MetaInfoMap so at least what's likely the most expensive case is a little better.
https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:257: meta_info.insert(old_meta_info->begin(), old_meta_info->end()); On 2014/09/05 04:07:37, Yaron wrote: > On 2014/09/05 03:40:26, Rune Fevang wrote: > > On 2014/09/05 03:16:53, Yaron wrote: > > > Some of these fields are quite long. Isn't this a speculative copy of the > > entire > > > map for every write to meta info? > > > > It is, however if the fields are set one at a time, sync might send partial > > writes to the server. If the fields are long enough that this is an issue that > > seems wrong. > > Hmm. That's a little concerning re: sync. It's not clear from the API that this > could be an issue but I see that creation uses a MetaInfoMap so at least what's > likely the most expensive case is a little better. The idea is that any write operations that "belong together" should happen in one update. This is just a convenience function for the one-field case (since we're now setting version and userEdit on those also).
https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/bookmark_image_service.h (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/bookmark_image_service.h:32: EnhancedBookmarkModel* enhanced_bookmark_model, Can't we access the bookmark_model inside the enhanced bookmark model instead of passing it as another argument? https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:106: std::string EnhancedBookmarkModel::GetRemoteIdForNode( Why did you rename everything to "Node"? I prefer the specific "Bookmark" to the generic "Node". It expresses better what the argument is. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:24: class EnhancedBookmarkModel : public KeyedService { This doesn't track changes in the model, and as such won't be able to detect changes to the model done without using this API (changes from other synced client, changes from extensions manipulating the bookmark_model directly, copy/paste… https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:39: const BookmarkNode::MetaInfoMap* meta_info); Why does this takes a metainfo? I thought the whole point of that class was to hide all the metainfo malarkey from the user of the API… https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:47: const BookmarkNode::MetaInfoMap* meta_info); Same here. I'd expect addURL to take an optional description and image, but not a whole map that as a user I can't create anyway as the constants to do so are in the .cc… Note that on iOS the bookmarks are created without any meta info as the image is retrieved asynchronously, so it is set later. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:56: bool user_edit); I don't understand user_edit. I thought the description was always user edited, and if not preset the snippet would be used instead. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:69: bool user_edit); Here user_edit is confusing, this is always the result of a user action, he visited a bookmarked page: user action. Bookmarked a page: user action. So this always be true. Or this need a better explanation in the comment. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:100: static bool SetAllImagesForBookmark(BookmarkModel* bookmark_model, The bookmark_model argument is not needed here anymore.
https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:257: meta_info.insert(old_meta_info->begin(), old_meta_info->end()); On 2014/09/05 05:04:58, Rune Fevang wrote: > On 2014/09/05 04:07:37, Yaron wrote: > > On 2014/09/05 03:40:26, Rune Fevang wrote: > > > On 2014/09/05 03:16:53, Yaron wrote: > > > > Some of these fields are quite long. Isn't this a speculative copy of the > > > entire > > > > map for every write to meta info? > > > > > > It is, however if the fields are set one at a time, sync might send partial > > > writes to the server. If the fields are long enough that this is an issue > that > > > seems wrong. > > > > Hmm. That's a little concerning re: sync. It's not clear from the API that > this > > could be an issue but I see that creation uses a MetaInfoMap so at least > what's > > likely the most expensive case is a little better. > > The idea is that any write operations that "belong together" should happen in > one update. This is just a convenience function for the one-field case (since > we're now setting version and userEdit on those also). Sync operates on SyncEntity fidelity. There is no such thing as a partial write in terms of Chrome Sync. Setting all of this information is fine provided the ordering does not change.
https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/bookmark_image_service.h (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/bookmark_image_service.h:32: EnhancedBookmarkModel* enhanced_bookmark_model, On 2014/09/05 12:04:43, noyau wrote: > Can't we access the bookmark_model inside the enhanced bookmark model instead of > passing it as another argument? Done. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:106: std::string EnhancedBookmarkModel::GetRemoteIdForNode( On 2014/09/05 12:04:43, noyau wrote: > Why did you rename everything to "Node"? I prefer the specific "Bookmark" to the > generic "Node". It expresses better what the argument is. Both bookmarks and folders have remote ids. I renamed as these methods generally refer to both of them. I can change it back for the ones specific to Bookmarks (images) if you like, I just liked the consistency. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:257: meta_info.insert(old_meta_info->begin(), old_meta_info->end()); On 2014/09/05 16:27:34, Mark wrote: > On 2014/09/05 05:04:58, Rune Fevang wrote: > > On 2014/09/05 04:07:37, Yaron wrote: > > > On 2014/09/05 03:40:26, Rune Fevang wrote: > > > > On 2014/09/05 03:16:53, Yaron wrote: > > > > > Some of these fields are quite long. Isn't this a speculative copy of > the > > > > entire > > > > > map for every write to meta info? > > > > > > > > It is, however if the fields are set one at a time, sync might send > partial > > > > writes to the server. If the fields are long enough that this is an issue > > that > > > > seems wrong. > > > > > > Hmm. That's a little concerning re: sync. It's not clear from the API that > > this > > > could be an issue but I see that creation uses a MetaInfoMap so at least > > what's > > > likely the most expensive case is a little better. > > > > The idea is that any write operations that "belong together" should happen in > > one update. This is just a convenience function for the one-field case (since > > we're now setting version and userEdit on those also). > > Sync operates on SyncEntity fidelity. There is no such thing as a partial write > in terms of Chrome Sync. Setting all of this information is fine provided the > ordering does not change. How do you distinguish between a partial write with an old value for the userEdit field and a complete write that sets it to the value it already have then? By partial write I mean that sync decides to send updates to the server before all the fields have changed here (it runs on a different thread). I really don't think doing three writes is cheaper than copying the map here, if I remember correctly sync will do a full copy of all fields every time it receives onMetaInfoChanged anyway. Also the extension API will do a copy when it receives onMetaInfoWillChange, then compare that to the one present after getting onMetaInfoChanged to get the diff. So at least in the case when the extension is running, three updates will be more expensive than doing it this way. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:24: class EnhancedBookmarkModel : public KeyedService { On 2014/09/05 12:04:43, noyau wrote: > This doesn't track changes in the model, and as such won't be able to detect > changes to the model done without using this API (changes from other synced > client, changes from extensions manipulating the bookmark_model directly, > copy/paste… Right, that will come in a later CL (task 4 in the timeline). https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:39: const BookmarkNode::MetaInfoMap* meta_info); On 2014/09/05 12:04:43, noyau wrote: > Why does this takes a metainfo? I thought the whole point of that class was to > hide all the metainfo malarkey from the user of the API… Yes that is the (at least long term) point, but as I'm not sure how you're currently creating nodes, I thought it was better to be flexible for now. You could always just pass NULL if you don't have anything to set. However, as the only things that are critical to set on node creation are id and placement, I agree it shouldn't be there. Removed. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:47: const BookmarkNode::MetaInfoMap* meta_info); On 2014/09/05 12:04:43, noyau wrote: > Same here. I'd expect addURL to take an optional description and image, but not > a whole map that as a user I can't create anyway as the constants to do so are > in the .cc… > > Note that on iOS the bookmarks are created without any meta info as the image is > retrieved asynchronously, so it is set later. That's fine, the main things that need to be set on creation is stars.id and position, which I was planning on adding in a later CL. Changed the implementation to set stars.id in this CL as the necessary framework is already in. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:56: bool user_edit); On 2014/09/05 12:04:43, noyau wrote: > I don't understand user_edit. I thought the description was always user edited, > and if not preset the snippet would be used instead. Discussed userEdit with mcolbert@. Decided that it was enough to just have it set on initialization/dup correction. So removed them again from the API and added a section in the design document. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:100: static bool SetAllImagesForBookmark(BookmarkModel* bookmark_model, On 2014/09/05 12:04:43, noyau wrote: > The bookmark_model argument is not needed here anymore. Done.
https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:106: std::string EnhancedBookmarkModel::GetRemoteIdForNode( On 2014/09/06 00:01:56, Rune Fevang wrote: > On 2014/09/05 12:04:43, noyau wrote: > > Why did you rename everything to "Node"? I prefer the specific "Bookmark" to > the > > generic "Node". It expresses better what the argument is. > > Both bookmarks and folders have remote ids. I renamed as these methods generally > refer to both of them. I can change it back for the ones specific to Bookmarks > (images) if you like, I just liked the consistency. I see, you consider folder are not bookmarks. I don't as the node themselves are all BookmarkNodes, nodes pointing out that thy are part of the tree type, and bookmark expessing what they contain. hence for me "Bookmark" is a better description in the function name here as the nodes are all bookmark node. (I'm rereading my self and I'm not sure what I am writing is understandable :) But I see where you are coming from. I would actually prefer everything be xxxForBookmark, but I would settle to leave the ID ones being xxxForNode, if you feel strongly about it. While you are at it can you add a DCHECK to the ones that are not supposed to take a folder? No need to set an image or description on those. https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.h (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.h:24: class EnhancedBookmarkModel : public KeyedService { On 2014/09/06 00:01:56, Rune Fevang wrote: > On 2014/09/05 12:04:43, noyau wrote: > > This doesn't track changes in the model, and as such won't be able to detect > > changes to the model done without using this API (changes from other synced > > client, changes from extensions manipulating the bookmark_model directly, > > copy/paste… > > Right, that will come in a later CL (task 4 in the timeline). Acknowledged.
https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:106: std::string EnhancedBookmarkModel::GetRemoteIdForNode( On 2014/09/08 09:04:18, noyau wrote: > On 2014/09/06 00:01:56, Rune Fevang wrote: > > On 2014/09/05 12:04:43, noyau wrote: > > > Why did you rename everything to "Node"? I prefer the specific "Bookmark" to > > the > > > generic "Node". It expresses better what the argument is. > > > > Both bookmarks and folders have remote ids. I renamed as these methods > generally > > refer to both of them. I can change it back for the ones specific to Bookmarks > > (images) if you like, I just liked the consistency. > > I see, you consider folder are not bookmarks. I don't as the node themselves are > all BookmarkNodes, nodes pointing out that thy are part of the tree type, and > bookmark expessing what they contain. hence for me "Bookmark" is a better > description in the function name here as the nodes are all bookmark node. (I'm > rereading my self and I'm not sure what I am writing is understandable :) > > But I see where you are coming from. I would actually prefer everything be > xxxForBookmark, but I would settle to leave the ID ones being xxxForNode, if you > feel strongly about it. > > While you are at it can you add a DCHECK to the ones that are not supposed to > take a folder? No need to set an image or description on those. How about omitting the ForBookmark/ForNode altogether? That's consistent with how the BookmarkModel names its methods. Added DCHECK to the image setting methods, folders are allowed to have descriptions.
https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/bookmark_image_service.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/bookmark_image_service.cc:202: bookmark, image_url, size.width(), size.height(), false); On 2014/09/05 03:40:26, Rune Fevang wrote: > On 2014/09/05 03:16:53, Yaron wrote: > > what does stars.userEdit mean? This can be as a result of the user adding a > new > > bookmark. What happens if this is set incorrectly? > > I don't understand the purpose of this field myself, hopefully Mark can answer. The idea is that not all client edits are necessarily done by explicit user action. For instance, if we set a meta info field for book-keeping purposes, this is not a user initiated action. It has been strongly requested by the ZipIt team that we only update the last modified timestamp for user initiated actions and not other actions that may be done behind the scenes. So, the purpose of this field is to tell the server not to update the last modified timestamp. Does that make sense?
lgtm but please wait for Eric https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/bookmark_image_service.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/bookmark_image_service.cc:202: bookmark, image_url, size.width(), size.height(), false); On 2014/09/08 22:47:08, Mark wrote: > On 2014/09/05 03:40:26, Rune Fevang wrote: > > On 2014/09/05 03:16:53, Yaron wrote: > > > what does stars.userEdit mean? This can be as a result of the user adding a > > new > > > bookmark. What happens if this is set incorrectly? > > > > I don't understand the purpose of this field myself, hopefully Mark can > answer. > > The idea is that not all client edits are necessarily done by explicit user > action. For instance, if we set a meta info field for book-keeping purposes, > this is not a user initiated action. It has been strongly requested by the > ZipIt team that we only update the last modified timestamp for user initiated > actions and not other actions that may be done behind the scenes. So, the > purpose of this field is to tell the server not to update the last modified > timestamp. Does that make sense? Sure, but then it should be true here which I see reflected now so we're on the same page. https://codereview.chromium.org/476573004/diff/140001/components/enhanced_boo... File components/enhanced_bookmarks/bookmark_image_service.h (right): https://codereview.chromium.org/476573004/diff/140001/components/enhanced_boo... components/enhanced_bookmarks/bookmark_image_service.h:105: // Cached pointers to the bookmark models. only one now
https://codereview.chromium.org/476573004/diff/140001/components/enhanced_boo... File components/enhanced_bookmarks/bookmark_image_service.h (right): https://codereview.chromium.org/476573004/diff/140001/components/enhanced_boo... components/enhanced_bookmarks/bookmark_image_service.h:105: // Cached pointers to the bookmark models. On 2014/09/08 23:30:39, Yaron wrote: > only one now Done.
lgtm https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right): https://codereview.chromium.org/476573004/diff/100001/components/enhanced_boo... components/enhanced_bookmarks/enhanced_bookmark_model.cc:106: std::string EnhancedBookmarkModel::GetRemoteIdForNode( On 2014/09/08 19:29:19, Rune Fevang wrote: >[...] > How about omitting the ForBookmark/ForNode altogether? That's consistent with > how the BookmarkModel names its methods. > Duh, this solution is so obviously right!
The CQ bit was checked by noyau@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/476573004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
On 2014/09/09 10:01:04, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_arm64_dbg_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) > android_chromium_gn_compile_rel on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) > android_clang_dbg_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) > android_dbg_tests_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) enhanced_bookmarks_bridge.cc is still using metadata_accessor.*. We can leave them for now and I'll clean up when I update the java bridge to use EnhancedBookmarkModel.
The CQ bit was checked by rfevang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/476573004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by rfevang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/476573004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as eaf16235f67b5aabb625c1cbc9908a11f6aef478
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a1bf3af9b614208729f14af603abd5e6b5bae698 Cr-Commit-Position: refs/heads/master@{#294028} |
