Use IOSImageDataFetcherWrapper in BrowserViewController

Use IOSImageDataFetcherWrapper in BrowserViewController

This CL changes the use of ImageDataFetcher in BrowserViewController.
This needs the ImageDataFetcher to be able to have the referrer set.

BUG=683918
Review-Url: https://codereview.chromium.org/2674653003
Cr-Commit-Position: refs/heads/master@{#448006}
Committed: https://chromium.googlesource.com/chromium/src/+/bdc07cc748ecfcd39f57c5759d7cfb3aac0817df

[gambard, 2017-02-02 14:49:30]

gambard@chromium.org changed reviewers:
+ markusheintz@chromium.org, treib@chromium.org

[gambard, 2017-02-02 14:49:31]

PTAL.
WDYT of the addition to the API of ImageDataFetcher?

[Marc Treib, 2017-02-02 15:19:13]

This API extension looks fine to me. Some comments below.

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
File components/image_fetcher/image_data_fetcher.cc (right):

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.cc:49: const GURL& url, const
ImageDataFetcherCallback& callback) {
Not your doing, but url should be image_url to match the header.

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.cc:51: url, callback, std::string(),
Please add a comment for the empty string param, like this:
/*referrer=*/std::string()

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.cc:59:
net::URLRequest::ReferrerPolicy referrer_policy) {
You should actually do something with the referrer and policy :)

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
File components/image_fetcher/image_data_fetcher.h (right):

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.h:41: // Helper to fetches images
data without a referrer.
s/fetches images/fetch image/

Though I might do the comments the other way around: Have the "main" comment
here, and then on the overload below, say just "Like above, but lets the caller
set a referrer". (Not a big deal either way)

[markusheintz_, 2017-02-02 16:20:42]

+1 to Marc's comments.

In general the looks good.

What do you plan to use the referrer for.

[gambard, 2017-02-03 12:11:34]

Thanks!
The referrer is used in the BrowserViewController (which is one of our
way-too-big core classes) to save images to devices or download it to do a
search by image.
As we are using WebKit to do the rendering we don't have access to the images
rendered and we need to download them again.

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
File components/image_fetcher/image_data_fetcher.cc (right):

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.cc:49: const GURL& url, const
ImageDataFetcherCallback& callback) {
On 2017/02/02 15:19:13, Marc Treib wrote:
> Not your doing, but url should be image_url to match the header.

Done.

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.cc:51: url, callback, std::string(),
On 2017/02/02 15:19:13, Marc Treib wrote:
> Please add a comment for the empty string param, like this:
> /*referrer=*/std::string()

Done.

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.cc:59:
net::URLRequest::ReferrerPolicy referrer_policy) {
On 2017/02/02 15:19:13, Marc Treib wrote:
> You should actually do something with the referrer and policy :)

Oooops :)
Done!

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
File components/image_fetcher/image_data_fetcher.h (right):

https://codereview.chromium.org/2674653003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.h:41: // Helper to fetches images
data without a referrer.
On 2017/02/02 15:19:13, Marc Treib wrote:
> s/fetches images/fetch image/
> 
> Though I might do the comments the other way around: Have the "main" comment
> here, and then on the overload below, say just "Like above, but lets the
caller
> set a referrer". (Not a big deal either way)

Done.

[gambard, 2017-02-03 12:17:10]

gambard@chromium.org changed reviewers:
+ marq@chromium.org

[gambard, 2017-02-03 12:17:11]

marq@: PTAL for BVC.
The main difference between the old ImageFetcher and the new one is that the old
one set the flags:
fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                        net::LOAD_DO_NOT_SAVE_COOKIES |
                        net::LOAD_DO_NOT_SEND_AUTH_DATA)
Doing some tests on protected pages I have not seen any differences with or
without. But maybe I have missed something.

https://codereview.chromium.org/2674653003/diff/20001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2674653003/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3060: [weakSelf
searchByImageData:data atURL:image_source_url];
The URL returned by the ImageDataFetcher was the OriginalURL. This should not
change anything.

