[Image Fetcher] Add support to disable cookies

[Image Fetcher] Fix cookies being enabled by default

image_fetcher is used by components that fetch images outside the
content area, hence cookies are undesirable.

A quick look at existing clients suggests none are relying on
cookies, hence we disable them by default and introduce no API for
now to toggle them.

BUG=644102, 702200
Review-Url: https://codereview.chromium.org/2756633003
Cr-Commit-Position: refs/heads/master@{#457811}
Committed: https://chromium.googlesource.com/chromium/src/+/4409b00db808ea391f7ba279126d12f3587e933c

[mastiz, 2017-03-16 09:46:56]

mastiz@chromium.org changed reviewers:
+ treib@chromium.org

[markusheintz_, 2017-03-16 09:56:00]

On 2017/03/16 09:46:56, mastiz wrote:

Is there any reason why we ever should include cookies?
Not sending cookies should be the default.

[markusheintz_, 2017-03-16 09:59:05]

markusheintz@chromium.org changed reviewers:
+ markusheintz@chromium.org

[markusheintz_, 2017-03-16 09:59:06]

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

https://codereview.chromium.org/2756633003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.cc:42: disable_cookies_(false) {}
The default should be true. 

If we need a way to enable cookies, we should have a EnableCookies method.

https://codereview.chromium.org/2756633003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.cc:78: if (disable_cookies_) {
Not fetching cookies should be the default.

[Marc Treib, 2017-03-16 10:04:56]

I agree with Markus: Not sending cookies should be the default. I'm not
convinced that we even need a way to turn them on.

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

https://codereview.chromium.org/2756633003/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.h:53: void DisableCookies();
I prefer formulating things positively: EnableCookies(bool), or
SetCookiesAllowed(bool)

[mastiz, 2017-03-16 12:18:52]

It's hard for me to claim that no clients of ImageFetcher relies on cookies.

Unless you guys are very confident about this fact, I'd rather not change the
default. WDYT?

[Marc Treib, 2017-03-16 13:29:06]

On 2017/03/16 12:18:52, mastiz wrote:
> It's hard for me to claim that no clients of ImageFetcher relies on cookies.
> 
> Unless you guys are very confident about this fact, I'd rather not change the
> default. WDYT?

Hm, fair enough. URLFetcher also has cookies enabled by default, so I guess it
makes sense to do the same.

[mastiz, 2017-03-16 14:10:12]

Description was changed from

==========
[Image Fetcher] Add support to disable cookies

We'd like to avoid sending cookies for certain use-cases like fetching
favicons from Google servers. Design Doc in go/favicon-pe-google-server.

BUG=644102
==========

to

==========
[Image Fetcher] Fix cookies being enabled by default

image_fetcher is used by components that fetch images outside the
content area, hence cookies are undesirable.

BUG=644102,702200
==========

[mastiz, 2017-03-16 14:11:57]

On 2017/03/16 13:29:06, Marc Treib wrote:
> On 2017/03/16 12:18:52, mastiz wrote:
> > It's hard for me to claim that no clients of ImageFetcher relies on cookies.
> > 
> > Unless you guys are very confident about this fact, I'd rather not change
the
> > default. WDYT?
> 
> Hm, fair enough. URLFetcher also has cookies enabled by default, so I guess it
> makes sense to do the same.

PTAL.

markusheinz@ was confident that this is accidental and that the image_fetcher,
being used outside the content area, should disable cookies.

He's also willing to take the blame if we break other features, but I'll try to
contact the corresponding owners for a heads-up.

[Marc Treib, 2017-03-16 14:19:54]

On 2017/03/16 14:11:57, mastiz wrote:
> On 2017/03/16 13:29:06, Marc Treib wrote:
> > On 2017/03/16 12:18:52, mastiz wrote:
> > > It's hard for me to claim that no clients of ImageFetcher relies on
cookies.
> > > 
> > > Unless you guys are very confident about this fact, I'd rather not change
> the
> > > default. WDYT?
> > 
> > Hm, fair enough. URLFetcher also has cookies enabled by default, so I guess
it
> > makes sense to do the same.
> 
> PTAL.
> 
> markusheinz@ was confident that this is accidental and that the image_fetcher,
> being used outside the content area, should disable cookies.
> 
> He's also willing to take the blame if we break other features, but I'll try
to
> contact the corresponding owners for a heads-up.

I'm also sure that this is accidental, but that doesn't mean that nobody depends
on it now :D
Anyway, this plan SGTM. Let's just disable cookies here and see if anything
breaks.

[mastiz, 2017-03-17 12:18:08]

Friendly ping, thx!

[Marc Treib, 2017-03-17 12:20:40]

lgtm

Sorry for the delay

[Marc Treib, 2017-03-17 12:21:16]

On 2017/03/17 12:20:40, Marc Treib wrote:
> lgtm
> 
> Sorry for the delay

nit: Maybe update the CL title, for posterity

[mastiz, 2017-03-17 12:55:02]

Description was changed from

==========
[Image Fetcher] Fix cookies being enabled by default

image_fetcher is used by components that fetch images outside the
content area, hence cookies are undesirable.

BUG=644102,702200
==========

to

==========
[Image Fetcher] Fix cookies being enabled by default

image_fetcher is used by components that fetch images outside the
content area, hence cookies are undesirable.

A quick look at existing clients suggests none are relying on
cookies, hence we disable them by default and introduce no API for
now to toggle them.

BUG=644102,702200
==========

[markusheintz_, 2017-03-17 12:56:26]

On 2017/03/17 12:21:16, Marc Treib wrote:
> On 2017/03/17 12:20:40, Marc Treib wrote:
> > lgtm
> > 
> > Sorry for the delay
> 
> nit: Maybe update the CL title, for posterity

LGTM.

Thanks a lot for fixing this.

[mastiz, 2017-03-17 12:57:30]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2017-03-17 12:57:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 12:57:44]

This CL has an open dependency (Issue 2750313002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[mastiz, 2017-03-17 16:59:07]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-03-17 16:59:08]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
markusheintz@chromium.org
Link to the patchset: https://codereview.chromium.org/2756633003/#ps60001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-03-17 16:59:34]

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

[commit-bot: I haz the power, 2017-03-17 17:54:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489769947804740,
"parent_rev": "53f956f3080cfc242b5639e636e34ffde7e3962f", "commit_rev":
"4409b00db808ea391f7ba279126d12f3587e933c"}

[commit-bot: I haz the power, 2017-03-17 17:55:24]

Description was changed from

==========
[Image Fetcher] Fix cookies being enabled by default

image_fetcher is used by components that fetch images outside the
content area, hence cookies are undesirable.

A quick look at existing clients suggests none are relying on
cookies, hence we disable them by default and introduce no API for
now to toggle them.

BUG=644102,702200
==========

to

==========
[Image Fetcher] Fix cookies being enabled by default

image_fetcher is used by components that fetch images outside the
content area, hence cookies are undesirable.

A quick look at existing clients suggests none are relying on
cookies, hence we disable them by default and introduce no API for
now to toggle them.

BUG=644102,702200

Review-Url: https://codereview.chromium.org/2756633003
Cr-Commit-Position: refs/heads/master@{#457811}
Committed:
https://chromium.googlesource.com/chromium/src/+/4409b00db808ea391f7ba279126d...
==========

[commit-bot: I haz the power, 2017-03-17 17:55:26]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4409b00db808ea391f7ba279126d...