Make several methods of TemplateUrlService const

Make several methods of TemplateUrlService const

BUG=672364
Review-Url: https://codereview.chromium.org/2811793007
Cr-Commit-Position: refs/heads/master@{#464630}
Committed: https://chromium.googlesource.com/chromium/src/+/9c4df02e620f303e9e3f65818323bc3dc2dc29aa

[jeffschiller, 2017-04-11 16:51:40]

Description was changed from

==========
Make several methods of TemplateUrlService const

BUG=672364
==========

to

==========
Make several methods of TemplateUrlService const

BUG=672364
==========

[jeffschiller, 2017-04-11 16:51:40]

jeffschiller@google.com changed reviewers:
+ pkasting@chromium.org

[jeffschiller, 2017-04-13 03:36:06]

Hi Peter,

I thought I'd tackle a good-first-bug in Chrome :)

Thanks,
Jeff

[Peter Kasting, 2017-04-13 04:04:52]

Yay for code cleanup!

https://codereview.chromium.org/2811793007/diff/1/components/search_engines/t...
File components/search_engines/template_url_service.h (right):

https://codereview.chromium.org/2811793007/diff/1/components/search_engines/t...
components/search_engines/template_url_service.h:169: TemplateURL*
GetTemplateURLForKeyword(const base::string16& keyword) const;
Const methods shouldn't return non-const pointers, so either these next three
need to remain non-const, or return pointers-to-const, or we need const and
non-const versions.

The same is true of the other methods below that return TemplateURL*s.

[jeffschiller, 2017-04-13 05:53:56]

On 2017/04/13 at 04:04:52, pkasting wrote:
> Yay for code cleanup!
> 
>
https://codereview.chromium.org/2811793007/diff/1/components/search_engines/t...
> File components/search_engines/template_url_service.h (right):
> 
>
https://codereview.chromium.org/2811793007/diff/1/components/search_engines/t...
> components/search_engines/template_url_service.h:169: TemplateURL*
GetTemplateURLForKeyword(const base::string16& keyword) const;
> Const methods shouldn't return non-const pointers, so either these next three
need to remain non-const, or return pointers-to-const, or we need const and
non-const versions.
> 
> The same is true of the other methods below that return TemplateURL*s.

PTAL.

Thanks, I'm still reviving my C++ knowledge, so I took care of the easiest ones
with this CL and will start to work on the methods that return pointers next.

[Peter Kasting, 2017-04-13 06:15:35]

LGTM

https://codereview.chromium.org/2811793007/diff/60001/components/search_engin...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2811793007/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1440: TemplateURL* engine2) {
Similarly, can probably revert this

https://codereview.chromium.org/2811793007/diff/60001/components/search_engin...
File components/search_engines/template_url_service.h (right):

https://codereview.chromium.org/2811793007/diff/60001/components/search_engin...
components/search_engines/template_url_service.h:498: TemplateURL* engine2);
Nit: Can probably revert this

[jeffschiller, 2017-04-13 21:55:57]

On 2017/04/13 at 06:15:35, pkasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/2811793007/diff/60001/components/search_engin...
> File components/search_engines/template_url_service.cc (right):
> 
>
https://codereview.chromium.org/2811793007/diff/60001/components/search_engin...
> components/search_engines/template_url_service.cc:1440: TemplateURL* engine2)
{
> Similarly, can probably revert this
> 
>
https://codereview.chromium.org/2811793007/diff/60001/components/search_engin...
> File components/search_engines/template_url_service.h (right):
> 
>
https://codereview.chromium.org/2811793007/diff/60001/components/search_engin...
> components/search_engines/template_url_service.h:498: TemplateURL* engine2);
> Nit: Can probably revert this

Thanks - PTAL.

I had to do a couple const_cast on "this" so that newly-const functions can
still call non-const functions.  Will work on making the member functions that
return TemplateURL* into const after this CL.

[Peter Kasting, 2017-04-13 22:42:52]

LGTM with one concern

https://codereview.chromium.org/2811793007/diff/120001/components/search_engi...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2811793007/diff/120001/components/search_engi...
components/search_engines/template_url_service.cc:855:
const_cast<TemplateURLService*>(this)->GetTemplateURLForKeyword(keyword);
If you're 100% sure you'll be able to convert these functions (or add parallel
versions) and this cast will only be alive for a few days, OK.

