Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2652893005:
Create the IOSImageDataFetcher (Closed)
|
Created:
3 years, 11 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate the IOSImageDataFetcher
Create a subclass of ImageDataFetcher for iOS.
As iOS does not decode WebP, this class has a method to returns the image decoded
if it is a WebP image.
BUG=683918
Patch Set 1 #Patch Set 2 : Use it in Suggestions #Patch Set 3 : Cleanup #Patch Set 4 : Remove virtual #
Total comments: 12
Patch Set 5 : Address comments #
Total comments: 4
Messages
Total messages: 28 (4 generated)
Description was changed from ========== Create the IOSImageDataFetcher Create a subclass of ImageDataFetcher for iOS. BUG=683918 ========== to ========== Create the IOSImageDataFetcher Create a subclass of ImageDataFetcher for iOS. As iOS does not decode WebP, this class has a method to returns the image decoded if it is a WebP image. BUG=683918 ==========
gambard@chromium.org changed reviewers: + eugenebut@chromium.org, justincohen@chromium.org, sdefresne@chromium.org, treib@chromium.org
PTAL. +justincohen@ for owners ios/chrome/browser/suggestions +eugenebut@ for ios/web/public include in DEPS.
components/image_fetcher/ios/DEPS and ios/web lgtm
It's great seeing components/image_fetcher getting more widespread usage, thanks for doing this! That said, I have some concerns with the design - my main goal is to make the ImageFetcher API consistent across platforms. Clients shouldn't have to worry about getting the correct instance for the platform. Concrete comments/suggestions below. Also +markusheintz, who previously worked on getting the component ready for ios, and who might have more thoughts about the design. Again, thanks for doing this, and sorry about the somewhat messy state the code is in right now wrt ios :-/ https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:30: const net::URLFetcher* source)>; You'll have to update the tests for the new param. It'd also be great to add some tests for the new behavior. That said, this seems like a somewhat weird interface: This class is supposed to abstract away the details of the URLFetcher etc. What exact info does your client need? Would it be enough to return some sort of error code instead of the URLFetcher? https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... File components/image_fetcher/ios/BUILD.gn (right): https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/ios/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher.h (right): https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher.h:15: class IOSImageDataFetcher : public ImageDataFetcher { I don't think inheritance is the right tool here. As it is, this class exposes two interfaces: The base ImageDataFetcher one, and the new method FetchImageDataWebpDecoded. And on ios, using the base interface would be wrong (at least in most circumstances), because it'll return unusable WebP data. I see a few alternatives; not sure yet which is the best: - Use composition instead of inheritance: Make this a separate class which just uses an instance of ImageDataFetcher. This is probably the least effort coming from the current CL, but it'll make ios very different from other platforms - essentially there won't be a common image_fetcher interface anymore. - Actually implement the ImageDataFetcher interface: Instead of defining a new method, override the existing FetchImageData to do the right thing. Then we can define a factory method somewhere that will instantiate the right thing based on the platform, and the rest of the code doesn't have to know. Downside: It's implementation inheritance, which is mostly considered a bad thing. - Similar, but maybe cleaner design-wise: Make ImageDataFetcher an interface, with different implementations for ios vs other platforms. This means factoring the common URL fetching code into a separate helper class. - Implement the transcoding in ios's ImageFetcherImpl, rather than in ImageDataFetcher. WDYT? https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher.mm (right): https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher.mm:85: // response nit: misformatted comment https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher.mm:94: if (http_response_code != net::HTTP_OK) { nit: I think this would be clearer as if (response_code == HTTP_OK || SchemeIs(kDataScheme)) instead of mangling the response code.
treib@chromium.org changed reviewers: + markusheintz@chromium.org
Actually +markusheintz. Markus, please look at my comment on the IOSImageDataFetcher class - have you already thought about the design for this particular part?
blundell@chromium.org changed reviewers: + blundell@chromium.org
https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher.h (right): https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher.h:15: class IOSImageDataFetcher : public ImageDataFetcher { On 2017/01/25 10:37:55, Marc Treib wrote: > I don't think inheritance is the right tool here. As it is, this class exposes > two interfaces: The base ImageDataFetcher one, and the new method > FetchImageDataWebpDecoded. And on ios, using the base interface would be wrong > (at least in most circumstances), because it'll return unusable WebP data. > > I see a few alternatives; not sure yet which is the best: > > - Use composition instead of inheritance: Make this a separate class which just > uses an instance of ImageDataFetcher. This is probably the least effort coming > from the current CL, but it'll make ios very different from other platforms - > essentially there won't be a common image_fetcher interface anymore. > > - Actually implement the ImageDataFetcher interface: Instead of defining a new > method, override the existing FetchImageData to do the right thing. Then we can > define a factory method somewhere that will instantiate the right thing based on > the platform, and the rest of the code doesn't have to know. Downside: It's > implementation inheritance, which is mostly considered a bad thing. > > - Similar, but maybe cleaner design-wise: Make ImageDataFetcher an interface, > with different implementations for ios vs other platforms. This means factoring > the common URL fetching code into a separate helper class. With the caveat that I'm not close to the details here, this one ^^^ reads as the cleanest option. > > - Implement the transcoding in ios's ImageFetcherImpl, rather than in > ImageDataFetcher. > > WDYT?
Thanks for your feedback! Only addressing the high level design comment: The WebP decoder is taking NSData instead of string, and I am not sure it makes sense to move std::string to NSData then back to std::string. I tried to do this at the beginning (this is why I am inheriting in the first place) and it was a lot of work. So the API is different (this is why I am defining a new callback type) and sharing a common interface might be difficult. Looking back at this CL, this looks like a helper class more than a subclass, and should probably be used as it: either as an util class or through composition. WDYT?
On 2017/01/25 11:56:57, gambard wrote: > Thanks for your feedback! > Only addressing the high level design comment: > The WebP decoder is taking NSData instead of string, and I am not sure it makes > sense to move std::string to NSData then back to std::string. I tried to do this > at the beginning (this is why I am inheriting in the first place) and it was a > lot of work. > So the API is different (this is why I am defining a new callback type) and > sharing a common interface might be difficult. > > Looking back at this CL, this looks like a helper class more than a subclass, > and should probably be used as it: either as an util class or through > composition. > > WDYT? This sounds more or less like my last suggestion above - the transcoding logic would move to the ImageFetcherImpl. I'd be okay with that, but it might be worth waiting for Markus' reply before investing too much time; maybe he has a better idea on how to share as much code as possible between platforms. Thanks!
Great that someone is picking this up. :-) I have to refresh my memory where in the process I stopped https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:30: const net::URLFetcher* source)>; On 2017/01/25 10:37:55, Marc Treib wrote: > You'll have to update the tests for the new param. It'd also be great to add > some tests for the new behavior. > > That said, this seems like a somewhat weird interface: This class is supposed to > abstract away the details of the URLFetcher etc. > What exact info does your client need? Would it be enough to return some sort of > error code instead of the URLFetcher? It would be great to learn what motivates this change.
https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:30: const net::URLFetcher* source)>; On 2017/01/25 12:03:45, markusheintz_ wrote: > On 2017/01/25 10:37:55, Marc Treib wrote: > > You'll have to update the tests for the new param. It'd also be great to add > > some tests for the new behavior. > > > > That said, this seems like a somewhat weird interface: This class is supposed > to > > abstract away the details of the URLFetcher etc. > > What exact info does your client need? Would it be enough to return some sort > of > > error code instead of the URLFetcher? > > It would be great to learn what motivates this change. The IOSImageDataFetcher class is using the URLFetcher to get the mime type (to check if it is WebP), the http response (needed by some classes in the callback, I need to add it) and the original url (to see if it is a data URI or not). Considering that, I think I need it in the callback I am using to decode the WebP. With the architecture as it is, it seemed to be the smallest change.
The original idea I had in mind was different. There already exists an ios_image_decoder that supports WebP. this means the ImageFetcher can be used accross all platforms but for the actual decoding part (which is the piece that is really platform dependend ) there are platform specific decoders. The advantage is that you can put images in caches oder local dbs and can use the plattfrom specific decoder to decode them too. This reduces code duplication. I have a design doc that I can share with you.
On 2017/01/25 13:19:50, markusheintz_ wrote: > The original idea I had in mind was different. > > There already exists an ios_image_decoder that supports WebP. > > this means the ImageFetcher can be used accross all platforms but for the actual > decoding part (which is the piece that is really platform dependend ) there are > platform specific decoders. > > The advantage is that you can put images in caches oder local dbs and can use > the plattfrom specific decoder to decode them too. This reduces code > duplication. > > I have a design doc that I can share with you. The image decoder lives in: src/ios/chrome/browser/suggestions/ios_image_decoder_impl.* The idea is: - The ImageDataFetch is only responsible for fetching the raw image data - The ImageDecoder is responsible for decoding raw image data - The ImageFetch owns a fetcher and decoder and returns the decoded image. -> Composition no inheritance see components/image_fetcher/image_fetcher_impl.* This means you can use image_date_fetcher also for fetching raw data and caching it. Later you can use the image decoder to get it from a cache. So ImageFetcher should be platform independend. However for each platform you need to create a ImageFetcher with the correct platform specific image decoder impl passed in. Sorry I never finished the ios part due to changing priorities.
On 2017/01/25 13:30:57, markusheintz_ wrote: > On 2017/01/25 13:19:50, markusheintz_ wrote: > > The original idea I had in mind was different. > > > > There already exists an ios_image_decoder that supports WebP. > > > > this means the ImageFetcher can be used accross all platforms but for the > actual > > decoding part (which is the piece that is really platform dependend ) there > are > > platform specific decoders. > > > > The advantage is that you can put images in caches oder local dbs and can use > > the plattfrom specific decoder to decode them too. This reduces code > > duplication. > > > > I have a design doc that I can share with you. > > The image decoder lives in: > src/ios/chrome/browser/suggestions/ios_image_decoder_impl.* > > The idea is: > - The ImageDataFetch is only responsible for fetching the raw image data > - The ImageDecoder is responsible for decoding raw image data > - The ImageFetch owns a fetcher and decoder and returns the decoded image. -> > Composition no inheritance see components/image_fetcher/image_fetcher_impl.* > > This means you can use image_date_fetcher also for fetching raw data and caching > it. Later you can use the image decoder to get it from a cache. > > So ImageFetcher should be platform independend. However for each platform you > need to create a ImageFetcher with the correct platform specific image decoder > impl passed in. > > Sorry I never finished the ios part due to changing priorities. Also image_fetcher_impl should go away and become image_fetcher. Feel free to grab me for a chat over VC if you like. I think a have a few spare cycles at the moment to finish my original work and/or help.
On 2017/01/25 13:35:28, markusheintz_ wrote: > On 2017/01/25 13:30:57, markusheintz_ wrote: > > On 2017/01/25 13:19:50, markusheintz_ wrote: > > > The original idea I had in mind was different. > > > > > > There already exists an ios_image_decoder that supports WebP. > > > > > > this means the ImageFetcher can be used accross all platforms but for the > > actual > > > decoding part (which is the piece that is really platform dependend ) there > > are > > > platform specific decoders. > > > > > > The advantage is that you can put images in caches oder local dbs and can > use > > > the plattfrom specific decoder to decode them too. This reduces code > > > duplication. > > > > > > I have a design doc that I can share with you. > > > > The image decoder lives in: > > src/ios/chrome/browser/suggestions/ios_image_decoder_impl.* > > > > The idea is: > > - The ImageDataFetch is only responsible for fetching the raw image data > > - The ImageDecoder is responsible for decoding raw image data > > - The ImageFetch owns a fetcher and decoder and returns the decoded image. -> > > Composition no inheritance see components/image_fetcher/image_fetcher_impl.* > > > > This means you can use image_date_fetcher also for fetching raw data and > caching > > it. Later you can use the image decoder to get it from a cache. > > > > So ImageFetcher should be platform independend. However for each platform you > > need to create a ImageFetcher with the correct platform specific image decoder > > impl passed in. > > > > Sorry I never finished the ios part due to changing priorities. > > Also image_fetcher_impl should go away and become image_fetcher. > > Feel free to grab me for a chat over VC if you like. > I think a have a few spare cycles at the moment to finish my original work > and/or help. Thanks for the explanation. I am aware of the DecodeImage class and I agree that in the end most of the classes would use the ImageFetcher with the iOS decoder. But some classes still need access to the raw data. I was creating a doc to explain this, I will finish/polish it and send it to you.
On 2017/01/25 13:39:12, gambard wrote: > On 2017/01/25 13:35:28, markusheintz_ wrote: > > On 2017/01/25 13:30:57, markusheintz_ wrote: > > > On 2017/01/25 13:19:50, markusheintz_ wrote: > > > > The original idea I had in mind was different. > > > > > > > > There already exists an ios_image_decoder that supports WebP. > > > > > > > > this means the ImageFetcher can be used accross all platforms but for the > > > actual > > > > decoding part (which is the piece that is really platform dependend ) > there > > > are > > > > platform specific decoders. > > > > > > > > The advantage is that you can put images in caches oder local dbs and can > > use > > > > the plattfrom specific decoder to decode them too. This reduces code > > > > duplication. > > > > > > > > I have a design doc that I can share with you. > > > > > > The image decoder lives in: > > > src/ios/chrome/browser/suggestions/ios_image_decoder_impl.* > > > > > > The idea is: > > > - The ImageDataFetch is only responsible for fetching the raw image data > > > - The ImageDecoder is responsible for decoding raw image data > > > - The ImageFetch owns a fetcher and decoder and returns the decoded image. > -> > > > Composition no inheritance see components/image_fetcher/image_fetcher_impl.* > > > > > > This means you can use image_date_fetcher also for fetching raw data and > > caching > > > it. Later you can use the image decoder to get it from a cache. > > > > > > So ImageFetcher should be platform independend. However for each platform > you > > > need to create a ImageFetcher with the correct platform specific image > decoder > > > impl passed in. > > > > > > Sorry I never finished the ios part due to changing priorities. > > > > Also image_fetcher_impl should go away and become image_fetcher. > > > > Feel free to grab me for a chat over VC if you like. > > I think a have a few spare cycles at the moment to finish my original work > > and/or help. > > Thanks for the explanation. I am aware of the DecodeImage class and I agree that > in the end most of the classes would use the ImageFetcher with the iOS decoder. > But some classes still need access to the raw data. I was creating a doc to > explain this, I will finish/polish it and send it to you. Thanks a lot. Looking forward to the doc. BTW: That's what the ImageFetcherDelegate is meant to be used for. It offer access to the raw data
On 2017/01/25 13:45:18, markusheintz_ wrote: > On 2017/01/25 13:39:12, gambard wrote: > > On 2017/01/25 13:35:28, markusheintz_ wrote: > > > On 2017/01/25 13:30:57, markusheintz_ wrote: > > > > On 2017/01/25 13:19:50, markusheintz_ wrote: > > > > > The original idea I had in mind was different. > > > > > > > > > > There already exists an ios_image_decoder that supports WebP. > > > > > > > > > > this means the ImageFetcher can be used accross all platforms but for > the > > > > actual > > > > > decoding part (which is the piece that is really platform dependend ) > > there > > > > are > > > > > platform specific decoders. > > > > > > > > > > The advantage is that you can put images in caches oder local dbs and > can > > > use > > > > > the plattfrom specific decoder to decode them too. This reduces code > > > > > duplication. > > > > > > > > > > I have a design doc that I can share with you. > > > > > > > > The image decoder lives in: > > > > src/ios/chrome/browser/suggestions/ios_image_decoder_impl.* > > > > > > > > The idea is: > > > > - The ImageDataFetch is only responsible for fetching the raw image data > > > > - The ImageDecoder is responsible for decoding raw image data > > > > - The ImageFetch owns a fetcher and decoder and returns the decoded image. > > -> > > > > Composition no inheritance see > components/image_fetcher/image_fetcher_impl.* > > > > > > > > This means you can use image_date_fetcher also for fetching raw data and > > > caching > > > > it. Later you can use the image decoder to get it from a cache. > > > > > > > > So ImageFetcher should be platform independend. However for each platform > > you > > > > need to create a ImageFetcher with the correct platform specific image > > decoder > > > > impl passed in. > > > > > > > > Sorry I never finished the ios part due to changing priorities. > > > > > > Also image_fetcher_impl should go away and become image_fetcher. > > > > > > Feel free to grab me for a chat over VC if you like. > > > I think a have a few spare cycles at the moment to finish my original work > > > and/or help. > > > > Thanks for the explanation. I am aware of the DecodeImage class and I agree > that > > in the end most of the classes would use the ImageFetcher with the iOS > decoder. > > But some classes still need access to the raw data. I was creating a doc to > > explain this, I will finish/polish it and send it to you. > > Thanks a lot. Looking forward to the doc. > > BTW: That's what the ImageFetcherDelegate is meant to be used for. It offer > access to the raw data See the src/components/ntp_snippets/remote/remote_suggestions_provider_impl.* as an example for how to do it
On 2017/01/25 13:48:36, markusheintz_ wrote: > On 2017/01/25 13:45:18, markusheintz_ wrote: > > On 2017/01/25 13:39:12, gambard wrote: > > > On 2017/01/25 13:35:28, markusheintz_ wrote: > > > > On 2017/01/25 13:30:57, markusheintz_ wrote: > > > > > On 2017/01/25 13:19:50, markusheintz_ wrote: > > > > > > The original idea I had in mind was different. > > > > > > > > > > > > There already exists an ios_image_decoder that supports WebP. > > > > > > > > > > > > this means the ImageFetcher can be used accross all platforms but for > > the > > > > > actual > > > > > > decoding part (which is the piece that is really platform dependend ) > > > there > > > > > are > > > > > > platform specific decoders. > > > > > > > > > > > > The advantage is that you can put images in caches oder local dbs and > > can > > > > use > > > > > > the plattfrom specific decoder to decode them too. This reduces code > > > > > > duplication. > > > > > > > > > > > > I have a design doc that I can share with you. > > > > > > > > > > The image decoder lives in: > > > > > src/ios/chrome/browser/suggestions/ios_image_decoder_impl.* > > > > > > > > > > The idea is: > > > > > - The ImageDataFetch is only responsible for fetching the raw image data > > > > > - The ImageDecoder is responsible for decoding raw image data > > > > > - The ImageFetch owns a fetcher and decoder and returns the decoded > image. > > > -> > > > > > Composition no inheritance see > > components/image_fetcher/image_fetcher_impl.* > > > > > > > > > > This means you can use image_date_fetcher also for fetching raw data and > > > > caching > > > > > it. Later you can use the image decoder to get it from a cache. > > > > > > > > > > So ImageFetcher should be platform independend. However for each > platform > > > you > > > > > need to create a ImageFetcher with the correct platform specific image > > > decoder > > > > > impl passed in. > > > > > > > > > > Sorry I never finished the ios part due to changing priorities. > > > > > > > > Also image_fetcher_impl should go away and become image_fetcher. > > > > > > > > Feel free to grab me for a chat over VC if you like. > > > > I think a have a few spare cycles at the moment to finish my original work > > > > and/or help. > > > > > > Thanks for the explanation. I am aware of the DecodeImage class and I agree > > that > > > in the end most of the classes would use the ImageFetcher with the iOS > > decoder. > > > But some classes still need access to the raw data. I was creating a doc to > > > explain this, I will finish/polish it and send it to you. > > > > Thanks a lot. Looking forward to the doc. > > > > BTW: That's what the ImageFetcherDelegate is meant to be used for. It offer > > access to the raw data > > See the src/components/ntp_snippets/remote/remote_suggestions_provider_impl.* as > an example for how to do it Actually the three remaining tasks that are left are: A) Create a factory for the ImageFetch that picks the right platform specific parts B) Get rid of the io_image_fetcher_impl. c) move all the code the to component
On 2017/01/25 13:48:36, markusheintz_ wrote: > On 2017/01/25 13:45:18, markusheintz_ wrote: > > On 2017/01/25 13:39:12, gambard wrote: > > > On 2017/01/25 13:35:28, markusheintz_ wrote: > > > > On 2017/01/25 13:30:57, markusheintz_ wrote: > > > > > On 2017/01/25 13:19:50, markusheintz_ wrote: > > > > > > The original idea I had in mind was different. > > > > > > > > > > > > There already exists an ios_image_decoder that supports WebP. > > > > > > > > > > > > this means the ImageFetcher can be used accross all platforms but for > > the > > > > > actual > > > > > > decoding part (which is the piece that is really platform dependend ) > > > there > > > > > are > > > > > > platform specific decoders. > > > > > > > > > > > > The advantage is that you can put images in caches oder local dbs and > > can > > > > use > > > > > > the plattfrom specific decoder to decode them too. This reduces code > > > > > > duplication. > > > > > > > > > > > > I have a design doc that I can share with you. > > > > > > > > > > The image decoder lives in: > > > > > src/ios/chrome/browser/suggestions/ios_image_decoder_impl.* > > > > > > > > > > The idea is: > > > > > - The ImageDataFetch is only responsible for fetching the raw image data > > > > > - The ImageDecoder is responsible for decoding raw image data > > > > > - The ImageFetch owns a fetcher and decoder and returns the decoded > image. > > > -> > > > > > Composition no inheritance see > > components/image_fetcher/image_fetcher_impl.* > > > > > > > > > > This means you can use image_date_fetcher also for fetching raw data and > > > > caching > > > > > it. Later you can use the image decoder to get it from a cache. > > > > > > > > > > So ImageFetcher should be platform independend. However for each > platform > > > you > > > > > need to create a ImageFetcher with the correct platform specific image > > > decoder > > > > > impl passed in. > > > > > > > > > > Sorry I never finished the ios part due to changing priorities. > > > > > > > > Also image_fetcher_impl should go away and become image_fetcher. > > > > > > > > Feel free to grab me for a chat over VC if you like. > > > > I think a have a few spare cycles at the moment to finish my original work > > > > and/or help. > > > > > > Thanks for the explanation. I am aware of the DecodeImage class and I agree > > that > > > in the end most of the classes would use the ImageFetcher with the iOS > > decoder. > > > But some classes still need access to the raw data. I was creating a doc to > > > explain this, I will finish/polish it and send it to you. > > > > Thanks a lot. Looking forward to the doc. > > > > BTW: That's what the ImageFetcherDelegate is meant to be used for. It offer > > access to the raw data > > See the src/components/ntp_snippets/remote/remote_suggestions_provider_impl.* as > an example for how to do it Oh. I have missed the delegate. This might solve my problem, I will take it into account. Thanks!
ios/chrome/browser/suggestions/ LGTM
Thanks, PTAL. https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... File components/image_fetcher/ios/BUILD.gn (right): https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/ios/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/25 10:37:55, Marc Treib wrote: > 2017 Done. https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher.h (right): https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher.h:15: class IOSImageDataFetcher : public ImageDataFetcher { On 2017/01/25 10:46:46, blundell wrote: > On 2017/01/25 10:37:55, Marc Treib wrote: > > I don't think inheritance is the right tool here. As it is, this class exposes > > two interfaces: The base ImageDataFetcher one, and the new method > > FetchImageDataWebpDecoded. And on ios, using the base interface would be wrong > > (at least in most circumstances), because it'll return unusable WebP data. > > > > I see a few alternatives; not sure yet which is the best: > > > > - Use composition instead of inheritance: Make this a separate class which > just > > uses an instance of ImageDataFetcher. This is probably the least effort coming > > from the current CL, but it'll make ios very different from other platforms - > > essentially there won't be a common image_fetcher interface anymore. > > > > - Actually implement the ImageDataFetcher interface: Instead of defining a new > > method, override the existing FetchImageData to do the right thing. Then we > can > > define a factory method somewhere that will instantiate the right thing based > on > > the platform, and the rest of the code doesn't have to know. Downside: It's > > implementation inheritance, which is mostly considered a bad thing. > > > > - Similar, but maybe cleaner design-wise: Make ImageDataFetcher an interface, > > with different implementations for ios vs other platforms. This means > factoring > > the common URL fetching code into a separate helper class. > > With the caveat that I'm not close to the details here, this one ^^^ reads as > the cleanest option. > > > > > - Implement the transcoding in ios's ImageFetcherImpl, rather than in > > ImageDataFetcher. > > > > WDYT? > Moving to composition. https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... File components/image_fetcher/ios/ios_image_data_fetcher.mm (right): https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher.mm:85: // response On 2017/01/25 10:37:55, Marc Treib wrote: > nit: misformatted comment Done. https://codereview.chromium.org/2652893005/diff/60001/components/image_fetche... components/image_fetcher/ios/ios_image_data_fetcher.mm:94: if (http_response_code != net::HTTP_OK) { On 2017/01/25 10:37:55, Marc Treib wrote: > nit: I think this would be clearer as > if (response_code == HTTP_OK || SchemeIs(kDataScheme)) > instead of mangling the response code. I am returning the response_code (I forgot in this patch, the next one has it). https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:30: const net::URLFetcher* source)>; As the URLFetcher should probably not be exposed, I can change it for: int response_code, std::string mime_type. What do you think?
Have you talked to Markus about the design? My latest understanding was that WebP transcoding was in fact already implemented somehow? https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:30: const net::URLFetcher* source)>; On 2017/01/30 13:37:53, gambard wrote: > As the URLFetcher should probably not be exposed, I can change it for: int > response_code, std::string mime_type. > What do you think? I still don't quite see why you need the response code? mime_type makes more sense IMO, though I'm not convinced it's necessary - IIRC, you can detect WebP from a header ("magic bytes") in the data.
Yes we discussed the design with Markus and this should be an acceptable solution. https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:30: const net::URLFetcher* source)>; On 2017/01/31 09:13:01, Marc Treib wrote: > On 2017/01/30 13:37:53, gambard wrote: > > As the URLFetcher should probably not be exposed, I can change it for: int > > response_code, std::string mime_type. > > What do you think? > > I still don't quite see why you need the response code? > mime_type makes more sense IMO, though I'm not convinced it's necessary - IIRC, > you can detect WebP from a header ("magic bytes") in the data. I agree that I can detect the type from the first bytes. If we want to keep the callback as simple as possible it is probably the best way. The response code is needed by the favicon driver: https://cs.chromium.org/chromium/src/components/favicon/core/favicon_driver_i...
Thanks! Comments below. Markus is looking too now; he's more familiar with the design (and the future plans for it) than I am. https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... components/image_fetcher/image_data_fetcher.h:30: const net::URLFetcher* source)>; On 2017/01/31 09:24:20, gambard wrote: > On 2017/01/31 09:13:01, Marc Treib wrote: > > On 2017/01/30 13:37:53, gambard wrote: > > > As the URLFetcher should probably not be exposed, I can change it for: int > > > response_code, std::string mime_type. > > > What do you think? > > > > I still don't quite see why you need the response code? > > mime_type makes more sense IMO, though I'm not convinced it's necessary - > IIRC, > > you can detect WebP from a header ("magic bytes") in the data. > > I agree that I can detect the type from the first bytes. If we want to keep the > callback as simple as possible it is probably the best way. I'd prefer that; not only to keep the callback simple, but mainly because I think it's more reliable. IIRC, Markus has done that exact thing at some point. > The response code is needed by the favicon driver: > https://cs.chromium.org/chromium/src/components/favicon/core/favicon_driver_i... And where is that used? It doesn't seem to be called anywhere in this CL...?
On 2017/01/31 09:24:20, gambard wrote: > Yes we discussed the design with Markus and this should be an acceptable > solution. > > https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... > File components/image_fetcher/image_data_fetcher.h (right): > > https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... > components/image_fetcher/image_data_fetcher.h:30: const net::URLFetcher* > source)>; > On 2017/01/31 09:13:01, Marc Treib wrote: > > On 2017/01/30 13:37:53, gambard wrote: > > > As the URLFetcher should probably not be exposed, I can change it for: int > > > response_code, std::string mime_type. > > > What do you think? > > > > I still don't quite see why you need the response code? > > mime_type makes more sense IMO, though I'm not convinced it's necessary - > IIRC, > > you can detect WebP from a header ("magic bytes") in the data. > > I agree that I can detect the type from the first bytes. If we want to keep the > callback as simple as possible it is probably the best way. > The response code is needed by the favicon driver: > https://cs.chromium.org/chromium/src/components/favicon/core/favicon_driver_i... Yes we discussed the topic and Gauthier created an excellent doc. We agreed that it is best to have a dedicated class for iOS that does the fetching and decoding similar to the ImageFetcher. The ImageFetcher could be used thanks to the Delegate interface but won't be used for the reasons mentioned in the related design doc. This new iOS specific class will uses the ImageDataFetcher (as it is) and the ImageDecoder (as it is) since they don't need to be changed. We also agreed that it is ok to name this new class ImageDataFetcher and rename the existing ImageDataFetcher to something different to prevent confusion. I think what we discussed it great and we should move forward with the discussed solution. It also highlight that we need a solution for .ico and animated gif files. There even exists a bug for this that was filed by Jan. NOT LGTM
On 2017/01/31 10:05:00, markusheintz_ wrote: > On 2017/01/31 09:24:20, gambard wrote: > > Yes we discussed the design with Markus and this should be an acceptable > > solution. > > > > > https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... > > File components/image_fetcher/image_data_fetcher.h (right): > > > > > https://codereview.chromium.org/2652893005/diff/80001/components/image_fetche... > > components/image_fetcher/image_data_fetcher.h:30: const net::URLFetcher* > > source)>; > > On 2017/01/31 09:13:01, Marc Treib wrote: > > > On 2017/01/30 13:37:53, gambard wrote: > > > > As the URLFetcher should probably not be exposed, I can change it for: int > > > > response_code, std::string mime_type. > > > > What do you think? > > > > > > I still don't quite see why you need the response code? > > > mime_type makes more sense IMO, though I'm not convinced it's necessary - > > IIRC, > > > you can detect WebP from a header ("magic bytes") in the data. > > > > I agree that I can detect the type from the first bytes. If we want to keep > the > > callback as simple as possible it is probably the best way. > > The response code is needed by the favicon driver: > > > https://cs.chromium.org/chromium/src/components/favicon/core/favicon_driver_i... > > Yes we discussed the topic and Gauthier created an excellent doc. > > We agreed that it is best to have a dedicated class for iOS that does the > fetching and decoding similar to the ImageFetcher. > The ImageFetcher could be used thanks to the Delegate interface but won't be > used for the reasons mentioned in the related design doc. > > This new iOS specific class will uses the ImageDataFetcher (as it is) and the > ImageDecoder (as it is) since they don't need to be changed. > > We also agreed that it is ok to name this new class ImageDataFetcher and rename > the existing ImageDataFetcher to something different to prevent confusion. > > I think what we discussed it great and we should move forward with the discussed > solution. It also highlight that we need a solution for .ico and animated gif > files. There even exists a bug for this that was filed by Jan. > > > NOT LGTM Closing this and opening https://codereview.chromium.org/2663213002. |
