Adding a parameter that will be used to request a transparent doodle.

Adding a parameter that will be used to request a transparent doodle.

As part of the query URL add an optional paramater that specifies whether
we want to download a transparent doodle.

BUG=610319
Committed: https://crrev.com/704e8bc7ebcd4f699df985b7b8685f651b3d4cf1
Cr-Commit-Position: refs/heads/master@{#392603}

[atanasova, 2016-05-09 15:24:24]

Description was changed from

==========
Adding a parameter that will be used to request a transparent doodle.

As part of the query URL add an optional paramater that specifies whether
we want to download a transparent doodle.

BUG=610319
==========

to

==========
Adding a parameter that will be used to request a transparent doodle.

As part of the query URL add an optional paramater that specifies whether
we want to download a transparent doodle.

BUG=610319
==========

[atanasova, 2016-05-09 15:24:24]

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

[atanasova, 2016-05-09 15:24:43]

The CQ bit was checked by atanasova@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-09 15:24:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962013002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962013002/1

[commit-bot: I haz the power, 2016-05-09 16:14:28]

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

[commit-bot: I haz the power, 2016-05-09 16:14:29]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-05-09 16:58:21]

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
File components/search_provider_logos/google_logo_api.cc (right):

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
components/search_provider_logos/google_logo_api.cc:41: if (wants_cta ||
transparent)
Hm, maybe it's time to push all the params into a vector and join them using
base::JoinString, instead of manually fiddling with commas? WDYT?

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
File components/search_provider_logos/google_logo_api.h (right):

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
components/search_provider_logos/google_logo_api.h:22: bool transparent =
false);
We generally don't use default values for arguments in Chromium. You'll have to
either update all call sites, or alternatively add an overload (probably with a
different name). I'd prefer updating the call sites though.

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
File components/search_provider_logos/logo_tracker.h (right):

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
components/search_provider_logos/logo_tracker.h:127: // |transparent| determines
whether to request a transparent logo
nit: Period at the end of the line please

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
components/search_provider_logos/logo_tracker.h:135: bool transparent = false);
Also here: No default values :(

[atanasova, 2016-05-10 08:32:17]

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
File components/search_provider_logos/google_logo_api.cc (right):

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
components/search_provider_logos/google_logo_api.cc:41: if (wants_cta ||
transparent)
On 2016/05/09 16:58:21, Marc Treib wrote:
> Hm, maybe it's time to push all the params into a vector and join them using
> base::JoinString, instead of manually fiddling with commas? WDYT?

Done.

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
File components/search_provider_logos/google_logo_api.h (right):

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
components/search_provider_logos/google_logo_api.h:22: bool transparent =
false);
On 2016/05/09 16:58:21, Marc Treib wrote:
> We generally don't use default values for arguments in Chromium. You'll have
to
> either update all call sites, or alternatively add an overload (probably with
a
> different name). I'd prefer updating the call sites though.

I'll update the code, my only concern is breaking the ios code that uses these
methods.

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
File components/search_provider_logos/logo_tracker.h (right):

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
components/search_provider_logos/logo_tracker.h:127: // |transparent| determines
whether to request a transparent logo
On 2016/05/09 16:58:21, Marc Treib wrote:
> nit: Period at the end of the line please

Done.

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
components/search_provider_logos/logo_tracker.h:135: bool transparent = false);
On 2016/05/09 16:58:21, Marc Treib wrote:
> Also here: No default values :(

Done.

[atanasova, 2016-05-10 08:32:22]

The CQ bit was checked by atanasova@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-10 08:32:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962013002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962013002/20001

[Marc Treib, 2016-05-10 08:48:44]

LGTM, thanks!
I don't actually own anything here though ;)

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
File components/search_provider_logos/google_logo_api.h (right):

https://codereview.chromium.org/1962013002/diff/1/components/search_provider_...
components/search_provider_logos/google_logo_api.h:22: bool transparent =
false);
On 2016/05/10 08:32:17, atanasova wrote:
> On 2016/05/09 16:58:21, Marc Treib wrote:
> > We generally don't use default values for arguments in Chromium. You'll have
> to
> > either update all call sites, or alternatively add an overload (probably
with
> a
> > different name). I'd prefer updating the call sites though.
> 
> I'll update the code, my only concern is breaking the ios code that uses these
> methods.