[Marc Treib, 2017-02-03 14:10:30]

image_fetcher LGTM with nits

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
File components/image_fetcher/image_data_fetcher.cc (right):

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
components/image_fetcher/image_data_fetcher.cc:51:
ImageDataFetcher::FetchImageData(
"ImageDataFetcher::" isn't needed.

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
components/image_fetcher/image_data_fetcher.cc:53:
net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE);
It's unfortunate that we have to bake in the default value here (though the
policy probably just doesn't matter if the referrer itself is empty).

I guess the alternative would be for the other method to take a
base::Optional<ReferrerPolicy> (I think callers could still just pass in a
value, without caring about the Optional), but that probably adds more
confusion. WDYT?

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right):

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:91:
net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE);
...and a second place where it's baked in.
IMO this is a strong hint that the design with the IosWrapper isn't quite there
yet. Not for this CL, but we should revisit this at some point.

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:93: void
IOSImageDataFetcherWrapper::FetchImageDataWebpDecoded(
nit: empty line before

[marq (ping after 24h), 2017-02-03 14:30:45]

ios/chrome/browser/ui LGTM

[gambard, 2017-02-03 14:56:48]

Thanks, PTAL at the change in IOSImageDataFetcherWrapper.

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
File components/image_fetcher/image_data_fetcher.cc (right):

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
components/image_fetcher/image_data_fetcher.cc:51:
ImageDataFetcher::FetchImageData(
On 2017/02/03 14:10:30, Marc Treib wrote:
> "ImageDataFetcher::" isn't needed.

Done.

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
components/image_fetcher/image_data_fetcher.cc:53:
net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE);
On 2017/02/03 14:10:30, Marc Treib wrote:
> It's unfortunate that we have to bake in the default value here (though the
> policy probably just doesn't matter if the referrer itself is empty).
> 
> I guess the alternative would be for the other method to take a
> base::Optional<ReferrerPolicy> (I think callers could still just pass in a
> value, without caring about the Optional), but that probably adds more
> confusion. WDYT?

I agree this not a perfect solution. I would have expected the URLRequest to
expose a static method to have the default.
I find this better than a base::Optional but I don't have a strong opinion on
it.

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right):

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:91:
net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE);
On 2017/02/03 14:10:30, Marc Treib wrote:
> ...and a second place where it's baked in.
> IMO this is a strong hint that the design with the IosWrapper isn't quite
there
> yet. Not for this CL, but we should revisit this at some point.

I have changed the interface. Maybe it makes more sense now.

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:93: void
IOSImageDataFetcherWrapper::FetchImageDataWebpDecoded(
On 2017/02/03 14:10:30, Marc Treib wrote:
> nit: empty line before

Done.

[Marc Treib, 2017-02-03 15:06:46]

lgtm

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right):

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:91:
net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE);
On 2017/02/03 14:56:47, gambard wrote:
> On 2017/02/03 14:10:30, Marc Treib wrote:
> > ...and a second place where it's baked in.
> > IMO this is a strong hint that the design with the IosWrapper isn't quite
> there
> > yet. Not for this CL, but we should revisit this at some point.
> 
> I have changed the interface. Maybe it makes more sense now.

Yep, I think that's a bit better, thanks!

At some point, we'll still have to re-think the Wrapper: Having a different
ImageDataFetcher interface on iOS makes it impossible to use the same
ImageFetcher implementation across platforms. But that's for another day :)

[gambard, 2017-02-03 15:11:14]

Thanks!

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right):

https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:91:
net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE);
On 2017/02/03 15:06:45, Marc Treib wrote:
> On 2017/02/03 14:56:47, gambard wrote:
> > On 2017/02/03 14:10:30, Marc Treib wrote:
> > > ...and a second place where it's baked in.
> > > IMO this is a strong hint that the design with the IosWrapper isn't quite
> > there
> > > yet. Not for this CL, but we should revisit this at some point.
> > 
> > I have changed the interface. Maybe it makes more sense now.
> 
> Yep, I think that's a bit better, thanks!
> 
> At some point, we'll still have to re-think the Wrapper: Having a different
> ImageDataFetcher interface on iOS makes it impossible to use the same
> ImageFetcher implementation across platforms. But that's for another day :)

