Move most importer code to chrome/utility/importer

Move most importer code to chrome/utility/importer

Moves the out-of-process parts from chrome/browser/importer into
chrome/utility/importer. The majority here are simple moves, mostly
into chrome/utility/importer and a few into chrome/common/importer.

A few places required splitting files up to put some in different
parts of browser|common|utility. These were:
- chrome/browser/favicon ReencodeFavicon -> utility (it depends on
Blink, so should only be in child processes)
- history::VisitSource as used by ImporterBridge -> new similar enum
in importer_data_types that's remapped once it's in the browser
process
- LogImporterUseToMetrics split out to importer_uma in browser.

Adds chrome/utility/importer/OWNERS identical to chrome/browser/importer.

Existing DEPS disallows both utility->browser and browser->utility.

TBR=gab@chromium.org, jam@chromium.org, joth@chromium.org, jschuh@chromium.org, thestig@chromium.org

BUG=254672, 237249
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211858

[scottmg, 2013-07-04 20:23:59]

(sorry it's so big, mostly renames except as noted in the description)

[gab, 2013-07-08 15:18:08]

1st pass, awesome work, can you ask someone else to look at tools/ (and does it
need to go in this CL or can this be done as a precursor CL?)?

Cheers!
Gab

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/DEPS:11:
"+chrome/utility/importer/bookmark_html_reader.h",
This feels like the reader should be in some common code then, no?!

This should at least be a TODO to make both the chrome and the utility DLL
depend on the bookmarks component (see comment at top of this file) once it's
here.

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/importer/f...
File chrome/browser/importer/firefox_importer_utils.h (left):

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/importer/f...
chrome/browser/importer/firefox_importer_utils.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Not sure why these chrome/browser/importer/firefox_importer_utils* don't show up
as deleted (D) in the diff...

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/importer/f...
File chrome/browser/importer/firefox_importer_utils_unittest.cc (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/importer/f...
chrome/browser/importer/firefox_importer_utils_unittest.cc:7: #include
"chrome/common/importer/firefox_importer_utils.h"
Shouldn't this entire test move with firefox_importer_utils.cc et al.?

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

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:206: ImporterVisitSource
visit_source) {
Can you introduce this new ImporterVisitSource in a precursor CLs, I would
prefer if this CL was a pure move-only (+includes rename) CL.

https://codereview.chromium.org/18501013/diff/34001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/chrome.gyp#newcode369
chrome/chrome.gyp:369: ['exclude', '^utility/importer/ie_importer.cc'],
Rename this file to ie_importer_win.cc instead (I've been wanting to do that for
some time, but this is the perfect opportunity).

https://codereview.chromium.org/18501013/diff/34001/chrome/chrome_tests_unit....
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/chrome_tests_unit....
chrome/chrome_tests_unit.gypi:2363:
'utility/importer/bookmark_html_reader_unittest.cc',
nit: alphabetical order

https://codereview.chromium.org/18501013/diff/34001/chrome/common/importer/sa...
File chrome/common/importer/safari_importer_utils.mm (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/common/importer/sa...
chrome/common/importer/safari_importer_utils.mm:11: bool
SafariImporterCanImport(const base::FilePath& library_dir,
I would also prefer if this was done in a separate follow-up CL, to keep this CL
to a pure move-only CL.

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/b...
File chrome/utility/importer/bookmark_html_reader.cc (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/b...
chrome/utility/importer/bookmark_html_reader.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
This should probably go in the "bookmarks" component once it is a component, I
would say add a TODO somewhere visible (maybe in the gyp file) to whoever the
owner is for moving bookmarks into a component to also move this to that
component.

imo it makes sense that both the utiltiy DLL and chrome would have to depend on
that component.

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/b...
File chrome/utility/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/b...
chrome/utility/importer/bookmarks_file_importer.cc:11: #include
"chrome/common/importer/importer_bridge.h"
nit: alphabetical order

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/f...
File chrome/utility/importer/favicon_reencode.h (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/f...
chrome/utility/importer/favicon_reencode.h:9: #include "base/basictypes.h"
nit: empty space between 8-9

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/i...
File chrome/utility/importer/importer_creator.cc (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/i...
chrome/utility/importer/importer_creator.cc:41: NOTREACHED();
I think these last 2 lines can be removed.

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/s...
File chrome/utility/importer/safari_importer.mm (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/s...
chrome/utility/importer/safari_importer.mm:20: #include
"chrome/common/importer/importer_bridge.h"
nit: alpha order

https://codereview.chromium.org/18501013/diff/34001/components/webdata/encryp...
File components/webdata/encryptor/ie_importer_unittest_win.cc (right):

https://codereview.chromium.org/18501013/diff/34001/components/webdata/encryp...
components/webdata/encryptor/ie_importer_unittest_win.cc:14:
TEST(IEImporterTest, IE7Importer) {
Rename this test and file to something like IE7Password and
ie7_password_unittest.cc

[gab, 2013-07-08 15:19:16]

Oh, it would be nice too if the CL description documented where each file moved,
e.g.:
c/b/i/file_1.cc => c/u/i/file_1.cc
...

Thanks!
Gab

[scottmg, 2013-07-09 16:20:48]

removed tools/ changes.

i tried a few git tweaks but couldn't get firefox_importer_utils* to be D's.

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/DEPS:11:
"+chrome/utility/importer/bookmark_html_reader.h",
On 2013/07/08 15:18:08, gab wrote:
> This feels like the reader should be in some common code then, no?!

The problem is that it depends on blink to reencode favicons, so can't go in
common or a component.

The reader isn't used outside of the importer other than
bookmarks_html_writer_unittest (where it's used to check that the bookmarks
roundtrip).
 
> This should at least be a TODO to make both the chrome and the utility DLL
> depend on the bookmarks component (see comment at top of this file) once it's
> here.

I think the "html reader for bookmarks" is reasonable to be distinct from the
bookmarks component. I'm not sure what the right resolution is, but perhaps
there should be a bookmarks_io component or something. The dependency on blink
is the difficult part to resolve though.

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/importer/f...
File chrome/browser/importer/firefox_importer_utils.h (left):

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/importer/f...
chrome/browser/importer/firefox_importer_utils.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/07/08 15:18:08, gab wrote:
> Not sure why these chrome/browser/importer/firefox_importer_utils* don't show
up
> as deleted (D) in the diff...

hmm, i'll try to tweak the --similarity maybe?

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/importer/f...
File chrome/browser/importer/firefox_importer_utils_unittest.cc (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/importer/f...
chrome/browser/importer/firefox_importer_utils_unittest.cc:7: #include
"chrome/common/importer/firefox_importer_utils.h"
On 2013/07/08 15:18:08, gab wrote:
> Shouldn't this entire test move with firefox_importer_utils.cc et al.?

Done.

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

https://codereview.chromium.org/18501013/diff/34001/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:206: ImporterVisitSource
visit_source) {
On 2013/07/08 15:18:08, gab wrote:
> Can you introduce this new ImporterVisitSource in a precursor CLs, I would
> prefer if this CL was a pure move-only (+includes rename) CL.

Done.

https://codereview.chromium.org/18501013/diff/34001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/chrome.gyp#newcode369
chrome/chrome.gyp:369: ['exclude', '^utility/importer/ie_importer.cc'],
On 2013/07/08 15:18:08, gab wrote:
> Rename this file to ie_importer_win.cc instead (I've been wanting to do that
for
> some time, but this is the perfect opportunity).

Done.

https://codereview.chromium.org/18501013/diff/34001/chrome/chrome_tests_unit....
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/chrome_tests_unit....
chrome/chrome_tests_unit.gypi:2363:
'utility/importer/bookmark_html_reader_unittest.cc',
On 2013/07/08 15:18:08, gab wrote:
> nit: alphabetical order

Done.

https://codereview.chromium.org/18501013/diff/34001/chrome/common/importer/sa...
File chrome/common/importer/safari_importer_utils.mm (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/common/importer/sa...
chrome/common/importer/safari_importer_utils.mm:11: bool
SafariImporterCanImport(const base::FilePath& library_dir,
On 2013/07/08 15:18:08, gab wrote:
> I would also prefer if this was done in a separate follow-up CL, to keep this
CL
> to a pure move-only CL.

hrm, ok. moved back into safari_importer.

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/b...
File chrome/utility/importer/bookmark_html_reader.cc (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/b...
chrome/utility/importer/bookmark_html_reader.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2013/07/08 15:18:08, gab wrote:
> This should probably go in the "bookmarks" component once it is a component, I
> would say add a TODO somewhere visible (maybe in the gyp file) to whoever the
> owner is for moving bookmarks into a component to also move this to that
> component.
> 
> imo it makes sense that both the utiltiy DLL and chrome would have to depend
on
> that component.

see comment in DEPS

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/b...
File chrome/utility/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/b...
chrome/utility/importer/bookmarks_file_importer.cc:11: #include
"chrome/common/importer/importer_bridge.h"
On 2013/07/08 15:18:08, gab wrote:
> nit: alphabetical order

? importeD vs importeR?

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/f...
File chrome/utility/importer/favicon_reencode.h (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/f...
chrome/utility/importer/favicon_reencode.h:9: #include "base/basictypes.h"
On 2013/07/08 15:18:08, gab wrote:
> nit: empty space between 8-9

Done.

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/i...
File chrome/utility/importer/importer_creator.cc (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/i...
chrome/utility/importer/importer_creator.cc:41: NOTREACHED();
On 2013/07/08 15:18:08, gab wrote:
> I think these last 2 lines can be removed.

Done.

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/s...
File chrome/utility/importer/safari_importer.mm (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/utility/importer/s...
chrome/utility/importer/safari_importer.mm:20: #include
"chrome/common/importer/importer_bridge.h"
On 2013/07/08 15:18:08, gab wrote:
> nit: alpha order

same

https://codereview.chromium.org/18501013/diff/34001/components/webdata/encryp...
File components/webdata/encryptor/ie_importer_unittest_win.cc (right):

https://codereview.chromium.org/18501013/diff/34001/components/webdata/encryp...
components/webdata/encryptor/ie_importer_unittest_win.cc:14:
TEST(IEImporterTest, IE7Importer) {
On 2013/07/08 15:18:08, gab wrote:
> Rename this test and file to something like IE7Password and
> ie7_password_unittest.cc

Done.

[scottmg, 2013-07-09 16:33:01]

(weird, in ps14 that failed upload with the "Base files missing" they're D, in
ps15, back to M. no idea.)

[gab, 2013-07-09 20:48:31]

Looks great, comments below, I'll do another full pass after that.

Cheers!
Gab

https://codereview.chromium.org/18501013/diff/34001/chrome/common/importer/sa...
File chrome/common/importer/safari_importer_utils.mm (right):

https://codereview.chromium.org/18501013/diff/34001/chrome/common/importer/sa...
chrome/common/importer/safari_importer_utils.mm:11: bool
SafariImporterCanImport(const base::FilePath& library_dir,
On 2013/07/09 16:20:49, scottmg wrote:
> On 2013/07/08 15:18:08, gab wrote:
> > I would also prefer if this was done in a separate follow-up CL, to keep
this
> CL
> > to a pure move-only CL.
> 
> hrm, ok. moved back into safari_importer.

err, my bad, I meant as a precursor CL rather than follow-up CL, either works I
guess.

https://codereview.chromium.org/18501013/diff/53244/chrome/browser/importer/f...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/browser/importer/f...
chrome/browser/importer/firefox_importer_browsertest.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
And this file shows up as 'D'... weird...

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

https://codereview.chromium.org/18501013/diff/53244/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:58: NOTREACHED();
I think these last 2 lines can be removed.

https://codereview.chromium.org/18501013/diff/53244/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/chrome.gyp#newcode291
chrome/chrome.gyp:291: '../crypto/crypto.gyp:crypto',  # nss on Linux.
So shouldn't this instead go in an 'OS=="linux" and use_openssl!=1' section or
something?

https://codereview.chromium.org/18501013/diff/53244/chrome/chrome.gyp#newcode310
chrome/chrome.gyp:310:
'utility/importer/firefox_importer_unittest_messages_internal.h',
nit: duplicate entry (there should probably be a presubmit check for this)

https://codereview.chromium.org/18501013/diff/53244/chrome/common/importer/fi...
File chrome/common/importer/firefox_importer_utils.h (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/common/importer/fi...
chrome/common/importer/firefox_importer_utils.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Do all of these method need to be common?

I feel most of them are probably only used by one side or the other, no?

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/DEPS#newco...
chrome/utility/DEPS:11: "+skia",
It seems like chrome/browser/DEPS only includes skia/include and skia/ext, is
either (or both) of those sufficient here?

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/O...
File chrome/utility/importer/OWNERS (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/O...
chrome/utility/importer/OWNERS:5: # For security review of IPC message files.
Does this section apply to c/u/i?

The only messages file seems to be for a unit test.

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/i...
File chrome/utility/importer/importer_creator.cc (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/i...
chrome/utility/importer/importer_creator.cc:38: NOTREACHED();
I don't think this needs a default: entry if all of the cases are handled above.

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/s...
File chrome/utility/importer/safari_importer_unittest.mm (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/s...
chrome/utility/importer/safari_importer_unittest.mm:20: #include
"chrome/common/importer/safari_importer_utils.h"
It seems you reverted some, but not all changes involving your new
chrome/common/importer/safari_importer_utils.h

Does this actually need to go in chrome/common? If so you'll need to do this as
a precursor CL, not a follow-up CL, right?

[scottmg, 2013-07-09 21:16:45]

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

https://codereview.chromium.org/18501013/diff/53244/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:58: NOTREACHED();
On 2013/07/09 20:48:31, gab wrote:
> I think these last 2 lines can be removed.

I think they should be kept in case new enums are added. I can put them in a
default: if you prefer.

The ; can definitely be nuked!

https://codereview.chromium.org/18501013/diff/53244/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/chrome.gyp#newcode291
chrome/chrome.gyp:291: '../crypto/crypto.gyp:crypto',  # nss on Linux.
On 2013/07/09 20:48:31, gab wrote:
> So shouldn't this instead go in an 'OS=="linux" and use_openssl!=1' section or
> something?

Actually, I think the comment is wrong, it's just a dependency of nss_*.
Removed.

https://codereview.chromium.org/18501013/diff/53244/chrome/chrome.gyp#newcode310
chrome/chrome.gyp:310:
'utility/importer/firefox_importer_unittest_messages_internal.h',
On 2013/07/09 20:48:31, gab wrote:
> nit: duplicate entry (there should probably be a presubmit check for this)

Done.

https://codereview.chromium.org/18501013/diff/53244/chrome/common/importer/fi...
File chrome/common/importer/firefox_importer_utils.h (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/common/importer/fi...
chrome/common/importer/firefox_importer_utils.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/07/09 20:48:31, gab wrote:
> Do all of these method need to be common?
> 
> I feel most of them are probably only used by one side or the other, no?

Possibly. The implementation of the functions was using the generic ones
(GetPrefsJsValue) so some has to be in common.

How about I try to tease them apart while doing the same thing for the
safari_importer_utils?

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/DEPS#newco...
chrome/utility/DEPS:11: "+skia",
On 2013/07/09 20:48:31, gab wrote:
> It seems like chrome/browser/DEPS only includes skia/include and skia/ext, is
> either (or both) of those sufficient here?

Done.

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/O...
File chrome/utility/importer/OWNERS (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/O...
chrome/utility/importer/OWNERS:5: # For security review of IPC message files.
On 2013/07/09 20:48:31, gab wrote:
> Does this section apply to c/u/i?
> 
> The only messages file seems to be for a unit test.

Yeah, it's just for the FF unittests. Seems less likely to incur the wrath of
the security team in case there's more messages added in the future.

(It'd be nice to have that globally instead of in all the folders)

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/s...
File chrome/utility/importer/safari_importer_unittest.mm (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/s...
chrome/utility/importer/safari_importer_unittest.mm:20: #include
"chrome/common/importer/safari_importer_utils.h"
On 2013/07/09 20:48:31, gab wrote:
> It seems you reverted some, but not all changes involving your new
> chrome/common/importer/safari_importer_utils.h

Done.

> 
> Does this actually need to go in chrome/common? If so you'll need to do this
as
> a precursor CL, not a follow-up CL, right?

Which "this"?

safari_importer_unittest.mm seems to belong with safari_importer.mm. 

safari_importer_utils.h has to go in common because it's used from
importer_list.

I made the DEPS temporarily ! instead of - for browser so that I could split it
up per previous request.

[gab, 2013-07-09 21:29:43]

Quick pass over comments, going home, will take another look tomorrow.

Thanks!
Gab

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

https://codereview.chromium.org/18501013/diff/53244/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:58: NOTREACHED();
On 2013/07/09 21:16:45, scottmg wrote:
> On 2013/07/09 20:48:31, gab wrote:
> > I think these last 2 lines can be removed.
> 
> I think they should be kept in case new enums are added. I can put them in a
> default: if you prefer.
> 
> The ; can definitely be nuked!

Well if they are removed and new enums are added, this won't compile, if we
leave them there, then it will compile, but not behave properly...

I prefer the former :).

https://codereview.chromium.org/18501013/diff/53244/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/chrome.gyp#newcode291
chrome/chrome.gyp:291: '../crypto/crypto.gyp:crypto',  # nss on Linux.
On 2013/07/09 21:16:45, scottmg wrote:
> On 2013/07/09 20:48:31, gab wrote:
> > So shouldn't this instead go in an 'OS=="linux" and use_openssl!=1' section
or
> > something?
> 
> Actually, I think the comment is wrong, it's just a dependency of nss_*.
> Removed.

Right, so the dependency should only exist if including the nss_* files, no?

https://codereview.chromium.org/18501013/diff/53244/chrome/common/importer/fi...
File chrome/common/importer/firefox_importer_utils.h (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/common/importer/fi...
chrome/common/importer/firefox_importer_utils.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/07/09 21:16:45, scottmg wrote:
> On 2013/07/09 20:48:31, gab wrote:
> > Do all of these method need to be common?
> > 
> > I feel most of them are probably only used by one side or the other, no?
> 
> Possibly. The implementation of the functions was using the generic ones
> (GetPrefsJsValue) so some has to be in common.
> 
> How about I try to tease them apart while doing the same thing for the
> safari_importer_utils?

sgtm.

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/s...
File chrome/utility/importer/safari_importer_unittest.mm (right):

https://codereview.chromium.org/18501013/diff/53244/chrome/utility/importer/s...
chrome/utility/importer/safari_importer_unittest.mm:20: #include
"chrome/common/importer/safari_importer_utils.h"
On 2013/07/09 21:16:45, scottmg wrote:
> On 2013/07/09 20:48:31, gab wrote:
> > It seems you reverted some, but not all changes involving your new
> > chrome/common/importer/safari_importer_utils.h
> 
> Done.
> 
> > 
> > Does this actually need to go in chrome/common? If so you'll need to do this
> as
> > a precursor CL, not a follow-up CL, right?
> 
> Which "this"?
> 
> safari_importer_unittest.mm seems to belong with safari_importer.mm. 
> 
> safari_importer_utils.h has to go in common because it's used from
> importer_list.
> 
> I made the DEPS temporarily ! instead of - for browser so that I could split
it
> up per previous request.

Ok, my question was, are you planning to introduce
"chrome/common/importer/safari_importer_utils.h" in a CL before or after this
one?

https://codereview.chromium.org/18501013/diff/76001/chrome/utility/importer/s...
File chrome/utility/importer/safari_importer_unittest.mm (right):

https://codereview.chromium.org/18501013/diff/76001/chrome/utility/importer/s...
chrome/utility/importer/safari_importer_unittest.mm:200:
EXPECT_FALSE(SafariImporterCanImport(fake_library_dir.path(), &items));
nit: This won't compile either now, right?

[scottmg, 2013-07-09 23:03:08]

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

https://codereview.chromium.org/18501013/diff/53244/chrome/browser/importer/i...
chrome/browser/importer/in_process_importer_bridge.cc:58: NOTREACHED();
On 2013/07/09 21:29:43, gab wrote:
> On 2013/07/09 21:16:45, scottmg wrote:
> > On 2013/07/09 20:48:31, gab wrote:
> > > I think these last 2 lines can be removed.
> > 
> > I think they should be kept in case new enums are added. I can put them in a
> > default: if you prefer.
> > 
> > The ; can definitely be nuked!
> 
> Well if they are removed and new enums are added, this won't compile, if we
> leave them there, then it will compile, but not behave properly...
> 
> I prefer the former :).

I don't believe that's true. e.g. win vs2010 release:

d:\src\cr3\src\chrome\browser\importer\in_process_importer_bridge.cc(58) :
warning C4715: '`anonymous
namespace'::ConvertImporterVisitSourceToHistoryVisitSource' : not all control
paths return a value

I can see a reasonable argument either way for having it inside or outside of
the switch though.

https://codereview.chromium.org/18501013/diff/76001/chrome/utility/importer/s...
File chrome/utility/importer/safari_importer_unittest.mm (right):

https://codereview.chromium.org/18501013/diff/76001/chrome/utility/importer/s...
chrome/utility/importer/safari_importer_unittest.mm:200:
EXPECT_FALSE(SafariImporterCanImport(fake_library_dir.path(), &items));
On 2013/07/09 21:29:44, gab wrote:
> nit: This won't compile either now, right?

Gah, thanks. Done.

I'll do the common/ as a subsequent CL.

[gab, 2013-07-10 13:40:44]

Looks great, seems like a lot of new DEPS are still missing from
chrome/utility/DEPS (I'm assuming some DEPS like base/ are implicit?), perhaps I
don't understand all the DEPS magic, but I thought it was simply listing allowed
includes for files in the current directory.

Cheers!
Gab

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/DEPS#newco...
chrome/browser/DEPS:75: "!chrome/utility",
Just checking, I've never seen '!' DEPS rules, I assume those result in warning
rather than errors?

Perhaps add a TODO here stating what remains to be done before this is turned
into a '-'? I don't feel strongly about this since I know you'll clean it up
soon after, up to you.

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/bookmarks/...
chrome/browser/bookmarks/DEPS:11:
"+chrome/utility/importer/bookmark_html_reader.h",
I still don't like this as it allows anyone in bookmarks/ to include the reader
which is wrong.

Perhaps make this a '!' rule (this is a warning if my previous assumptions are
correct?)?

Otherwise we could add a c/b/bookmarks/test directory, but that might be
overkill for a single test..?

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/importer/f...
File chrome/browser/importer/firefox_proxy_settings.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/importer/f...
chrome/browser/importer/firefox_proxy_settings.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
It seems firefox_proxy_settings.h is only used by
chrome/browser/net/connection_tester.cc and should probably move to c/b/net/.

This is beyond the scope of this CL, but something I'm noting for myself
(http://crbug.com/258876)

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/importer/i...
File chrome/browser/importer/importer_uma.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/importer/i...
chrome/browser/importer/importer_uma.cc:70: 
nit: Looks like you have more than a single empty line at the end of this file.

https://codereview.chromium.org/18501013/diff/80002/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/chrome.gyp#newcode365
chrome/chrome.gyp:365: ['OS!="win" and OS!="mac" and use_openssl==0', {
I think it would be cleaner to also only include the sources here, to keep it
together (instead of always including the sources except if !(X || Y) and adding
the dependency if (X || Y)...).

https://codereview.chromium.org/18501013/diff/80002/chrome/common/importer/im...
File chrome/common/importer/importer_bridge.h (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/common/importer/im...
chrome/common/importer/importer_bridge.h:17: #include
"chrome/common/importer/importer_visit_source.h"
nit: Remove this include along with the file.

https://codereview.chromium.org/18501013/diff/80002/chrome/common/importer/im...
chrome/common/importer/importer_bridge.h:35: class ImporterBridge : public
base::RefCountedThreadSafe<ImporterBridge> {
I think this should be a pure-virtual interface instead of having it's own .cc
file for no real purpose.

i.e., Remove constructor and make protected destructor pure-virtual?

I don't think this needs to go in this CL, but I think it would be nice to
minimize .cc files in chrome/browser/common

https://codereview.chromium.org/18501013/diff/80002/chrome/common/importer/im...
File chrome/common/importer/importer_visit_source.h (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/common/importer/im...
chrome/common/importer/importer_visit_source.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
We put this in c/b/c/importer_data_types.h in the previous CL, no?

Remove this file and its entry in the gyp files.

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/DEPS#newcode2
chrome/utility/DEPS:2: # For out of process profile import. This code needs to
be factored out to
This comment can be removed as it was directed at the remove rule for
"+chrome/browser/importer"

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/DEPS#newcode4
chrome/utility/DEPS:4: "+chrome/common/local_discovery",
Add "+chrome/common/importer" too?

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/b...
File chrome/utility/importer/bookmark_html_reader.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/b...
chrome/utility/importer/bookmark_html_reader.cc:17: #include
"content/public/common/url_constants.h"
need DEPS on content/public/common

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/b...
chrome/utility/importer/bookmark_html_reader.cc:18: #include
"net/base/data_url.h"
need DEPS on net/base

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/e...
File chrome/utility/importer/external_process_importer_bridge.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/e...
chrome/utility/importer/external_process_importer_bridge.cc:18: #include
"ipc/ipc_sender.h"
+ipc/ deps

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/e...
chrome/utility/importer/external_process_importer_bridge.cc:18: #include
"ipc/ipc_sender.h"
need DEPS on ipc/

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/e...
File chrome/utility/importer/external_process_importer_bridge.h (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/e...
chrome/utility/importer/external_process_importer_bridge.h:15: #include
"chrome/common/importer/importer_visit_source.h"
nit: Remove this include with the file.

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/f...
File chrome/utility/importer/favicon_reencode.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/f...
chrome/utility/importer/favicon_reencode.cc:11: #include "ui/gfx/favicon_size.h"
Need to add DEPS on ui/gfx and third_party/skia

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/f...
File chrome/utility/importer/firefox3_importer.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/f...
chrome/utility/importer/firefox3_importer.cc:28: #include "sql/connection.h"
need DEPS on sql/

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/f...
chrome/utility/importer/firefox3_importer.cc:30: #include "url/gurl.h"
need DEPS on url/

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/i...
File chrome/utility/importer/importer_creator.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/i...
chrome/utility/importer/importer_creator.cc:17: #include
"base/mac/foundation_util.h"
I think we usually put an empty line between system headers and chrome headers,
even in ifdefs, e.g.:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/imp...

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/n...
File chrome/utility/importer/nss_decryptor_system_nss.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/n...
chrome/utility/importer/nss_decryptor_system_nss.cc:14: #include
"crypto/nss_util.h"
+DEPS on crypto/

[tfarina, 2013-07-10 13:44:36]

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/DEPS#newco...
chrome/browser/DEPS:75: "!chrome/utility",
On 2013/07/10 13:40:44, gab wrote:
> Just checking, I've never seen '!' DEPS rules, I assume those result in
warning
> rather than errors?
> 
it means this rule is temporary and will generate a presubmit warning when
someone tries to add chrome/utility in a file under chrome/browser.

> Perhaps add a TODO here stating what remains to be done before this is turned
> into a '-'? I don't feel strongly about this since I know you'll clean it up
> soon after, up to you.

[tfarina, 2013-07-10 13:46:54]

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/bookmarks/...
chrome/browser/bookmarks/DEPS:11:
"+chrome/utility/importer/bookmark_html_reader.h",
On 2013/07/10 13:40:44, gab wrote:
> I still don't like this as it allows anyone in bookmarks/ to include the
reader
> which is wrong.
> 
> Perhaps make this a '!' rule (this is a warning if my previous assumptions are
> correct?)?
> 
> Otherwise we could add a c/b/bookmarks/test directory, but that might be
> overkill for a single test..?
you can use specific_include_rules for this.
see a good example of how to do this for a single unittest that needs this
include like here: components/autofill/content/browser/DEPS

[tfarina, 2013-07-10 13:48:26]

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/f...
File chrome/utility/importer/firefox3_importer.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/f...
chrome/utility/importer/firefox3_importer.cc:30: #include "url/gurl.h"
On 2013/07/10 13:40:44, gab wrote:
> need DEPS on url/
no, everybody is allowed to include from url/, see the master DEPS file in
src/DEPS.

[gab, 2013-07-10 13:53:21]

I see, thanks for the clarifications Thiago.

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/DEPS#newcode4
chrome/utility/DEPS:4: "+chrome/common/local_discovery",
On 2013/07/10 13:40:44, gab wrote:
> Add "+chrome/common/importer" too?

Actually chrome/DEPS seems to always allow anything in chrome/ to include from
chrome/common so neither "+chrome/common/importer", nor the current
"+chrome/common/local_discovery" are needed here I'd say.

[scottmg, 2013-07-15 16:06:01]

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/bookmarks/...
chrome/browser/bookmarks/DEPS:11:
"+chrome/utility/importer/bookmark_html_reader.h",
On 2013/07/10 13:46:55, tfarina wrote:
> On 2013/07/10 13:40:44, gab wrote:
> > I still don't like this as it allows anyone in bookmarks/ to include the
> reader
> > which is wrong.
> > 
> > Perhaps make this a '!' rule (this is a warning if my previous assumptions
are
> > correct?)?
> > 
> > Otherwise we could add a c/b/bookmarks/test directory, but that might be
> > overkill for a single test..?
> you can use specific_include_rules for this.
> see a good example of how to do this for a single unittest that needs this
> include like here: components/autofill/content/browser/DEPS

Thanks Thiago, changed to a specific_include_rules.

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/importer/i...
File chrome/browser/importer/importer_uma.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/browser/importer/i...
chrome/browser/importer/importer_uma.cc:70: 
On 2013/07/10 13:40:44, gab wrote:
> nit: Looks like you have more than a single empty line at the end of this
file.

Done.

https://codereview.chromium.org/18501013/diff/80002/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/chrome.gyp#newcode365
chrome/chrome.gyp:365: ['OS!="win" and OS!="mac" and use_openssl==0', {
On 2013/07/10 13:40:44, gab wrote:
> I think it would be cleaner to also only include the sources here, to keep it
> together (instead of always including the sources except if !(X || Y) and
adding
> the dependency if (X || Y)...).

Done.

https://codereview.chromium.org/18501013/diff/80002/chrome/common/importer/im...
File chrome/common/importer/importer_bridge.h (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/common/importer/im...
chrome/common/importer/importer_bridge.h:17: #include
"chrome/common/importer/importer_visit_source.h"
On 2013/07/10 13:40:44, gab wrote:
> nit: Remove this include along with the file.

Done.

https://codereview.chromium.org/18501013/diff/80002/chrome/common/importer/im...
File chrome/common/importer/importer_visit_source.h (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/common/importer/im...
chrome/common/importer/importer_visit_source.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/07/10 13:40:44, gab wrote:
> We put this in c/b/c/importer_data_types.h in the previous CL, no?
> 
> Remove this file and its entry in the gyp files.

Done.

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/DEPS#newcode2
chrome/utility/DEPS:2: # For out of process profile import. This code needs to
be factored out to
On 2013/07/10 13:40:44, gab wrote:
> This comment can be removed as it was directed at the remove rule for
> "+chrome/browser/importer"

Done.

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/DEPS#newcode4
chrome/utility/DEPS:4: "+chrome/common/local_discovery",
On 2013/07/10 13:53:21, gab wrote:
> On 2013/07/10 13:40:44, gab wrote:
> > Add "+chrome/common/importer" too?
> 
> Actually chrome/DEPS seems to always allow anything in chrome/ to include from
> chrome/common so neither "+chrome/common/importer", nor the current
> "+chrome/common/local_discovery" are needed here I'd say.

Done.

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/e...
File chrome/utility/importer/external_process_importer_bridge.h (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/e...
chrome/utility/importer/external_process_importer_bridge.h:15: #include
"chrome/common/importer/importer_visit_source.h"
On 2013/07/10 13:40:44, gab wrote:
> nit: Remove this include with the file.

Done.

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/i...
File chrome/utility/importer/importer_creator.cc (right):

https://codereview.chromium.org/18501013/diff/80002/chrome/utility/importer/i...
chrome/utility/importer/importer_creator.cc:17: #include
"base/mac/foundation_util.h"
On 2013/07/10 13:40:44, gab wrote:
> I think we usually put an empty line between system headers and chrome
headers,
> even in ifdefs, e.g.:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/imp...

Done.

[scottmg, 2013-07-15 19:05:41]

+jam. Per CL description, this is mostly just renames except for a couple files
that had to be split.

(looks like gab's on a plane, but also need a higher up OWNERS for some files)

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/DEPS#newc...
chrome/browser/DEPS:77: "!chrome/utility",
Follow up CL here to switch to -: https://codereview.chromium.org/19251002/

[jam, 2013-07-15 19:54:13]

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/DEPS#newc...
chrome/browser/DEPS:77: "!chrome/utility",
On 2013/07/15 19:05:41, scottmg wrote:
> Follow up CL here to switch to -: https://codereview.chromium.org/19251002/

why not just do both together in one cl so you don't need this?

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/bookmarks...
chrome/browser/bookmarks/DEPS:29:
"+chrome/utility/importer/bookmark_html_reader.h",
we shouldn't have this, even for test. unit tests which use code that runs the
utility process now can run that in-process, i.e.
https://codereview.chromium.org/19085002/diff/1/chrome/browser/extensions/upd...

https://codereview.chromium.org/18501013/diff/129001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/18501013/diff/129001/chrome/utility/DEPS#newc...
chrome/utility/DEPS:12: "-chrome/browser",
also don't need this, chrome/ subdirs dont have access to other subdirs by
default, see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/DEPS&q=file...

[scottmg, 2013-07-15 20:20:43]

thanks

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/DEPS#newc...
chrome/browser/DEPS:77: "!chrome/utility",
On 2013/07/15 19:54:14, jam wrote:
> On 2013/07/15 19:05:41, scottmg wrote:
> > Follow up CL here to switch to -: https://codereview.chromium.org/19251002/
> 
> why not just do both together in one cl so you don't need this?

it was in this CL in patchset 11
https://codereview.chromium.org/18501013/diff/34001/chrome/common/importer/sa...
but was moved out to a separate CL by request.

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/bookmarks...
chrome/browser/bookmarks/DEPS:29:
"+chrome/utility/importer/bookmark_html_reader.h",
On 2013/07/15 19:54:14, jam wrote:
> we shouldn't have this, even for test. unit tests which use code that runs the
> utility process now can run that in-process, i.e.
>
https://codereview.chromium.org/19085002/diff/1/chrome/browser/extensions/upd...

Sorry, I don't think I understand.

bookmark_html_writer_unittest (i.e. only the test code) uses
bookmark_html_reader to verify that the bookmarks.html file that it writes is
valid.

bookmark_html_reader uses WebKit to decode favicons, so we don't want a
dependency on that from the browser.

Can you suggest a better approach?

https://codereview.chromium.org/18501013/diff/129001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/18501013/diff/129001/chrome/utility/DEPS#newc...
chrome/utility/DEPS:12: "-chrome/browser",
On 2013/07/15 19:54:14, jam wrote:
> also don't need this, chrome/ subdirs dont have access to other subdirs by
> default, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/DEPS&q=file...

Ah, thanks. Done.

[jam, 2013-07-15 20:25:54]

lgtm

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/DEPS#newc...
chrome/browser/DEPS:77: "!chrome/utility",
On 2013/07/15 20:20:43, scottmg wrote:
> On 2013/07/15 19:54:14, jam wrote:
> > On 2013/07/15 19:05:41, scottmg wrote:
> > > Follow up CL here to switch to -:
https://codereview.chromium.org/19251002/
> > 
> > why not just do both together in one cl so you don't need this?
> 
> it was in this CL in patchset 11
>
https://codereview.chromium.org/18501013/diff/34001/chrome/common/importer/sa...
> but was moved out to a separate CL by request.

i see, ok..

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/18501013/diff/129001/chrome/browser/bookmarks...
chrome/browser/bookmarks/DEPS:29:
"+chrome/utility/importer/bookmark_html_reader.h",
On 2013/07/15 20:20:43, scottmg wrote:
> On 2013/07/15 19:54:14, jam wrote:
> > we shouldn't have this, even for test. unit tests which use code that runs
the
> > utility process now can run that in-process, i.e.
> >
>
https://codereview.chromium.org/19085002/diff/1/chrome/browser/extensions/upd...
> 
> Sorry, I don't think I understand.
> 
> bookmark_html_writer_unittest (i.e. only the test code) uses
> bookmark_html_reader to verify that the bookmarks.html file that it writes is
> valid.
> 
> bookmark_html_reader uses WebKit to decode favicons, so we don't want a
> dependency on that from the browser.
> 
> Can you suggest a better approach?

hmm, i can't, so ok. perhaps we should have a subdir under chrome/test for these
kind of tests, but that's outside the scope of this change

[scottmg, 2013-07-15 20:31:35]

tbr for some renames:

joth@ for android_webview\tools\third_party_files_whitelist.txt
(referenced files renamed)

jschuh@ for
chrome\utility\importer\firefox_importer_unittest_messages_internal.h renamed

thestig@ for test renamed in
components\webdata\encryptor\ie7_password_unittest_win.cc

[joth, 2013-07-15 20:32:38]

lgtm rubberstamp

[commit-bot: I haz the power, 2013-07-15 20:54:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/18501013/144001

[jschuh, 2013-07-15 21:15:32]

ipc security rubberstamp lgtm. unsandboxed <-> unsandboxed.

[Lei Zhang, 2013-07-15 22:41:20]

components/webdata/encryptor/ie7_password_unittest_win.cc lgtm

https://codereview.chromium.org/18501013/diff/144001/chrome/utility/importer/...
File chrome/utility/importer/OWNERS (right):

https://codereview.chromium.org/18501013/diff/144001/chrome/utility/importer/...
chrome/utility/importer/OWNERS:5: # For security review of IPC message files.
There's no files that match *_messages*.h in c/u/importer.

[gab, 2013-07-15 22:41:23]

lgtm, thanks!

https://codereview.chromium.org/18501013/diff/144001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/18501013/diff/144001/chrome/chrome.gyp#newcod...
chrome/chrome.gyp:359: 'utility/importer/nss_decryptor.cc',
It seems weird to me that this is not with the rest of the NSS file, and even
more weird that the nss_decryptor_*.(cc|mm) files are not here?!

(I know the current state mimics the old behavior, but this new way of including
the files makes it clearer something is wrong here... This is thus not a blocker
for this CL... but something to keep in mind... is use_openssl==1 even
supported?!)

[scottmg, 2013-07-15 22:46:37]

https://codereview.chromium.org/18501013/diff/144001/chrome/utility/importer/...
File chrome/utility/importer/OWNERS (right):

https://codereview.chromium.org/18501013/diff/144001/chrome/utility/importer/...
chrome/utility/importer/OWNERS:5: # For security review of IPC message files.
On 2013/07/15 22:41:21, Lei Zhang wrote:
> There's no files that match *_messages*.h in c/u/importer.

they're globs, not regex?

so c/u/importer/firefox_importer_unittest_messages_internal.h, matches i think.
it's debatable if we really need the security team to review those (test-only)
messages but i didn't want to raise anyone's hackles.

[Lei Zhang, 2013-07-15 22:49:36]

On 2013/07/15 22:46:37, scottmg wrote:
>
https://codereview.chromium.org/18501013/diff/144001/chrome/utility/importer/...
> File chrome/utility/importer/OWNERS (right):
> 
>
https://codereview.chromium.org/18501013/diff/144001/chrome/utility/importer/...
> chrome/utility/importer/OWNERS:5: # For security review of IPC message files.
> On 2013/07/15 22:41:21, Lei Zhang wrote:
> > There's no files that match *_messages*.h in c/u/importer.
> 
> they're globs, not regex?
> 
> so c/u/importer/firefox_importer_unittest_messages_internal.h, matches i
think.
> it's debatable if we really need the security team to review those (test-only)
> messages but i didn't want to raise anyone's hackles.

Whoops, I missed that. Nevermind.

[commit-bot: I haz the power, 2013-07-15 23:44:26]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-15 23:46:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/18501013/144001

[commit-bot: I haz the power, 2013-07-16 00:36:17]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 00:40:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/18501013/144001

[commit-bot: I haz the power, 2013-07-16 01:04:59]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 01:28:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/18501013/158004

[commit-bot: I haz the power, 2013-07-16 06:34:07]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[gab, 2013-07-16 14:13:43]

https://chromiumcodereview.appspot.com/18501013/diff/158004/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://chromiumcodereview.appspot.com/18501013/diff/158004/chrome/chrome.gyp...
chrome/chrome.gyp:375: ['exclude', '^utility/importer/'],
Doesn't this include '^utility/importer/bookmark_html_reader\.cc'
?

(either way, fix alphabetical order)

[commit-bot: I haz the power, 2013-07-16 16:10:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/18501013/159003

[commit-bot: I haz the power, 2013-07-16 18:26:38]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 19:13:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/18501013/159003

[scottmg, 2013-07-16 21:58:10]

Committed patchset #26 manually as r211858 (presubmit successful).