Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
299713007:
[Suggestions] Adding a Thumbnail Manager for the Suggestions Service (Closed)
|
Created:
6 years, 7 months ago by Mathieu Modified:
6 years, 7 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Suggestions] Adding a Thumbnail Manager for the Suggestions Service
Will fetch server thumbnails if they are available. Using the BitmapFetcher to obtain and decode images.
I've tested with a subsequent CL that modifies suggestions_source.cc.
Next CLs will implement local caching once the image is fetched.
BUG=None
TEST=ThumbnailManagerTest,ThumbnailManagerBrowserTest
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273012
Patch Set 1 #Patch Set 2 : clean #
Total comments: 34
Patch Set 3 : addressed comments #
Total comments: 44
Patch Set 4 : Addressed comments #Patch Set 5 : removed C++11 statement #
Total comments: 36
Patch Set 6 : latest round #
Total comments: 4
Patch Set 7 : nits #Patch Set 8 : clang quirk #
Total comments: 5
Patch Set 9 : remove typedef #Patch Set 10 : delete pointer on destruction #
Total comments: 4
Patch Set 11 : no guard #Patch Set 12 : remove comment #
Total comments: 3
Patch Set 13 : Now swapping #Messages
Total messages: 29 (0 generated)
Hi, this is ready for a review. I was thinking Sam: Main review (familiar with code/goal) Jered: Approval
First round. My recommendations in thumbnail_manager.cc might be irrelevant if you decide to implement what I wrote in thumbnail_manager.h... https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/proto/suggestions.proto (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/proto/suggestions.proto:29: // The url of the favicon associated with this page. NIT: s/url/URL/ in comment for consistency with file. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/suggestions_service.h (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/suggestions_service.h:50: void GetURLThumbnail( Suggest that you shorten this to: // Retrieves stored thumbnail for |url| asynchronously. Returns thumbnail image a Bitmap pointer to |callback| if found, and NULL otherwise. Also, you can unwrap the 2 parameters to be aligned at "(". https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:24: if (suggestion.has_thumbnail()) { What if there are multiple suggestions with the same URL? Also, some may have thumbnails and some may not. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:45: if (pending_callbacks_[thumbnail_url].size() == 1) { I'd prefer using queue<>::empty() instead of checking for size == 1. How about: // Allocates a new queue if none exists. std::queue<BitmapResponseCallback>* pending_callback = &pending_callbacks_[thumbnail_url]; // Fetch request if this is no in-flight request for |thumbnail_url|. bool fetch_required = pending_callback->empty(); // Enqueue |callback| before fetching to be robust against race condition. pending_callback->push(callback); if (fetch_required) { ... } https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:50: url_request_context_, std::string(), NIT: 1 parameter per line. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:59: scoped_ptr<chrome::BitmapFetcher> fetcher_destructor( You only need to do one lookup if you store the iterator: std::map<GURL, std::pair<GURL, chrome::BitmapFetcher*>>::iterator it = fetcher_map_.find(thumbnail_url); // Make sure the BitmapFetcher gets destroyed. scoped_ptr<chrome::BitmapFetcher> fetcher_destructor(it->second); GURL url(it->first); fetcher_map_.erase(it); https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:67: std::queue<BitmapResponseCallback> callbacks = I think it's cleaner to just swap data data: std::queue<BitmapResponseCallback> callbacks; callbacks.swap(pending_callbacks_[thumbnail_url]); I'm not sure if this works on all platform though. http://www.cplusplus.com/ says this is C++11... https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:73: pending_callbacks_.erase(pending_callbacks_.find(thumbnail_url)); You can just direct erase with the key: pending_callbacks_.erase(thumbnail_url); https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:8: #include <memory> #include <map> #include <utility> (for std::pair) Probably don't need <memory> and <vector>. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:24: // An interface to maintain and fetch server thumbnails asynchronously. NIT: "base class" instead of interface? To me interface sounds like a class whose member functions are all virtual, and has no member variables. This class is not thread-safe. Perhaps state on which thread is this meant to be run? https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:37: // For a given |url|, if a thumbnail URL is known for it, will start the fetch Suggested comment: // Retrieves stored thumbnail for |url| asynchronously. Returns thumbnail image a Bitmap pointer to |callback| if found, and NULL otherwise. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:66: // Map from each thumbnail URL to a queue of pending callbacks. Both |pending_callbacks_| and |fetcher_map_| use the same keys. Perhaps combine the two structures, and define: class ThumbnailManagerSession { public: GURL url; scoped_ptr<chrome::BitmapFetcher> fetcher; std::queue<BitmapResponseCallback> callbacks; private: DISALLOW_COPY_AND_ASSIGN(ThumbnailManagerSession); }; then have std::map<GURL, ThumbnailManagerSession> session_map_; Another advantage is that deallocation of fetcher is automatic on destruction of this class, and when you do session_map_.erase(url); https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:73: Profile* profile_; |profile_| seems to be unused? https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:69: } // namespace. NIT: don't need period at end. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:157: } // namespace suggestions. No period at end. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_unittest.cc:24: } // namespace. No period at end. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_unittest.cc:46: } // namespace suggestions. No period at end.
Thanks for your comments and help, have a look. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/proto/suggestions.proto (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/proto/suggestions.proto:29: // The url of the favicon associated with this page. On 2014/05/22 06:17:35, huangs1 wrote: > NIT: s/url/URL/ in comment for consistency with file. Done. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/suggestions_service.h (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/suggestions_service.h:50: void GetURLThumbnail( On 2014/05/22 06:17:35, huangs1 wrote: > Suggest that you shorten this to: > > // Retrieves stored thumbnail for |url| asynchronously. Returns thumbnail image > a Bitmap pointer to |callback| if found, and NULL otherwise. > > Also, you can unwrap the 2 parameters to be aligned at "(". Done. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:24: if (suggestion.has_thumbnail()) { On 2014/05/22 06:17:35, huangs1 wrote: > What if there are multiple suggestions with the same URL? Also, some may have > thumbnails and some may not. I feel like the server should worry about this. In our current version it is not possible. I'm comfortable with undefined behavior on the client (wrt which would get a thumbnail), but thanks for bringing it up. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:45: if (pending_callbacks_[thumbnail_url].size() == 1) { On 2014/05/22 06:17:35, huangs1 wrote: > I'd prefer using queue<>::empty() instead of checking for size == 1. How about: > > // Allocates a new queue if none exists. > std::queue<BitmapResponseCallback>* pending_callback = > &pending_callbacks_[thumbnail_url]; > > // Fetch request if this is no in-flight request for |thumbnail_url|. > bool fetch_required = pending_callback->empty(); > > // Enqueue |callback| before fetching to be robust against race condition. > pending_callback->push(callback); > > if (fetch_required) { > ... > } Reworked this, let me know https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:50: url_request_context_, std::string(), On 2014/05/22 06:17:35, huangs1 wrote: > NIT: 1 parameter per line. clang-format would disagree! https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:59: scoped_ptr<chrome::BitmapFetcher> fetcher_destructor( On 2014/05/22 06:17:35, huangs1 wrote: > You only need to do one lookup if you store the iterator: > > std::map<GURL, std::pair<GURL, chrome::BitmapFetcher*>>::iterator it = > fetcher_map_.find(thumbnail_url); > // Make sure the BitmapFetcher gets destroyed. > scoped_ptr<chrome::BitmapFetcher> fetcher_destructor(it->second); > GURL url(it->first); > fetcher_map_.erase(it); Done. Have a look! https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:67: std::queue<BitmapResponseCallback> callbacks = On 2014/05/22 06:17:35, huangs1 wrote: > I think it's cleaner to just swap data data: > > std::queue<BitmapResponseCallback> callbacks; > callbacks.swap(pending_callbacks_[thumbnail_url]); > > I'm not sure if this works on all platform though. http://www.cplusplus.com/ > says this is C++11... Done. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:73: pending_callbacks_.erase(pending_callbacks_.find(thumbnail_url)); On 2014/05/22 06:17:35, huangs1 wrote: > You can just direct erase with the key: > > pending_callbacks_.erase(thumbnail_url); Have a look at how it is now? https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:8: #include <memory> On 2014/05/22 06:17:35, huangs1 wrote: > #include <map> > > #include <utility> (for std::pair) > > Probably don't need <memory> and <vector>. Done. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:24: // An interface to maintain and fetch server thumbnails asynchronously. On 2014/05/22 06:17:35, huangs1 wrote: > NIT: "base class" instead of interface? To me interface sounds like a class > whose member functions are all virtual, and has no member variables. > > This class is not thread-safe. Perhaps state on which thread is this meant to > be run? I stated it below in a comment to GetPageThumbnail (previously GetURLThumbnail) https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:37: // For a given |url|, if a thumbnail URL is known for it, will start the fetch On 2014/05/22 06:17:35, huangs1 wrote: > Suggested comment: > > // Retrieves stored thumbnail for |url| asynchronously. Returns thumbnail image > a Bitmap pointer to |callback| if found, and NULL otherwise. Done. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:66: // Map from each thumbnail URL to a queue of pending callbacks. On 2014/05/22 06:17:35, huangs1 wrote: > Both |pending_callbacks_| and |fetcher_map_| use the same keys. > > Perhaps combine the two structures, and define: > > class ThumbnailManagerSession { > public: > GURL url; > scoped_ptr<chrome::BitmapFetcher> fetcher; > std::queue<BitmapResponseCallback> callbacks; > > private: > DISALLOW_COPY_AND_ASSIGN(ThumbnailManagerSession); > }; > > then have > > std::map<GURL, ThumbnailManagerSession> session_map_; > > Another advantage is that deallocation of fetcher is automatic on destruction of > this class, and when you do > > session_map_.erase(url); Done. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:73: Profile* profile_; On 2014/05/22 06:17:35, huangs1 wrote: > |profile_| seems to be unused? Done. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:69: } // namespace. On 2014/05/22 06:17:35, huangs1 wrote: > NIT: don't need period at end. Done. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:157: } // namespace suggestions. On 2014/05/22 06:17:35, huangs1 wrote: > No period at end. Done. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_unittest.cc:24: } // namespace. On 2014/05/22 06:17:35, huangs1 wrote: > No period at end. Done. https://chromiumcodereview.appspot.com/299713007/diff/20001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_unittest.cc:46: } // namespace suggestions. On 2014/05/22 06:17:35, huangs1 wrote: > No period at end. Done.
More comments. I'm concerned about thread safety. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... File chrome/browser/search/suggestions/proto/suggestions.proto (right): https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/proto/suggestions.proto:32: // Specification for the thumbnail to be used. Is thumbnail an URL? Please state if so, else mention what's the format. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... File chrome/browser/search/suggestions/suggestions_service.h (right): https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/suggestions_service.h:16: #include "chrome/browser/search/suggestions/thumbnail_manager.h" Prefer to forward-declare ThumbnailManager; move the #include to the .cc file. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:19: ThumbnailManager::Session::Session() {} Can erase these, per. comment in .h file. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:35: BitmapResponseCallback callback) { #include "content/public/browser/browser_thread.h" ... DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:36: // If the |url| is not in the |thumbnail_map_|, there is no associated Perhaps: // If |url| is not found in |thumbnail_map_|, then invoke |callback| with NULL since there is no associated thumbnail. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:44: // Get or create the ThumbnailManagerSession. In the case of a creation, // Look for a fetch in progress for |thumbnail_url|. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:48: // There are no fetches in progress for |thumbnail_url|. Create a session // |thumbnail_url| is not being fetched, so create a session and initiate the fetch. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:53: session->fetcher_ = new chrome::BitmapFetcher(thumbnail_url, this); // This is deallocated in OnFetchComplete(). https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:89: // Not found. NIT: can be shortened to 2 liens: if (it == thumbnail_map_.end()) return false; // Not found. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:26: NIT: remove extra empty lines. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:35: // Initialize the |thumbnail_map_| with the proper mapping from URL to NIT: s/Initialize/Initializes/ https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:37: void InitializeThumbnailMap(const SuggestionsProfile& suggestions); Should this routine only be called once? What if it's called while there are pending requests? https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:45: virtual void OnFetchComplete(const GURL thumbnail_url, I suspect this is not called from the UI thread? If this were the case, then GetPageThumbnail() and OnFetchComplete() may conflict over |session_map_|. To solve this, you can add Mutex to safeguard |session_map_|. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:55: // Contains the information related with a thumbnail fetch (fetcher, pending also mention |url|? https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:57: class Session { Since Session doesn't do anything interesting anymore, may as well make it a struct, and get rid of consturctors / destructors. Also, public member variables don't have to end with "_". https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:64: std::queue<BitmapResponseCallback> callbacks_; // Queue for pending callbacks, which may accumulate while the request is in flight. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:69: // Given a website |url|, fill in a |thumbnail_url| and return whether it // Looks up thumbnail for |url|. If found, writes the result to |thumbnail_url| and returns true. Otherwise just returns false. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:82: // Map from each thumbnail URL to the session information (fetcher, url is also part of session information. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:84: thumbnail_manager.GetPageThumbnail( // Fetch existing URL. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:94: thumbnail_manager.GetPageThumbnail( // Fetch non-existing URL. https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:116: thumbnail_manager.GetPageThumbnail( // Fetch non-existing URL, and add more while request is in flight. Put into for loop? https://chromiumcodereview.appspot.com/299713007/diff/40001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:141: TestingProfile profile; // Fetch existing URL that has invalid thumbnail.
Thanks! https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... File chrome/browser/search/suggestions/proto/suggestions.proto (right): https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/proto/suggestions.proto:32: // Specification for the thumbnail to be used. On 2014/05/22 18:08:22, huangs1 wrote: > Is thumbnail an URL? Please state if so, else mention what's the format. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:16: #include "chrome/browser/search/suggestions/thumbnail_manager.h" On 2014/05/22 18:08:22, huangs1 wrote: > Prefer to forward-declare ThumbnailManager; move the #include to the .cc file. Can't because of BitmapResponseCallback https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.cc:19: ThumbnailManager::Session::Session() {} On 2014/05/22 18:08:22, huangs1 wrote: > Can erase these, per. comment in .h file. I don't think so :/ https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.cc:35: BitmapResponseCallback callback) { On 2014/05/22 18:08:22, huangs1 wrote: > #include "content/public/browser/browser_thread.h" > > ... > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.cc:36: // If the |url| is not in the |thumbnail_map_|, there is no associated On 2014/05/22 18:08:22, huangs1 wrote: > Perhaps: > > // If |url| is not found in |thumbnail_map_|, then invoke |callback| with NULL > since there is no associated thumbnail. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.cc:44: // Get or create the ThumbnailManagerSession. In the case of a creation, On 2014/05/22 18:08:22, huangs1 wrote: > // Look for a fetch in progress for |thumbnail_url|. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.cc:48: // There are no fetches in progress for |thumbnail_url|. Create a session On 2014/05/22 18:08:22, huangs1 wrote: > // |thumbnail_url| is not being fetched, so create a session and initiate the > fetch. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.cc:53: session->fetcher_ = new chrome::BitmapFetcher(thumbnail_url, this); On 2014/05/22 18:08:22, huangs1 wrote: > // This is deallocated in OnFetchComplete(). Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.cc:89: // Not found. On 2014/05/22 18:08:22, huangs1 wrote: > NIT: can be shortened to 2 liens: > > if (it == thumbnail_map_.end()) > return false; // Not found. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:26: On 2014/05/22 18:08:22, huangs1 wrote: > NIT: remove extra empty lines. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:35: // Initialize the |thumbnail_map_| with the proper mapping from URL to On 2014/05/22 18:08:22, huangs1 wrote: > NIT: s/Initialize/Initializes/ Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:37: void InitializeThumbnailMap(const SuggestionsProfile& suggestions); On 2014/05/22 18:08:22, huangs1 wrote: > Should this routine only be called once? What if it's called while there are > pending requests? It can be updated when new suggestions are fetched. The thumbnail_map_ is only accessed when starting requests, so it's not affected by pending requests. The worst that can happen if it's being refreshed while a request is being prepared is that the thumbnail URL is not found. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:45: virtual void OnFetchComplete(const GURL thumbnail_url, On 2014/05/22 18:08:22, huangs1 wrote: > I suspect this is not called from the UI thread? If this were the case, then > GetPageThumbnail() and OnFetchComplete() may conflict over |session_map_|. To > solve this, you can add Mutex to safeguard |session_map_|. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:55: // Contains the information related with a thumbnail fetch (fetcher, pending On 2014/05/22 18:08:22, huangs1 wrote: > also mention |url|? Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:57: class Session { On 2014/05/22 18:08:22, huangs1 wrote: > Since Session doesn't do anything interesting anymore, may as well make it a > struct, and get rid of consturctors / destructors. > > Also, public member variables don't have to end with "_". I can't get rid of the constructors, nor inline them, etc. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:64: std::queue<BitmapResponseCallback> callbacks_; On 2014/05/22 18:08:22, huangs1 wrote: > // Queue for pending callbacks, which may accumulate while the request is in > flight. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:69: // Given a website |url|, fill in a |thumbnail_url| and return whether it On 2014/05/22 18:08:22, huangs1 wrote: > // Looks up thumbnail for |url|. If found, writes the result to |thumbnail_url| > and returns true. Otherwise just returns false. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:82: // Map from each thumbnail URL to the session information (fetcher, On 2014/05/22 18:08:22, huangs1 wrote: > url is also part of session information. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... File chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc (right): https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:84: thumbnail_manager.GetPageThumbnail( On 2014/05/22 18:08:22, huangs1 wrote: > // Fetch existing URL. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:94: thumbnail_manager.GetPageThumbnail( On 2014/05/22 18:08:22, huangs1 wrote: > // Fetch non-existing URL. Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:116: thumbnail_manager.GetPageThumbnail( On 2014/05/22 18:08:22, huangs1 wrote: > // Fetch non-existing URL, and add more while request is in flight. > > Put into for loop? Done. https://codereview.chromium.org/299713007/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager_browsertest.cc:141: TestingProfile profile; On 2014/05/22 18:08:22, huangs1 wrote: > // Fetch existing URL that has invalid thumbnail. Done.
https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... File chrome/browser/search/suggestions/proto/suggestions.proto (right): https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/proto/suggestions.proto:32: // Specification for the thumbnail to be used, specified as a URL. nit: "Specification" is vague. Is this just an URL? If so, then I'd reword the comment to say "The URL for the thumbnail" and call the field thumbnail_url. If not, this needs more detail about what kind of specification this is. https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:47: // Retrieves stored thumbnail for website |url| asynchronously. Returns s/Returns (.*) to/Calls |callback| with/ https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.cc:20: ThumbnailManager::Session::Session() {} Initialize the struct fields. https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.cc:51: Session* session = &session_map_[thumbnail_url]; A different approach here that would help you to clarify the ownership semantics of session->fetcher would be: Session owns the BitmapFetcher. ~Session would destroy it. You'd add a Session(chrome::BitmapFetcher*) constructor. Here, you'd write Session request(new chrome::BitmapFetcher(thumbnail_url, this)); request->callbacks.push(callback); request->fetcher->Start(...); session_map_[thumbnail_url] = request; https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:27: // A base class to maintain and fetch server thumbnails asynchronously. Here, explain what you mean by "maintain". Does this mean "cache"? Since this is a base class, what functionality does it provide by default, and what does it expect to be customized? https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:33: // Initializes the |thumbnail_map_| with the proper mapping from URL to Does "URL" here mean "website URL"? https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:37: // Retrieves stored thumbnail for website |url| asynchronously. Returns Same comment about "returns" here. https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:53: // Contains the information related with a thumbnail fetch (associated website Omit "contains the". information -> state, related with -> related to https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:55: struct Session { "ThumbnailRequest" might be a better name for this struct. https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:63: std::queue<BitmapResponseCallback> callbacks; Use a vector here. You don't really need linked allocation, since you're just going to walk over these once in order and then delete them all---better to do that all at once. https://codereview.chromium.org/299713007/diff/80001/chrome/browser/search/su... chrome/browser/search/suggestions/thumbnail_manager.h:83: ThumbnailSessionMap session_map_; I'd call this pending_requests_.
Nitty details. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/suggestions_service.h (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/suggestions_service.h:16: #include "chrome/browser/search/suggestions/thumbnail_manager.h" thumbnail_manager.h is included solely for typedef BitmapResponseCallback Otherwise ThumbnailManager can be pre-declared. This seems rather odd. You're using SuggestionsService::GetPageThumbnail to isolate ThumbnailManager details from caller of SuggestionsService, BUT the caller requires thumbnail_manager.h to grab BitmapResponseCallback. Discussion alternatives in person. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:20: ThumbnailManager::Session::Session() {} Per discussion, can remove ThumbnailManager::Session consturctor and destructor. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:80: std::queue<BitmapResponseCallback> callbacks = session->callbacks; Comment that you're purposefully creating a copy of session->callbacks. Actually, this is mainly useful for thread safety; the idea is that you would lock |session_map_|, copy the stuff you need, clear from |session_map_|, unlock, then loop over the copy and dispatch at the current thread, without blocking the other thread that writes to |session_map_|. Since you're always on the UI thread, an alternative (to commenting) is that you can just operate on session->callbacks directly.
Thanks for your detailed comments, all! https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/proto/suggestions.proto (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/proto/suggestions.proto:32: // Specification for the thumbnail to be used, specified as a URL. On 2014/05/23 16:57:48, Jered wrote: > nit: "Specification" is vague. Is this just an URL? If so, then I'd reword the > comment to say "The URL for the thumbnail" and call the field thumbnail_url. If > not, this needs more detail about what kind of specification this is. Done. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/suggestions_service.h (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/suggestions_service.h:16: #include "chrome/browser/search/suggestions/thumbnail_manager.h" On 2014/05/23 17:23:35, huangs1 wrote: > thumbnail_manager.h is included solely for > > typedef BitmapResponseCallback > > Otherwise ThumbnailManager can be pre-declared. This seems rather odd. You're > using SuggestionsService::GetPageThumbnail to isolate ThumbnailManager details > from caller of SuggestionsService, BUT the caller requires thumbnail_manager.h > to grab BitmapResponseCallback. Discussion alternatives in person. Removed the typedef.. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/suggestions_service.h:47: // Retrieves stored thumbnail for website |url| asynchronously. Returns On 2014/05/23 16:57:48, Jered wrote: > s/Returns (.*) to/Calls |callback| with/ Done. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:20: ThumbnailManager::Session::Session() {} On 2014/05/23 16:57:48, Jered wrote: > Initialize the struct fields. Actually removed this. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:20: ThumbnailManager::Session::Session() {} On 2014/05/23 17:23:35, huangs1 wrote: > Per discussion, can remove ThumbnailManager::Session consturctor and destructor. Done. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:51: Session* session = &session_map_[thumbnail_url]; On 2014/05/23 16:57:48, Jered wrote: > A different approach here that would help you to clarify the ownership semantics > of session->fetcher would be: Session owns the BitmapFetcher. > ~Session would destroy it. You'd add a Session(chrome::BitmapFetcher*) > constructor. Here, you'd write > > Session request(new chrome::BitmapFetcher(thumbnail_url, this)); > request->callbacks.push(callback); > request->fetcher->Start(...); > session_map_[thumbnail_url] = request; I actually like the approach Sam and I had come up with. Having the map create the default instance and initializing it manually avoids a copy. Since it's a private struct it's pretty safe to manage deallocation ourselves. Let me know if you feel strongly. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:80: std::queue<BitmapResponseCallback> callbacks = session->callbacks; On 2014/05/23 17:23:35, huangs1 wrote: > Comment that you're purposefully creating a copy of session->callbacks. > > Actually, this is mainly useful for thread safety; the idea is that you would > lock |session_map_|, copy the stuff you need, clear from |session_map_|, unlock, > then loop over the copy and dispatch at the current thread, without blocking the > other thread that writes to |session_map_|. > > Since you're always on the UI thread, an alternative (to commenting) is that you > can just operate on session->callbacks directly. No copying sounds good since we're on the UI thread, thanks. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:27: // A base class to maintain and fetch server thumbnails asynchronously. On 2014/05/23 16:57:48, Jered wrote: > Here, explain what you mean by "maintain". Does this mean "cache"? Since this is > a base class, what functionality does it provide by default, and what does it > expect to be customized? Reworded. It doesn't cache for now, will add it when it does. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:33: // Initializes the |thumbnail_map_| with the proper mapping from URL to On 2014/05/23 16:57:48, Jered wrote: > Does "URL" here mean "website URL"? Done. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:37: // Retrieves stored thumbnail for website |url| asynchronously. Returns On 2014/05/23 16:57:48, Jered wrote: > Same comment about "returns" here. Done. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:53: // Contains the information related with a thumbnail fetch (associated website On 2014/05/23 16:57:48, Jered wrote: > Omit "contains the". information -> state, related with -> related to Done. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:55: struct Session { On 2014/05/23 16:57:48, Jered wrote: > "ThumbnailRequest" might be a better name for this struct. Done. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:63: std::queue<BitmapResponseCallback> callbacks; On 2014/05/23 16:57:48, Jered wrote: > Use a vector here. You don't really need linked allocation, since you're just > going to walk over these once in order and then delete them all---better to do > that all at once. Done. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:83: ThumbnailSessionMap session_map_; On 2014/05/23 16:57:48, Jered wrote: > I'd call this pending_requests_. Done.
Sorry for the delay. Few more comments. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:51: Session* session = &session_map_[thumbnail_url]; To Jered: Since we use std::map<>, the suggested change would require BitmapFetcher to have a default constructor anyway. And if ~Session destroys fetcher, then we would need std::swap(session_map_[thumbnail_url], request); https://chromiumcodereview.appspot.com/299713007/diff/90001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://chromiumcodereview.appspot.com/299713007/diff/90001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:45: virtual void OnFetchComplete(const GURL thumbnail_url, Can this be private, to prevent direct call? https://chromiumcodereview.appspot.com/299713007/diff/90001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:83: // Map from each thumbnail URL to the session information (associated website NB the word "session" is used (keep it if that's intended).
lgtm
https://chromiumcodereview.appspot.com/299713007/diff/90001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://chromiumcodereview.appspot.com/299713007/diff/90001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:45: virtual void OnFetchComplete(const GURL thumbnail_url, On 2014/05/23 20:16:22, huangs1 wrote: > Can this be private, to prevent direct call? Done. Good point. https://chromiumcodereview.appspot.com/299713007/diff/90001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.h:83: // Map from each thumbnail URL to the session information (associated website On 2014/05/23 20:16:22, huangs1 wrote: > NB the word "session" is used (keep it if that's intended). Done.
My latest patchset (8) needs to be there otherwise it doesn't compile with Clang++.
https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:51: Session* session = &session_map_[thumbnail_url]; On 2014/05/23 20:16:22, huangs1 wrote: > To Jered: Since we use std::map<>, the suggested change would require > BitmapFetcher to have a default constructor anyway. And if ~Session destroys > fetcher, then we would need > > std::swap(session_map_[thumbnail_url], request); > A default constructor seems reasonable. map.erase() calls the destructor for class types. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:51: Session* session = &session_map_[thumbnail_url]; On 2014/05/23 19:21:58, Mathieu Perreault wrote: > On 2014/05/23 16:57:48, Jered wrote: > > A different approach here that would help you to clarify the ownership > semantics > > of session->fetcher would be: Session owns the BitmapFetcher. > > ~Session would destroy it. You'd add a Session(chrome::BitmapFetcher*) > > constructor. Here, you'd write > > > > Session request(new chrome::BitmapFetcher(thumbnail_url, this)); > > request->callbacks.push(callback); > > request->fetcher->Start(...); > > session_map_[thumbnail_url] = request; > > I actually like the approach Sam and I had come up with. Having the map create > the default instance and initializing it manually avoids a copy. Since it's a > private struct it's pretty safe to manage deallocation ourselves. Let me know if > you feel strongly. This is one of my pet peeves in C++... correctness is more important than eliding copies of small objects, that's premature optimization. Here, the correctness question is who owns the fetcher pointer? It's tied to the lifetime of the thumbnail request, and there's an object for that. With RAII, we should tie the ownership to the lifecycle of the object. so struct ThumbnailRequest { ThumbnailRequest(chrome::BitmapFetcher* f) : fetcher(f) {} scoped_ptr<chrome::BitmapFetcher*> fetcher; }; This is nice because you don't have to "remember" to destroy the pointer later. https://chromiumcodereview.appspot.com/299713007/diff/130001/chrome/browser/s... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://chromiumcodereview.appspot.com/299713007/diff/130001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.h:42: base::Callback<void(const GURL&, const SkBitmap*)> callback); This doesn't use the typedef above? https://chromiumcodereview.appspot.com/299713007/diff/130001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.h:44: nit: omit this extra blank line
https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:51: Session* session = &session_map_[thumbnail_url]; We tried this, but std::map<GURL, ThumbnailSessionMap> will need to copy (no C++11 emplace() yet), and then scope_ptr<> complains. What we ultimately want is to have std::map<T, scoped_ptr<U> >, but this is unavailable... https://chromiumcodereview.appspot.com/299713007/diff/130001/chrome/browser/s... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://chromiumcodereview.appspot.com/299713007/diff/130001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.h:42: base::Callback<void(const GURL&, const SkBitmap*)> callback); We decided to not use typedef BitmapResponseCallback; because it breaks encapsulation, i.e., caller of SuggestionsService should not know about ThumbnailManager. So lines 22-24 should be deleted.
https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:51: Session* session = &session_map_[thumbnail_url]; On 2014/05/23 22:34:26, huangs1 wrote: > We tried this, but std::map<GURL, ThumbnailSessionMap> will need to copy (no > C++11 emplace() yet), and then scope_ptr<> complains. > > What we ultimately want is to have std::map<T, scoped_ptr<U> >, but this is > unavailable... Even if we can't use scoped_ptr, a raw pointer with a destructor that calls delete still seems clearer to me. :-)
Have a look! https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:51: Session* session = &session_map_[thumbnail_url]; On 2014/05/23 22:34:26, huangs1 wrote: > We tried this, but std::map<GURL, ThumbnailSessionMap> will need to copy (no > C++11 emplace() yet), and then scope_ptr<> complains. > > What we ultimately want is to have std::map<T, scoped_ptr<U> >, but this is > unavailable... Indeed we tried this along with many variations (Sam means std::map<GURL, ThumbnailRequest>) and it didn't work. The only sensible way is to have the fetcher as a pointer, which is low-risk being private. Use scoped_ptr<> in a map and you're going to have a bad day... I'm interested in doing it the right way but we just can't seem to find it. https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:51: Session* session = &session_map_[thumbnail_url]; On 2014/05/23 21:25:00, Jered wrote: > On 2014/05/23 20:16:22, huangs1 wrote: > > To Jered: Since we use std::map<>, the suggested change would require > > BitmapFetcher to have a default constructor anyway. And if ~Session destroys > > fetcher, then we would need > > > > std::swap(session_map_[thumbnail_url], request); > > > > A default constructor seems reasonable. map.erase() calls the destructor for > class types. Ack. https://chromiumcodereview.appspot.com/299713007/diff/130001/chrome/browser/s... File chrome/browser/search/suggestions/thumbnail_manager.h (right): https://chromiumcodereview.appspot.com/299713007/diff/130001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.h:42: base::Callback<void(const GURL&, const SkBitmap*)> callback); On 2014/05/23 21:25:00, Jered wrote: > This doesn't use the typedef above? Oops I meant to remove the typedef. I want to avoid callers relying on this file to be included (even indirectly) to be informed of the callback type. https://chromiumcodereview.appspot.com/299713007/diff/130001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.h:44: On 2014/05/23 21:25:00, Jered wrote: > nit: omit this extra blank line Done.
Thanks! https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/80001/chrome/browser/se... chrome/browser/search/suggestions/thumbnail_manager.cc:51: Session* session = &session_map_[thumbnail_url]; On 2014/05/23 22:37:01, Jered wrote: > On 2014/05/23 22:34:26, huangs1 wrote: > > We tried this, but std::map<GURL, ThumbnailSessionMap> will need to copy (no > > C++11 emplace() yet), and then scope_ptr<> complains. > > > > What we ultimately want is to have std::map<T, scoped_ptr<U> >, but this is > > unavailable... > > Even if we can't use scoped_ptr, a raw pointer with a destructor that calls > delete still seems clearer to me. :-) Have a look at the latest version. Had to leave the default constructor on ThumbnailRequest for it to compile, and thus put a guard on the deletion of fetcher. Not sure if that's too much. Let me know.
Thanks, this makes me happier :-) https://chromiumcodereview.appspot.com/299713007/diff/160001/chrome/browser/s... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/160001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.cc:19: ThumbnailManager::ThumbnailRequest::ThumbnailRequest() {} say :fetcher(NULL) here https://chromiumcodereview.appspot.com/299713007/diff/160001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.cc:25: if (fetcher) delete fetcher; delete NULL is fine
Thanks! I'm satisfied as well! https://chromiumcodereview.appspot.com/299713007/diff/160001/chrome/browser/s... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/160001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.cc:19: ThumbnailManager::ThumbnailRequest::ThumbnailRequest() {} On 2014/05/23 22:57:38, Jered wrote: > say :fetcher(NULL) here Done. https://chromiumcodereview.appspot.com/299713007/diff/160001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.cc:25: if (fetcher) delete fetcher; On 2014/05/23 22:57:38, Jered wrote: > delete NULL is fine Done.
Some concerns on free-after-use? https://chromiumcodereview.appspot.com/299713007/diff/200001/chrome/browser/s... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/200001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.cc:63: pending_requests_[thumbnail_url] = request; Wouldn't |request|'s destructor get called when it goes out of scope, resulting in use-after-free? Here's a test program: ******** #include <iostream> #include <map> class Test { public: Test() : x(0) {} explicit Test (int x_in) : x(x_in) {} ~Test() { std::cerr << "Destroy " << x << std::endl;} int x; }; int main () { std::map<int, Test> m; Test a(5); m[3] = a; return 0; } ******** Output is: ======== Destroy 0 (some temporary while computing |m[3]|?) Destroy 0 (old value of |m[3]|). Destroy 5 (new value of |m[3]|). Destroy 5 (temp variable |a|) ======== Perhaps we can implement our own swap() in ThumbnailRequest()? So: (.h file change etc.) #include <algorithm> ... ThumbnailManager::ThumbnailRequest::swap(ThumbnailManager::ThumbnailRequest* other) { std::swap(fetcher, other->fetcher); } ... pending_requests_[thumbnail_url].swap(&request);
I mean use-after-free. :)
Have a look! https://chromiumcodereview.appspot.com/299713007/diff/200001/chrome/browser/s... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/200001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.cc:63: pending_requests_[thumbnail_url] = request; On 2014/05/24 03:45:40, huangs1 wrote: > Wouldn't |request|'s destructor get called when it goes out of scope, resulting > in use-after-free? Here's a test program: > ******** > #include <iostream> > #include <map> > > class Test { > public: > Test() : x(0) {} > explicit Test (int x_in) : x(x_in) {} > ~Test() { std::cerr << "Destroy " << x << std::endl;} > int x; > }; > > int main () { > std::map<int, Test> m; > Test a(5); > m[3] = a; > return 0; > } > ******** > Output is: > ======== > Destroy 0 (some temporary while computing |m[3]|?) > Destroy 0 (old value of |m[3]|). > Destroy 5 (new value of |m[3]|). > Destroy 5 (temp variable |a|) > ======== > > Perhaps we can implement our own swap() in ThumbnailRequest()? So: > > (.h file change etc.) > > #include <algorithm> > ... > ThumbnailManager::ThumbnailRequest::swap(ThumbnailManager::ThumbnailRequest* > other) { > std::swap(fetcher, other->fetcher); > } > ... > pending_requests_[thumbnail_url].swap(&request); > Good catch, thanks for paying attention. See new patch.
LGTM. However, note that std::swap is in <algorithm> in C++98, and in <utility> for C++11. Run some try jobs, and if any compile problems occur then include <algoritm> in the .h file and try again.
On 2014/05/26 19:30:14, huangs1 wrote: > LGTM. However, note that std::swap is in <algorithm> in C++98, and in <utility> > for C++11. Run some try jobs, and if any compile problems occur then include > <algoritm> in the .h file and try again. lgtm
lgtm https://chromiumcodereview.appspot.com/299713007/diff/200001/chrome/browser/s... File chrome/browser/search/suggestions/thumbnail_manager.cc (right): https://chromiumcodereview.appspot.com/299713007/diff/200001/chrome/browser/s... chrome/browser/search/suggestions/thumbnail_manager.cc:63: pending_requests_[thumbnail_url] = request; On 2014/05/26 18:55:30, Mathieu Perreault wrote: > On 2014/05/24 03:45:40, huangs1 wrote: > > Wouldn't |request|'s destructor get called when it goes out of scope, > resulting > > in use-after-free? Here's a test program: > > ******** > > #include <iostream> > > #include <map> > > > > class Test { > > public: > > Test() : x(0) {} > > explicit Test (int x_in) : x(x_in) {} > > ~Test() { std::cerr << "Destroy " << x << std::endl;} > > int x; > > }; > > > > int main () { > > std::map<int, Test> m; > > Test a(5); > > m[3] = a; > > return 0; > > } > > ******** > > Output is: > > ======== > > Destroy 0 (some temporary while computing |m[3]|?) > > Destroy 0 (old value of |m[3]|). > > Destroy 5 (new value of |m[3]|). > > Destroy 5 (temp variable |a|) > > ======== > > > > Perhaps we can implement our own swap() in ThumbnailRequest()? So: > > > > (.h file change etc.) > > > > #include <algorithm> > > ... > > ThumbnailManager::ThumbnailRequest::swap(ThumbnailManager::ThumbnailRequest* > > other) { > > std::swap(fetcher, other->fetcher); > > } > > ... > > pending_requests_[thumbnail_url].swap(&request); > > > > Good catch, thanks for paying attention. See new patch. :-/ Sorry I missed that. This is a little unfortunate. Anyway probably enough C++ gymnastics for this CL.
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/299713007/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
Message was sent while issue was closed.
Change committed as 273012 |