Actually no :)
There are two usages of image fetchers in iOS: those needing access to the raw
data (which will be using this one) and the one needing access to gfx::Image
(which will be using the shared ImageFetcher).
The WebP decoding will be done in the iOS specific ImageDecoder that will be
passed to the ImageFetcher. We discussed it at go/chrome-ios-image-fetchers

[gambard, 2017-02-03 15:11:24]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-02-03 15:11:25]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2674653003/#ps40001
(title: "Change IOSImageDataFetcherWrapper wrapping")

[commit-bot: I haz the power, 2017-02-03 15:11:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2017-02-03 15:18:37]

On 2017/02/03 15:11:14, gambard wrote:
> Thanks!
> 
>
https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
> File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm (right):
> 
>
https://codereview.chromium.org/2674653003/diff/20001/components/image_fetche...
> components/image_fetcher/ios/ios_image_data_fetcher_wrapper.mm:91:
> net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE);
> On 2017/02/03 15:06:45, Marc Treib wrote:
> > On 2017/02/03 14:56:47, gambard wrote:
> > > On 2017/02/03 14:10:30, Marc Treib wrote:
> > > > ...and a second place where it's baked in.
> > > > IMO this is a strong hint that the design with the IosWrapper isn't
quite
> > > there
> > > > yet. Not for this CL, but we should revisit this at some point.
> > > 
> > > I have changed the interface. Maybe it makes more sense now.
> > 
> > Yep, I think that's a bit better, thanks!
> > 
> > At some point, we'll still have to re-think the Wrapper: Having a different
> > ImageDataFetcher interface on iOS makes it impossible to use the same
> > ImageFetcher implementation across platforms. But that's for another day :)
> 
> Actually no :)
> There are two usages of image fetchers in iOS: those needing access to the raw
> data (which will be using this one) and the one needing access to gfx::Image
> (which will be using the shared ImageFetcher).
> The WebP decoding will be done in the iOS specific ImageDecoder that will be
> passed to the ImageFetcher. We discussed it at go/chrome-ios-image-fetchers

Ah, I see, thanks for the pointer!
That makes it better than I thought. It's still all a bit weird, since the
ImageFetcher *also* provides the raw data, but before WebP transcoding.
In my ideal world, there would be some ImageDataFetcher::Create(..) function (or
a factory or whatever) that instantiates the right thing for the current
platform, so any client code using it wouldn't have to care about all this. But,
as I said, that's a problem for another day :)

[commit-bot: I haz the power, 2017-02-03 16:43:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486134684983920,
"parent_rev": "d349889b8ea1d9c331f0473a6356c9d3c524c46d", "commit_rev":
"bdc07cc748ecfcd39f57c5759d7cfb3aac0817df"}

[commit-bot: I haz the power, 2017-02-03 16:43:50]

Description was changed from

==========
Use IOSImageDataFetcherWrapper in BrowserViewController

This CL changes the use of ImageDataFetcher in BrowserViewController.
This needs the ImageDataFetcher to be able to have the referrer set.

BUG=683918
==========

to

==========
Use IOSImageDataFetcherWrapper in BrowserViewController

This CL changes the use of ImageDataFetcher in BrowserViewController.
This needs the ImageDataFetcher to be able to have the referrer set.

BUG=683918

Review-Url: https://codereview.chromium.org/2674653003
Cr-Commit-Position: refs/heads/master@{#448006}
Committed:
https://chromium.googlesource.com/chromium/src/+/bdc07cc748ecfcd39f57c5759d7c...
==========

[commit-bot: I haz the power, 2017-02-03 16:43:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/bdc07cc748ecfcd39f57c5759d7c...