Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12498002:
InstantExtended: Adding InstantRestrictedIDCache. (Closed)
|
Created:
7 years, 9 months ago by Shishir Modified:
7 years, 9 months ago CC:
chromium-reviews, melevin, gideonwald, dominich, Aaron Boodman, David Black, samarth+watch_chromium.org, chromium-apps-reviews_chromium.org, darin-cc_chromium.org, Jered Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInstantExtended: Adding InstantRestrictedIDCache for caching and looking up restricted IDs.
In InstantExtended, cross origin iframes are used to display objects which can
only be referenced by the Instant page using an ID (restricted ID). These IDs
need to be unique and and cached for a while so that the SearchBox API can
fetch the object info based on the ID. The reason to use a cache of N items as
against just the last set of results is that there may be race conditions
between the internal state of the SearchBox and the data being displayed.
This CL introduces a template class InstantRestrictedIDCache that can be used
to generate the restricted IDs for objects, cache them and fetch objects based
on the IDs.
BUG=181870
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190052
Patch Set 1 #
Total comments: 32
Patch Set 2 : Adding new API to add items with restricted ID assigned. #Patch Set 3 : Merging David's and Sreeram's changes. #
Total comments: 69
Patch Set 4 : Addressing chris's and david's comments. #
Total comments: 26
Patch Set 5 : Addressing sreeram's comments. #Patch Set 6 : Merging conflicts. #
Total comments: 12
Patch Set 7 : Adding test and addressing comments.wq #Patch Set 8 : Minor test fix. #
Total comments: 2
Patch Set 9 : Using MRUCache to implement InstantRestrcitedIDCache. #
Total comments: 6
Patch Set 10 : Addressing chris's comments. #Patch Set 11 : Rebasing for commit. #Patch Set 12 : Fixing android compile error. #Messages
Total messages: 37 (0 generated)
PTAL
https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:18: // T needs to be copyable. nit: Please add documentation. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:25: // Adds items to the cache, assigning restricted ids in the process. nit: s/ids/IDs https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:28: // Returns the last set of items added to the cache. Please mention that this set will change upon the next call to |AddItems|. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:29: void GetCurrentItems(std::vector<std::pair<size_t, T> >* items) const; size_t seems wrong, it implies a size. int or int64 maybe? typedef int RestrictedID; maybe? https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:33: bool GetItemWithRestrictedId(size_t restricted_id, T* item) const; Is this only from the "current" items? https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:42: void AddItem(const T& item); This should be grouped with |AddItems|. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:44: std::deque<T> cache_; These should be private. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:58: CHECK(max_cache_size_); DCHECK since this is programmer error. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:77: DCHECK(items); no need for DCHECK since next line will crash. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:84: std::max(0, static_cast<int>(cache_.size() - last_addition_size_))); Seems like last_addition_size_ should never be greater than cache_.size(), no? And if not, then this should be DCHECK and not mutate with std::max. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:93: T* item) const { nit: indent https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... File chrome/common/instant_restricted_id_cache_unittest.cc (right): https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache_unittest.cc:14: TestData() { } nit: {} https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache_unittest.cc:15: explicit TestData(const std::string& i_value) : value(i_value) { } nit: {} https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache_unittest.cc:17: bool operator==(const TestData& rhs) const { Is this a requirement for any T? If so, this should be documented in the header. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache_unittest.cc:26: class InstantRestrictedIdCacheTest : public testing::Test { I usually just use a typedef in these simple cases. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache_unittest.cc:114: } nit: extra line
Last week when we discussed this, I forgot that, for most visited items, while
we generate the RIDs in the browser, we still have to send over the entire data
(with RIDs) to the renderer, in particular so that it can hand the title to the
page.
So, new requirements:
// T must be copyable.
template <class T>
class RestrictedIDCache {
public:
typedef uint64 RestrictedID;
typedef std::pair<RestrictedID, T> RestrictedIDItemPair;
explicit RestrictedIDCache(int max_items);
// May evict older items from the cache.
void AddItems(const std::vector<T>&);
// Will overwrite existing items with the same RestrictedID. May evict older
items from the cache.
void AddItemsWithRestrictedID(const std::vector<RestrictedIDItemPair>&);
// Returns false if no such item.
bool GetItemWithRestrictedID(RestrictedID, T*) const;
// Returns the last set of items added through AddItems() or
AddItemsWithRestrictedID().
void GetCurrentItems(std::vector<RestrictedIDItemPair>*);
};
On 2013/03/11 18:48:08, sreeram wrote: > void GetCurrentItems(std::vector<RestrictedIDItemPair>*); Should be const: void GetCurrentItems(std::vector<RestrictedIDItemPair>*) const;
For implementation, I suggest that we store a sorted list (or vector or whatever) of RestrictedIDItemPair, sorted by RID. Maybe we can add a constraint that AddItemsWithRestrictedID() must have entirely new RIDs, and always greater than the ones currently in the cache? (Then you can remove the "Will overwrite" comment.) In which case, both AddItems and AddItemsWithRestrictedID will just append to the sorted list. GetCurrentItems() returns the last appended set. GetItemWithRestrictedID is a log(n) lookup into the vector (binary search).
On 2013/03/11 18:58:07, sreeram wrote: > For implementation, I suggest that we store a sorted list (or vector or > whatever) of RestrictedIDItemPair, sorted by RID. Per offline discussion, Shishir suggested keeping the deque, with an addition private map that indexes from RID to a pointer within the deque. This sounds much better to me, since it doesn't impose the restrictions that a sorted vector would.
Made API changes as per discussion. Also addressed David's comments. PTAL. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:18: // T needs to be copyable. On 2013/03/06 02:12:58, dhollowa wrote: > nit: Please add documentation. Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:25: // Adds items to the cache, assigning restricted ids in the process. On 2013/03/06 02:12:58, dhollowa wrote: > nit: s/ids/IDs Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:28: // Returns the last set of items added to the cache. On 2013/03/06 02:12:58, dhollowa wrote: > Please mention that this set will change upon the next call to |AddItems|. Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:29: void GetCurrentItems(std::vector<std::pair<size_t, T> >* items) const; On 2013/03/06 02:12:58, dhollowa wrote: > size_t seems wrong, it implies a size. int or int64 maybe? typedef int > RestrictedID; maybe? Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:33: bool GetItemWithRestrictedId(size_t restricted_id, T* item) const; On 2013/03/06 02:12:58, dhollowa wrote: > Is this only from the "current" items? No. It can be any id. Top level comments should make this clearer. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:42: void AddItem(const T& item); On 2013/03/06 02:12:58, dhollowa wrote: > This should be grouped with |AddItems|. Function does not exist. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:44: std::deque<T> cache_; On 2013/03/06 02:12:58, dhollowa wrote: > These should be private. Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:58: CHECK(max_cache_size_); On 2013/03/06 02:12:58, dhollowa wrote: > DCHECK since this is programmer error. Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:77: DCHECK(items); On 2013/03/06 02:12:58, dhollowa wrote: > no need for DCHECK since next line will crash. Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:84: std::max(0, static_cast<int>(cache_.size() - last_addition_size_))); On 2013/03/06 02:12:58, dhollowa wrote: > Seems like last_addition_size_ should never be greater than cache_.size(), no? > And if not, then this should be DCHECK and not mutate with std::max. It can be greater. While retrieving we take care of it. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache.h:93: T* item) const { On 2013/03/06 02:12:58, dhollowa wrote: > nit: indent Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... File chrome/common/instant_restricted_id_cache_unittest.cc (right): https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache_unittest.cc:14: TestData() { } On 2013/03/06 02:12:58, dhollowa wrote: > nit: {} Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache_unittest.cc:15: explicit TestData(const std::string& i_value) : value(i_value) { } On 2013/03/06 02:12:58, dhollowa wrote: > nit: {} Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache_unittest.cc:17: bool operator==(const TestData& rhs) const { On 2013/03/06 02:12:58, dhollowa wrote: > Is this a requirement for any T? If so, this should be documented in the > header. Documented. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache_unittest.cc:26: class InstantRestrictedIdCacheTest : public testing::Test { On 2013/03/06 02:12:58, dhollowa wrote: > I usually just use a typedef in these simple cases. Done. https://codereview.chromium.org/12498002/diff/1/chrome/common/instant_restric... chrome/common/instant_restricted_id_cache_unittest.cc:114: } On 2013/03/06 02:12:58, dhollowa wrote: > nit: extra line Done.
Merged all RID generation/lookup into this CL. PTAL.
https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_io_context.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_io_context.cc:18: const int kMostVisitedItemCacheSize = 1000; You define this in two .cc files; should be in a shared .h to avoid the two definitions drifting apart. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_service.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:77: int restricted_id = 0; Should be InstantRestrictedID, not int. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:107: for (typename std::vector<RestrictedIDItemPair>::const_iterator it = NIT: I might make a more friendly-looking typedef for std::vector<RestrictedIDItemPair>::const_iterator. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:128: for (int i = start; i < static_cast<int>(cache_.size()); ++i) { static_cast is a sign that |start| and |i| should be size_t. :) And above too, in the call to std::max. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:139: typename std::map<InstantRestrictedID, RestrictedIDItemPair*>::const_iterator NIT: Consider a friendly typedef here too. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... File chrome/common/instant_types.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... chrome/common/instant_types.h:16: typedef int InstantRestrictedID; This should be an unsigned int (it never goes < 0, *and* it's used as an index into a vector). https://codereview.chromium.org/12498002/diff/14001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:17: const size_t kMaxResultsCacheSize = 100; Don't the other instances of this have it at 1000?
https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:201: InstantRestrictedID restricted_id, Keep |most_visited_item_id|. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:890: InstantRestrictedID restricted_id) { Keep |most_visited_item_id|. (Throughout). https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_controller.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.h:254: virtual void DeleteMostVisitedItem(InstantRestrictedID restricted_id) Lets keep |most_visited_item_id| for clarity. The type now makes it clear that this is a "restricted" ID. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.h:259: virtual void UndoMostVisitedDeletion(InstantRestrictedID restricted_id) Ditto. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_io_context.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_io_context.cc:18: const int kMostVisitedItemCacheSize = 1000; How are you picking this number? It seems too high. An extreme argument could be made to say make this "4", i.e. the number of items shown on the NTP. Since if the MV items change, they should update on all tabs, therefore we're only ever need 4 in the cache. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_io_context.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_io_context.h:57: const std::vector<InstantMostVisitedItemIDPair>& items); This can't be a const-ref since you'll have a dangling ref when the caller goes out of scope. Just make a copy here instead. i.e. |std::vector<IMVIIDPair> items|. This was a bug in the original code too. My bad. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_io_context.h:63: // Returns true if the |restricted_id| is known, and |url| is set. Returns From prior review, this comment was meant to change to: // If there is a mapping for the |most_visited_item_id|, sets |url| and // returns true. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_service.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:26: const int kMostVisitedItemCacheSize = 1000; This needs to be the same number as in instant_io_context.h or the IDs could mismatch due to overflow. Move this constant into a common area and use in both places. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:77: int restricted_id = 0; s/int/InstantRestrictedID/ https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:78: if (base::StringToInt(path, &restricted_id)) { DCHECK_EQ(sizeof(InstantRestrictedID), sizeof(unsigned)). Then if InstantRestrictedID changes size we'll catch it here. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:78: if (base::StringToInt(path, &restricted_id)) { StringToUint https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_service.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.h:65: // Returns the last added InstantMostVisitedItems. Mention that this is the way to get the newly assigned IDs. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.h:67: std::vector<InstantMostVisitedItemIDPair>* items); const method. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.h:69: // Returns true is an InstantMostVisitedItem is found with |restricted_id| and Please keep the "If ..., sets... and returns true." format of the original comment. It reads better. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.h:88: // A cache of the InstantMostVisitedItems sent to the Instant Pages. Please retain the original comments, there is important information there: (1) That the id/url mappings are determined here, and that the IDs are used to hide the urls. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:18: #include "content/public/common/page_transition_types.h" Needed? https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:21: // In InstantExtended, iframes are used to display objects which can only be Not only iframes, but Most Visited information. It would be good also to note that these iframes are used to display the Instant suggestions. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:28: // can also be added which already have IDs assigned, but the caller has to "but the caller has to ensure that the IDs do not repeat." This is not super-clear. How would I do this as a caller? Especially when the core purpose of this class is to assign/generate IDs for me? https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:45: // Adds items to the cache using the supplied restricted IDs. May delete The "May delete..." aspect of this has me a bit worried. Are we relying on the size of the cache and probabilistic behavior to avoid the race of using a stale ID before the new data has fully propagated? This seems pretty central to the reason-for-existence of this cache so should be documented at the top. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:55: bool GetItemWithRestrictedID(InstantRestrictedID restricted_id, This searches ALL items, not just "current" items, correct? Please add a note to that effect. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... File chrome/common/instant_types.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... chrome/common/instant_types.h:15: // visisted items) that the Instant page needs access to. nit: "Autocomplete results", "Most Visited items" https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... chrome/common/instant_types.h:16: typedef int InstantRestrictedID; unsigned int https://codereview.chromium.org/12498002/diff/14001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:17: const size_t kMaxResultsCacheSize = 100; Move to common place. https://codereview.chromium.org/12498002/diff/14001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:117: InstantRestrictedID restricted_id, s/restricted_id/autocomplete_item_id/ (Throughout).
PTAL https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:201: InstantRestrictedID restricted_id, On 2013/03/14 00:02:43, dhollowa wrote: > Keep |most_visited_item_id|. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:890: InstantRestrictedID restricted_id) { On 2013/03/14 00:02:43, dhollowa wrote: > Keep |most_visited_item_id|. (Throughout). Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_controller.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.h:254: virtual void DeleteMostVisitedItem(InstantRestrictedID restricted_id) On 2013/03/14 00:02:43, dhollowa wrote: > Lets keep |most_visited_item_id| for clarity. The type now makes it clear that > this is a "restricted" ID. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.h:259: virtual void UndoMostVisitedDeletion(InstantRestrictedID restricted_id) On 2013/03/14 00:02:43, dhollowa wrote: > Ditto. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_io_context.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_io_context.cc:18: const int kMostVisitedItemCacheSize = 1000; On 2013/03/14 00:02:43, dhollowa wrote: > How are you picking this number? It seems too high. An extreme argument could > be made to say make this "4", i.e. the number of items shown on the NTP. Since > if the MV items change, they should update on all tabs, therefore we're only > ever need 4 in the cache. We need a larger number because of the race conditions we talked about. 1000 does seem a bit high. Setting to 100. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_io_context.cc:18: const int kMostVisitedItemCacheSize = 1000; On 2013/03/13 23:50:37, Chris P. wrote: > You define this in two .cc files; should be in a shared .h to avoid the two > definitions drifting apart. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_io_context.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_io_context.h:57: const std::vector<InstantMostVisitedItemIDPair>& items); On 2013/03/14 00:02:43, dhollowa wrote: > This can't be a const-ref since you'll have a dangling ref when the caller goes > out of scope. Just make a copy here instead. i.e. |std::vector<IMVIIDPair> > items|. > > This was a bug in the original code too. My bad. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_io_context.h:63: // Returns true if the |restricted_id| is known, and |url| is set. Returns On 2013/03/14 00:02:43, dhollowa wrote: > From prior review, this comment was meant to change to: > > // If there is a mapping for the |most_visited_item_id|, sets |url| and > // returns true. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_service.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:26: const int kMostVisitedItemCacheSize = 1000; On 2013/03/14 00:02:43, dhollowa wrote: > This needs to be the same number as in instant_io_context.h or the IDs could > mismatch due to overflow. > > Move this constant into a common area and use in both places. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:77: int restricted_id = 0; On 2013/03/13 23:50:37, Chris P. wrote: > Should be InstantRestrictedID, not int. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:77: int restricted_id = 0; On 2013/03/14 00:02:43, dhollowa wrote: > s/int/InstantRestrictedID/ Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:78: if (base::StringToInt(path, &restricted_id)) { On 2013/03/14 00:02:43, dhollowa wrote: > StringToUint Please comment about unsigned. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:78: if (base::StringToInt(path, &restricted_id)) { On 2013/03/14 00:02:43, dhollowa wrote: > DCHECK_EQ(sizeof(InstantRestrictedID), sizeof(unsigned)). > Then if InstantRestrictedID changes size we'll catch it here. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_service.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.h:65: // Returns the last added InstantMostVisitedItems. On 2013/03/14 00:02:43, dhollowa wrote: > Mention that this is the way to get the newly assigned IDs. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.h:67: std::vector<InstantMostVisitedItemIDPair>* items); On 2013/03/14 00:02:43, dhollowa wrote: > const method. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.h:69: // Returns true is an InstantMostVisitedItem is found with |restricted_id| and On 2013/03/14 00:02:43, dhollowa wrote: > Please keep the "If ..., sets... and returns true." format of the original > comment. It reads better. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.h:88: // A cache of the InstantMostVisitedItems sent to the Instant Pages. On 2013/03/14 00:02:43, dhollowa wrote: > Please retain the original comments, there is important information there: (1) > That the id/url mappings are determined here, and that the IDs are used to hide > the urls. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:18: #include "content/public/common/page_transition_types.h" On 2013/03/14 00:02:43, dhollowa wrote: > Needed? Done. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:21: // In InstantExtended, iframes are used to display objects which can only be On 2013/03/14 00:02:43, dhollowa wrote: > Not only iframes, but Most Visited information. It would be good also to note > that these iframes are used to display the Instant suggestions. Even the Most visited Items is moving to iframes shortly. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:28: // can also be added which already have IDs assigned, but the caller has to On 2013/03/14 00:02:43, dhollowa wrote: > "but the caller has to ensure that the IDs do not repeat." This is not > super-clear. How would I do this as a caller? Especially when the core purpose > of this class is to assign/generate IDs for me? Done. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:45: // Adds items to the cache using the supplied restricted IDs. May delete On 2013/03/14 00:02:43, dhollowa wrote: > The "May delete..." aspect of this has me a bit worried. Are we relying on the > size of the cache and probabilistic behavior to avoid the race of using a stale > ID before the new data has fully propagated? > > This seems pretty central to the reason-for-existence of this cache so should be > documented at the top. Yes the cache is to avoid races. Added comment to top. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:55: bool GetItemWithRestrictedID(InstantRestrictedID restricted_id, On 2013/03/14 00:02:43, dhollowa wrote: > This searches ALL items, not just "current" items, correct? Please add a note > to that effect. Isnt this explained because we say "in the cache"? https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:107: for (typename std::vector<RestrictedIDItemPair>::const_iterator it = On 2013/03/13 23:50:37, Chris P. wrote: > NIT: I might make a more friendly-looking typedef for > std::vector<RestrictedIDItemPair>::const_iterator. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:128: for (int i = start; i < static_cast<int>(cache_.size()); ++i) { On 2013/03/13 23:50:37, Chris P. wrote: > static_cast is a sign that |start| and |i| should be size_t. :) And above too, > in the call to std::max. Like my comment from base/basictypes, we should be using ints in loops. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:139: typename std::map<InstantRestrictedID, RestrictedIDItemPair*>::const_iterator On 2013/03/13 23:50:37, Chris P. wrote: > NIT: Consider a friendly typedef here too. Done. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... File chrome/common/instant_types.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... chrome/common/instant_types.h:15: // visisted items) that the Instant page needs access to. On 2013/03/14 00:02:43, dhollowa wrote: > nit: "Autocomplete results", "Most Visited items" Done. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... chrome/common/instant_types.h:16: typedef int InstantRestrictedID; On 2013/03/13 23:50:37, Chris P. wrote: > This should be an unsigned int (it never goes < 0, *and* it's used as an index > into a vector). From "base/basictypes.h" // NOTE: unsigned types are DANGEROUS in loops and other arithmetical // places. Use the signed types unless your variable represents a bit // pattern (eg a hash value) or you really need the extra bit. Do NOT // use 'unsigned' to express "this value should always be positive"; // use assertions for this. I had made this uint first and then changed it after reading this. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... chrome/common/instant_types.h:16: typedef int InstantRestrictedID; On 2013/03/14 00:02:43, dhollowa wrote: > unsigned int Ditto. https://codereview.chromium.org/12498002/diff/14001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:17: const size_t kMaxResultsCacheSize = 100; On 2013/03/13 23:50:37, Chris P. wrote: > Don't the other instances of this have it at 1000? Done. https://codereview.chromium.org/12498002/diff/14001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:17: const size_t kMaxResultsCacheSize = 100; On 2013/03/14 00:02:43, dhollowa wrote: > Move to common place. Done for the most visited item. The Autocomplete cache is only instantiated here. https://codereview.chromium.org/12498002/diff/14001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:117: InstantRestrictedID restricted_id, On 2013/03/14 00:02:43, dhollowa wrote: > s/restricted_id/autocomplete_item_id/ (Throughout). Done.
https://codereview.chromium.org/12498002/diff/27001/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): https://codereview.chromium.org/12498002/diff/27001/chrome/chrome_common.gypi... chrome/chrome_common.gypi:285: 'chrome/common/instant_restricted_id_cache.h', Remove "chrome/" from the path. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:21: // referred by the Instant page using an ID (restricted ID). These IDs need to referred by -> referred to by or referred by -> referenced by https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:24: // use a cache of N items as aginst just the last set of results is that there aginst -> against https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:30: // 1. To store items and assign restrcited IDs to them. The cache will store restrcited -> restricted https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:31: // a max of |max_cache_size_| items and assign them unique ids. ids -> IDs https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:33: // 2. To store items that already have restrcited IDs assigned to them (like restrcited -> restricted (like -> (e.g., https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:35: // and does not make any garantees of the uniqueness of the IDs. If multiple garantees -> guarantees https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:36: // items are inserted with the same ID, the cahce cahce -> cache ... rest of the comment? https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:58: // |AddItemsWithRestrictedId|. |AddItems| -> AddItems() |AddItemsWithRestrictedId| -> AddItemsWithRestrictedID() (pipes are only for parameters and data members, not functions/methods or class names.) https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:78: std::deque<ItemIDPair*> cache_; std::list<ItemIDPair> seems like a better choice here. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:79: STLElementDeleter<std::deque<ItemIDPair*> > cache_deleter_; This doesn't seem necessary. If you want to ensure that stuff gets deleted, you can do so in the destructor. No? https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:80: std::map<InstantRestrictedID, ItemIDPair*> restricted_id_item_map_; ItemIDMap item_id_map_; https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_typ... File chrome/common/instant_types.h (right): https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_typ... chrome/common/instant_types.h:8: #include <string> #include <utility>
lgtm https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_io_context.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_io_context.cc:18: const int kMostVisitedItemCacheSize = 1000; On 2013/03/14 19:53:03, Shishir wrote: > On 2013/03/13 23:50:37, Chris P. wrote: > > You define this in two .cc files; should be in a shared .h to avoid the two > > definitions drifting apart. > > Done. This should be removed in favor of the common definition. https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... File chrome/browser/instant/instant_service.cc (right): https://codereview.chromium.org/12498002/diff/14001/chrome/browser/instant/in... chrome/browser/instant/instant_service.cc:78: if (base::StringToInt(path, &restricted_id)) { On 2013/03/14 19:53:03, Shishir wrote: > On 2013/03/14 00:02:43, dhollowa wrote: > > StringToUint > > Please comment about unsigned. I like int. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:21: // In InstantExtended, iframes are used to display objects which can only be On 2013/03/14 19:53:03, Shishir wrote: > On 2013/03/14 00:02:43, dhollowa wrote: > > Not only iframes, but Most Visited information. It would be good also to note > > that these iframes are used to display the Instant suggestions. > > Even the Most visited Items is moving to iframes shortly. Ya, but the MV items will still have IDs for the thumbnail urls, no? The iframes won't have src=chrome://... right? So they'll still need thumbnails/favicons of the form src=chrome-search://thumb/<id>, correct? https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:55: bool GetItemWithRestrictedID(InstantRestrictedID restricted_id, On 2013/03/14 19:53:03, Shishir wrote: > On 2013/03/14 00:02:43, dhollowa wrote: > > This searches ALL items, not just "current" items, correct? Please add a note > > to that effect. > > Isnt this explained because we say "in the cache"? Ok. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... File chrome/common/instant_types.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... chrome/common/instant_types.h:16: typedef int InstantRestrictedID; On 2013/03/14 19:53:03, Shishir wrote: > On 2013/03/13 23:50:37, Chris P. wrote: > > This should be an unsigned int (it never goes < 0, *and* it's used as an index > > into a vector). > > From "base/basictypes.h" > > // NOTE: unsigned types are DANGEROUS in loops and other arithmetical > // places. Use the signed types unless your variable represents a bit > // pattern (eg a hash value) or you really need the extra bit. Do NOT > // use 'unsigned' to express "this value should always be positive"; > // use assertions for this. > > I had made this uint first and then changed it after reading this. > > I'm fine with int.
https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... File chrome/common/instant_types.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... chrome/common/instant_types.h:16: typedef int InstantRestrictedID; > > // NOTE: unsigned types are DANGEROUS in loops and other arithmetical > > // places. Use the signed types unless your variable represents a bit > > // pattern (eg a hash value) or you really need the extra bit. Do NOT > > // use 'unsigned' to express "this value should always be positive"; > > // use assertions for this. That comment is wrong. The problem is when you compare types of different signedness, not merely the use of unsigned types. It's especially ironic that they say unsigned types are dangerous in other arithmetical places" — signed types have *undefined* behavior when the arithmetic overflows; only unsigned types have defined behavior. Unsigned integers are the *correct* and *safe* types to use for sizes, offsets, array and vector indices, and so on. Signed types are affirmatively *unsafe* in those uses.
PTAL. https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... File chrome/common/instant_types.h (right): https://codereview.chromium.org/12498002/diff/14001/chrome/common/instant_typ... chrome/common/instant_types.h:16: typedef int InstantRestrictedID; On 2013/03/15 01:47:53, Chris P. wrote: > > > // NOTE: unsigned types are DANGEROUS in loops and other arithmetical > > > // places. Use the signed types unless your variable represents a bit > > > // pattern (eg a hash value) or you really need the extra bit. Do NOT > > > // use 'unsigned' to express "this value should always be positive"; > > > // use assertions for this. > > That comment is wrong. The problem is when you compare types of different > signedness, not merely the use of unsigned types. > > It's especially ironic that they say unsigned types are dangerous in other > arithmetical places" — signed types have *undefined* behavior when the > arithmetic overflows; only unsigned types have defined behavior. > > Unsigned integers are the *correct* and *safe* types to use for sizes, offsets, > array and vector indices, and so on. Signed types are affirmatively *unsafe* in > those uses. With the current code there is no strict requirement that the number can't be negative. There are no array like accesses any more, no std::max kind of stuff so I think we can leave it an int. https://codereview.chromium.org/12498002/diff/27001/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): https://codereview.chromium.org/12498002/diff/27001/chrome/chrome_common.gypi... chrome/chrome_common.gypi:285: 'chrome/common/instant_restricted_id_cache.h', On 2013/03/14 23:05:36, sreeram wrote: > Remove "chrome/" from the path. Done. Wonder how it compiled with this? https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:21: // referred by the Instant page using an ID (restricted ID). These IDs need to On 2013/03/14 23:05:36, sreeram wrote: > referred by -> referred to by > or > referred by -> referenced by Done. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:24: // use a cache of N items as aginst just the last set of results is that there On 2013/03/14 23:05:36, sreeram wrote: > aginst -> against Done. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:30: // 1. To store items and assign restrcited IDs to them. The cache will store On 2013/03/14 23:05:36, sreeram wrote: > restrcited -> restricted Done. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:31: // a max of |max_cache_size_| items and assign them unique ids. On 2013/03/14 23:05:36, sreeram wrote: > ids -> IDs Done. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:33: // 2. To store items that already have restrcited IDs assigned to them (like On 2013/03/14 23:05:36, sreeram wrote: > restrcited -> restricted > (like -> (e.g., Sorry, new keyboard! https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:35: // and does not make any garantees of the uniqueness of the IDs. If multiple On 2013/03/14 23:05:36, sreeram wrote: > garantees -> guarantees Done. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:36: // items are inserted with the same ID, the cahce On 2013/03/14 23:05:36, sreeram wrote: > cahce -> cache > ... rest of the comment? Done. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:58: // |AddItemsWithRestrictedId|. On 2013/03/14 23:05:36, sreeram wrote: > |AddItems| -> AddItems() > |AddItemsWithRestrictedId| -> AddItemsWithRestrictedID() > > (pipes are only for parameters and data members, not functions/methods or class > names.) Done. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:78: std::deque<ItemIDPair*> cache_; On 2013/03/14 23:05:36, sreeram wrote: > std::list<ItemIDPair> seems like a better choice here. There was array access requirement when this CL started, which isnt present now. Changed to list. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:79: STLElementDeleter<std::deque<ItemIDPair*> > cache_deleter_; On 2013/03/14 23:05:36, sreeram wrote: > This doesn't seem necessary. If you want to ensure that stuff gets deleted, you > can do so in the destructor. No? Can do that too. But this is an equally good approach, NO? https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:80: std::map<InstantRestrictedID, ItemIDPair*> restricted_id_item_map_; On 2013/03/14 23:05:36, sreeram wrote: > ItemIDMap item_id_map_; Done. https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_typ... File chrome/common/instant_types.h (right): https://codereview.chromium.org/12498002/diff/27001/chrome/common/instant_typ... chrome/common/instant_types.h:8: #include <string> On 2013/03/14 23:05:36, sreeram wrote: > #include <utility> Done.
Friendly ping. This will block my iframes CL.
On 2013/03/19 19:16:18, Shishir wrote: > Friendly ping. This will block my iframes CL. Reviewing now...
lgtm https://codereview.chromium.org/12498002/diff/46001/chrome/browser/search/ins... File chrome/browser/search/instant_io_context.cc (right): https://codereview.chromium.org/12498002/diff/46001/chrome/browser/search/ins... chrome/browser/search/instant_io_context.cc:18: const int kMostVisitedItemCacheSize = 100; Not being used. Can be removed. https://codereview.chromium.org/12498002/diff/46001/chrome/browser/search/ins... File chrome/browser/search/instant_service.h (right): https://codereview.chromium.org/12498002/diff/46001/chrome/browser/search/ins... chrome/browser/search/instant_service.h:59: ///// Most visited item API. Too many slashes. https://codereview.chromium.org/12498002/diff/46001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/46001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:31: // 2. To store items that already have restrcited IDs assigned to them (e.g. restrcited -> restricted https://codereview.chromium.org/12498002/diff/46001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:57: // AddItemsWithRestrictedId(). AddItemsWithRestrictedId -> AddItemsWithRestrictedID https://codereview.chromium.org/12498002/diff/46001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.h (right): https://codereview.chromium.org/12498002/diff/46001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.h:81: bool GetAutocompleteResultWithId(InstantRestrictedID autocomplete_result_id, GetAutocompleteResultWithId -> GetAutocompleteResultWithID
https://codereview.chromium.org/12498002/diff/46001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache_unittest.cc (right): https://codereview.chromium.org/12498002/diff/46001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache_unittest.cc:246: EXPECT_FALSE(cache.GetItemWithRestrictedID(7, &t)); Could you please add some tests that get and set very silly ID values, like -1, -2, 0x7fffffff, and so on? That will make me feel better about indexing/searching the vector. :)
Adding sky for chrome/common/... approval. PTAL. https://codereview.chromium.org/12498002/diff/46001/chrome/browser/search/ins... File chrome/browser/search/instant_io_context.cc (right): https://codereview.chromium.org/12498002/diff/46001/chrome/browser/search/ins... chrome/browser/search/instant_io_context.cc:18: const int kMostVisitedItemCacheSize = 100; On 2013/03/19 21:36:17, sreeram wrote: > Not being used. Can be removed. Done. https://codereview.chromium.org/12498002/diff/46001/chrome/browser/search/ins... File chrome/browser/search/instant_service.h (right): https://codereview.chromium.org/12498002/diff/46001/chrome/browser/search/ins... chrome/browser/search/instant_service.h:59: ///// Most visited item API. On 2013/03/19 21:36:17, sreeram wrote: > Too many slashes. Done. https://codereview.chromium.org/12498002/diff/46001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/46001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:31: // 2. To store items that already have restrcited IDs assigned to them (e.g. On 2013/03/19 21:36:17, sreeram wrote: > restrcited -> restricted Done. https://codereview.chromium.org/12498002/diff/46001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:57: // AddItemsWithRestrictedId(). On 2013/03/19 21:36:17, sreeram wrote: > AddItemsWithRestrictedId -> AddItemsWithRestrictedID Done. https://codereview.chromium.org/12498002/diff/46001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache_unittest.cc (right): https://codereview.chromium.org/12498002/diff/46001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache_unittest.cc:246: EXPECT_FALSE(cache.GetItemWithRestrictedID(7, &t)); On 2013/03/19 21:48:30, Chris P. wrote: > Could you please add some tests that get and set very silly ID values, like -1, > -2, 0x7fffffff, and so on? That will make me feel better about > indexing/searching the vector. :) Done. https://codereview.chromium.org/12498002/diff/46001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.h (right): https://codereview.chromium.org/12498002/diff/46001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.h:81: bool GetAutocompleteResultWithId(InstantRestrictedID autocomplete_result_id, On 2013/03/19 21:36:17, sreeram wrote: > GetAutocompleteResultWithId -> GetAutocompleteResultWithID Done.
sky->brettw Brett, Shishir needs review of changes to chrome/common.
We're trying to get better CL descriptions. This description doesn't really have any info on why the change was made and what it does. The bug is also fairly spare. Can you expand on this so that somebody looking at the log can tell what you're doing?
On 2013/03/20 20:52:46, brettw wrote: > We're trying to get better CL descriptions. This description doesn't really have > any info on why the change was made and what it does. The bug is also fairly > spare. Can you expand on this so that somebody looking at the log can tell what > you're doing? Updated description. PTAL.
https://codereview.chromium.org/12498002/diff/61001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/61001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:156: while (cache_.size() > max_cache_size_) { I wonder if this could be done using an MRUCache to avoid writing so much custom logic? base/containers/mru_cache.h. It seems that this class is just an MRUCache with key autogeneration.
https://codereview.chromium.org/12498002/diff/61001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/61001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:156: while (cache_.size() > max_cache_size_) { On 2013/03/20 21:53:53, brettw wrote: > I wonder if this could be done using an MRUCache to avoid writing so much custom > logic? base/containers/mru_cache.h. It seems that this class is just an MRUCache > with key autogeneration. This is not a strict MRU cache, but it definitely looks like it could be done on top of the MRUCache (thanks to the peek method), but we would still have to write custom code to deal with ID generation. All the functions except MaybeDropItemsFromCache will still have to be implemented. We will have the simplicity of not having a map and a list but we would have to maintain additional data to support the use case for GetCurrentItems.
Hi Brett, I change the cache to use an MRUCache as you suggested. PTAL. Thanks.
On 2013/03/21 06:53:05, Shishir wrote: > Hi Brett, > > I change the cache to use an MRUCache as you suggested. PTAL. > > Thanks. Hi Brett, Could you please take a look. This CL blocks another that we need to get in before the monday branch point. Thanks.
https://codereview.chromium.org/12498002/diff/78001/chrome/browser/search/ins... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/12498002/diff/78001/chrome/browser/search/ins... chrome/browser/search/instant_service.cc:103: return base::StringToUint64(path, &dummy); This code should be updated too. We could end up with a URL that passes this test yet which cannot be parsed as a true instant URL, because base::StringToUint64 handles a different range than base::StringToInt. https://codereview.chromium.org/12498002/diff/78001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/78001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:38: class InstantRestrictedIDCache { This seems large and inefficient. The contract described in the documentation could be implemented with a thin wrapper around a std::vector<T>, as far as I can tell. Dumb linear search doesn't really get bad until max_cache_size gets bigger than this class' callers intend, as far as I understand. Do we intend to store more than, say, 100 items in this cache? https://codereview.chromium.org/12498002/diff/78001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/12498002/diff/78001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:17: const size_t kMaxInstantAutocompleteResultItemCacheSize = 100; Yes, given this, linear search is not bad (and possibly even preferable, given the low constant factor of indexing a vector).
https://codereview.chromium.org/12498002/diff/78001/chrome/browser/search/ins... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/12498002/diff/78001/chrome/browser/search/ins... chrome/browser/search/instant_service.cc:103: return base::StringToUint64(path, &dummy); On 2013/03/21 22:58:04, Chris P. wrote: > This code should be updated too. We could end up with a URL that passes this > test yet which cannot be parsed as a true instant URL, because > base::StringToUint64 handles a different range than base::StringToInt. Done. https://codereview.chromium.org/12498002/diff/78001/chrome/common/instant_res... File chrome/common/instant_restricted_id_cache.h (right): https://codereview.chromium.org/12498002/diff/78001/chrome/common/instant_res... chrome/common/instant_restricted_id_cache.h:38: class InstantRestrictedIDCache { On 2013/03/21 22:58:04, Chris P. wrote: > This seems large and inefficient. The contract described in the documentation > could be implemented with a thin wrapper around a std::vector<T>, as far as I > can tell. Dumb linear search doesn't really get bad until max_cache_size gets > bigger than this class' callers intend, as far as I understand. Do we intend to > store more than, say, 100 items in this cache? The class did start off being a wrapper over a list and evolved to use the MRUCache which as brett pointed out did much of what we need. Give that we will be storing 100 elements at max, and the extra memory is a map with 100 ints and iterators, is it very inefficient? Are we really worried about the memory overhead here? https://codereview.chromium.org/12498002/diff/78001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/12498002/diff/78001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:17: const size_t kMaxInstantAutocompleteResultItemCacheSize = 100; On 2013/03/21 22:58:04, Chris P. wrote: > Yes, given this, linear search is not bad (and possibly even preferable, given > the low constant factor of indexing a vector). Please see reply in the restricted_id.h file.
lgtm
owners lgtm but I don't understand the big picture, nor did I check the small details.
On 2013/03/22 04:33:55, brettw wrote: > owners lgtm but I don't understand the big picture, nor did I check the small > details. Hi Brett, The code in chrome/common has been reviewed by sreeram. As to why we are doing this, I will follow up with you over email and make any design changes you recommend in a following CL. I will go ahead an commit this since it blocks another CL. Thanks
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/12498002/88001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/12498002/88001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/12498002/85021
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/12498002/85021
Message was sent while issue was closed.
Change committed as 190052 |
