Most visited thumbnails and favicons need id-based urls

chrome/browser/instant/instant_service.h

Index: chrome/browser/instant/instant_service.h
diff --git a/chrome/browser/instant/instant_service.h b/chrome/browser/instant/instant_service.h
index 4e08d89e10c693e60eb5767f4f4fe81f108d4ef1..97fae83269a6c26018604b7d4ad332b6d837d6d1 100644
--- a/chrome/browser/instant/instant_service.h
+++ b/chrome/browser/instant/instant_service.h
@@ -5,7 +5,9 @@
 #ifndef CHROME_BROWSER_INSTANT_INSTANT_SERVICE_H_
 #define CHROME_BROWSER_INSTANT_INSTANT_SERVICE_H_
 
+#include <map>
 #include <set>
+#include <string>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
@@ -14,6 +16,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
+class GURL;
 class InstantIOContext;
 class Profile;
 
@@ -24,6 +27,13 @@ class InstantService : public ProfileKeyedService,
   explicit InstantService(Profile* profile);
   virtual ~InstantService();
 
+  // A utility to translate an Instant path if it is of restricted ID
+  // form.  If path is a restricted ID and we have a URL for it, then

[palmer, 2013/03/11 20:42:31]

What kind of path? What is in these strings?

[dhollowa, 2013/03/11 23:27:59]

Done.

+  // this URL is returned in string form.
+  static const std::string MaybeTranslateInstantPath(Profile* profile,

[palmer, 2013/03/11 20:42:31]

Can |profile| be const?

[dhollowa, 2013/03/11 23:27:59]

No, very difficult as mentioned.

+                                                     const std::string& path);
+  static bool IsInstantPath(const GURL& url);
+
   // Add, remove, and query RenderProcessHost IDs that are associated with
   // Instant processes.
   void AddInstantProcess(int process_id);
@@ -35,6 +45,18 @@ class InstantService : public ProfileKeyedService,
   }
 #endif
 
+  // If |url| is known the exiting restricted ID is returned.  Otherwise a new

[Evan Stade, 2013/03/11 21:01:27]

sp: existing

[dhollowa, 2013/03/11 23:27:59]

Done.

+  // restricted ID is associated with the |url| and returned.
+  uint64 AddURL(const GURL& url);
+
+  // Returns true if the |url| is known, and |restricted_id| is set.

[Evan Stade, 2013/03/11 19:47:08]

this comment format doesn't make sense to me. What is "known"? "is set" sounds
like it's something that may have happened before you pass the parameter.
"Returns false otherwise" is implied.

How about: If there is a mapping for |url|, sets |restricted_id| and returns
true.

[dhollowa, 2013/03/11 20:05:23]

Done.  And below.  (Thanks for the better wording.)

+  // Returns false otherwise.
+  bool GetRestrictedIDForURL(const GURL& url, uint64 *restricted_id);

[Evan Stade, 2013/03/11 19:47:08]

asterisk always on left

[dhollowa, 2013/03/11 20:05:23]

Done.

+
+  // Returns true if the |restricted_id| is known, and |url| is set.
+  // Returns false otherwise.
+  bool GetURLForRestrictedId(uint64 restricted_id, GURL* url);
+
  private:
   // Overridden from ProfileKeyedService:
   virtual void Shutdown() OVERRIDE;
@@ -49,6 +71,12 @@ class InstantService : public ProfileKeyedService,
   // The process ids associated with Instant processes.
   std::set<int> process_ids_;
 
+  // A mapping of restricted IDs to URLs.  Used to hide Most Visted and Favicon
+  // URLs from the Instant search provider.
+  uint64 last_url_id_;
+  std::map<uint64, GURL> id_to_url_map_;

[Evan Stade, 2013/03/11 19:47:08]

what kind of ID is this?

[dhollowa, 2013/03/11 20:05:23]

I'm not quite sure what you're asking here.  It is a restricted id.  Are you
looking for better comments, or objecting to the shortened member name and you'd
prefer |url_to_restricted_id_map_|?

[Evan Stade, 2013/03/11 21:01:27]

I don't know what a restricted ID is. By looking at other parts of your patch
(which would be hard to find once the code is committed), I find the comment:

// A private identifier used on the browser side when retrieving assets.

this provides some assistance, except it seems like the identifier is not
private; the instant page actually knows the ID but doesn't know what URL it
refers to. So something like opaque_id (with comments as to its purpose) would
make more sense, if I'm understanding it correctly.

[dhollowa, 2013/03/11 23:27:59]

I've changed to most_visited_item_id to make things clearer.   And renamed
methods accordingly.  I've left the 'opaque' terminology off as that seemed
implied by the 'id' part.

+  std::map<GURL, uint64> url_to_id_map_;
+
   content::NotificationRegistrar registrar_;
 
   scoped_refptr<InstantIOContext> instant_io_context_;