[Enhanced Bookmark]Upstream image fetching code in android

components/enhanced_bookmarks/bookmark_image_service.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/enhanced_bookmarks/bookmark_image_service.h"
 
 #include "base/single_thread_task_runner.h"
 #include "base/task_runner_util.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/sequenced_worker_pool.h"
@@ skipping 83 matching lines @@
 
 void BookmarkImageService::Shutdown() {
   DCHECK(CalledOnValidThread());
   enhanced_bookmark_model_->bookmark_model()->RemoveObserver(this);
   enhanced_bookmark_model_ = NULL;
 }
 
 void BookmarkImageService::SalientImageForUrl(const GURL& page_url,
                                               ImageCallback callback) {
   DCHECK(CalledOnValidThread());
-  SalientImageForUrl(page_url, true, callback);
+  SalientImageFromWeb(page_url, callback);
 }
 
 void BookmarkImageService::RetrieveImageFromStore(const GURL& page_url,
                                                   ImageCallback callback) {
   DCHECK(CalledOnValidThread());
   pool_->PostSequencedWorkerTaskWithShutdownBehavior(
       pool_->GetNamedSequenceToken(kSequenceToken),
       FROM_HERE,
       base::Bind(&RetrieveImageFromStoreRelay,
                  base::Unretained(store_.get()),
@@ skipping 38 matching lines @@
     // Either the record was in the store or there is no image in the store, but
     // an URL for a record is present, indicating that a previous attempt to
     // download the image failed. Just return the record.
     original_callback.Run(record);
   } else {
     // There is no record in the store, and no previous attempts to retrieve
     // one. Start a request to retrieve a salient image if there is an image
     // url set on a bookmark, and then enqueue the request for the record to
     // be triggered when the retrieval is finished.
     RetrieveSalientImageForPageUrl(page_url);
-    SalientImageForUrl(page_url, false, original_callback);
+    SalientImageFromLocal(page_url, original_callback);
   }
 }
 
-void BookmarkImageService::SalientImageForUrl(const GURL& page_url,
-                                              bool fetch_from_bookmark,
-                                              ImageCallback callback) {
+void BookmarkImageService::SalientImageFromLocal(const GURL& page_url,
+                                                 ImageCallback callback) {
   DCHECK(CalledOnValidThread());
-
   // If the request is done while the image is currently being retrieved, just
   // store the appropriate callbacks to call once the image is retrieved.
   if (IsPageUrlInProgress(page_url)) {
     callbacks_[page_url].push_back(callback);
     return;
   }
+  RetrieveImageFromStore(page_url, callback);
+}
 
-  if (!fetch_from_bookmark) {
-    RetrieveImageFromStore(page_url, callback);
-  } else {
-    RetrieveImageFromStore(page_url,
-                           base::Bind(&BookmarkImageService::FetchCallback,
-                                      base::Unretained(this),
-                                      page_url,
-                                      callback));
+void BookmarkImageService::SalientImageFromWeb(const GURL& page_url,
+                                               ImageCallback callback) {
+  // If the request is done while the image is currently being retrieved, just

[lpromero, 2015/02/04 13:54:51]

Add DCHECK called on valid thread.

[Ian Wen, 2015/02/05 18:33:17]

Reverted the signature change.

+  // store the appropriate callbacks to call once the image is retrieved.
+  if (IsPageUrlInProgress(page_url)) {
+    callbacks_[page_url].push_back(callback);
+    return;

[noyau (Ping after 24h), 2015/02/04 12:45:27]

The refactoring leads to the duplication of this code, something that I was
trying to avoid originally. Personally I prefer the original version and don't
see an improvement on readability here.

[Ian Wen, 2015/02/05 18:33:17]

Reverted.

   }
+  RetrieveImageFromStore(
+      page_url, base::Bind(&BookmarkImageService::FetchCallback,
+                           base::Unretained(this), page_url, callback));
 }
 
 void BookmarkImageService::ProcessNewImage(const GURL& page_url,
                                            bool update_bookmarks,

[Kibeom Kim (inactive), 2015/02/05 01:19:24]

Question to noyau@ or lpromero@: can we remove update_bookmarks? Seems it's
unnecessary now.

[noyau (Ping after 24h), 2015/02/05 08:49:59]

This is not unnecessary. This decides if the image url is set on the bookmark.
And is necessary because if the image url comes from the bookmark in the first
place there is no need to update it.

[Ian Wen, 2015/02/05 18:33:17]

update_bookmarks is useful to prefetch the image when enhanced bookmark is not
turned on. We want the users to see some images when he turns on this feature.

[Kibeom Kim (inactive), 2015/02/05 18:51:34]

I see I missed that part.


On 2015/02/05 18:33:17, Ian Wen wrote:
If the feature is turned off, this function shouldn't be called at the first
place so it doesn't seem that's the usecase.

e.g., line #34 is always true.
https://codereview.chromium.org/899653003/diff/20001/chrome/browser/enhanced_...

                                            const gfx::Image& image,
                                            const GURL& image_url) {
   DCHECK(CalledOnValidThread());
   PostTaskToStoreImage(image, image_url, page_url);
   if (update_bookmarks && image_url.is_valid()) {
     const BookmarkNode* bookmark =
         enhanced_bookmark_model_->bookmark_model()
             ->GetMostRecentlyAddedUserNodeForURL(page_url);
     if (bookmark) {
       const gfx::Size& size = image.Size();
@@ skipping 156 matching lines @@
     const BookmarkNode* node) {
 }
 
 void BookmarkImageService::BookmarkAllUserNodesRemoved(
     BookmarkModel* model,
     const std::set<GURL>& removed_urls) {
   ClearAll();
 }
 
 }  // namespace enhanced_bookmarks