Move most TemplateURL data members to a new struct, TemplateURLData. This allows us to eliminate t…

Move most TemplateURL data members to a new struct, TemplateURLData.  This allows us to eliminate the TemplateURL NULL constructor, most public non-const TemplateURL functions, and most TemplateURL friend declarations.

This is also a necessary precursor to changing TemplateURLService's APIs to convert most "const TemplateURL*" cases to "TemplateURL*", which I'll explain once I actually make the change.

There is some awkwardness here around keywords, as keyword autogeneration requires a TemplateURL but the state bits are kept on TemplateURLData.  This will go away in the future when I remove keyword autogeneration from TemplateURL entirely.

BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131019

[Peter Kasting, 2012-04-04 22:38:40]

sky: review
mnissler: policy OWNERS
atwilson: sync OWNERS

[Peter Kasting, 2012-04-04 22:41:40]

BTW, just to note, there are some nontrivial changes in a few places to convert
to more "SSA-style" usage of TemplateURLs, so don't just rubber-stamp this.

[sky, 2012-04-05 03:42:15]

Every looks good, except for
chrome/browser/search_engines/template_url_service.cc:1265: .

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/impo...
File chrome/browser/importer/profile_import_process_messages.h (right):

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/impo...
chrome/browser/importer/profile_import_process_messages.h:197: WriteParam(m,
p.short_name);
Can you add a comment as to why you don't serialize certain values.

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url.h (right):

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url.h:246: 
I think you'll want to declare the destructor too. For something this size I
suspect the clang bot will through a fit if you don't.

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url.h:250: string16 short_name;
move after functions.

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url.h:266: void EnsureKeyword(const
TemplateURL* t_url) const;
Description?

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_parser.h (right):

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url_parser.h:40: bool
show_in_default_list,
nit: use an enum.

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service.cc (left):

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service.cc:1265:
(*default_provider)->set_show_in_default_list(true);
How come you're removing this?

[Mattias Nissler (ping if slow), 2012-04-05 08:28:06]

lgtm

[Peter Kasting, 2012-04-05 19:52:21]

New snap up.

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/impo...
File chrome/browser/importer/profile_import_process_messages.h (right):

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/impo...
chrome/browser/importer/profile_import_process_messages.h:197: WriteParam(m,
p.short_name);
On 2012/04/05 03:42:15, sky wrote:
> Can you add a comment as to why you don't serialize certain values.

It wasn't my intent to miss any.  I've added |id|, |created_by_policy| and
|sync_guid|.  Were there any others I missed?

(Technically, I don't think any of the above would have been touched by the
importer code, but I don't think this layer should rely on that.)

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url.h (right):

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url.h:246: 
On 2012/04/05 03:42:15, sky wrote:
> I think you'll want to declare the destructor too. For something this size I
> suspect the clang bot will through a fit if you don't.

It did, and I now have.

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url.h:250: string16 short_name;
On 2012/04/05 03:42:15, sky wrote:
> move after functions.

Normally I'd agree.  In this class I was trying to present the appearance of a
standard "data-only" struct, except with access control for the keyword and URL
fields.  (When I eliminate keyword autogeneration, four of the six
keyword-related functions here will vanish, leaving only a getter/setter.)  So I
wanted to keep the order "logical" -- name, keyword, URLs, etc.  This also
matches the order in which I set this data in the various callers.

WDYT?

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url.h:266: void EnsureKeyword(const
TemplateURL* t_url) const;
On 2012/04/05 03:42:15, sky wrote:
> Description?

OK.  Moved the one in TemplateURL to here.

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_parser.h (right):

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url_parser.h:40: bool
show_in_default_list,
On 2012/04/05 03:42:15, sky wrote:
> nit: use an enum.

Ended up not doing this because it would require adding a conditional to pass
the "real" bool to a setter internally.

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service.cc (left):

https://chromiumcodereview.appspot.com/9982018/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service.cc:1265:
(*default_provider)->set_show_in_default_list(true);
On 2012/04/05 03:42:15, sky wrote:
> How come you're removing this?

Not removing it -- it's moved up a bit, look on line 1280.

[sky, 2012-04-05 20:14:52]

LGTM

[Andrew T Wilson (Slow), 2012-04-05 20:33:28]

browser/sync LGTM

[nsylvain, 2012-04-06 14:19:05]

Our perf memory tests stopped running around this cl. Do you think it could have
caused this? 

[ RUN      ] GeneralMixMemoryTest.SingleTabTest
[2491:2491:255910355:ERROR:spellcheck_host_impl.cc(331)] Failure to download
dictionary.
[2553:2553:256477531:ERROR:gl_surface_glx.cc(65)] GLX 1.3 or later is required.
[2553:2553:256499001:ERROR:gl_surface_linux.cc(64)]
GLSurfaceGLX::InitializeOneOff failed.
[2565:2565:257133302:ERROR:gl_surface_glx.cc(65)] GLX 1.3 or later is required.
[2565:2565:257133380:ERROR:gl_surface_linux.cc(64)]
GLSurfaceGLX::InitializeOneOff failed.
[2489:2489:0405/153229:259208843:ERROR:automation_proxy.cc(453)] Disconnecting
channel after error!
chrome/test/ui/ui_test.cc:428: Failure
Value of: window.get()
  Actual: false
Expected: true
Google Test trace:
chrome/test/perf/memory_test.cc:121: <NEWTAB>
[2489:2489:0405/153229:259209010:ERROR:automation_proxy.cc(440)] Automation
channel has been closed; dropping message!
chrome/test/perf/memory_test.cc:134: Failure
Value of: window->AppendTab(GURL(url))
  Actual: false
Expected: true
Google Test trace:
chrome/test/perf/memory_test.cc:121: <NEWTAB>
[2489:2489:0405/153229:259209062:ERROR:automation_proxy.cc(440)] Automation
channel has been closed; dropping message!
chrome/test/ui/ui_test.cc:428: Failure
Value of: window.get()
  Actual: false
Expected: true
Google Test trace:
chrome/test/perf/memory_test.cc:121: <NEWTAB>
[2489:2489:0405

[brettw, 2012-04-06 19:45:33]

I'm definitely hitting a use-after-free of t_url in
TemplateURLService::SaveDefaultSearchProviderToPrefs