Most visited thumbnails and favicons need id-based urls

chrome/browser/instant/instant_service.h

 // Copyright 2013 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 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"
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/profiles/profile_keyed_service.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
+class GURL;
 class InstantIOContext;
 class Profile;
 
 // Tracks render process host IDs that are associated with Instant.
 class InstantService : public ProfileKeyedService,
                        public content::NotificationObserver {
  public:
   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);
   bool IsInstantProcess(int process_id) const;
 
 #if defined(UNIT_TEST)
   int GetInstantProcessCount() const {
     return process_ids_.size();
   }
 #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;
 
   // Overridden from content::NotificationObserver:
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
   Profile* const profile_;
 
   // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(InstantService);
 };
 
 #endif  // CHROME_BROWSER_INSTANT_INSTANT_SERVICE_H_