Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2663213002:
Create the IOSImageDataFetcherWrapper (Closed)
|
Created:
3 years, 10 months ago by gambard Modified:
3 years, 10 months ago CC:
chromium-reviews, pkl (ping after 24h if needed), droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, Eugene But (OOO till 7-30), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org, Marc Treib Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate the IOSImageDataFetcherWrapper
Create a wrapper of ImageDataFetcher for iOS.
This class has a method to fetches and returns the image decoded, if it is a
WebP image, as iOS does not decode WebP.
BUG=683918
Review-Url: https://codereview.chromium.org/2663213002
Cr-Commit-Position: refs/heads/master@{#447752}
Committed: https://chromium.googlesource.com/chromium/src/+/3c3d18557ba0b9d0e31598ed62e7129f97a582c7
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address comments #
Total comments: 47
Patch Set 3 : Address comments #
Total comments: 10
Patch Set 4 : Address comments #Messages
Total messages: 20 (8 generated)
gambard@chromium.org changed reviewers: + markusheintz@chromium.org
PTAL.
Description was changed from ========== Create the IOSImageDataFetcherWrapper Create a wrapper of ImageDataFetcher for iOS. This class has a method to fetches and returns the image decoded, if it is a WebP image, as iOS does not decode WebP. BUG=683918 ========== to ========== Create the IOSImageDataFetcherWrapper Create a wrapper of ImageDataFetcher for iOS. This class has a method to fetches and returns the image decoded, if it is a WebP image, as iOS does not decode WebP. BUG=683918 ==========
https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right): https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:27: class WebpDecoderDelegate : public webp_transcode::WebpDecoder::Delegate { Don't need to do this in this CL. Just want to document it for followups and to get your thoughts on this: This is almost an identical copy of the inner class in ios/chrome/browser/suggestions/ios_image_decoder_impl.mm I wonder whether this logic should be shared by having a WebpDecoderDelegate that is used by both files. I guess the problem of the ImageDecoder interface is that returns a gfx::Image which is not what you want. Maybe this can be tweaked too. probably harder.
treib@chromium.org changed reviewers: + treib@chromium.org
Just a few comments. I'll leave the bulk of the review to the ios experts :) https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h (right): https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:36: // The TaskRunner is used to eventually decode the image. ...in case it is WebP (right?) https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right): https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:52: static const char kWEBPFirstBytes[4] = {'R', 'I', 'F', 'F'}; Is this enough to accurately identify WebPs? The format seems to be RIFF????WEBP (where ? means any character) so this might result in some false positives? https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:77: base::MakeUnique<ImageDataFetcher>(url_request_context_getter); This could move into the initializer list above, and I think it could be a direct member rather than a pointer. https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:86: if (!callback) Don't handle DCHECK failures, see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2663213002/diff/1/ios/chrome/browser/suggesti... File ios/chrome/browser/suggestions/image_fetcher_impl.mm (left): https://codereview.chromium.org/2663213002/diff/1/ios/chrome/browser/suggesti... ios/chrome/browser/suggestions/image_fetcher_impl.mm:12: #import "ios/web/public/image_fetcher/image_data_fetcher.h" So is the plan to eventually remove the ImageDataFetcher in ios/web/public/ ?
gambard@chromium.org changed reviewers: + sdefresne@chromium.org
Thanks, PTAL. https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h (right): https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:36: // The TaskRunner is used to eventually decode the image. On 2017/01/31 16:17:25, Marc Treib wrote: > ...in case it is WebP (right?) Done. https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right): https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:27: class WebpDecoderDelegate : public webp_transcode::WebpDecoder::Delegate { On 2017/01/31 15:51:41, markusheintz_ wrote: > Don't need to do this in this CL. Just want to document it for followups and to > get your thoughts on this: > > This is almost an identical copy of the inner class in > ios/chrome/browser/suggestions/ios_image_decoder_impl.mm > > I wonder whether this logic should be shared by having a WebpDecoderDelegate > that is used by both files. > > I guess the problem of the ImageDecoder interface is that returns a gfx::Image > which is not what you want. > Maybe this can be tweaked too. probably harder. Yes, I plan to move this code to a shared location with the image decoder and the tests. But this CL is already getting big so it will be in a future CL. https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:52: static const char kWEBPFirstBytes[4] = {'R', 'I', 'F', 'F'}; On 2017/01/31 16:17:25, Marc Treib wrote: > Is this enough to accurately identify WebPs? The format seems to be > RIFF????WEBP (where ? means any character) > so this might result in some false positives? When I move the WebpDecoderDelegate to a shared location I will use the IsWebpImage currently in ios/chrome/browser/suggestions/ios_image_decoder_impl.mm which checks for correct header. But I should probably do it the right way now :) https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:77: base::MakeUnique<ImageDataFetcher>(url_request_context_getter); On 2017/01/31 16:17:25, Marc Treib wrote: > This could move into the initializer list above, and I think it could be a > direct member rather than a pointer. Done. I usually use pointer to use forward declaration. https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:86: if (!callback) On 2017/01/31 16:17:25, Marc Treib wrote: > Don't handle DCHECK failures, see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2663213002/diff/1/ios/chrome/browser/suggesti... File ios/chrome/browser/suggestions/image_fetcher_impl.mm (left): https://codereview.chromium.org/2663213002/diff/1/ios/chrome/browser/suggesti... ios/chrome/browser/suggestions/image_fetcher_impl.mm:12: #import "ios/web/public/image_fetcher/image_data_fetcher.h" On 2017/01/31 16:17:25, Marc Treib wrote: > So is the plan to eventually remove the ImageDataFetcher in ios/web/public/ ? Yes, the first step is to remove the ImageDataFetcher in ios/web/public, the second to remove this ImageFetcher and use components/image_fetcher/image_fetcher_impl.h (which would become ImageFetcher).
LGTM, but please wait for an ios expert to also approve, since I'm quite unfamiliar with all that :) https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right): https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:52: static const char kWEBPFirstBytes[4] = {'R', 'I', 'F', 'F'}; On 2017/01/31 16:55:54, gambard wrote: > On 2017/01/31 16:17:25, Marc Treib wrote: > > Is this enough to accurately identify WebPs? The format seems to be > > RIFF????WEBP (where ? means any character) > > so this might result in some false positives? > > When I move the WebpDecoderDelegate to a shared location I will use the > IsWebpImage currently in > ios/chrome/browser/suggestions/ios_image_decoder_impl.mm which checks for > correct header. > > But I should probably do it the right way now :) Yup, I think that's better :) https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:77: base::MakeUnique<ImageDataFetcher>(url_request_context_getter); On 2017/01/31 16:55:54, gambard wrote: > On 2017/01/31 16:17:25, Marc Treib wrote: > > This could move into the initializer list above, and I think it could be a > > direct member rather than a pointer. > > Done. > I usually use pointer to use forward declaration. IIRC, the style guide recommends against using a pointer for only that reason. But then, there's so many cases where you need a pointer anyway, so.. meh. https://codereview.chromium.org/2663213002/diff/1/ios/chrome/browser/suggesti... File ios/chrome/browser/suggestions/image_fetcher_impl.mm (left): https://codereview.chromium.org/2663213002/diff/1/ios/chrome/browser/suggesti... ios/chrome/browser/suggestions/image_fetcher_impl.mm:12: #import "ios/web/public/image_fetcher/image_data_fetcher.h" On 2017/01/31 16:55:54, gambard wrote: > On 2017/01/31 16:17:25, Marc Treib wrote: > > So is the plan to eventually remove the ImageDataFetcher in ios/web/public/ ? > > Yes, the first step is to remove the ImageDataFetcher in ios/web/public, the > second to remove this ImageFetcher and use > components/image_fetcher/image_fetcher_impl.h (which would become ImageFetcher). Alright. I don't quite see the path yet; the ImageDataFetcherWrapper will make it hard to have a common ImageFetcher implementation. But we'll get there somehow :) https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right): https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:52: static const char kWEBPSecondByte[] = "WEBP"; nit: "static" isn't required, since this is inside an anonymous namespace (unless there's some difference between C++ and ObjC that I'm not aware of?)
Overall approach LGTM. Please check for the correct image header . And please let some iOS expert take another look :-) Thanks a lot. https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right): https://codereview.chromium.org/2663213002/diff/1/components/image_fetcher/io... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:52: static const char kWEBPFirstBytes[4] = {'R', 'I', 'F', 'F'}; On 2017/01/31 17:25:24, Marc Treib wrote: > On 2017/01/31 16:55:54, gambard wrote: > > On 2017/01/31 16:17:25, Marc Treib wrote: > > > Is this enough to accurately identify WebPs? The format seems to be > > > RIFF????WEBP (where ? means any character) > > > so this might result in some false positives? > > > > When I move the WebpDecoderDelegate to a shared location I will use the > > IsWebpImage currently in > > ios/chrome/browser/suggestions/ios_image_decoder_impl.mm which checks for > > correct header. > > > > But I should probably do it the right way now :) > > Yup, I think that's better :) +1
https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... File components/image_fetcher/ios/DEPS (right): https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/DEPS:4: nit: remove blank line https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h (right): https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:12: #import "components/image_fetcher/image_data_fetcher.h" #import -> #include https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:37: virtual ~IOSImageDataFetcherWrapper(); I think you don't need "virtual" here. The class does not have virtual methods and does not appear to be designed to be inherited from. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:54: }; DISALLOW_COPY_AND_ASSIGN(IOSImageDataFetcherWrapper) https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:55: } // namespace image_fetcher nit: blank line before closing the namespace or remove the one after opening it https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right): https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:47: base::scoped_nsobject<NSMutableData> decoded_image_; Just use a raw Objective-C pointer here, ARC takes care of the reference for you. NSMutableData* decoded_image_; https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:48: }; DISALLOW_COPY_AND_ASSIGN(WebpDecoderDelegate) https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:51: static const char kWEBPFirstByte[] = "RIFF"; nit: I do not think those are bytes (i.e. one char) but instead words (i.e. four chars), so maybe rename them too the following const char kWEBPFirstMagicWord[] = "RIFF"; const char kWEBPSecondMagicWord[] = "WEBP"; or maybe even const char kWEBPFirstMagicPattern[] = "RIFF"; const char kWEBPSecondMagicPattern[] = "WEBP"; https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:52: static const char kWEBPSecondByte[] = "WEBP"; On 2017/01/31 17:25:24, Marc Treib wrote: > nit: "static" isn't required, since this is inside an anonymous namespace > (unless there's some difference between C++ and ObjC that I'm not aware of?) +1, "static" is not required here. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:62: return base::scoped_nsobject<NSData>(delegate->data()); If this code were to be build without ARC, then this line would lead to overreleasing the NSData* because base::scoped_nsobject<> steal the reference that it is passed, and WebpDecoderDelegate::data() does not increase the reference of the object before returning it. This would lead to two different base::scoped_nsobject<> calling -release on the NSData* but with only one corresponding -retain (the one implicit in the -alloc -init dance). Moreover, since WebpDecoderDelegate::data() returns a NSData* and this method returns a NSData* too, the roundtrip to a base::scoped_nsobject<> is useless (in addition of being harmful) whether you build with or without ARC. TL;DR: remove this base::scoped_nsobject<> here. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:98: if (image_data.compare(0, 4, kWEBPFirstByte) == 0 && Can you change this test in early return mode too? if (image_data.size() < 12 || image_data.compare(0, 4, kWEBPFirstByte) != 0 || image_data.compare(8, 4, kWEBPSecondByte) != 0) { callback(data); return; } base::PostTaskAndReplyWithResult( task_runner.get(), FROM_HERE, base::Bind(&DecodeWebpImage, data), base::BindBlockArc(callback)); https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm (right): https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:25: I think this file is compiled with ARC (otherwise it is leaking the NSMutableData). Please add the guard here. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:28: static unsigned char kJPGImage[] = { "static" is not necessary here, but you need "const" const unsigned char kJPGImage[] = { https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:49: 0, 178, 192, 7, 255, 217}; nit: use a trailing comma in the array definition and clang-format will put the closing brace on a separate line, i.e. it will be formatted as this I think: 0, 178, 192, 7, 255, 217, }; https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:51: static unsigned char kPNGImage[] = { ditto https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:74: 130}; ditto https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:76: static unsigned char kWEBPImage[] = { ditto https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:81: 3, 0, 52, 37, 164, 0, 3, 112, 0, 254, 251, 253, 80, 0}; ditto https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:83: static const char kTestUrl[] = "http://www.img.com"; "static" is not necessary here https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:85: static const char kWEBPHeaderResponse[] = ditto https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:88: class WebpDecoderDelegate : public webp_transcode::WebpDecoder::Delegate { This code looks like it is copy-n-pasted from the implementation file (both the class and the DecodeWebpImage). Could they be exported by the implementation and the code shared instead of using copy in the test? https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:151: base::Thread worker_thread_; I don't think the Thread is necessary here. Can you try to remove it and instead pass the MessageLoop task runner to the image_fetcher? I think it should work (it is usually unnecessary to use multiple thread in tests, a sequential task runner is enough and this is what MessageLoop provides). https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:175: fetcher->SetResponseString(std::string((char*)kJPGImage, sizeof(kJPGImage))); C style cast are forbidden according to the style guide (https://google.github.io/styleguide/cppguide.html#Casting), so please use explicit cast here and below for the whole file. fetcher->SetResponseString(std::string(static_cast<const char*>(kJPGImage), sizeof(kJPGImage)));
Thanks, PTAL. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... File components/image_fetcher/ios/DEPS (right): https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/DEPS:4: On 2017/02/01 09:43:44, sdefresne wrote: > nit: remove blank line Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h (right): https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:12: #import "components/image_fetcher/image_data_fetcher.h" On 2017/02/01 09:43:44, sdefresne wrote: > #import -> #include Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:37: virtual ~IOSImageDataFetcherWrapper(); On 2017/02/01 09:43:44, sdefresne wrote: > I think you don't need "virtual" here. The class does not have virtual methods > and does not appear to be designed to be inherited from. I am inheriting to create mock for tests (in a future CL). https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:54: }; On 2017/02/01 09:43:44, sdefresne wrote: > DISALLOW_COPY_AND_ASSIGN(IOSImageDataFetcherWrapper) Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:55: } // namespace image_fetcher On 2017/02/01 09:43:44, sdefresne wrote: > nit: blank line before closing the namespace or remove the one after opening it Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right): https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:47: base::scoped_nsobject<NSMutableData> decoded_image_; On 2017/02/01 09:43:44, sdefresne wrote: > Just use a raw Objective-C pointer here, ARC takes care of the reference for > you. > > NSMutableData* decoded_image_; Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:48: }; On 2017/02/01 09:43:44, sdefresne wrote: > DISALLOW_COPY_AND_ASSIGN(WebpDecoderDelegate) Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:51: static const char kWEBPFirstByte[] = "RIFF"; On 2017/02/01 09:43:44, sdefresne wrote: > nit: I do not think those are bytes (i.e. one char) but instead words (i.e. four > chars), so maybe rename them too the following > > const char kWEBPFirstMagicWord[] = "RIFF"; > const char kWEBPSecondMagicWord[] = "WEBP"; > > or maybe even > > const char kWEBPFirstMagicPattern[] = "RIFF"; > const char kWEBPSecondMagicPattern[] = "WEBP"; Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:52: static const char kWEBPSecondByte[] = "WEBP"; On 2017/02/01 09:43:44, sdefresne wrote: > On 2017/01/31 17:25:24, Marc Treib wrote: > > nit: "static" isn't required, since this is inside an anonymous namespace > > (unless there's some difference between C++ and ObjC that I'm not aware of?) > > +1, "static" is not required here. Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:62: return base::scoped_nsobject<NSData>(delegate->data()); On 2017/02/01 09:43:44, sdefresne wrote: > If this code were to be build without ARC, then this line would lead to > overreleasing the NSData* because base::scoped_nsobject<> steal the reference > that it is passed, and WebpDecoderDelegate::data() does not increase the > reference of the object before returning it. This would lead to two different > base::scoped_nsobject<> calling -release on the NSData* but with only one > corresponding -retain (the one implicit in the -alloc -init dance). Moreover, > since WebpDecoderDelegate::data() returns a NSData* and this method returns a > NSData* too, the roundtrip to a base::scoped_nsobject<> is useless (in addition > of being harmful) whether you build with or without ARC. > > TL;DR: remove this base::scoped_nsobject<> here. Done. Thanks for the explanation! https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:98: if (image_data.compare(0, 4, kWEBPFirstByte) == 0 && On 2017/02/01 09:43:44, sdefresne wrote: > Can you change this test in early return mode too? > > if (image_data.size() < 12 || > image_data.compare(0, 4, kWEBPFirstByte) != 0 || > image_data.compare(8, 4, kWEBPSecondByte) != 0) { > callback(data); > return; > } > > base::PostTaskAndReplyWithResult( > task_runner.get(), FROM_HERE, > base::Bind(&DecodeWebpImage, data), > base::BindBlockArc(callback)); Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm (right): https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:25: On 2017/02/01 09:43:44, sdefresne wrote: > I think this file is compiled with ARC (otherwise it is leaking the > NSMutableData). Please add the guard here. Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:28: static unsigned char kJPGImage[] = { On 2017/02/01 09:43:44, sdefresne wrote: > "static" is not necessary here, but you need "const" > > const unsigned char kJPGImage[] = { Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:49: 0, 178, 192, 7, 255, 217}; On 2017/02/01 09:43:45, sdefresne wrote: > nit: use a trailing comma in the array definition and clang-format will put the > closing brace on a separate line, i.e. it will be formatted as this I think: > > 0, 178, 192, 7, 255, 217, > }; Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:51: static unsigned char kPNGImage[] = { On 2017/02/01 09:43:45, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:74: 130}; On 2017/02/01 09:43:45, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:76: static unsigned char kWEBPImage[] = { On 2017/02/01 09:43:45, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:81: 3, 0, 52, 37, 164, 0, 3, 112, 0, 254, 251, 253, 80, 0}; On 2017/02/01 09:43:44, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:83: static const char kTestUrl[] = "http://www.img.com"; On 2017/02/01 09:43:44, sdefresne wrote: > "static" is not necessary here Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:85: static const char kWEBPHeaderResponse[] = On 2017/02/01 09:43:44, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:88: class WebpDecoderDelegate : public webp_transcode::WebpDecoder::Delegate { On 2017/02/01 09:43:45, sdefresne wrote: > This code looks like it is copy-n-pasted from the implementation file (both the > class and the DecodeWebpImage). Could they be exported by the implementation and > the code shared instead of using copy in the test? Yes, but it is also shared with other classes. I will create a common class in a future CL. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:151: base::Thread worker_thread_; On 2017/02/01 09:43:45, sdefresne wrote: > I don't think the Thread is necessary here. Can you try to remove it and instead > pass the MessageLoop task runner to the image_fetcher? I think it should work > (it is usually unnecessary to use multiple thread in tests, a sequential task > runner is enough and this is what MessageLoop provides). Done. https://codereview.chromium.org/2663213002/diff/20001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:175: fetcher->SetResponseString(std::string((char*)kJPGImage, sizeof(kJPGImage))); On 2017/02/01 09:43:45, sdefresne wrote: > C style cast are forbidden according to the style guide > (https://google.github.io/styleguide/cppguide.html#Casting), so please use > explicit cast here and below for the whole file. > > fetcher->SetResponseString(std::string(static_cast<const char*>(kJPGImage), > sizeof(kJPGImage))); Done. Changed to reinterpret_cast because static_cast from unsigned char* to char* is not allowed.
lgtm all nits/comments can be addressed in a followup CL if you want https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... File components/image_fetcher/ios/BUILD.gn (right): https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... components/image_fetcher/ios/BUILD.gn:6: configs += [ "//build/config/compiler:enable_arc" ] nit: please move the "configs" line at the bottom of the source_set declaration (I know that the declaration added by the conversion script are at the top, but I think they are better at the end and plan to move them all once the conversion is over) https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... components/image_fetcher/ios/BUILD.gn:20: configs += [ "//build/config/compiler:enable_arc" ] ditto https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h (right): https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:37: virtual ~IOSImageDataFetcherWrapper(); nit: can you mark as virtual in that CL the method that you'll mock in the followup CL? https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right): https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:48: ~WebpDecoderDelegate() override {} nit: use "= default" here too for consistency https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm (right): https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:95: class WebpDecoderDelegate : public webp_transcode::WebpDecoder::Delegate { nit: please add a TODO() to refactor this with the production class
Thanks! https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... File components/image_fetcher/ios/BUILD.gn (right): https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... components/image_fetcher/ios/BUILD.gn:6: configs += [ "//build/config/compiler:enable_arc" ] On 2017/02/02 10:42:05, sdefresne wrote: > nit: please move the "configs" line at the bottom of the source_set declaration > (I know that the declaration added by the conversion script are at the top, but > I think they are better at the end and plan to move them all once the conversion > is over) Done. https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... components/image_fetcher/ios/BUILD.gn:20: configs += [ "//build/config/compiler:enable_arc" ] On 2017/02/02 10:42:05, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h (right): https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:37: virtual ~IOSImageDataFetcherWrapper(); On 2017/02/02 10:42:05, sdefresne wrote: > nit: can you mark as virtual in that CL the method that you'll mock in the > followup CL? Done. https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right): https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:48: ~WebpDecoderDelegate() override {} On 2017/02/02 10:42:05, sdefresne wrote: > nit: use "= default" here too for consistency Done. https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm (right): https://codereview.chromium.org/2663213002/diff/40001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm:95: class WebpDecoderDelegate : public webp_transcode::WebpDecoder::Delegate { On 2017/02/02 10:42:05, sdefresne wrote: > nit: please add a TODO() to refactor this with the production class Done.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, markusheintz@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2663213002/#ps60001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486036627027850,
"parent_rev": "a5d0ce613e889795f21667d99e7c080292bcdbe8", "commit_rev":
"3c3d18557ba0b9d0e31598ed62e7129f97a582c7"}
Message was sent while issue was closed.
Description was changed from ========== Create the IOSImageDataFetcherWrapper Create a wrapper of ImageDataFetcher for iOS. This class has a method to fetches and returns the image decoded, if it is a WebP image, as iOS does not decode WebP. BUG=683918 ========== to ========== Create the IOSImageDataFetcherWrapper Create a wrapper of ImageDataFetcher for iOS. This class has a method to fetches and returns the image decoded, if it is a WebP image, as iOS does not decode WebP. BUG=683918 Review-Url: https://codereview.chromium.org/2663213002 Cr-Commit-Position: refs/heads/master@{#447752} Committed: https://chromium.googlesource.com/chromium/src/+/3c3d18557ba0b9d0e31598ed62e7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3c3d18557ba0b9d0e31598ed62e7... |
