The browser importer code which runs in the utility process should not depend on chrome\browser dat…

The browser importer code which runs in the utility process should not depend on chrome\browser data types like TemplateURL, etc.

This CL removes the usage of chrome\browser\search_engines\template_url** from the ExternalProcessImporterBridge class.
To achieve this, a structure URLKeywordInfo has been added to the importer_data_types.h file. The
ImporterBridge::SetKeywords virtual function has been changed to accept a vector of the URLKeywordInfo structures.
This is converted to TempateURL instances in the InProcessImporterBridge class which lives in the browser process.

The other change is in the Firefox3 importer where in we pass the Firefox profile XML data in a vector to the browser
via the newly added virtual function ImporterBridge::SetFirefoxSearchEnginesXMLData. The ExternalProcessImporterBridge 
implementation of this function sends over the XML data via an IPC to the browser. The InProcessImporterBridge implementation
parses this data to retrieve the search engine information which is then imported.

This is continuation of the ongoing work for bug https://code.google.com/p/chromium/issues/detail?can=2&q=237249
which is to split chrome.dll into a browser and a renderer/child component due to build issues on Windows. To achieve
this we need to ensure that the browser code does not depend on webkit/child. 

BUG=237249
R=gab@chromium.org, jschuh@chromium.org, scottmg@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209072

[scottmg, 2013-06-27 15:47:21]

lgtm, but gab will know better :)

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/e...
File chrome/browser/importer/external_process_importer_client.cc (right):

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_client.cc:106:
OnFirefoxSearchEngineDataReceived)
nit; make indent match other messages

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/i...
File chrome/browser/importer/importer_bridge.h (right):

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/i...
chrome/browser/importer/importer_bridge.h:55: virtual void
SetFirefoxSearchEnginesXMLData(
nit; any way this name could be more descriptive? "XMLData" and a vector<string>
doesn't really describe much. maybe just a comment here to describe where that
xml comes from.

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/p...
File chrome/browser/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/p...
chrome/browser/importer/profile_import_process_messages.h:297:
ProfileImportProcessHostMsg_NotifyFirefoxSearchEngineData,
nit; indent match

[ananta, 2013-06-27 15:56:49]

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/e...
File chrome/browser/importer/external_process_importer_client.cc (right):

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_client.cc:106:
OnFirefoxSearchEngineDataReceived)
On 2013/06/27 15:47:21, scottmg wrote:
> nit; make indent match other messages

