Reland "Add option to import from bookmarks html file to ...""

Reland "Add option to import from bookmarks html file to ...""

This patch relands crrev.com/216860 with the fix for the first run tests.

To test this new feature one can do the following:

On chrome, open the "Import Bookmarks and Settings..." dialog, you can do this
by three ways, one is to navigate to chrome://settings/importData, another is
to open chrome://settings (either through wrench menu or navigating to it through
omnibox) and then under the Users category click on "Import Bookmarks..." button
to open the dialog. The third way to open this dialog
is Wrench->Bookmarks->Import Bookmarks and Settings...".

Once the dialog is opened, open the "From" combobox and select
"Bookmarks HTML File" item. That should show only the "Favorites/Bookmarks"
checkbox checked and enabled and present a "Choose file" button in the place
where "Import" button was. Click on that new button and select your bookmarks
html file, once you do that the import process should start, and if you file is
ok, chrome should import and present you with a success message.

BUG=80685
TBR=dbeam@chromium.org, isherman@chromium.org, cpu@chromium.org

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

[tfarina, 2013-08-10 18:43:30]

I can't reproduce the failure that happened on the bots with either without the
patch, with the patch, and with the patch with the "fix".

I was running:

$ out/Debug/browser_tests --disable-setuid-sandbox --gtest_filter=FirstRun*

I'll reland if browser_tests goes green.

[tfarina, 2013-08-10 19:00:26]

Stupid trybot!

It is skipping browser_tests on linux_rel.

[tfarina, 2013-08-10 19:19:16]

FirstRunMasterPrefsImportDefault.* seems to have passed at least on
mac_rel:swarm_triggered

I'll wait for more bots to finish to put this on CQ again.

[tfarina, 2013-08-10 21:56:02]

On 2013/08/10 19:19:16, tfarina wrote:
> FirstRunMasterPrefsImportDefault.* seems to have passed at least on
> mac_rel:swarm_triggered
> 
win_rel:swarm_triggered and linux_rel:swarm_triggered are red, but do not report
anything about FirstRun*

Putting this on CQ!

> I'll wait for more bots to finish to put this on CQ again.

[commit-bot: I haz the power, 2013-08-10 21:56:13]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[commit-bot: I haz the power, 2013-08-10 21:57:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/22560003/3001

[commit-bot: I haz the power, 2013-08-12 06:01:14]

Change committed as 216947

[gab, 2013-09-17 22:06:00]

I just noticed this code, I like the idea behind it, but it's incorrect as-is,
see comments below.

Also, when re-committing make sure to include a link to the original CL -- helps
see the original reviews.

In this case it appears you didn't get approval from a chrome/browser/first_run
OWNER; it's ok to TBR previous reviewers on a quick test fix + recommit, but if
it touches files with new OWNERS you must have them explicitly review, TBRing is
not okay in that case.

Cheers,
Gab

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/firs...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/firs...
chrome/browser/first_run/first_run_browsertest.cc:149: if (source_profile_count
== 1)
This is wrong, as mentioned in another comment, AutoImport should *never* import
from bookmarks file.

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/impo...
File chrome/browser/importer/importer_list.cc (right):

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/impo...
chrome/browser/importer/importer_list.cc:178: importer::SourceProfile*
bookmarks_file = new importer::SourceProfile;
Adding this here is problematic on AutoImport where we always import from the
first importer in this list (this could previously be empty, but will now
contain this entry).

This entry is not however suitable for silent AutoImport as:
1) TYPE_BOOKMARKS_FILE has no entry in
importer_creator.cc::CreateImporterByType()
2) Even if it did, it's unclear which file it should silently import from...

As it is if Chrome goes through first run on a computer with no other browsers,
it will hit NOTREACHED() @
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/utility/imp...

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/import_data_handler.cc:193: importer_host_ =
NULL;
I just realized that the same ImportDataHandler lives throughout the lifetime of
the browser process; thus it's not impossible for a second import request to be
made before the first one ended.

We should make sure to do:
if (importer_host_)
  importer_host_->set_observer(NULL);

before creating a new importer host.

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/import_data_handler.cc:206: int index,
Comment out unused parameter, i.e.:
int /* index *,

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/import_data_handler.cc:207: void* params) {
Same for this parameter.

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/import_data_handler.cc:213: importer_host_ = new
ExternalProcessImporterHost();
Move 213-223 in a common method used by both this method and ImportData().

[gab, 2013-09-17 22:52:19]

FYI, took some of these nits on as part of fixing a crash; only thing remaining
here I think is to fix AutoImport (see previous comment).

Cheers,
Gab

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/import_data_handler.cc:193: importer_host_ =
NULL;
On 2013/09/17 22:06:00, gab wrote:
> I just realized that the same ImportDataHandler lives throughout the lifetime
of
> the browser process; thus it's not impossible for a second import request to
be
> made before the first one ended.
> 
> We should make sure to do:
> if (importer_host_)
>   importer_host_->set_observer(NULL);
> 
> before creating a new importer host.

Handling this myself in https://codereview.chromium.org/24195005/

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/import_data_handler.cc:206: int index,
On 2013/09/17 22:06:00, gab wrote:
> Comment out unused parameter, i.e.:
> int /* index *,

Handling this myself in https://codereview.chromium.org/24195005/

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/import_data_handler.cc:207: void* params) {
On 2013/09/17 22:06:00, gab wrote:
> Same for this parameter.

Handling this myself in https://codereview.chromium.org/24195005/

https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/import_data_handler.cc:213: importer_host_ = new
ExternalProcessImporterHost();
On 2013/09/17 22:06:00, gab wrote:
> Move 213-223 in a common method used by both this method and ImportData().

Handling this myself in https://codereview.chromium.org/24195005/