gab: importer stuff
sky: bookmark stuff

https://codereview.chromium.org/14575004/diff/2001/chrome/browser/importer/bo...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/2001/chrome/browser/importer/bo...
chrome/browser/importer/bookmarks_file_importer.h:8: #include <set>
looks like you can remove this include.

https://codereview.chromium.org/14575004/diff/2001/chrome/browser/importer/bo...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/2001/chrome/browser/importer/bo...
chrome/browser/importer/bookmarks_file_importer.cc:277: bool
BookmarksFileImporter::ParseCharsetFromLine(const std::string& line,
tiny nit: since these are private static functions, they could be moved into an
unnamed namespace in this source file.
But I know you didn't do this way, to not make the diff uglier and hard to read.

That way was pretty easy to see what has changed from one file to another.

https://codereview.chromium.org/14575004/diff/2001/chrome/browser/importer/bo...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/2001/chrome/browser/importer/bo...
chrome/browser/importer/bookmarks_file_importer.cc:277: bool
BookmarksFileImporter::ParseCharsetFromLine(const std::string& line,
On 2013/05/07 02:15:55, tfarina wrote:
> tiny nit: since these are private static functions, they could be moved into
an
> unnamed namespace in this source file.

No, they cannot be. These functions are used by the unit tests.

(There are some other functions that are not, and those functions I did move to
an anonymous namespace.)

> But I know you didn't do this way, to not make the diff uglier and hard to
read.

You give me far too much credit. This change is to get as optimal a result as I
can, with no concern for how the diff looked. In my worldview, if I write a CL
with diff prettiness in mind, I'm doing it wrong.

https://codereview.chromium.org/14575004/diff/2001/chrome/browser/importer/bo...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/2001/chrome/browser/importer/bo...
chrome/browser/importer/bookmarks_file_importer.h:8: #include <set>
On 2013/05/07 01:03:19, tfarina wrote:
> looks like you can remove this include.

Yes! Good catch.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:70: string16* folder_name,
base::string16

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:70: string16* folder_name,
On 2013/05/07 16:08:15, tfarina wrote:
> base::string16

Good point; will update those in the next patch.

Thanks for doing this, comments below.

Cheers!
Gab

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (left):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:234: // add it as our
keywords.
Why remove support for keywords?

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:68:
!favicon_data.SchemeIs(chrome::kDataScheme))
#include "content/public/common/url_constants.h" for chrome::kDataScheme

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:98: 
Add a section comment:

////////////////////////////////////////////////////////////////////////////////
// BookmarksFileImporter, public:

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

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:111: base::FilePath
source_path = source_profile.source_path;
can be inline on line 122

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:112: base::FilePath app_path
= source_profile.app_path;
unused.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:127: // Write data into
profile.
// Send bookmarks over the bridge.
?

(although the code is pretty explicit and I'd be fine with no comments at all)

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:156: size_t toolbar_folder =
0;
Took me some time to understand what this variable represented.

How about renaming it to toolbar_folder_index (i.e., it seems to be the index of
the toolbar folder in |path|; perhaps adding a comment saying this here could
help too)?

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:275: 
Add a section comment:

////////////////////////////////////////////////////////////////////////////////
// BookmarksFileImporter, private:

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

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:349: const char kItemOpen[] =
"<DT><A";
optional: The compiler should optimize this by putting const char[] in .rodata
and optimizing the pointer out, but I tend to err on the side of caution and
explicitly make local constants static.

(this article explains why this can be a problem at scale:
http://glandium.org/blog/?p=2361).

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:434: const char kItemOpen[] =
"<DT><A";
optional: static (see comment above).

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:15: #include
"chrome/browser/importer/profile_writer.h"
This include is only for ProfileWriter::BookmarkEntry right? You could forward
declare it like history::ImportedFaviconUsage?

I don't feel strongly though, this would also imply you have to explicitly
include this in the cc file (like history_types.h is being explicitly included
there now), do we have a rule for when to forward declare and when to simply
include in the header? Strictly speaking mostly everything (i.e. even string16)
can be forward declared in headers, but that feels like it adds a lot of boiler
plate to the header...

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:33: virtual void
StartImport(const importer::SourceProfile& source_profile,
#include "chrome/browser/importer/importer_data_types.h"
for this

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:35: ImporterBridge* bridge)
OVERRIDE;
#include "chrome/browser/importer/importer_bridge.h"
for this

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:42: static void
ImportBookmarksFile(
Seems the only reason this is static is to be used directly in tests.

I think this should not be static and be moved to the private section to only be
invoked by StartImport (and would then also no longer need to take a
|cancellation_callback|).

The tests could then test the private method directly (they are already
friends...) or they could be made into browser_tests and use the regular import
flow which is used to test all other importers (except for some unit_tests
directly testing the parsing methods) -- e.g., ie_importer_browsertest_win.cc
and firefox_importer_browsertest.cc.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:67: std::string* charset);
#include <string>
for this

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:70: string16* folder_name,
#include "base/string16.h"
for this

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:82: base::Time* add_date,
#include "base/time.h"
for this

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer_unittest.cc (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer_unittest.cc:140:
importer->ImportBookmarksFile(empty_folder_path, NULL, &bookmarks, NULL);
This is a static method, no need to create an importer for this (same comment
for multiple places in this file).

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:98: 
On 2013/05/07 17:47:54, gab wrote:
> Add a section comment:
> 
>
////////////////////////////////////////////////////////////////////////////////
> // BookmarksFileImporter, public:
> 
> ref:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

I wouldn't encourage the use of this kind of comment. Ben seems to use it
though.

Of course this up to Avi.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:349: const char kItemOpen[] =
"<DT><A";
On 2013/05/07 17:47:54, gab wrote:
> optional: The compiler should optimize this by putting const char[] in .rodata
> and optimizing the pointer out, but I tend to err on the side of caution and
> explicitly make local constants static.
> 
> (this article explains why this can be a problem at scale:
> http://glandium.org/blog/?p=2361).

There was a thread about this:

https://groups.google.com/a/chromium.org/forum/?hl=en&fromgroups#!searchin/ch...

All issues addressed unless otherwise noted.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (left):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:234: // add it as our
keywords.
On 2013/05/07 17:47:54, gab wrote:
> Why remove support for keywords?

Because it's a feature of the FF2 importer, not of the subset that is the
bookmarks file importer.

This is intended to be no code change from how things work today. Today, the
bookmarks file importer doesn't know about keywords, and no caller expects it
to. So I'm removing it.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:98: 
Most code doesn't use comments like that; not going to do it.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:349: const char kItemOpen[] =
"<DT><A";
gab: Does that article refer to declarations _inside_ functions? I'm not seeing
it do so.

tfarina: After reading that thread, my head is spinning. If this is really a
problem then it's a problem across chrome and needs to be addressed at that
level.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:15: #include
"chrome/browser/importer/profile_writer.h"
We forward-declare whenever possible.

history:: is a namespace; ProfileWriter is a class. You can't forward-declare a
nested class.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:42: static void
ImportBookmarksFile(
On 2013/05/07 17:47:54, gab wrote:
> Seems the only reason this is static is to be used directly in tests.

A future CL that makes the FF3 importer not depend on the FF2 importer also will
end up calling this. https://codereview.chromium.org/15039004

> The tests could then test the private method directly (they are already
> friends...) or they could be made into browser_tests

I'd rather keep tests as unit tests rather than browser tests. The question that
I have is how the FF3 importer should end up looking.

Re the search engine import, look at Firefox2Importer::ImportBookmarks line 316:

  if (!parsing_bookmarks_html_file_ && !template_urls.empty() && !cancelled())
    bridge_->SetKeywords(template_urls, false);
  else
    STLDeleteElements(&template_urls);

If it's being used as a bookmarks file importer (and thus
parsing_bookmarks_html_file_ is true), then templates aren't imported.

On 2013/05/07 20:29:32, Avi wrote:
> Re the search engine import, look at Firefox2Importer::ImportBookmarks line
316:
> 
>   if (!parsing_bookmarks_html_file_ && !template_urls.empty() && !cancelled())
>     bridge_->SetKeywords(template_urls, false);
>   else
>     STLDeleteElements(&template_urls);
> 
> If it's being used as a bookmarks file importer (and thus
> parsing_bookmarks_html_file_ is true), then templates aren't imported.

Ah I see, thanks for clarifying (reviewing rest atm).

On 2013/05/08 14:36:40, gab wrote:
> On 2013/05/07 20:29:32, Avi wrote:
> > Re the search engine import, look at Firefox2Importer::ImportBookmarks line
> 316:
> > 
> >   if (!parsing_bookmarks_html_file_ && !template_urls.empty() &&
!cancelled())
> >     bridge_->SetKeywords(template_urls, false);
> >   else
> >     STLDeleteElements(&template_urls);
> > 
> > If it's being used as a bookmarks file importer (and thus
> > parsing_bookmarks_html_file_ is true), then templates aren't imported.
> 
> Ah I see, thanks for clarifying (reviewing rest atm).

Seems weird that we would do this though...

lg, just a couple replies below.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:98: 
On 2013/05/07 18:29:39, Avi wrote:
> Most code doesn't use comments like that; not going to do it.

Ok, I'd seen it around and thought it was useful when reading the CC file, but I
won't insist given that it's not a majority of the code that does this.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:349: const char kItemOpen[] =
"<DT><A";
On 2013/05/07 18:29:39, Avi wrote:
> gab: Does that article refer to declarations _inside_ functions? I'm not
seeing
> it do so.

Ah right, it refers to file scope, not local method scope.

> 
> tfarina: After reading that thread, my head is spinning. If this is really a
> problem then it's a problem across chrome and needs to be addressed at that
> level.

So I searched through the codebase with the following regexes:
^((?!namespace).*)\{\n( )*static const char [a-zA-Z]*\[\] = \".*\";
and
^((?!namespace).*)\{\n( )*const char [a-zA-Z]*\[\] = \".*\";

(i.e. anywhere where a const char[] is declared on the line following an open
brace (which is not simply the opening of a namespace) -- all of these seem to
match local scope declarations as desired)

Seems the use of static is less common than cases where it's omitted for local
scope const char arrays:
51 matches across 36 files
vs
213 matches across 81 files (although a lot of these are in test code)

FWIW, I also generalized this research to all POD constants:
^((?!namespace).*)\{\n( )*const [a-z0-9]* k[a-zA-Z]*(\[\])? = .*;
622 matches across 317 files
^((?!namespace).*)\{\n( )*static const [a-z0-9]* k[a-zA-Z]*(\[\])? = .*;
176 matches across 111 files

For inlineable constants though it probably makes sense that they not be static
(e.g., int, bool, etc.) to yield a better chance that the compiler will inline
it.

Arguably, const char[] is a simple pointer underneath  and can probably be
considered an inlineable constant as well (**and thus omitting static here is
probably OK**).

For more complex things like const char[][], it makes sense to use static to
avoid extra initialization though (as I just discussed with Etienne Bergeron and
as demonstrated by Albert Wong and summarized by Ami Fischman @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/xnJY6...
)

(I'm currently analyzing this further with the help of Etienne Bergeron, looks
like static indeed helps here, but this is far from being the only instance
where the static keyword is omitted so you can feel free to ignore this for this
CL).

Cheers!

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:42: static void
ImportBookmarksFile(
On 2013/05/07 18:29:39, Avi wrote:
> On 2013/05/07 17:47:54, gab wrote:
> > Seems the only reason this is static is to be used directly in tests.
> 
> A future CL that makes the FF3 importer not depend on the FF2 importer also
will
> end up calling this. https://codereview.chromium.org/15039004
> 
> > The tests could then test the private method directly (they are already
> > friends...) or they could be made into browser_tests
> 
> I'd rather keep tests as unit tests rather than browser tests. The question
that
> I have is how the FF3 importer should end up looking.

Sounds like a case for a common virtual parent class.

i.e.

class Foo : public Importer {
  ... (keep StartImport() virtual)
 protected:
  void ImportBookmarksFromFile(
      const base::FilePath& file_path,
      std::vector<ProfileWriter::BookmarkEntry>* bookmarks,
      std::vector<history::ImportedFaviconUsage>* favicons);
};

And then both Firefox3Importer and BookmarksFileImporter can subclass Foo.

This would allow ImportBookmarksFromFile() to directly check cancelled().

I'm not sure I like adding this complexity just to avoid the callback though...


I'm just generally not a fan of having a static method in a class which is used
by other classes (other than factory methods to create instances of the same
class, etc.), in particular in this case since it involves a method of the class
itself passing a callback to one of its static methods to check something the
static method could just check itself was it not static...

Perhaps simply keep it as is, but pull the whole method out of this class into
bookmarks_importer_utils.h or something and put it in the importer namespace?

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:349: const char kItemOpen[] =
"<DT><A";
I'd like to take a pass on this for now. It would be nice to handle this on a
cross-Chromium level and to have guidelines established once and for all.

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/14001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:42: static void
ImportBookmarksFile(
On 2013/05/08 16:24:34, gab wrote:
> Perhaps simply keep it as is, but pull the whole method out of this class into
> bookmarks_importer_utils.h or something and put it in the importer namespace?

That points to something that tfarina asked, where there's a dependency here
that he wanted to eliminate, one where bookmarks code depended on this code in
the importer.

If I pull this into a function in the utils class, then it might be easier to
move it to the bookmarks code and make the importer code depend on the bookmarks
code. I was thinking about doing that now, but that's more rewriting than I'd
like for one CL. This has dependencies on Profiles and history, etc, so I'd like
an intermediate stage.

Suppose we moved ImportBookmarksFile into the bookmarks module, and then made
the two importers depend on it. I'd be OK with that.

The two questions are what to do with the types.

First, we have ProfileWriter::BookmarkEntry. It's used only by the importer
system, and I would have no problems moving it to the bookmarks module
wholesale.

The other is history::ImportedFaviconUsage. It's used in the history module,
favicon module, importer module, and of course the bookmark module. Which is the
lowest one in that list that should get it?

On 2013/05/08 19:23:00, Avi wrote:
> Suppose we moved ImportBookmarksFile into the bookmarks module, and then made
> the two importers depend on it. I'd be OK with that.
> 
> The two questions are what to do with the types.
> 
> First, we have ProfileWriter::BookmarkEntry. It's used only by the importer
> system, and I would have no problems moving it to the bookmarks module
> wholesale.
> 
> The other is history::ImportedFaviconUsage. It's used in the history module,
> favicon module, importer module, and of course the bookmark module. Which is
the
> lowest one in that list that should get it?

I don't really have a preference here, I'll let Thiago and others chime in as to
where things should go, all I'd like is for ImportBookmarksFile to be extracted
from BookmarksFileImporter :).

Thanks!
Gab

On 2013/05/08 19:23:00, Avi wrote:
> Suppose we moved ImportBookmarksFile into the bookmarks module, and then made
> the two importers depend on it. I'd be OK with that.
> 
> The two questions are what to do with the types.
> 
> First, we have ProfileWriter::BookmarkEntry. It's used only by the importer
> system, and I would have no problems moving it to the bookmarks module
> wholesale.

SGTM

> The other is history::ImportedFaviconUsage. It's used in the history module,
> favicon module, importer module, and of course the bookmark module. Which is
the
> lowest one in that list that should get it?

Seems like it should be moved based on name to the favicon module. That nicely
balances BookmarkEntry existing in bookmarks.

On 2013/05/09 15:57:21, sky wrote:
> On 2013/05/08 19:23:00, Avi wrote:
> > The other is history::ImportedFaviconUsage
> Seems like it should be moved based on name to the favicon module.

It's OK if bookmarks/ depends on favicons?

On Thu, May 9, 2013 at 2:48 PM,  <avi@chromium.org> wrote:
> On 2013/05/09 15:57:21, sky wrote:
>>
>> On 2013/05/08 19:23:00, Avi wrote:
>> > The other is history::ImportedFaviconUsage
>>
>> Seems like it should be moved based on name to the favicon module.
>
> It's OK if bookmarks/ depends on favicons?
>
It already depends on c/b/favicons. We have it in c/b/bookmarks/DEPS
to remove this dep as well. Although adding one more header file to
DEPS from favicons is no worse than the current situation. So it's
fine by me. Scott might have other opinions about it though. I left
the decision to him as he is the OWNER there.

--
Thiago

I told Avi I was fine with his proposal.

On Thu, May 9, 2013 at 10:58 AM, Thiago Farina <tfarina@chromium.org> wrote:
> On Thu, May 9, 2013 at 2:48 PM,  <avi@chromium.org> wrote:
>> On 2013/05/09 15:57:21, sky wrote:
>>>
>>> On 2013/05/08 19:23:00, Avi wrote:
>>> > The other is history::ImportedFaviconUsage
>>>
>>> Seems like it should be moved based on name to the favicon module.
>>
>> It's OK if bookmarks/ depends on favicons?
>>
> It already depends on c/b/favicons. We have it in c/b/bookmarks/DEPS
> to remove this dep as well. Although adding one more header file to
> DEPS from favicons is no worse than the current situation. So it's
> fine by me. Scott might have other opinions about it though. I left
> the decision to him as he is the OWNER there.
>
> --
> Thiago

The plan is to move ProfileWriter::BookmarkEntry to bookmarks/. As for
history::ImportedFaviconUsage, I have no idea. The bookmarks code already has a
DEPS exception for it, so this will be no worse a situation. I'm going to move
the bookmark file reading code to bookmarks, and someone eventually with figure
out what to do with history::ImportedFaviconUsage and fix it.

So my understanding is that Avi is going to move void ImportBookmarksFile()
function into c/b/bookmarks. Is this going to happen in this patch? Does it need
to be in this patch?

If we do in a follow up then we don't have to worry about the deps of
ProfileWriter::BookmarkEntry and history::ImporterFaviconUsage.

I am rewriting this patch to move ImportBookmarksFile into bookmarks because it
appears to be clear that this CL will not get approval if I don't.

On 2013/05/09 19:58:05, Avi wrote:
> I am rewriting this patch to move ImportBookmarksFile into bookmarks because
it
> appears to be clear that this CL will not get approval if I don't.

OK, ImportBookmarksFile is not moving cleanly into bookmarks. It depends on
favicon code that lives in importer (importer::ReencodeFavicon) and the firefox
importer utils (CanImportURL) :/

Whee. Still working on it.

gab, sky, please take a look again.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/DEPS (left):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/DEPS:37:
"!chrome/browser/importer/firefox2_importer.h",
please, include 144783 in BUG= line for this.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.h (right):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.h:13: #include "base/time.h"
you can also forward declare this

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.h:14: #include
"googleurl/src/gurl.h"
can you forward declare this or the base::Callback doesn't allow you?

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/imported_bookmark_entry.h (right):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/imported_bookmark_entry.h:17: bool operator==(const
ImportedBookmarkEntry& other) const;
nit: add blank line above.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/imported_bookmark_entry.h:22: std::vector<string16>
path;
base::string16

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:10: class ImporterBridge;
no need of this forwarding too.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:13: struct SourceProfile;
you don't need this forward because it's part of importer.h:StartImport

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:23: ImporterBridge* bridge)
OVERRIDE;
I'd include compiler_specific.h and basictypes.h just in case.

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.cc:33: begin =
attribute_list.find(kQuote, begin) + 1;
On 2013/05/10 19:35:20, sky wrote:
> Any reason you didn't use:
> begin += attribute.size() + 1?

This was straight from the FF2 code. That is simpler; done.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/DEPS (left):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/DEPS:37:
"!chrome/browser/importer/firefox2_importer.h",
On 2013/05/10 19:58:30, tfarina wrote:
> please, include 144783 in BUG= line for this.

Done.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.h (right):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.h:13: #include "base/time.h"
On 2013/05/10 19:58:30, tfarina wrote:
> you can also forward declare this

Done.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.h:14: #include
"googleurl/src/gurl.h"
On 2013/05/10 19:58:30, tfarina wrote:
> can you forward declare this or the base::Callback doesn't allow you?

Done.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/imported_bookmark_entry.h (right):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/imported_bookmark_entry.h:17: bool operator==(const
ImportedBookmarkEntry& other) const;
On 2013/05/10 19:58:30, tfarina wrote:
> nit: add blank line above.

Done.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/imported_bookmark_entry.h:22: std::vector<string16>
path;
On 2013/05/10 19:58:30, tfarina wrote:
> base::string16

Done.

lg, comments below. Can you mark comment on all the methods that changed in the
move (i.e. I noticed DataURLToFaviconUsage changed, but I assume a lot of the
method moves are pure copy/paste and I'd like to avoid having to compare them
line-by-line and just trust you on which ones were changed and should be looked
at).

Thanks!
Gab

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.h (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.h:52: namespace
exposed_for_testing {
I think we usually name such namespaces "internal"

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/imported_bookmark_entry.h (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/imported_bookmark_entry.h:16: ~ImportedBookmarkEntry();
I don't think you need to declare a no-op destructor here.

Also, should this have DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:18: bool
ImporterCancelBridge(BookmarksFileImporter* importer) {
nit: I find this name rather vague, rename to IsImporterCancelled()?

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:24:
BookmarksFileImporter::BookmarksFileImporter() {}
Inline empty constructors in the header.

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:54:
history::ImportedFaviconUsage& favicon = dirty_favicons[i];
Use a *, non-const & are not allowed by the style guide.

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:26: virtual
~BookmarksFileImporter();
Why is this private? I don't even think you need to declare a destructor here.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.cc:53: void DataURLToFaviconUsage(
Why remove the ReencodeFavicon call from this method? Previously all callers of
ImportBookmarksFile where guaranteed to import re-encoded icons, now they're
not.

The call is in c/b/importer yes, but it doesn't look like it depends on anything
else so I think it could easily move here with the rest, no? It would need to be
exposed publicly for its usage in Firefox3Importer, but that's about it.

GetAttribute optimized a bit due to Scott's comment.
DataURLToFaviconUsage no longer calls the importer::ReencodeFavicon; it's up to
the caller to do it.
ImportBookmarksFile has refactoring changes only; two direct calls were replaced
with callbacks.
ParseCharsetFromLine was unchanged.
ParseFolderNameFromLine was unchanged except for moving to net::UnescapeForHTML.
ParseBookmarkFromLine was unchanged except for moving to net::UnescapeForHTML
and moving constants into the function.
ParseMinimumBookmarkFromLine was unchanged except for moving to
net::UnescapeForHTML.

On 2013/05/10 21:05:52, Avi wrote:
> GetAttribute optimized a bit due to Scott's comment.
> DataURLToFaviconUsage no longer calls the importer::ReencodeFavicon; it's up
to
> the caller to do it.
> ImportBookmarksFile has refactoring changes only; two direct calls were
replaced
> with callbacks.
> ParseCharsetFromLine was unchanged.
> ParseFolderNameFromLine was unchanged except for moving to
net::UnescapeForHTML.
> ParseBookmarkFromLine was unchanged except for moving to net::UnescapeForHTML
> and moving constants into the function.
> ParseMinimumBookmarkFromLine was unchanged except for moving to
> net::UnescapeForHTML.

Thanks for the summary, all of these make sense to me except for
DataURLToFaviconUsage, see my comment on patchset 8.

Gab

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.h (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.h:52: namespace
exposed_for_testing {
On 2013/05/10 20:37:57, gab wrote:
> I think we usually name such namespaces "internal"

Done.

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/imported_bookmark_entry.h (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/imported_bookmark_entry.h:16: ~ImportedBookmarkEntry();
On 2013/05/10 20:37:57, gab wrote:
> I don't think you need to declare a no-op destructor here.

../../chrome/browser/bookmarks/imported_bookmark_entry.h:15:1: error:
[chromium-style] Complex class/struct needs an explicit out-of-line destructor.

> Also, should this have DISALLOW_COPY_AND_ASSIGN?

No, this is a small utility struct, and we shove it into vectors and copy it all
over the place. I can't disallow copying it without completely rewriting all the
code that uses it.

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:18: bool
ImporterCancelBridge(BookmarksFileImporter* importer) {
On 2013/05/10 20:37:57, gab wrote:
> nit: I find this name rather vague, rename to IsImporterCancelled()?

Done.

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:24:
BookmarksFileImporter::BookmarksFileImporter() {}
On 2013/05/10 20:37:57, gab wrote:
> Inline empty constructors in the header.

Why? We put empty constructors in .cc files all the time.

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:54:
history::ImportedFaviconUsage& favicon = dirty_favicons[i];
On 2013/05/10 20:37:57, gab wrote:
> Use a *, non-const & are not allowed by the style guide.

They're disallowed as parameters; where are they disallowed in function bodies?

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:26: virtual
~BookmarksFileImporter();
On 2013/05/10 20:37:57, gab wrote:
> Why is this private? I don't even think you need to declare a destructor here.

This is an Importer which is ref counted, therefore it needs a private
destructor.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.cc:53: void DataURLToFaviconUsage(
On 2013/05/10 20:37:57, gab wrote:
> Why remove the ReencodeFavicon call from this method?

It's used by everyone
(https://code.google.com/p/chromium/codesearch#search/&q=ReencodeFavicon), not
just FF3. If I had to pick a location where it belonged I would pick the favicon
folder; but it's still not clear to me if I can make bookmarks depend on
favicons.

On Fri, May 10, 2013 at 6:34 PM,  <avi@chromium.org> wrote:
>
https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
> chrome/browser/bookmarks/bookmark_html_reader.cc:53: void
> DataURLToFaviconUsage(
> On 2013/05/10 20:37:57, gab wrote:
>>
>> Why remove the ReencodeFavicon call from this method?
>
>
> It's used by everyone
> (https://code.google.com/p/chromium/codesearch#search/&q=ReencodeFavicon),
> not just FF3. If I had to pick a location where it belonged I would pick
> the favicon folder; but it's still not clear to me if I can make
> bookmarks depend on favicons.
>
It isn't desired, but you can.

$ g grep chrome/browser/favicon/ chrome/browser/bookmarks/
chrome/browser/bookmarks/DEPS:  "!chrome/browser/favicon/favicon_service.h",
chrome/browser/bookmarks/DEPS:
"!chrome/browser/favicon/favicon_service_factory.h",
chrome/browser/bookmarks/bookmark_html_writer.cc:#include
"chrome/browser/favicon/favicon_service.h"
chrome/browser/bookmarks/bookmark_html_writer.cc:#include
"chrome/browser/favicon/favicon_service_factory.h"
chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:#include
"chrome/browser/favicon/favicon_service.h"
chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:#include
"chrome/browser/favicon/favicon_service_factory.h"
chrome/browser/bookmarks/bookmark_model.cc:#include
"chrome/browser/favicon/favicon_service.h"
chrome/browser/bookmarks/bookmark_model.cc:#include
"chrome/browser/favicon/favicon_service_factory.h"

Regards,

--
Thiago

On 2013/05/10 21:39:25, tfarina wrote:
> It isn't desired, but you can.
> 
> $ g grep chrome/browser/favicon/ chrome/browser/bookmarks/
> chrome/browser/bookmarks/DEPS:  "!chrome/browser/favicon/favicon_service.h",

Read the DEPS file; it's going away. I don't want to make things worse here.

If we're going to officially make the bookmarks code depend on the favicon code,
sure, I'll do it. But I won't say, "oh, we already have dependencies we're
trying to get rid of, so let me make more."

Scott, what do you have to say?

Thanks for clarifying, I'm only unhappy about ReencodeFavicon; everything else
looks great!

Cheers,
Gab

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.cc (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:24:
BookmarksFileImporter::BookmarksFileImporter() {}
On 2013/05/10 21:34:53, Avi wrote:
> On 2013/05/10 20:37:57, gab wrote:
> > Inline empty constructors in the header.
> 
> Why? We put empty constructors in .cc files all the time.

Ah ok, I guess that's fine, I thought I'd mostly seen them inlined in the
header; I think not doing that prevents it from being inlined in the caller
resulting in an extra jmp in the assembly.

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.cc:54:
history::ImportedFaviconUsage& favicon = dirty_favicons[i];
On 2013/05/10 21:34:53, Avi wrote:
> On 2013/05/10 20:37:57, gab wrote:
> > Use a *, non-const & are not allowed by the style guide.
> 
> They're disallowed as parameters; where are they disallowed in function
bodies?

You're right, the style only mentions parameters... I guess this is fine.

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
File chrome/browser/importer/bookmarks_file_importer.h (right):

https://codereview.chromium.org/14575004/diff/59001/chrome/browser/importer/b...
chrome/browser/importer/bookmarks_file_importer.h:26: virtual
~BookmarksFileImporter();
On 2013/05/10 21:34:53, Avi wrote:
> On 2013/05/10 20:37:57, gab wrote:
> > Why is this private? I don't even think you need to declare a destructor
here.
> 
> This is an Importer which is ref counted, therefore it needs a private
> destructor.

Ah I see, thanks for clarifying.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.cc:53: void DataURLToFaviconUsage(
On 2013/05/10 21:34:53, Avi wrote:
> On 2013/05/10 20:37:57, gab wrote:
> > Why remove the ReencodeFavicon call from this method?
> 
> It's used by everyone
> (https://code.google.com/p/chromium/codesearch#search/&q=ReencodeFavicon), not
> just FF3. If I had to pick a location where it belonged I would pick the
favicon
> folder; but it's still not clear to me if I can make bookmarks depend on
> favicons.

Ok, it just seems weird to me that ImportBookmarksFile returns a set of favicons
which aren't properly encoded...

Either we make this very explicit in the method's comments or we move the method
so that it can be called from c/b/bookmarks/ (I much prefer the latter).

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.cc:53: void DataURLToFaviconUsage(
I am not happy about this either.

The way I see it, ReencodeFavicon is only used by import code, and so if it
belongs anywhere else it belongs in favicon code. Is it worth bringing it to the
bookmark code? It feels weird to think that I will alter all the importers to
bring in a file from the bookmark code that reworks favicons.

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.cc:19: #include
"googleurl/src/gurl.h"
sort

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.h (right):

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.h:96: }  // namespace
exposed_for_testing
s/exposed_for_testing/internal

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader_unittest.cc (right):

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:21: result =
bookmark_html_reader::internal::ParseCharsetFromLine(
could you wrap this file in bookmark_html_reader namespace?

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.cc:19: #include
"googleurl/src/gurl.h"
On 2013/05/10 21:57:04, tfarina wrote:
> sort

Done.

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.h (right):

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.h:96: }  // namespace
exposed_for_testing
On 2013/05/10 21:57:04, tfarina wrote:
> s/exposed_for_testing/internal

Done.

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader_unittest.cc (right):

https://codereview.chromium.org/14575004/diff/72018/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:21: result =
bookmark_html_reader::internal::ParseCharsetFromLine(
On 2013/05/10 21:57:04, tfarina wrote:
> could you wrap this file in bookmark_html_reader namespace?

Done.

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/65001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.cc:53: void DataURLToFaviconUsage(
On 2013/05/10 21:51:43, Avi wrote:
> I am not happy about this either.
> 
> The way I see it, ReencodeFavicon is only used by import code, and so if it
> belongs anywhere else it belongs in favicon code. Is it worth bringing it to
the
> bookmark code? It feels weird to think that I will alter all the importers to
> bring in a file from the bookmark code that reworks favicons.

Right, it feels even weirder that the importers should have to know how to
reencode favicons it gets back from ImportBookmarksFile.

Why can't bookmarks depend on favicons again? Feels like a natural dependency to
me.

On 2013/05/13 13:34:42, gab wrote:
> Why can't bookmarks depend on favicons again? Feels like a natural dependency
to
> me.

If they're turning bookmarks into a module, it's their choice what dependencies
it should have. I'm asking if we can make favicons a dependency since it solves
problems and imo makes sense, but it's not my call.

Scott says that the dependency is OK. Going for it.

On May 13, 2013 10:41 AM, <avi@chromium.org> wrote:
>
> Scott says that the dependency is OK. Going for it.

Awesome, thanks!

>
> https://codereview.chromium.org/14575004/

Please take a look.

https://codereview.chromium.org/14575004/diff/99001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.h (right):

https://codereview.chromium.org/14575004/diff/99001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.h:43: base::Callback<bool(void)>*
cancellation_callback,
output parameters should come after input parameters. That means, I'd put
|file_path| first.

https://codereview.chromium.org/14575004/diff/99001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/imported_bookmark_entry.h (right):

https://codereview.chromium.org/14575004/diff/99001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/imported_bookmark_entry.h:10: #include
"base/basictypes.h"
can we remove this include?

https://codereview.chromium.org/14575004/diff/99001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader.h (right):

https://codereview.chromium.org/14575004/diff/99001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader.h:43: base::Callback<bool(void)>*
cancellation_callback,
On 2013/05/13 18:08:38, tfarina wrote:
> output parameters should come after input parameters. That means, I'd put
> |file_path| first.

The callbacks aren't output parameters.

SLGTM

Chris, can you look at the messages change? I moved two classes from one
directory to another; I changed nothing about what they contain or how they are
used.

https://codereview.chromium.org/14575004/diff/99001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/imported_bookmark_entry.h (right):

https://codereview.chromium.org/14575004/diff/99001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/imported_bookmark_entry.h:10: #include
"base/basictypes.h"
On 2013/05/13 18:08:38, tfarina wrote:
> can we remove this include?

Yes.

https://codereview.chromium.org/14575004/diff/110001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/110001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_html_reader.cc:89: void
ImportBookmarksFile(base::Callback<bool(void)>* cancellation_callback,
why don't you pass by const-ref so?

https://codereview.chromium.org/14575004/diff/110001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_html_reader.cc:109: (!cancellation_callback ||
!cancellation_callback->Run());
then you can do cancellation_callaback.is_null().

https://codereview.chromium.org/14575004/diff/110001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/110001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_html_reader.cc:89: void
ImportBookmarksFile(base::Callback<bool(void)>* cancellation_callback,
why don't you pass by const-ref so?

https://codereview.chromium.org/14575004/diff/110001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_html_reader.cc:109: (!cancellation_callback ||
!cancellation_callback->Run());
then you can do cancellation_callaback.is_null().

https://codereview.chromium.org/14575004/diff/110001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_html_reader.cc (right):

https://codereview.chromium.org/14575004/diff/110001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_html_reader.cc:109: (!cancellation_callback ||
!cancellation_callback->Run());
On 2013/05/13 20:18:18, tfarina wrote:
> then you can do cancellation_callaback.is_null().

I was not aware of that feature of base::Callback. Thank you!

https://codereview.chromium.org/14575004/diff/118001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_html_reader_unittest.cc (right):

https://codereview.chromium.org/14575004/diff/118001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:143:
ImportBookmarksFile(base::Callback<bool(void)>(),
On 2013/05/13 21:53:01, gab wrote:
> I suggest you also add tests to test the functionality of the callbacks.
> e.g.,
> 1) Launch import with multiple bookmarks: return true to import the first one,
> return false to CanImport the second one, and return true to the 3rd one (to
> make sure CanImport doesn't stick)...
> 2) Try to cancel after 1 or 2 bookmarks and check that the right thing
happens.

Done.

Awesome, thanks! Some comments/suggestions below.

Gab

https://codereview.chromium.org/14575004/diff/88003/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_html_reader_unittest.cc (right):

https://codereview.chromium.org/14575004/diff/88003/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:200: // Import
Epiphany bookmarks from a file to bookmark bar.
I think the EXPECT block below is the same as the one just above (starting on
line 181).

Consider making the fixture suggested in comments below a bit more generic to
include these Epiphany tests (which I think should be in a separate test instead
of clearing |bookmarks| and doing it in the same test).

https://codereview.chromium.org/14575004/diff/88003/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:249: entry = *it++;
nit: You can simplify this to
entry = bookmarks[0];
The iterator is overkill imo.

https://codereview.chromium.org/14575004/diff/88003/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:258: class
InvalidDomain {
I think this should be a separate test, it doesn't really need to be in the same
test as the test code leading up to this (apart from the first 4 lines which you
could put in a test fixture if duplicating them bothers you).

https://codereview.chromium.org/14575004/diff/88003/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:260: static bool
IsValid(const GURL& url) {
Add a method comment like:

// Accepts all domains found in firefox2.html, but tamurayukari.com.

https://codereview.chromium.org/14575004/diff/88003/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:272: entry = *it++;
entry = bookmarks[0];

https://codereview.chromium.org/14575004/diff/88003/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:273:
EXPECT_EQ(ASCIIToUTF16("Empty"), entry.title);
These EXPECT blocks are duplicated all over the place.

I'd suggest making a to do the common setup and have 3 helper methods to check
IsFirstFirefox2Bookmark(const ImporterBookmarkEntry&),
IsSecondFirefox2Bookmark(...), IsThirdFirefox2Bookmark(...).

And then here simply call back to back:

IsFirstFirefox2Bookmark(bookmarks[0]);
IsThirdFirefox2Bookmark(bookmarks[1]);

https://codereview.chromium.org/14575004/diff/88003/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:280: entry = *it++;
entry = bookmarks[1];

Massive cleanup of the test. You were right; it's a lot nicer this way.

https://codereview.chromium.org/14575004/diff/154001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_html_reader_unittest.cc (right):

https://codereview.chromium.org/14575004/diff/154001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_html_reader_unittest.cc:217: class
CancelAfterFifteenCalls {
On 2013/05/14 15:34:04, gab wrote:
> optional: I think I preferred your previous style of declaring the test
classes
> inside the test methods themselves.

I did too, but GCC on Linux refused to compile it with a template error.

> In particular I'm not sure that having multiple unnamed namespaces in the same
> file is common.

True, though I justify this with "declare/define as close as possible to site of
use".

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/14575004/154001