Changed the IPC name to ProfileImportProcessHostMsg_NotifyFirefoxSearchEngData
to get this to
fit within 80 columns

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/i...
File chrome/browser/importer/importer_bridge.h (right):

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/i...
chrome/browser/importer/importer_bridge.h:55: virtual void
SetFirefoxSearchEnginesXMLData(
On 2013/06/27 15:47:21, scottmg wrote:
> nit; any way this name could be more descriptive? "XMLData" and a
vector<string>
> doesn't really describe much. maybe just a comment here to describe where that
> xml comes from.

Done.

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/p...
File chrome/browser/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/p...
chrome/browser/importer/profile_import_process_messages.h:297:
ProfileImportProcessHostMsg_NotifyFirefoxSearchEngineData,
On 2013/06/27 15:47:21, scottmg wrote:
> nit; indent match
Changed the IPC name to fit this within 80 columns

[jschuh, 2013-06-27 16:16:28]

ipc security lgtm. communication is privileged process to privileged process, so
there's no attack surface exposed.

[scottmg, 2013-06-27 16:54:08]

On 2013/06/27 15:56:49, ananta wrote:
>
https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/e...
> File chrome/browser/importer/external_process_importer_client.cc (right):
> 
>
https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/e...
> chrome/browser/importer/external_process_importer_client.cc:106:
> OnFirefoxSearchEngineDataReceived)
> On 2013/06/27 15:47:21, scottmg wrote:
> > nit; make indent match other messages
> 
> Changed the IPC name to ProfileImportProcessHostMsg_NotifyFirefoxSearchEngData
> to get this to
> fit within 80 columns
> 
>
https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/i...
> File chrome/browser/importer/importer_bridge.h (right):
> 
>
https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/i...
> chrome/browser/importer/importer_bridge.h:55: virtual void
> SetFirefoxSearchEnginesXMLData(
> On 2013/06/27 15:47:21, scottmg wrote:
> > nit; any way this name could be more descriptive? "XMLData" and a
> vector<string>
> > doesn't really describe much. maybe just a comment here to describe where
that
> > xml comes from.
> 
> Done.
> 
>
https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/p...
> File chrome/browser/importer/profile_import_process_messages.h (right):
> 
>
https://codereview.chromium.org/18064002/diff/36003/chrome/browser/importer/p...
> chrome/browser/importer/profile_import_process_messages.h:297:
> ProfileImportProcessHostMsg_NotifyFirefoxSearchEngineData,
> On 2013/06/27 15:47:21, scottmg wrote:
> > nit; indent match
> Changed the IPC name to fit this within 80 columns

Oh, sorry. Your original names were better. I didn't notice they were too long.

[gab, 2013-06-27 22:07:29]

Looks good, comments below.

Cheers!
Gab

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
File chrome/browser/importer/external_process_importer_bridge.cc (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_bridge.cc:129: const
std::vector<importer::URLKeywordInfo>& url_keywords,
#include "chrome/browser/importer/importer_data_types.h"

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
File chrome/browser/importer/external_process_importer_bridge.h (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_bridge.h:60: const
std::vector<importer::URLKeywordInfo>& url_keywords,
Forward-declare URLKeywordInfo

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
File chrome/browser/importer/external_process_importer_client.h (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_client.h:13: #include
"base/files/file_path.h"
Why add this include?

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_client.h:25: class
TemplateURL;
Remove fwd-decl

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_client.h:70: const
std::vector<importer::URLKeywordInfo>& url_keywords,
Fwd-decl URLKeywordInfo.

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/f...
File chrome/browser/importer/firefox3_importer.cc (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/f...
chrome/browser/importer/firefox3_importer.cc:357:
bridge_->SetFirefoxSearchEnginesXMLData(search_engine_data);
The reason to do the import out-of-process is mostly to avoid crashing the
browser process while reading arbitrary data.

It seems ParseSearchEnginesFromXMLFiles() was only used by this call, can't we
keep this call and move it in the utility side of the split along with the
importers (changing the call slightly to use URLKeywordInfo instead of
TemplateURL)?

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/i...
File chrome/browser/importer/importer_bridge.h (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/i...
chrome/browser/importer/importer_bridge.h:23: class TemplateURL;
Remove fwd-decl.

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/i...
chrome/browser/importer/importer_bridge.h:52: const
std::vector<importer::URLKeywordInfo>& url_keywords,
Fwd-decl URLKeywordInfo

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/i...
chrome/browser/importer/importer_bridge.h:57: virtual void
SetFirefoxSearchEnginesXMLData(
I would rather not add a Firefox only method to this interface. As mentioned in
a previous comment I think it is better to move ParseSearchEnginesFromXMLFiles()
on the utility side of the split and keep sending data via SetKeywords() for
this import mechanism.

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/p...
File chrome/browser/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/p...
chrome/browser/importer/profile_import_process_messages.h:200: template <>
Add a comment above this line:
// Traits for importer::URLKeywordInfo

to match other definitions in this file.

[ananta, 2013-06-27 22:29:55]

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
File chrome/browser/importer/external_process_importer_bridge.cc (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_bridge.cc:129: const
std::vector<importer::URLKeywordInfo>& url_keywords,
On 2013/06/27 22:07:29, gab wrote:
> #include "chrome/browser/importer/importer_data_types.h"

Done.

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
File chrome/browser/importer/external_process_importer_bridge.h (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_bridge.h:60: const
std::vector<importer::URLKeywordInfo>& url_keywords,
On 2013/06/27 22:07:29, gab wrote:
> Forward-declare URLKeywordInfo

Done.

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
File chrome/browser/importer/external_process_importer_client.h (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_client.h:13: #include
"base/files/file_path.h"
On 2013/06/27 22:07:29, gab wrote:
> Why add this include?

Artifact from a previous patch iteration. Gone

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_client.h:25: class
TemplateURL;
On 2013/06/27 22:07:29, gab wrote:
> Remove fwd-decl

Done.

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_client.h:70: const
std::vector<importer::URLKeywordInfo>& url_keywords,
On 2013/06/27 22:07:29, gab wrote:
> Fwd-decl URLKeywordInfo.

Done.

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/f...
File chrome/browser/importer/firefox3_importer.cc (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/f...
chrome/browser/importer/firefox3_importer.cc:357:
bridge_->SetFirefoxSearchEnginesXMLData(search_engine_data);
On 2013/06/27 22:07:29, gab wrote:
> The reason to do the import out-of-process is mostly to avoid crashing the
> browser process while reading arbitrary data.
> 
> It seems ParseSearchEnginesFromXMLFiles() was only used by this call, can't we
> keep this call and move it in the utility side of the split along with the
> importers (changing the call slightly to use URLKeywordInfo instead of
> TemplateURL)?


The ParseSearchEnginesFromXMLFiles function uses the TemplateURLParser class to
parse the SAX XML data from Firefox. This class lives in
chrome\browser\search_engines and sadly depends on Profile* etc.
which is why I moved the whole function out

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/i...
File chrome/browser/importer/importer_bridge.h (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/i...
chrome/browser/importer/importer_bridge.h:52: const
std::vector<importer::URLKeywordInfo>& url_keywords,
On 2013/06/27 22:07:29, gab wrote:
> Fwd-decl URLKeywordInfo

Done.

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/i...
chrome/browser/importer/importer_bridge.h:57: virtual void
SetFirefoxSearchEnginesXMLData(
On 2013/06/27 22:07:29, gab wrote:
> I would rather not add a Firefox only method to this interface. As mentioned
in
> a previous comment I think it is better to move
ParseSearchEnginesFromXMLFiles()
> on the utility side of the split and keep sending data via SetKeywords() for
> this import mechanism.
Please look at the comments for the ParseSearchEnginesFromXMLFiles function as
to why this is needed.

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/p...
File chrome/browser/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/18064002/diff/40001/chrome/browser/importer/p...
chrome/browser/importer/profile_import_process_messages.h:200: template <>
On 2013/06/27 22:07:29, gab wrote:
> Add a comment above this line:
> // Traits for importer::URLKeywordInfo
> 
> to match other definitions in this file.

Done.

[gab, 2013-06-27 23:12:44]

As discussed offline, I don't like the potential effect on stability this could
have, the reason import is done out-of-process is so that crashes while parsing
unknown data do not affect the browser process (this is even more critical since
this code runs automatically on first run).

What can we do as a follow-up to make sure this doesn't happen? (waiting to see
if this makes top crashers isn't sufficient as first run crashes are much worst
than regular crashes imo as they affect the first impression a user as of
chrome, so even if this crashes only a little bit, it's not acceptable imo...
for example: the user could be downloading Chrome because their FF profile is
corrupted, yet Chrome crashes on first run because of this change and the user
decides that Chrome isn't much better...).

Anyways, comments below.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
File chrome/browser/importer/importer_bridge.h (right):

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/importer_bridge.h:12: #include "base/files/file_path.h"
Remove this import.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
File chrome/browser/importer/in_process_importer_bridge.cc (right):

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:44: low_value.find("moz:")
!= std::string::npos )
Remove extra space before ')'

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:44: low_value.find("moz:")
!= std::string::npos )
Wrap in {} since conditional is more than 1 line.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:63: if (keyword.empty()) {
I realize that you are copying the old behavior for both IE and FF in a single
function here, but doing either based on keyword.empty() seems very obscure;
it's not clear to me why they need to be different at all (other than generating
the keyword if it's empty).

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:80: void
ParseSearchEnginesFromXMLData(
Add "Firefox" to the name of this method since comments in the method suggest
this is parsing data specific to the way Firefox writes it.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:93:
FirefoxURLParameterFilter param_filter;
Can |param_filter| live outside the loop instead?

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:125: }
+  // namespace

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
File chrome/browser/importer/in_process_importer_bridge.h (right):

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.h:47: const
std::vector<importer::URLKeywordInfo>& url_keywords,
fwd-decl URLKeywordInfo

[ananta, 2013-06-27 23:26:09]

Will watch the crashes once this lands. We can revisit if this causes a spike.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
File chrome/browser/importer/importer_bridge.h (right):

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/importer_bridge.h:12: #include "base/files/file_path.h"
On 2013/06/27 23:12:44, gab wrote:
> Remove this import.

Done.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
File chrome/browser/importer/in_process_importer_bridge.cc (right):

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:44: low_value.find("moz:")
!= std::string::npos )
On 2013/06/27 23:12:44, gab wrote:
> Remove extra space before ')'

Done.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:44: low_value.find("moz:")
!= std::string::npos )
On 2013/06/27 23:12:44, gab wrote:
> Remove extra space before ')'

Done.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:63: if (keyword.empty()) {
On 2013/06/27 23:12:44, gab wrote:
> I realize that you are copying the old behavior for both IE and FF in a single
> function here, but doing either based on keyword.empty() seems very obscure;
> it's not clear to me why they need to be different at all (other than
generating
> the keyword if it's empty).

Done.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:80: void
ParseSearchEnginesFromXMLData(
On 2013/06/27 23:12:44, gab wrote:
> Add "Firefox" to the name of this method since comments in the method suggest
> this is parsing data specific to the way Firefox writes it.

Done.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:93:
FirefoxURLParameterFilter param_filter;
On 2013/06/27 23:12:44, gab wrote:
> Can |param_filter| live outside the loop instead?

Done.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:125: }
On 2013/06/27 23:12:44, gab wrote:
> +  // namespace

Done.

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
File chrome/browser/importer/in_process_importer_bridge.h (right):

https://codereview.chromium.org/18064002/diff/55001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.h:47: const
std::vector<importer::URLKeywordInfo>& url_keywords,
On 2013/06/27 23:12:44, gab wrote:
> fwd-decl URLKeywordInfo

Done.

[scottmg, 2013-06-27 23:44:06]

On 2013/06/27 23:12:44, gab wrote:
> As discussed offline, I don't like the potential effect on stability this
could
> have, the reason import is done out-of-process is so that crashes while
parsing
> unknown data do not affect the browser process (this is even more critical
since
> this code runs automatically on first run).

Hmm, I guess we weren't that aware of all the goals of import being in an
external process.

The primary goal here is:
- importer moves to utility,
- so: utility target lives in the child dll,
- so: blink code can be removed from browser.

If it's really important to have all the file access and parsing done in the
external process, then I guess other options are:

1. duplicate all the parsing functionality into the importer rather than sending
over raw data and parsing in the browser.

2. keep importer in the browser (so it can use the browser types/parsers), and
move whatever we need for the importer from blink into blink_common so that both
the browser and the child dlls can use that.

3. keep importer in the browser, but have the utility process itself start a
subprocess that loads a child (containing webkit) and uses IPC to do the child
stuff.

(+jamesr)

[ananta, 2013-06-27 23:53:52]

On 2013/06/27 23:44:06, scottmg wrote:
> On 2013/06/27 23:12:44, gab wrote:
> > As discussed offline, I don't like the potential effect on stability this
> could
> > have, the reason import is done out-of-process is so that crashes while
> parsing
> > unknown data do not affect the browser process (this is even more critical
> since
> > this code runs automatically on first run).
> 
> Hmm, I guess we weren't that aware of all the goals of import being in an
> external process.
> 
> The primary goal here is:
> - importer moves to utility,
> - so: utility target lives in the child dll,
> - so: blink code can be removed from browser.
> 
> If it's really important to have all the file access and parsing done in the
> external process, then I guess other options are:
> 
> 1. duplicate all the parsing functionality into the importer rather than
sending
> over raw data and parsing in the browser.
> 
> 2. keep importer in the browser (so it can use the browser types/parsers), and
> move whatever we need for the importer from blink into blink_common so that
both
> the browser and the child dlls can use that.
> 
> 3. keep importer in the browser, but have the utility process itself start a
> subprocess that loads a child (containing webkit) and uses IPC to do the child
> stuff.
> 
> (+jamesr)

We should know with crash data from canary if this causes stability issues.
Currently we don't get
any crash data from the utility process as per gab. As a result this is an
unknown at this point.

If this turns out to be too crashy we can always move this back to the utility
process and look into
2 as mentioned by scott.

Some surgery on the TemplateURL code which provides the parsing functionality to
make this work without
relying on browser types could also be in order.

[gab, 2013-06-28 00:28:44]

lgtm w/ nit+question below.

Thanks!
Gab

https://codereview.chromium.org/18064002/diff/38003/chrome/browser/importer/i...
File chrome/browser/importer/in_process_importer_bridge.cc (right):

https://codereview.chromium.org/18064002/diff/38003/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:71:
data.SetKeyword(keyword);
nit: Remove this line, keyword is already taken care of above.

https://codereview.chromium.org/18064002/diff/38003/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:72:
data.SetURL(TemplateURLRef::DisplayURLToURLRef(UTF8ToUTF16(url.spec())));
What's the difference between taking
TemplateURLRef::DisplayURLToURLRef(UTF8ToUTF16(url.spec()) versus url.spec() (IE
and FF didn't do the same thing here and it's not clear to me why... was one
impl simply incorrect?)

[ananta, 2013-06-28 01:03:29]

https://codereview.chromium.org/18064002/diff/38003/chrome/browser/importer/i...
File chrome/browser/importer/in_process_importer_bridge.cc (right):

https://codereview.chromium.org/18064002/diff/38003/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:71:
data.SetKeyword(keyword);
On 2013/06/28 00:28:45, gab wrote:
> nit: Remove this line, keyword is already taken care of above.

Done.

https://codereview.chromium.org/18064002/diff/38003/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:72:
data.SetURL(TemplateURLRef::DisplayURLToURLRef(UTF8ToUTF16(url.spec())));
On 2013/06/28 00:28:45, gab wrote:
> What's the difference between taking
> TemplateURLRef::DisplayURLToURLRef(UTF8ToUTF16(url.spec()) versus url.spec()
(IE
> and FF didn't do the same thing here and it's not clear to me why... was one
> impl simply incorrect?)

The DisplayURLToURLRef function does some processing like replacing %s in the
url with {searchTerms}, etc. This works correctly for both IE and FF

[gab, 2013-06-28 01:55:07]

https://codereview.chromium.org/18064002/diff/38003/chrome/browser/importer/i...
File chrome/browser/importer/in_process_importer_bridge.cc (right):

https://codereview.chromium.org/18064002/diff/38003/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:72:
data.SetURL(TemplateURLRef::DisplayURLToURLRef(UTF8ToUTF16(url.spec())));
On 2013/06/28 01:03:29, ananta wrote:
> On 2013/06/28 00:28:45, gab wrote:
> > What's the difference between taking
> > TemplateURLRef::DisplayURLToURLRef(UTF8ToUTF16(url.spec()) versus url.spec()
> (IE
> > and FF didn't do the same thing here and it's not clear to me why... was one
> > impl simply incorrect?)
> 
> The DisplayURLToURLRef function does some processing like replacing %s in the
> url with {searchTerms}, etc. This works correctly for both IE and FF

I see thanks. re-lgtm.

[ananta, 2013-06-28 02:19:47]

Committed patchset #10 manually as r209072 (presubmit successful).