|
|
Created:
3 years, 10 months ago by Marc Treib Modified:
3 years, 10 months ago Reviewers:
markusheintz_ CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd/update some comments in components/image_fetcher
BUG=none
Review-Url: https://codereview.chromium.org/2702693002
Cr-Commit-Position: refs/heads/master@{#451641}
Committed: https://chromium.googlesource.com/chromium/src/+/4317ace9456f17a6fd22049f008d35ac4c32e469
Patch Set 1 #
Total comments: 7
Patch Set 2 : review #
Messages
Total messages: 15 (6 generated)
treib@chromium.org changed reviewers: + markusheintz@chromium.org
PTAL at this comments-only CL :) https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... File components/image_fetcher/image_fetcher.h (right): https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... components/image_fetcher/image_fetcher.h:45: // crbug.com/689020), add a getter for the ImageDecoder here. Right now, we have to pass both an ImageFetcher and an ImageDecoder to RemoteSuggestionsProvider. If we expose the ImageDecoder here (which the Impl has anyway), we only need to pass one. So far, we couldn't do that, because the iOS Impl didn't have a Decoder, but that's changing now :) https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... File components/image_fetcher/image_fetcher_impl.h (left): https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... components/image_fetcher/image_fetcher_impl.h:32: // removed merge the two classes ImageFetcher and ImageFetcherImpl. I think it makes sense to keep a separate interface, so that it's easy to inject a FakeImageFetcher in tests.
On 2017/02/17 09:44:32, Marc Treib wrote: > PTAL at this comments-only CL :) > > https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... > File components/image_fetcher/image_fetcher.h (right): > > https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... > components/image_fetcher/image_fetcher.h:45: // crbug.com/689020), add a getter > for the ImageDecoder here. > Right now, we have to pass both an ImageFetcher and an ImageDecoder to > RemoteSuggestionsProvider. If we expose the ImageDecoder here (which the Impl > has anyway), we only need to pass one. > So far, we couldn't do that, because the iOS Impl didn't have a Decoder, but > that's changing now :) > > https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... > File components/image_fetcher/image_fetcher_impl.h (left): > > https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... > components/image_fetcher/image_fetcher_impl.h:32: // removed merge the two > classes ImageFetcher and ImageFetcherImpl. > I think it makes sense to keep a separate interface, so that it's easy to inject > a FakeImageFetcher in tests. Ping!
https://codereview.chromium.org/2702693002/diff/1/chrome/browser/search/sugge... File chrome/browser/search/suggestions/image_decoder_impl.h (right): https://codereview.chromium.org/2702693002/diff/1/chrome/browser/search/sugge... chrome/browser/search/suggestions/image_decoder_impl.h:18: // nothing to do with suggestions. This is the corresponding bug: https://bugs.chromium.org/p/chromium/issues/detail?id=624761 Please reference. https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... File components/image_fetcher/image_fetcher.h (right): https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... components/image_fetcher/image_fetcher.h:45: // crbug.com/689020), add a getter for the ImageDecoder here. On 2017/02/17 09:44:31, Marc Treib wrote: > Right now, we have to pass both an ImageFetcher and an ImageDecoder to > RemoteSuggestionsProvider. If we expose the ImageDecoder here (which the Impl > has anyway), we only need to pass one. > So far, we couldn't do that, because the iOS Impl didn't have a Decoder, but > that's changing now :) We can still pass the reference to the same ImageDecoder used by the ImageFetcher to the RemoteSuggestionsProvider. Then we don't have to expose it on the interface. Its more a question how to construct. I still prefer this. However I can see that if we add a getter here the ownership of the decoder will be easier to understand in the code. https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... File components/image_fetcher/image_fetcher_impl.h (left): https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... components/image_fetcher/image_fetcher_impl.h:32: // removed merge the two classes ImageFetcher and ImageFetcherImpl. On 2017/02/17 09:44:31, Marc Treib wrote: > I think it makes sense to keep a separate interface, so that it's easy to inject > a FakeImageFetcher in tests. Full agree.
Thanks! PTAL again https://codereview.chromium.org/2702693002/diff/1/chrome/browser/search/sugge... File chrome/browser/search/suggestions/image_decoder_impl.h (right): https://codereview.chromium.org/2702693002/diff/1/chrome/browser/search/sugge... chrome/browser/search/suggestions/image_decoder_impl.h:18: // nothing to do with suggestions. On 2017/02/20 13:14:44, markusheintz_ wrote: > This is the corresponding bug: > https://bugs.chromium.org/p/chromium/issues/detail?id=624761 > > Please reference. Thanks for the pointer! Added. https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... File components/image_fetcher/image_fetcher.h (right): https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... components/image_fetcher/image_fetcher.h:45: // crbug.com/689020), add a getter for the ImageDecoder here. On 2017/02/20 13:14:44, markusheintz_ wrote: > On 2017/02/17 09:44:31, Marc Treib wrote: > > Right now, we have to pass both an ImageFetcher and an ImageDecoder to > > RemoteSuggestionsProvider. If we expose the ImageDecoder here (which the Impl > > has anyway), we only need to pass one. > > So far, we couldn't do that, because the iOS Impl didn't have a Decoder, but > > that's changing now :) > > We can still pass the reference to the same ImageDecoder used by the > ImageFetcher to the RemoteSuggestionsProvider. Right now we can't, because we're passing in a unique_ptr. We *could* pass in a non-owning pointer, but that seems more awkward to me. > Then we don't have to expose it on the interface. Its more a question how to > construct. I still prefer this. > > However I can see that if we add a getter here the ownership of the decoder will > be easier to understand in the code. To me, the ImageFetcher really provides two things: 1) Downloading and 2) decoding. It already provides an access point between the two steps, by passing the raw image data to the delegate. It seems only consistent to also provide a way to decode that raw data, by exposing the ImageDecoder.
On 2017/02/20 14:23:36, Marc Treib wrote: > Thanks! PTAL again > > https://codereview.chromium.org/2702693002/diff/1/chrome/browser/search/sugge... > File chrome/browser/search/suggestions/image_decoder_impl.h (right): > > https://codereview.chromium.org/2702693002/diff/1/chrome/browser/search/sugge... > chrome/browser/search/suggestions/image_decoder_impl.h:18: // nothing to do with > suggestions. > On 2017/02/20 13:14:44, markusheintz_ wrote: > > This is the corresponding bug: > > https://bugs.chromium.org/p/chromium/issues/detail?id=624761 > > > > Please reference. > > Thanks for the pointer! Added. > > https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... > File components/image_fetcher/image_fetcher.h (right): > > https://codereview.chromium.org/2702693002/diff/1/components/image_fetcher/im... > components/image_fetcher/image_fetcher.h:45: // crbug.com/689020), add a getter > for the ImageDecoder here. > On 2017/02/20 13:14:44, markusheintz_ wrote: > > On 2017/02/17 09:44:31, Marc Treib wrote: > > > Right now, we have to pass both an ImageFetcher and an ImageDecoder to > > > RemoteSuggestionsProvider. If we expose the ImageDecoder here (which the > Impl > > > has anyway), we only need to pass one. > > > So far, we couldn't do that, because the iOS Impl didn't have a Decoder, but > > > that's changing now :) > > > > We can still pass the reference to the same ImageDecoder used by the > > ImageFetcher to the RemoteSuggestionsProvider. > > Right now we can't, because we're passing in a unique_ptr. We *could* pass in a > non-owning pointer, but that seems more awkward to me. > > > Then we don't have to expose it on the interface. Its more a question how to > > construct. I still prefer this. > > > > However I can see that if we add a getter here the ownership of the decoder > will > > be easier to understand in the code. > > To me, the ImageFetcher really provides two things: 1) Downloading and 2) > decoding. It already provides an access point between the two steps, by passing > the raw image data to the delegate. It seems only consistent to also provide a > way to decode that raw data, by exposing the ImageDecoder. Since I can't come up with a better counter proposal: LGTM :-)
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by treib@chromium.org
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": 20001, "attempt_start_ts": 1487608354357130, "parent_rev": "05507771bafaa57f4fda753553aeb9f99b74b7e2", "commit_rev": "4317ace9456f17a6fd22049f008d35ac4c32e469"}
Message was sent while issue was closed.
Description was changed from ========== Add/update some comments in components/image_fetcher BUG=none ========== to ========== Add/update some comments in components/image_fetcher BUG=none Review-Url: https://codereview.chromium.org/2702693002 Cr-Commit-Position: refs/heads/master@{#451641} Committed: https://chromium.googlesource.com/chromium/src/+/4317ace9456f17a6fd22049f008d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4317ace9456f17a6fd22049f008d... |