Many fixes to bookmark importing.

Many fixes to bookmark importing.

In particular:
  * All bookmarks are imported to the toolbar -- nothing goes to Other Bookmarks.  If there are initially no bookmarks on the toolbar, we try to reduce nesting of the imported bookmarks as much as possible.  If there are already bookmarks on the toolbar, all the imported bookmarks end up in a new folder -- e.g. "Imported from Safari" -- on the toolbar.

  * All importers explicitly include a containing folder for bookmarks in the toolbar.
    o The ProfileWriter is responsible for stripping this folder off when the bookmarks should be imported directly to the toolbar.

  * All importers do *not* include a containing folder for the remaining bookmarks.
    o The ProfileWriter is responsible for creating this folder as appropriate.  In fact, this is how things used to work previously, too, since the folder name needed to be uniquified.  This CL makes the logic much clearer though (I hope).

  * All importers should now be able to handle importing empty folders.

  * The ProfileWriter no longer takes in a bitset of options for importing bookmarks.  These options were all either set identically by all clients, or could be more accurately computed locally.

  * Some implementation details for ProfileWriter have been removed from the header file.  Others have just been completely nuked from orbit, and replaced by simpler code (again, I hope).

BUG=79427, 79433, 71351
TEST=unit_tests --gtest_filter=*Import*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86861

[Ilya Sherman, 2011-05-25 07:24:43]

Note: I have not yet tested this on Windows, so I'm not sure whether the IE
importer code is exactly right.  I'll update the CL once I get my Windows box to
cooperate -- but in the scheme of this CL, the IE importer code is just a small
corner.  (I did try -- several times -- to split this CL up into multiple
smaller ones, but there were too many dependencies and it got too yucky. 
Sorry.)

Also note: I *have* tested this on Mac, and the
SafariImporterTest.BookmarkImport test passes.  It will probably fail on the
trybots, though, as it depends on a binary resource that I am changing in this
CL.  So, ignore that particular trybot failure :)

[Takashi Toyoshima, 2011-05-25 08:58:11]

This change seems to fix Issue 71351, too.
Coud you add it into BUG= line and take over its owner from me?

[Miranda Callahan, 2011-05-25 13:20:13]

I love the simplification -- but it looks like firefox importer tests are
failing on Windows...?

http://codereview.chromium.org/6979007/diff/1/chrome/browser/importer/firefox...
File chrome/browser/importer/firefox3_importer.cc (right):

http://codereview.chromium.org/6979007/diff/1/chrome/browser/importer/firefox...
chrome/browser/importer/firefox3_importer.cc:252: // We've reached a root node,
hooray!
+1 for good cheer in the comments!

[tfarina, 2011-05-25 17:01:27]

http://codereview.chromium.org/6979007/diff/1/chrome/browser/importer/externa...
File chrome/browser/importer/external_process_importer_host.cc (right):