Otherwise, I'd either avoid converting these functions in this patch, or send a
larger patch that goes ahead and converts the callees, so we don't have to add
const_cast.

https://codereview.chromium.org/2811793007/diff/120001/components/search_engi...
components/search_engines/template_url_service.cc:2238: const TemplateURL*
templateURL =
Nit: C++ variable names are lowercase, with underscores; see
http://google.github.io/styleguide/cppguide.html#Variable_Names

https://codereview.chromium.org/2811793007/diff/120001/components/search_engi...
components/search_engines/template_url_service.cc:2241: DCHECK(templateURL);
Nit: While I'm normally fine with pulling temps out of expressions to avoid
long, multi-line things, the temp above is only used in this DCHECK, so pulling
it out basically means that when DCHECKs are off, the temp is dead code. 
Because it makes a function call, that probably means the compiler won't
eliminate it, and more critically, it might not be instantly obvious to readers
that it's only used there (although it's easier in this specific case).

So I'd weakly prefer leaving the expression inlined here, even though it's ugly.

[jeffschiller, 2017-04-13 23:46:10]

On 2017/04/13 at 22:42:52, pkasting wrote:
> LGTM with one concern
> 
>
https://codereview.chromium.org/2811793007/diff/120001/components/search_engi...
> File components/search_engines/template_url_service.cc (right):
> 
>
https://codereview.chromium.org/2811793007/diff/120001/components/search_engi...
> components/search_engines/template_url_service.cc:855:
const_cast<TemplateURLService*>(this)->GetTemplateURLForKeyword(keyword);
> If you're 100% sure you'll be able to convert these functions (or add parallel
versions) and this cast will only be alive for a few days, OK.
> 
> Otherwise, I'd either avoid converting these functions in this patch, or send
a larger patch that goes ahead and converts the callees, so we don't have to add
const_cast.

Intention is to do this next, so yes, just a few days.  I've added TODOs.

> 
>
https://codereview.chromium.org/2811793007/diff/120001/components/search_engi...
> components/search_engines/template_url_service.cc:2238: const TemplateURL*
templateURL =
> Nit: C++ variable names are lowercase, with underscores; see
http://google.github.io/styleguide/cppguide.html#Variable_Names
> 
>
https://codereview.chromium.org/2811793007/diff/120001/components/search_engi...
> components/search_engines/template_url_service.cc:2241: DCHECK(templateURL);
> Nit: While I'm normally fine with pulling temps out of expressions to avoid
long, multi-line things, the temp above is only used in this DCHECK, so pulling
it out basically means that when DCHECKs are off, the temp is dead code. 
Because it makes a function call, that probably means the compiler won't
eliminate it, and more critically, it might not be instantly obvious to readers
that it's only used there (although it's easier in this specific case).
> 
> So I'd weakly prefer leaving the expression inlined here, even though it's
ugly.

Done.

[jeffschiller, 2017-04-13 23:48:28]

The CQ bit was checked by jeffschiller@google.com

[jeffschiller, 2017-04-13 23:48:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2811793007/#ps140001
(title: "Remove unnecessary local variable and add a couple TODOs")

[commit-bot: I haz the power, 2017-04-13 23:48:59]

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-04-14 00:47:51]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492127308382130,
"parent_rev": "e6f79e605ba482c827e07f30378eb5a61a43a677", "commit_rev":
"9c4df02e620f303e9e3f65818323bc3dc2dc29aa"}

[commit-bot: I haz the power, 2017-04-14 00:48:41]

Description was changed from

==========
Make several methods of TemplateUrlService const

BUG=672364
==========

to

==========
Make several methods of TemplateUrlService const

BUG=672364

Review-Url: https://codereview.chromium.org/2811793007
Cr-Commit-Position: refs/heads/master@{#464630}
Committed:
https://chromium.googlesource.com/chromium/src/+/9c4df02e620f303e9e3f65818323...
==========

[commit-bot: I haz the power, 2017-04-14 00:48:42]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/9c4df02e620f303e9e3f65818323...