Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2045233002:
Split the code for fetching images and for decoding images into two separate classes. This will all… (Closed)
|
Created:
4 years, 6 months ago by markusheintz_ Modified:
4 years, 6 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, droger+watchlist_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, sdefresne+watchlist_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, blundell+watchlist_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit the code for fetching images and for decoding images into two separate classes. This will allow to use common code for image URL fetching and platform specific implementations for image decoding.
BUG=609127
Committed: https://crrev.com/d8513537879d4e6d452cbd66ac4c203752eab916
Cr-Commit-Position: refs/heads/master@{#399457}
Patch Set 1 #Patch Set 2 : Fix some final nits. Ready for review. #
Total comments: 92
Patch Set 3 : Address comments treib@, bauerb@ and markusheintz@ #Patch Set 4 : Make ~DecodeImageRequest() final #
Total comments: 32
Patch Set 5 : Address comments round 2 treib@, blundell@ #Patch Set 6 : Address comments round 2 bauerb@ #Patch Set 7 : Revert the uses of std::map::emplace again since it is not fully supported yet #Patch Set 8 : s/final/override/ in ~DecodeImageRequest #Patch Set 9 : " #Patch Set 10 : Update components/image_fetcher.gypi #Patch Set 11 : Fix long line #
Total comments: 12
Patch Set 12 : Address comments mmenke@ #Patch Set 13 : Update chrome_browser.gypi to use depend on the image_fetcher component #Patch Set 14 : Add dep to net in components/image_fetcher.gypi #Patch Set 15 : Yet another update to chrome_browser.gypi #Patch Set 16 : " #Patch Set 17 : " #Patch Set 18 : " #Patch Set 19 : "CL https://codereview.chromium.org/2045233002: Split the code for fetching images and for decodin… #Messages
Total messages: 50 (11 generated)
Description was changed from ========== Split the code for fetching images and for decoding images into two separate classes. This will allow to use comon code for image URL fetching and platform specific implementations for image dicoding. BUG=609127 ========== to ========== Split the code for fetching images and for decoding images into two separate classes. This will allow to use comon code for image URL fetching and platform specific implementations for image dicoding. BUG=609127 ==========
markusheintz@chromium.org changed reviewers: + treib@chromium.org
markusheintz@chromium.org changed reviewers: + bauerb@google.com
Marc and Bernhard please take a look at the CL
markusheintz@chromium.org changed reviewers: + bauerb@chromium.org - bauerb@google.com
blundell@chromium.org changed reviewers: + blundell@chromium.org
https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.h (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:19: class ImageDecoderImpl : public image_fetcher::ImageDecoder { drive-by: This class should be named ChromeImageDecoder (and likewise for the existing ImageFetcherImpl class).
On 2016/06/08 12:38:00, blundell wrote: > https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... > File chrome/browser/search/suggestions/image_decoder_impl.h (right): > > https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... > chrome/browser/search/suggestions/image_decoder_impl.h:19: class > ImageDecoderImpl : public image_fetcher::ImageDecoder { > drive-by: This class should be named ChromeImageDecoder (and likewise for the > existing ImageFetcherImpl class). Great suggestion. There will be some follow up CLs since this is the first of some refactoring. I'm not sure yet whether this needs to be tailored to our needs or not. Would it be ok if I do the renaming in a follow up CL? I would file a bug for this.
On 2016/06/08 12:56:47, markusheintz_ wrote: > On 2016/06/08 12:38:00, blundell wrote: > > > https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... > > File chrome/browser/search/suggestions/image_decoder_impl.h (right): > > > > > https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... > > chrome/browser/search/suggestions/image_decoder_impl.h:19: class > > ImageDecoderImpl : public image_fetcher::ImageDecoder { > > drive-by: This class should be named ChromeImageDecoder (and likewise for the > > existing ImageFetcherImpl class). > > Great suggestion. There will be some follow up CLs since this is the first of > some refactoring. I'm not sure yet whether this needs to be tailored to our > needs or not. Would it be ok if I do the renaming in a follow up CL? I would > file a bug for this. I'm happy to change the namespace in this CL if you like.
Description was changed from ========== Split the code for fetching images and for decoding images into two separate classes. This will allow to use comon code for image URL fetching and platform specific implementations for image dicoding. BUG=609127 ========== to ========== Split the code for fetching images and for decoding images into two separate classes. This will allow to use common code for image URL fetching and platform specific implementations for image decoding. BUG=609127 ==========
Lots of comments, but mostly small ones :) https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.cc (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.cc:23: // outlives the coresponding request. When an ImageRequest is destroyed, it cancels itself, so this should be fine. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.cc:46: decoder_->RemoveDecodeImageRequest(this); Hm. One possible alternative: Do "delete this;" instead, then you won't need |decode_image_requests_| at all. That's what others do, e.g.: https://cs.chromium.org/chromium/src/chrome/browser/download/download_command... Downside: We won't have any way to cancel requests. WDYT? https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.h (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:16: namespace chrome { We don't usually use this namespace. If you rename the class per blundell's suggestion, then we don't need a namespace at all. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:25: image_fetcher::ImageDecodedCallback callback) override; Pass the callback by const ref, to avoid refcount churn https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:26: nit: extra empty line https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:33: image_fetcher::ImageDecodedCallback callback) Also here: const ref please https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:40: // Methods inherited from ImageDecoder::ImageRequest These can and IMO should be private https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:42: // Called when image is decoded. |decoder| is used to identify the image in There's no |decoder| here. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:44: // on the UI thread. Er, won't it? I think it *is* called on the UI thread, no? https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.cc (left): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:5: #include "chrome/browser/search/suggestions/image_fetcher_impl.h" This should remain here https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.cc (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:22: image_url_fetcher_ = std::unique_ptr<image_fetcher::ImageURLFetcher>( Init |image_url_fetcher_| in the initializer list above? https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:52: pending_net_requests_[image_url].swap(&request); Not your doing, but this is weird... can we do pending_net_requests_.emplace(image_url, request); instead? Then we also won't need the swap() method anymore. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:65: const std::string& image_data) { We should add a method OnImageDataFetched on the delegate and call that here. That can wait for another CL though! https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.h (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:31: class ImageFetcherImpl : public image_fetcher::ImageFetcher { The plan is to eventually move this class into the component and inject the platform-dependent ImageDecoder implementation, right? (another CL of course) https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/BUILD.gn (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/BUILD.gn:16: "//url", You'll have to add //net here too https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/image_decoder.h (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_decoder.h:8: #include "base/callback.h" callback_forward.h should be enough https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_decoder.h:9: #include "url/gurl.h" Not used https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_decoder.h:27: // decoding the image and empty image is passed to the callback. s/and/an https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_decoder.h:29: ImageDecodedCallback callback) = 0; const ref https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/image_url_fetcher.cc (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:53: if (url_fetcher_.get() != NULL) Just if (url_fetcher_) should do. But I think this can never happen anyway? In that case, just DCHECK(!url_fetcher_); https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:70: // Return an empty string when the URL requests does complete successfully. does *not* complete successfully, right? https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:81: image_url_fetcher_->RemoveImageURLFetcherRequest(url_); I think here we can get rid of the destruction weirdness by: - Making the request_queue_ a map<URLFetcher*, unique_ptr<Request>> - Making the ImageURLFetcher itself be the URLFetcherDelegate => On URL request completion, it gets passed the corresponding URLFetcher*, and can thus find the right Request in the map. WDYT? https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/image_url_fetcher.h (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:23: class ImageURLFetcher { I'd call this ImageDataFetcher. WDYT? https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:34: void FetchImageURL(const GURL& image_url, ImageURLFetcherCallback callback); const ref https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:43: ImageURLFetcherCallback callback, const ref https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:58: void OnURLFetchComplete(const net::URLFetcher* source) override; can be private
On 2016/06/08 12:56:47, markusheintz_ wrote: > On 2016/06/08 12:38:00, blundell wrote: > > > https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... > > File chrome/browser/search/suggestions/image_decoder_impl.h (right): > > > > > https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... > > chrome/browser/search/suggestions/image_decoder_impl.h:19: class > > ImageDecoderImpl : public image_fetcher::ImageDecoder { > > drive-by: This class should be named ChromeImageDecoder (and likewise for the > > existing ImageFetcherImpl class). > > Great suggestion. There will be some follow up CLs since this is the first of > some refactoring. I'm not sure yet whether this needs to be tailored to our > needs or not. Would it be ok if I do the renaming in a follow up CL? I would > file a bug for this. Sure.
On 2016/06/08 12:58:45, markusheintz_ wrote: > On 2016/06/08 12:56:47, markusheintz_ wrote: > > On 2016/06/08 12:38:00, blundell wrote: > > > > > > https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... > > > File chrome/browser/search/suggestions/image_decoder_impl.h (right): > > > > > > > > > https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... > > > chrome/browser/search/suggestions/image_decoder_impl.h:19: class > > > ImageDecoderImpl : public image_fetcher::ImageDecoder { > > > drive-by: This class should be named ChromeImageDecoder (and likewise for > the > > > existing ImageFetcherImpl class). > > > > Great suggestion. There will be some follow up CLs since this is the first of > > some refactoring. I'm not sure yet whether this needs to be tailored to our > > needs or not. Would it be ok if I do the renaming in a follow up CL? I would > > file a bug for this. > > I'm happy to change the namespace in this CL if you like. I would definitely get rid of the ::chrome namespace, as it's discouraged in Chromium to namespace top-level (embedder) code.
https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.h (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:30: class DecodeImageRequest : public ::ImageDecoder::ImageRequest { You could move the class definition to the .cc file and only forward-declare it here. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:38: void RunCallbackAndRemoveRequest(const gfx::Image& image); This method could be private. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:59: using DecodeImageRequestQueue = Not sure you need this typedef -- you could use the type directly for the member, and auto for the iterator type in RemoveDecodeImageRequest(). https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.cc (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:19: : delegate_(NULL), url_request_context_(url_request_context), NULL is deprecated -- use nullptr instead. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.h (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:66: // Processes image url fetched events. This is the continuation method used Nit: "URL" in all caps. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:74: using ImageRequestMap = std::map<const GURL, ImageRequest>; Typedefs should come before method declarations. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/image_decoder.h (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_decoder.h:17: using ImageDecodedCallback = base::Callback<void(const gfx::Image&)> ; Nit: no space before semicolon. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/image_url_fetcher.cc (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:15: :url_request_context_getter_(url_request_context_getter) {} Space after colon. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:24: std::string(), If you are passing the same values in here every time, just do that inside of the method. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:35: request_queue_.erase(request_it); Just directly use the overload that takes a key. You can DCHECK the result to verify that it actually removed an element. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:67: // TODO(markusheintz): Check if the if else condition can be inverted. Extrac Nit: Your devious plan to get around the 80 column limit by cutting off the last character of the line will not succeed :) https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:73: } else { else is not necessary if you return in the if branch. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/image_url_fetcher.h (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:37: // This class represents an active image url fetcher requests. The class Nits: * "This class represents" is redundant. * "URL" should be in all caps. * "request" in singular. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:40: class ImageURLFetcherRequest : public net::URLFetcherDelegate { Same as in ImageDecoderImpl: The class definition could move to the .cc file. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:56: // This will be called when the URL has been fetched, successfully or not. This comment is redundant, as the method contract is just the one from the interface. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:65: // be run even if the image data could not be fetche successfully. Nit: "fetched"
Thanks a lot for the great comments. Sorry for the many nits I forgot to fix. I addressed or comments on your comments. Even on two of my own comments on my own cl ;-) PTAL https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.cc (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.cc:23: // outlives the coresponding request. On 2016/06/08 13:33:28, Marc Treib wrote: > When an ImageRequest is destroyed, it cancels itself, so this should be fine. Great! https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.cc:46: decoder_->RemoveDecodeImageRequest(this); On 2016/06/08 13:33:28, Marc Treib wrote: > Hm. One possible alternative: Do "delete this;" instead, then you won't need > |decode_image_requests_| at all. That's what others do, e.g.: > https://cs.chromium.org/chromium/src/chrome/browser/download/download_command... > > Downside: We won't have any way to cancel requests. WDYT? Another downside would still have in item in the queue since this would only delete the request object, but not remove the queue item pointing to it. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.h (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:16: namespace chrome { On 2016/06/08 13:33:28, Marc Treib wrote: > We don't usually use this namespace. If you rename the class per blundell's > suggestion, then we don't need a namespace at all. I would actually prefer to use on of our namespaces suggestions as long as the file lives in this directory. I don't remember why I chose to use the chrome namespace in first place. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:19: class ImageDecoderImpl : public image_fetcher::ImageDecoder { On 2016/06/08 12:38:00, blundell wrote: > drive-by: This class should be named ChromeImageDecoder (and likewise for the > existing ImageFetcherImpl class). As mentioned in the other comment I'll try to do this in a separate CL. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:25: image_fetcher::ImageDecodedCallback callback) override; On 2016/06/08 13:33:28, Marc Treib wrote: > Pass the callback by const ref, to avoid refcount churn Done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:26: On 2016/06/08 13:33:28, Marc Treib wrote: > nit: extra empty line removed https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:30: class DecodeImageRequest : public ::ImageDecoder::ImageRequest { On 2016/06/09 10:14:30, Bernhard Bauer wrote: > You could move the class definition to the .cc file and only forward-declare it > here. Done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:33: image_fetcher::ImageDecodedCallback callback) On 2016/06/08 13:33:28, Marc Treib wrote: > Also here: const ref please Done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:38: void RunCallbackAndRemoveRequest(const gfx::Image& image); On 2016/06/09 10:14:30, Bernhard Bauer wrote: > This method could be private. Done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:40: // Methods inherited from ImageDecoder::ImageRequest On 2016/06/08 13:33:28, Marc Treib wrote: > These can and IMO should be private Done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:42: // Called when image is decoded. |decoder| is used to identify the image in On 2016/06/08 13:33:28, Marc Treib wrote: > There's no |decoder| here. I removed the comment since we have a comment that mentions from were the methods are inherited. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:44: // on the UI thread. On 2016/06/08 13:33:28, Marc Treib wrote: > Er, won't it? I think it *is* called on the UI thread, no? Sorry the comment is obsolete https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:59: using DecodeImageRequestQueue = On 2016/06/09 10:14:30, Bernhard Bauer wrote: > Not sure you need this typedef -- you could use the type directly for the > member, and auto for the iterator type in RemoveDecodeImageRequest(). Done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.cc (left): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:5: #include "chrome/browser/search/suggestions/image_fetcher_impl.h" On 2016/06/08 13:33:29, Marc Treib wrote: > This should remain here Ugh sorry. Done https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.cc (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:19: : delegate_(NULL), url_request_context_(url_request_context), On 2016/06/09 10:14:30, Bernhard Bauer wrote: > NULL is deprecated -- use nullptr instead. Done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:22: image_url_fetcher_ = std::unique_ptr<image_fetcher::ImageURLFetcher>( On 2016/06/08 13:33:28, Marc Treib wrote: > Init |image_url_fetcher_| in the initializer list above? Done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:52: pending_net_requests_[image_url].swap(&request); On 2016/06/08 13:33:29, Marc Treib wrote: > Not your doing, but this is weird... can we do > pending_net_requests_.emplace(image_url, request); > instead? Then we also won't need the swap() method anymore. Done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:65: const std::string& image_data) { On 2016/06/08 13:33:29, Marc Treib wrote: > We should add a method OnImageDataFetched on the delegate and call that here. > That can wait for another CL though! Added a TODO https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.h (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:31: class ImageFetcherImpl : public image_fetcher::ImageFetcher { On 2016/06/08 13:33:29, Marc Treib wrote: > The plan is to eventually move this class into the component and inject the > platform-dependent ImageDecoder implementation, right? (another CL of course) Correct. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:66: // Processes image url fetched events. This is the continuation method used On 2016/06/09 10:14:30, Bernhard Bauer wrote: > Nit: "URL" in all caps. Done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:74: using ImageRequestMap = std::map<const GURL, ImageRequest>; On 2016/06/09 10:14:30, Bernhard Bauer wrote: > Typedefs should come before method declarations. Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/BUILD.gn (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/BUILD.gn:16: "//url", On 2016/06/08 13:33:29, Marc Treib wrote: > You'll have to add //net here too Good catch https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/image_decoder.h (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_decoder.h:8: #include "base/callback.h" On 2016/06/08 13:33:29, Marc Treib wrote: > callback_forward.h should be enough Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_decoder.h:9: #include "url/gurl.h" On 2016/06/08 13:33:29, Marc Treib wrote: > Not used Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_decoder.h:17: using ImageDecodedCallback = base::Callback<void(const gfx::Image&)> ; On 2016/06/09 10:14:30, Bernhard Bauer wrote: > Nit: no space before semicolon. Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_decoder.h:27: // decoding the image and empty image is passed to the callback. On 2016/06/08 13:33:29, Marc Treib wrote: > s/and/an Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_decoder.h:29: ImageDecodedCallback callback) = 0; On 2016/06/08 13:33:29, Marc Treib wrote: > const ref Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/image_url_fetcher.cc (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:15: :url_request_context_getter_(url_request_context_getter) {} On 2016/06/09 10:14:30, Bernhard Bauer wrote: > Space after colon. Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:24: std::string(), On 2016/06/09 10:14:30, Bernhard Bauer wrote: > If you are passing the same values in here every time, just do that inside of > the method. Sorry I forgot to do this. I remembered that someone once asked me to do it this way for some reason I don't remember. Anyway this seems half baked As discussed with Marc earlier too I moved them inside the method. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:28: request_queue_[url] = std::move(request); I'm not sure if I win anything if I replace this with request_queue_.emplace( url, std::unique_ptr<ImageURLFetcherRequest>( new ImageURLFetcherRequest(url, callback, this))); https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:35: request_queue_.erase(request_it); On 2016/06/09 10:14:30, Bernhard Bauer wrote: > Just directly use the overload that takes a key. You can DCHECK the result to > verify that it actually removed an element. Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:53: if (url_fetcher_.get() != NULL) On 2016/06/08 13:33:29, Marc Treib wrote: > Just > if (url_fetcher_) > should do. But I think this can never happen anyway? In that case, just > DCHECK(!url_fetcher_); Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:67: // TODO(markusheintz): Check if the if else condition can be inverted. Extrac On 2016/06/09 10:14:30, Bernhard Bauer wrote: > Nit: Your devious plan to get around the 80 column limit by cutting off the last > character of the line will not succeed :) It was worth a try. And if I would have counted correctly then I wouldn't even have to cut it off ;-) https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:70: // Return an empty string when the URL requests does complete successfully. On 2016/06/08 13:33:29, Marc Treib wrote: > does *not* complete successfully, right? My code so self descriptive that it points out my wrong comments. I deleted the comment completely as it does only state the obvious. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:73: } else { On 2016/06/09 10:14:30, Bernhard Bauer wrote: > else is not necessary if you return in the if branch. Sorry I forgot to remove the return. Good catch. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:81: image_url_fetcher_->RemoveImageURLFetcherRequest(url_); On 2016/06/08 13:33:29, Marc Treib wrote: > I think here we can get rid of the destruction weirdness by: > - Making the request_queue_ a map<URLFetcher*, unique_ptr<Request>> > - Making the ImageURLFetcher itself be the URLFetcherDelegate > => On URL request completion, it gets passed the corresponding URLFetcher*, and > can thus find the right Request in the map. > > WDYT? puh ... next CL? https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/image_url_fetcher.h (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:23: class ImageURLFetcher { On 2016/06/08 13:33:29, Marc Treib wrote: > I'd call this ImageDataFetcher. WDYT? I like it. Done! https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:34: void FetchImageURL(const GURL& image_url, ImageURLFetcherCallback callback); On 2016/06/08 13:33:29, Marc Treib wrote: > const ref Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:37: // This class represents an active image url fetcher requests. The class On 2016/06/09 10:14:30, Bernhard Bauer wrote: > Nits: > * "This class represents" is redundant. > * "URL" should be in all caps. > * "request" in singular. Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:40: class ImageURLFetcherRequest : public net::URLFetcherDelegate { On 2016/06/09 10:14:30, Bernhard Bauer wrote: > Same as in ImageDecoderImpl: The class definition could move to the .cc file. Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:43: ImageURLFetcherCallback callback, On 2016/06/08 13:33:29, Marc Treib wrote: > const ref Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:56: // This will be called when the URL has been fetched, successfully or not. On 2016/06/09 10:14:30, Bernhard Bauer wrote: > This comment is redundant, as the method contract is just the one from the > interface. Comment removed. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:58: void OnURLFetchComplete(const net::URLFetcher* source) override; On 2016/06/08 13:33:29, Marc Treib wrote: > can be private Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:65: // be run even if the image data could not be fetche successfully. On 2016/06/09 10:14:30, Bernhard Bauer wrote: > Nit: "fetched" Done. https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.h:80: using ImageURLFetcherRequestQueue = I was able to delete this type def too.
LGTM with some more nits. Thanks! https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.cc (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.cc:46: decoder_->RemoveDecodeImageRequest(this); On 2016/06/09 19:58:19, markusheintz_ wrote: > On 2016/06/08 13:33:28, Marc Treib wrote: > > Hm. One possible alternative: Do "delete this;" instead, then you won't need > > |decode_image_requests_| at all. That's what others do, e.g.: > > > https://cs.chromium.org/chromium/src/chrome/browser/download/download_command... > > > > Downside: We won't have any way to cancel requests. WDYT? > > Another downside would still have in item in the queue since this would only > delete the request object, but not remove the queue item pointing to it. My point was that we wouldn't even need the queue. Just create a request and forget about it; it'll delete itself when it's done. https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.h (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.h:16: namespace chrome { On 2016/06/09 19:58:19, markusheintz_ wrote: > On 2016/06/08 13:33:28, Marc Treib wrote: > > We don't usually use this namespace. If you rename the class per blundell's > > suggestion, then we don't need a namespace at all. > > I would actually prefer to use on of our namespaces suggestions as long as the > file lives in this directory. > > I don't remember why I chose to use the chrome namespace in first place. namespace suggestions makes sense, thanks! https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.cc (right): https://codereview.chromium.org/2045233002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:52: pending_net_requests_[image_url].swap(&request); On 2016/06/09 19:58:20, markusheintz_ wrote: > On 2016/06/08 13:33:29, Marc Treib wrote: > > Not your doing, but this is weird... can we do > > pending_net_requests_.emplace(image_url, request); > > instead? Then we also won't need the swap() method anymore. > > Done. Argh, turns out map::emplace isn't available on all platforms yet :-/ (see https://chromium-cpp.appspot.com/). Something like ".insert(std::make_pair(..., ...))" might work, or you can just revert to the previous version. Sorry about the confusion! https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.h (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:55: void swap(ImageRequest* other) { I think this can be removed now https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:83: std::unique_ptr<image_fetcher::ImageDataFetcher> image_url_fetcher_; image_data_fetcher_ now? https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... File components/image_fetcher/image_data_fetcher.cc (right): https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:53: nit: remove the empty line https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:73: // common code from the conditions. I guess you could define the "std::string image_data" outside, and then "if (..success..) GetResponse(&image_data);", and run the callback after the if? https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:89: :url_request_context_getter_(url_request_context_getter) {} nit: there should be a space after the : https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:94: const ImageDataFetcherCallback& callback) { misaligned https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:34: void FetchImageURL(const GURL& image_url, FetchImageData? https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:38: class ImageDataFetcherRequest; optional: Since this is a private inner class, you could also just call it Request? https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:45: std::map<const GURL, std::unique_ptr<ImageDataFetcherRequest>> request_queue_; Just requests_, or maybe pending_requests_? It's not actually a queue. https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... File components/image_fetcher/image_decoder.h (right): https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_decoder.h:7: #include <string>
https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... File components/image_fetcher/image_url_fetcher.cc (right): https://codereview.chromium.org/2045233002/diff/20001/components/image_fetche... components/image_fetcher/image_url_fetcher.cc:81: image_url_fetcher_->RemoveImageURLFetcherRequest(url_); On 2016/06/09 19:58:21, markusheintz_ wrote: > On 2016/06/08 13:33:29, Marc Treib wrote: > > I think here we can get rid of the destruction weirdness by: > > - Making the request_queue_ a map<URLFetcher*, unique_ptr<Request>> > > - Making the ImageURLFetcher itself be the URLFetcherDelegate > > => On URL request completion, it gets passed the corresponding URLFetcher*, > and > > can thus find the right Request in the map. > > > > WDYT? > > puh ... next CL? I think this is a great suggestion. I guess we can then also turn the ImageURLFetcherRequest into a struct. Trying to do this quickly.
I'm a little confused now as to the purpose of these Impl classes: are they actually embedder-level or not? https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.h (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:28: namespace suggestions { If this code is intended to stay in //chrome it shouldn't have any namespace at all. If it's intended to move into //components then this is the right namespace, but then please add a TODO about moving it into //components. Same comment on ImageDecoderImpl (but in that case if it's intended to go into //components why not just add it there now?).
On 2016/06/10 08:38:12, blundell wrote: > I'm a little confused now as to the purpose of these Impl classes: are they > actually embedder-level or not? > > https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... > File chrome/browser/search/suggestions/image_fetcher_impl.h (right): > > https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... > chrome/browser/search/suggestions/image_fetcher_impl.h:28: namespace suggestions > { > If this code is intended to stay in //chrome it shouldn't have any namespace at > all. If it's intended to move into //components then this is the right > namespace, but then please add a TODO about moving it into //components. Same > comment on ImageDecoderImpl (but in that case if it's intended to go into > //components why not just add it there now?). The Decoder needs to stay in Chrome (= the embedder level? ) because it uses SkBitmap which is not used on all platforms e.g. iOS. The image_fetcher component defines the only the interface. Different platforms will provide the implementation. This allows us platform specific imagedecoding e.g. UIImage on iOS. The ImageFetcherImpl. Can be moved to component. But I would love to do this step by step. I added a TODO for this in the next patchset I'll upload
On 2016/06/10 08:51:20, markusheintz_ wrote: > On 2016/06/10 08:38:12, blundell wrote: > > I'm a little confused now as to the purpose of these Impl classes: are they > > actually embedder-level or not? > > > > > https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... > > File chrome/browser/search/suggestions/image_fetcher_impl.h (right): > > > > > https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... > > chrome/browser/search/suggestions/image_fetcher_impl.h:28: namespace > suggestions > > { > > If this code is intended to stay in //chrome it shouldn't have any namespace > at > > all. If it's intended to move into //components then this is the right > > namespace, but then please add a TODO about moving it into //components. Same > > comment on ImageDecoderImpl (but in that case if it's intended to go into > > //components why not just add it there now?). > > The Decoder needs to stay in Chrome (= the embedder level? ) because it uses > SkBitmap which is not used on all platforms e.g. iOS. The image_fetcher > component defines the only the interface. Different platforms will provide the > implementation. This allows us platform specific imagedecoding e.g. UIImage on > iOS. > > The ImageFetcherImpl. Can be moved to component. But I would love to do this > step by step. I added a TODO for this in the next patchset I'll upload SGTM, then ImageDecoderImpl should have no namespace and be named ChromeImageDecoder (and the one in //ios/chrome would be IOSChromeImageDecoder). Fine doing this in followups for you to land this but please make a note of it too.
Address comments round 2 treib@, blundell@
LGTM modulo Marc's nits and Colin's comment. https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.cc (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.cc:20: virtual ~DecodeImageRequest() final {} Not that I'm totally against it, but why is the destructor final? https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.cc (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:67: // call that here. Talk to treib@ about this. Nit: If that last sentence is addressing yourself, there are better reminder systems than the code base of an open-source project ;-) If it is addressing whoever might want to fix that code, you should mark it TODO(treib): instead -- the named person is the point of contact, not the person signed up to do it. https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... File components/image_fetcher/image_data_fetcher.cc (right): https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:73: // common code from the conditions. On 2016/06/10 08:32:37, Marc Treib wrote: > I guess you could define the "std::string image_data" outside, and then "if > (..success..) GetResponse(&image_data);", and run the callback after the if? +1.
https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.cc (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.cc:20: virtual ~DecodeImageRequest() final {} On 2016/06/10 09:02:08, Bernhard Bauer wrote: > Not that I'm totally against it, but why is the destructor final? because on of the builders didn't want to build it without final or override. Since I'm not overriding anything I added final just to get the builder green. https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.cc (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.cc:67: // call that here. Talk to treib@ about this. On 2016/06/10 09:02:08, Bernhard Bauer wrote: > Nit: If that last sentence is addressing yourself, there are better reminder > systems than the code base of an open-source project ;-) If it is addressing > whoever might want to fix that code, you should mark it TODO(treib): instead -- > the named person is the point of contact, not the person signed up to do it. Removed the last sentence. https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.h (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:28: namespace suggestions { On 2016/06/10 08:38:12, blundell wrote: > If this code is intended to stay in //chrome it shouldn't have any namespace at > all. If it's intended to move into //components then this is the right > namespace, but then please add a TODO about moving it into //components. Same > comment on ImageDecoderImpl (but in that case if it's intended to go into > //components why not just add it there now?). Done. https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:55: void swap(ImageRequest* other) { On 2016/06/10 08:32:37, Marc Treib wrote: > I think this can be removed now Done. https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:83: std::unique_ptr<image_fetcher::ImageDataFetcher> image_url_fetcher_; On 2016/06/10 08:32:37, Marc Treib wrote: > image_data_fetcher_ now? Done. https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... File components/image_fetcher/image_data_fetcher.cc (right): https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:53: On 2016/06/10 08:32:37, Marc Treib wrote: > nit: remove the empty line Done. https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:73: // common code from the conditions. On 2016/06/10 08:32:37, Marc Treib wrote: > I guess you could define the "std::string image_data" outside, and then "if > (..success..) GetResponse(&image_data);", and run the callback after the if? Done https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:73: // common code from the conditions. On 2016/06/10 09:02:08, Bernhard Bauer wrote: > On 2016/06/10 08:32:37, Marc Treib wrote: > > I guess you could define the "std::string image_data" outside, and then "if > > (..success..) GetResponse(&image_data);", and run the callback after the if? > > +1. Done. https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:89: :url_request_context_getter_(url_request_context_getter) {} On 2016/06/10 08:32:37, Marc Treib wrote: > nit: there should be a space after the : Done. https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:94: const ImageDataFetcherCallback& callback) { On 2016/06/10 08:32:37, Marc Treib wrote: > misaligned Already Done. I also renamed the method. https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:34: void FetchImageURL(const GURL& image_url, On 2016/06/10 08:32:37, Marc Treib wrote: > FetchImageData? Already done. Sorry I forgot last night and quickly did it this morning in a separate patchset https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:38: class ImageDataFetcherRequest; On 2016/06/10 08:32:37, Marc Treib wrote: > optional: Since this is a private inner class, you could also just call it > Request? Acknowledged. https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:45: std::map<const GURL, std::unique_ptr<ImageDataFetcherRequest>> request_queue_; On 2016/06/10 08:32:37, Marc Treib wrote: > Just requests_, or maybe pending_requests_? It's not actually a queue. I chose pending_requests_. https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... File components/image_fetcher/image_decoder.h (right): https://codereview.chromium.org/2045233002/diff/60001/components/image_fetche... components/image_fetcher/image_decoder.h:7: On 2016/06/10 08:32:37, Marc Treib wrote: > #include <string> Done.
Revert the uses of std::map::emplace again since it is not fully supported yet
s/final/override/ in ~DecodeImageRequest
"
lgtm
markusheintz@chromium.org changed reviewers: + mmenke@chromium.org
Adding mmenke@ for DEPs approval on /net
Fix long line
https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_decoder_impl.cc (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_decoder_impl.cc:20: virtual ~DecodeImageRequest() final {} On 2016/06/10 09:43:14, markusheintz_ wrote: > On 2016/06/10 09:02:08, Bernhard Bauer wrote: > > Not that I'm totally against it, but why is the destructor final? > > because on of the builders didn't want to build it without final or override. > Since I'm not overriding anything I added final just to get the builder green. No, you were overriding the virtual destructor from the superclass, but I see you changed it. https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.h (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:55: void swap(ImageRequest* other) { On 2016/06/10 09:43:14, markusheintz_ wrote: > On 2016/06/10 08:32:37, Marc Treib wrote: > > I think this can be removed now > > Done. Not really?
https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.h (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:55: void swap(ImageRequest* other) { On 2016/06/10 12:48:25, Bernhard Bauer wrote: > On 2016/06/10 09:43:14, markusheintz_ wrote: > > On 2016/06/10 08:32:37, Marc Treib wrote: > > > I think this can be removed now > > > > Done. > > Not really? It could have been removed if we could use map::emplace, but turns out that's not available everywhere yet :( So unfortunately this can't be done yet.
https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... File chrome/browser/search/suggestions/image_fetcher_impl.h (right): https://codereview.chromium.org/2045233002/diff/60001/chrome/browser/search/s... chrome/browser/search/suggestions/image_fetcher_impl.h:55: void swap(ImageRequest* other) { On 2016/06/10 12:50:29, Marc Treib wrote: > On 2016/06/10 12:48:25, Bernhard Bauer wrote: > > On 2016/06/10 09:43:14, markusheintz_ wrote: > > > On 2016/06/10 08:32:37, Marc Treib wrote: > > > > I think this can be removed now > > > > > > Done. > > > > Not really? > > It could have been removed if we could use map::emplace, but turns out that's > not available everywhere yet :( > So unfortunately this can't be done yet. Oh, sorry, I misinterpreted the meaning of "Done" 😃
net dependency (And use of net code) LGTM https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... File components/image_fetcher/image_data_fetcher.cc (right): https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.cc:63: net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE); You don't need to set referrer or referrer policy. Without a referrer set, we don't send a referrer. And referrer policy isn't relevant in that case, either. https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.cc:64: url_fetcher_->SetLoadFlags(net::LOAD_NORMAL); Not needed. This is the default, and always will be. https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:14: #include "net/url_request/url_request.h" None of these includes are needed here (URLRequest isn't needed at all, other two should be in the cc file). https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:26: base::Callback<void(const std::string& image_data)>; include <string>, callback.h https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:28: ImageDataFetcher(net::URLRequestContextGetter* url_request_context_getter); explicit https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:48: net::URLRequestContextGetter* url_request_context_getter_; scoped_refptr (And include ref_counted.h)
https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... File components/image_fetcher/image_data_fetcher.cc (right): https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.cc:63: net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE); On 2016/06/10 17:19:58, mmenke wrote: > You don't need to set referrer or referrer policy. Without a referrer set, we > don't send a referrer. And referrer policy isn't relevant in that case, either. I removed the SetReferrer and SetReferrerPolicy calls. https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.cc:64: url_fetcher_->SetLoadFlags(net::LOAD_NORMAL); On 2016/06/10 17:19:58, mmenke wrote: > Not needed. This is the default, and always will be. Removed https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:14: #include "net/url_request/url_request.h" On 2016/06/10 17:19:58, mmenke wrote: > None of these includes are needed here (URLRequest isn't needed at all, other > two should be in the cc file). Good catch. I forgot to move them when I moved the inner class to the cc file. https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:26: base::Callback<void(const std::string& image_data)>; On 2016/06/10 17:19:58, mmenke wrote: > include <string>, callback.h Done. https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:28: ImageDataFetcher(net::URLRequestContextGetter* url_request_context_getter); On 2016/06/10 17:19:58, mmenke wrote: > explicit Done. https://codereview.chromium.org/2045233002/diff/200001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:48: net::URLRequestContextGetter* url_request_context_getter_; On 2016/06/10 17:19:58, mmenke wrote: > scoped_refptr (And include ref_counted.h) Done.
Address comments mmenke@
Add dep to net in components/image_fetcher.gypi
"
"
"
The CQ bit was checked by markusheintz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org, blundell@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2045233002/#ps340001 (title: """)
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2045233002/340001
Message was sent while issue was closed.
Description was changed from ========== Split the code for fetching images and for decoding images into two separate classes. This will allow to use common code for image URL fetching and platform specific implementations for image decoding. BUG=609127 ========== to ========== Split the code for fetching images and for decoding images into two separate classes. This will allow to use common code for image URL fetching and platform specific implementations for image decoding. BUG=609127 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Split the code for fetching images and for decoding images into two separate classes. This will allow to use common code for image URL fetching and platform specific implementations for image decoding. BUG=609127 ========== to ========== Split the code for fetching images and for decoding images into two separate classes. This will allow to use common code for image URL fetching and platform specific implementations for image decoding. BUG=609127 Committed: https://crrev.com/d8513537879d4e6d452cbd66ac4c203752eab916 Cr-Commit-Position: refs/heads/master@{#399457} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/d8513537879d4e6d452cbd66ac4c203752eab916 Cr-Commit-Position: refs/heads/master@{#399457}
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.chromium.org/2068783003/ by mfoltz@chromium.org. The reason for reverting is: Causing https://bugs.chromium.org/p/chromium/issues/detail?id=620000 and is ReleaseBlock-Dev. NOTREECHECKS=true NOPRESUBMIT=true NOTRY=true TBR=markusheintz .
Message was sent while issue was closed.
On 2016/06/14 19:05:19, mark a. foltz wrote: > A revert of this CL (patchset #18 id:340001) has been created in > https://codereview.chromium.org/2068783003/ by mailto:mfoltz@chromium.org. > > The reason for reverting is: Causing > https://bugs.chromium.org/p/chromium/issues/detail?id=620000 and is > ReleaseBlock-Dev. > > NOTREECHECKS=true > NOPRESUBMIT=true > NOTRY=true > TBR=markusheintz > . Sorry that this CL caused some trouble. I found the issue: https://codereview.chromium.org/2064793004/diff2/1:20001/chrome/browser/searc... |