http://codereview.chromium.org/6979007/diff/1/chrome/browser/importer/externa...
chrome/browser/importer/external_process_importer_host.cc:56: client_ = new
ExternalProcessImporterClient(this, *source_profile_, items_,
When the args doesn't fit on the same line, write one arg per line. I'd prefer
you don't change it all if possible. This just increase the size of the diff and
makes it less readable (with no real benefit). Here and elsewhere you changed to
that style.

[Ilya Sherman, 2011-05-26 00:13:20]

On 2011/05/25 08:58:11, toyoshim wrote:
> This change seems to fix Issue 71351, too.
> Coud you add it into BUG= line and take over its owner from me?

Oh, that's a nice bonus.  Done =)

On 2011/05/25 13:20:13, Miranda Callahan wrote:
> I love the simplification -- but it looks like firefox importer tests are
> failing on Windows...?

Ah, these are disabled on Mac, so I didn't see the failures locally.  Fixed!

I've also verified that IE importing is working correctly, modulo one detail: 
"Favorites Bar" bookmarks imported to a subfolder called "Links" when there are
already bookmarks in the Chrome toolbar.  That folder should instead be called
"Favorites Bar".  But I don't know offhand how to fix that, and it seems ok to
call the folder "Links" for now.

http://codereview.chromium.org/6979007/diff/1/chrome/browser/importer/externa...
File chrome/browser/importer/external_process_importer_host.cc (right):

http://codereview.chromium.org/6979007/diff/1/chrome/browser/importer/externa...
chrome/browser/importer/external_process_importer_host.cc:56: client_ = new
ExternalProcessImporterClient(this, *source_profile_, items_,
On 2011/05/25 17:01:27, tfarina wrote:
> When the args doesn't fit on the same line, write one arg per line.

This is not part of the style guide -- see [1].
[1] http://www.chromium.org/developers/coding-style#TOC-Code-formatting

> I'd prefer you don't change it all if possible. This just increase the size of
the diff and
> makes it less readable (with no real benefit). Here and elsewhere you changed
to
> that style.

I am not just re-wrapping the arguments here, I am also removing one.  So, the
diff would not be much smaller were the arguments not re-wrapped.

[tfarina, 2011-05-26 00:23:39]

On 2011/05/26 00:13:20, Ilya Sherman wrote:
>
http://codereview.chromium.org/6979007/diff/1/chrome/browser/importer/externa...
> File chrome/browser/importer/external_process_importer_host.cc (right):
> 
>
http://codereview.chromium.org/6979007/diff/1/chrome/browser/importer/externa...
> chrome/browser/importer/external_process_importer_host.cc:56: client_ = new
> ExternalProcessImporterClient(this, *source_profile_, items_,
> On 2011/05/25 17:01:27, tfarina wrote:
> > When the args doesn't fit on the same line, write one arg per line.
> 
> This is not part of the style guide -- see [1].
> [1] http://www.chromium.org/developers/coding-style#TOC-Code-formatting
> 
> > I'd prefer you don't change it all if possible. This just increase the size
of
> the diff and
> > makes it less readable (with no real benefit). Here and elsewhere you
changed
> to
> > that style.
> 
> I am not just re-wrapping the arguments here, I am also removing one.  So, the
> diff would not be much smaller were the arguments not re-wrapped.

OK, but please could you try avoid imposing your personal style when doing these
changes? As someone that has touched this code before, I prefer to stick with
what we have know. It doesn't hurt the readability and it also saves vertical
spaces. If you could avoid doing this kind of change in future that would be
really appreciate.

[Ilya Sherman, 2011-05-26 00:35:47]

On 2011/05/26 00:23:39, tfarina wrote:
> On 2011/05/26 00:13:20, Ilya Sherman wrote:
> >
>
http://codereview.chromium.org/6979007/diff/1/chrome/browser/importer/externa...
> > File chrome/browser/importer/external_process_importer_host.cc (right):
> > 
> >
>
http://codereview.chromium.org/6979007/diff/1/chrome/browser/importer/externa...
> > chrome/browser/importer/external_process_importer_host.cc:56: client_ = new
> > ExternalProcessImporterClient(this, *source_profile_, items_,
> > On 2011/05/25 17:01:27, tfarina wrote:
> > > When the args doesn't fit on the same line, write one arg per line.
> > 
> > This is not part of the style guide -- see [1].
> > [1] http://www.chromium.org/developers/coding-style#TOC-Code-formatting
> > 
> > > I'd prefer you don't change it all if possible. This just increase the
size
> of
> > the diff and
> > > makes it less readable (with no real benefit). Here and elsewhere you
> changed
> > to
> > > that style.
> > 
> > I am not just re-wrapping the arguments here, I am also removing one.  So,
the
> > diff would not be much smaller were the arguments not re-wrapped.
> 
> OK, but please could you try avoid imposing your personal style when doing
these
> changes? As someone that has touched this code before, I prefer to stick with
> what we have know. It doesn't hurt the readability and it also saves vertical
> spaces. If you could avoid doing this kind of change in future that would be
> really appreciate.

This bikeshed-ish conversation might be easier to pursue in a different forum
(e.g. via IRC), but:  Scanning through the remaining files in the "importer"
directory, it looks to me like the style I am using matches the prevalent style
in these files.  If there were someone listed in the OWNERS, I would ask them
what style they prefer, and try to stick to that.  As is, I'm just trying to use
my best judgement on what makes the code most readable ;)

That said, if it's really important to you and you want to go through and
highlight all of the places in this CL that I should revert to the pre-existing
style, feel free to do that and I can follow up accordingly.

[tfarina, 2011-05-26 00:40:02]

On 2011/05/26 00:35:47, Ilya Sherman wrote:
> That said, if it's really important to you and you want to go through and
> highlight all of the places in this CL that I should revert to the
pre-existing
> style, feel free to do that and I can follow up accordingly.
No, no. I won't do that. Do what you think it's best, I just wanted to point
this out. It isn't a thing to discuss so much, I don't really care much more
about this. I'm fine either way. Thanks. ;)

[Miranda Callahan, 2011-05-26 12:54:02]

The Mac test is the one you said fails because of the binary, right?  Take it on
home! LGTM!