New issue
Advanced search Search tips

Issue 850019 link

Starred by 1 user
Status: Fixed
Owner:
sdefresne@chromium.org
Closed: Jun 2018
Cc:
treib@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

Two types named ImageDataFetcherCallback in components/image_fetcher

Project Member Reported by sdefresne@chromium.org, Jun 6 2018 
There are two very similar types named ImageDataFetcherCallback defined in components/image_fetcher/core:

ImageFetcher::ImageDataFetcherCallback
ImageDataFetcher::ImageDataFetcherCallback

They are both callback of type void(const std::string& image_data, const RequestMetadata& request_metadata), but one is a repeating callback while the other is a once callback.

Since they are both only invoked once, they should probably be merged and ImageDataFetcher fixed to receive the callback by value instead of const reference.
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 6 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56f4b7fe5a8498abca1217b306a591db12382f5f

commit 56f4b7fe5a8498abca1217b306a591db12382f5f
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Wed Jun 06 14:32:42 2018

Fix duplicated type definition

Two inner types where named ImageDataFetcherCallback in
components/image_fetcher/core, one in ImageFetcher and
the other in ImageDataFetcher.

The two types where typedef for callbacks accepting the
same arguments except one was a RepeatingCallback while
the other was a OnceCallback. This duplication caused
confusion.

Move ImageDataFetcherCallback into the image_fetcher
namespace. Also move the related types ImageFetcherCallback
and IOSImageDataFetcherCallback (this type is also renamed
to ImageDataFetcherBlock as it is a typedef for a block).

Since the callback is only called once, convert it to
a base::OnceCallback<> and fix the code accordingly.

Bug:  850019 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Iab24263d8ac6e2fd8e48db5274e1a64b15e4ad16
Reviewed-on: https://chromium-review.googlesource.com/1088701
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564876}
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/favicon/ios/web_favicon_driver.mm
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/image_fetcher/core/BUILD.gn
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/image_fetcher/core/image_data_fetcher.cc
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/image_fetcher/core/image_data_fetcher.h
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/image_fetcher/core/image_data_fetcher_unittest.cc
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/image_fetcher/core/image_fetcher.h
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/image_fetcher/core/image_fetcher_impl_unittest.cc
[add] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/image_fetcher/core/image_fetcher_types.h
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/image_fetcher/ios/ios_image_data_fetcher_wrapper_unittest.mm
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/ntp_snippets/remote/cached_image_fetcher.cc
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/ios/chrome/browser/payments/ios_payment_instrument_finder.mm
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/56f4b7fe5a8498abca1217b306a591db12382f5f/ios/chrome/browser/ui/omnibox/popup/omnibox_popup_mediator.mm

Comment 2 by sdefresne@chromium.org, Jun 6 2018

Status: Fixed (was: Started)
►

Sign in to add a comment