The iOS bots will tell you if anything breaks, and fixing it will be trivial :)
(If anything breaks downstream, then it's the gardener's job to fix it. And,
again, that'll be trivial.)

https://codereview.chromium.org/1962013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/logo_service.cc (right):

https://codereview.chromium.org/1962013002/diff/20001/chrome/browser/android/...
chrome/browser/android/logo_service.cc:129: false);
optional nit: You could add comments saying what these are for, like
true /* wants_cta */

[atanasova, 2016-05-10 09:04:54]

atanasova@chromium.org changed reviewers:
+ bauerb@chromium.org, justincohen@chromium.org

[atanasova, 2016-05-10 09:04:55]

@justincohen
Please review the changes in search_provider_logos. As mentioned in the
description this is done in order to request a transparent doodle.

https://codereview.chromium.org/1962013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/logo_service.cc (right):

https://codereview.chromium.org/1962013002/diff/20001/chrome/browser/android/...
chrome/browser/android/logo_service.cc:129: false);
On 2016/05/10 08:48:44, Marc Treib wrote:
> optional nit: You could add comments saying what these are for, like
> true /* wants_cta */

Done.

[Bernhard Bauer, 2016-05-10 09:15:24]

lgtm

https://codereview.chromium.org/1962013002/diff/40001/components/search_provi...
File components/search_provider_logos/google_logo_api.cc (right):

https://codereview.chromium.org/1962013002/diff/40001/components/search_provi...
components/search_provider_logos/google_logo_api.cc:47:
params.push_back("transp:1");
Nit: empty line afterwards.

[justincohen, 2016-05-10 13:47:26]

I'm assuming iOS can safely set this to false, since the doodle sits over a
white background?  Or is there something else going on, and iOS should also
request a transparent doodle?

Changes LGTM

[atanasova, 2016-05-10 13:51:25]

iOS can set it to false. This change is required in Android that is moving to a
material design NTP which has a non-white background.

https://codereview.chromium.org/1962013002/diff/40001/components/search_provi...
File components/search_provider_logos/google_logo_api.cc (right):

https://codereview.chromium.org/1962013002/diff/40001/components/search_provi...
components/search_provider_logos/google_logo_api.cc:47:
params.push_back("transp:1");
On 2016/05/10 09:15:24, Bernhard Bauer wrote:
> Nit: empty line afterwards.

Done.

[atanasova, 2016-05-10 14:11:30]

The CQ bit was checked by atanasova@chromium.org

[atanasova, 2016-05-10 14:11:31]

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

[commit-bot: I haz the power, 2016-05-10 14:11:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962013002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962013002/60001

[commit-bot: I haz the power, 2016-05-10 15:01:50]

Description was changed from

==========
Adding a parameter that will be used to request a transparent doodle.

As part of the query URL add an optional paramater that specifies whether
we want to download a transparent doodle.

BUG=610319
==========

to

==========
Adding a parameter that will be used to request a transparent doodle.

As part of the query URL add an optional paramater that specifies whether
we want to download a transparent doodle.

BUG=610319
==========

[commit-bot: I haz the power, 2016-05-10 15:01:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-10 15:03:25]

Description was changed from

==========
Adding a parameter that will be used to request a transparent doodle.

As part of the query URL add an optional paramater that specifies whether
we want to download a transparent doodle.

BUG=610319
==========

to

==========
Adding a parameter that will be used to request a transparent doodle.

As part of the query URL add an optional paramater that specifies whether
we want to download a transparent doodle.

BUG=610319

Committed: https://crrev.com/704e8bc7ebcd4f699df985b7b8685f651b3d4cf1
Cr-Commit-Position: refs/heads/master@{#392603}
==========

[commit-bot: I haz the power, 2016-05-10 15:03:26]

Patchset 4 (id:??) landed as
https://crrev.com/704e8bc7ebcd4f699df985b7b8685f651b3d4cf1
Cr-Commit-Position: refs/heads/master@{#392603}