Set version field when changes are made to enhanced bookmarks.

components/enhanced_bookmarks/enhanced_bookmark_model.h

Index: components/enhanced_bookmarks/enhanced_bookmark_model.h
diff --git a/components/enhanced_bookmarks/enhanced_bookmark_model.h b/components/enhanced_bookmarks/enhanced_bookmark_model.h
new file mode 100644
index 0000000000000000000000000000000000000000..9a24f9d35c42d7dde51a9106be506d65439a2e14
--- /dev/null
+++ b/components/enhanced_bookmarks/enhanced_bookmark_model.h
@@ -0,0 +1,81 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_ENHANCED_BOOKMARKS_ENHANCED_BOOKMARK_MODEL_H_
+#define COMPONENTS_ENHANCED_BOOKMARKS_ENHANCED_BOOKMARK_MODEL_H_
+
+#include <string>
+
+class BookmarkModel;
+class BookmarkNode;
+class GURL;
+
+namespace enhanced_bookmarks {
+// Wrapper around BookmarkModel providing utility functions for enhanced
+// bookmarks.
+class EnhancedBookmarkModel {
+ public:
+  explicit EnhancedBookmarkModel(BookmarkModel* bookmark_model);

[noyau (Ping after 24h), 2014/08/26 11:17:47]

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.

[Yaron, 2014/08/26 16:55:22]

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

[Rune Fevang, 2014/08/26 18:47:52]

The plan was to make it a KeyedService in a follow-up CL where I provide an
extension function to call Initialize.

[Yaron, 2014/08/27 23:32:36]

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.

[Rune Fevang, 2014/08/27 23:52:22]

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.

[Rune Fevang, 2014/08/29 01:10:10]

Made it a KeyedService like we talked about.

+  ~EnhancedBookmarkModel();
+
+  void Initialize(const std::string& version);

[noyau (Ping after 24h), 2014/08/26 11:17:47]

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.

[Rune Fevang, 2014/08/26 18:47:52]

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.

[Yaron, 2014/08/27 23:32:36]

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?

[Rune Fevang, 2014/08/27 23:52:22]

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.

[Yaron, 2014/08/28 00:11:49]

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.

[Rune Fevang, 2014/08/28 00:30:22]

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.

[noyau (Ping after 24h), 2014/08/28 08:36:53]

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.

[Rune Fevang, 2014/08/29 01:10:10]

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.

+
+  // Returns the remote id for a bookmark.
+  std::string GetRemoteIdForNode(const BookmarkNode* node);
+
+  // Sets the description of a bookmark.
+  void SetDescriptionForNode(const BookmarkNode* node,
+                             const std::string& description);
+
+  // Returns the description of a bookmark.
+  std::string GetDescriptionForNode(const BookmarkNode* node);
+
+  // Sets the URL of an image representative of the page.
+  // Expects the URL to be valid and not empty.
+  // Returns true if the metainfo is successfully populated.
+  bool SetOriginalImageForNode(const BookmarkNode* node,
+                               const GURL& url,
+                               int width,
+                               int height);
+
+  // Returns the url and dimensions of the original scraped image.
+  // Returns true if the out variables are populated, false otherwise.
+  bool GetOriginalImageForNode(const BookmarkNode* node,
+                               GURL* url,
+                               int* width,
+                               int* height);
+
+  // Returns the url and dimensions of the server provided thumbnail image.
+  // Returns true if the out variables are populated, false otherwise.
+  bool GetThumbnailImageForNode(const BookmarkNode* node,
+                                GURL* url,
+                                int* width,
+                                int* height);
+
+  // Returns a brief server provided synopsis of the bookmarked page.
+  // Returns the empty string if the snippet could not be extracted.
+  std::string GetSnippetForNode(const BookmarkNode* node);
+
+  // Returns the version of the last enhanced bookmark client to change this
+  // node. Returns the empty string if the node doesn't have one set.
+  std::string GetVersionForNode(const BookmarkNode* node);
+
+ private:
+  // Generates and sets a remote id for the given bookmark. Returns the id set.
+  std::string SetRemoteIdForNode(const BookmarkNode* node);
+
+  // Helper method for setting a meta info field on a node. Also updates the
+  // version field.
+  void SetNodeMetaInfo(const BookmarkNode* node,
+                       const std::string& field,
+                       const std::string& value);
+
+  BookmarkModel* bookmark_model_;

[Yaron, 2014/08/27 23:32:36]

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.

[Rune Fevang, 2014/08/27 23:52:22]

Acknowledged.

+  bool initialized_;
+  std::string version_;
+};
+
+}  // namespace enhanced_bookmarks
+
+#endif  // COMPONENTS_ENHANCED_BOOKMARKS_ENHANCED_BOOKMARK_MODEL_H_