Do not add "Bookmarks File" to ImporterList.

Add "Bookmarks File" to ImporterList interactively.

Otherwise it becomes an option on first run AutoImport and that is not desired. 

Instead we force the clients to make the decision on whether they want or not to add the "interactive profile", aka "Bookmarks HTML File" to the ImporterList, which can make it available in the "Import Bookmarks and Settings" dialog.

BUG=295755
TEST=launch chrome, go to chrome://settings/importData, verifies that
"Bookmarks HTML file" option is present in the "From:" combobox and still works.
R=gab@chromium.org

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

[tfarina, 2013-09-22 15:31:01]

Hi Gabriel, could you review this to me?

Thanks,

[gab, 2013-09-23 14:39:14]

Thanks for fixing this, comments below.

https://codereview.chromium.org/24345002/diff/1/chrome/browser/importer/impor...
File chrome/browser/importer/importer_list.cc (left):

https://codereview.chromium.org/24345002/diff/1/chrome/browser/importer/impor...
chrome/browser/importer/importer_list.cc:178: importer::SourceProfile*
bookmarks_file = new importer::SourceProfile;
You will also need to revert your change to
chrome/browser/first_run/first_run_browsertest.cc (ref
https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/firs...)

https://codereview.chromium.org/24345002/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/24345002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/import_data_handler.cc:140:
importer_list_->GetSourceProfileAt(browser_index);
This needs the source_profile being imported to be in the importer_list_.

Perhaps we could add a method to the ImporterList to allow injecting an extra
profile after it's done initializing the default source profiles?

Otherwise we could also change ImporterList::DetectSourceProfiles() to take an
extra argument "bool include_interactive_profiles" which would need to be set to
true to get profiles such as bookmarks_file which can't be imported silently.

I think I prefer the 2nd suggestion as changing the method's signature
explicitly forces us in this CL to make a call for all call sites that
DetectSourceProfiles(); making sure we don't forget any (and also making sure
any new call site has to understand this and make a decision too).

[tfarina, 2013-09-28 17:10:04]

https://codereview.chromium.org/24345002/diff/1/chrome/browser/importer/impor...
File chrome/browser/importer/importer_list.cc (left):

https://codereview.chromium.org/24345002/diff/1/chrome/browser/importer/impor...
chrome/browser/importer/importer_list.cc:178: importer::SourceProfile*
bookmarks_file = new importer::SourceProfile;
On 2013/09/23 14:39:14, gab wrote:
> You will also need to revert your change to
> chrome/browser/first_run/first_run_browsertest.cc (ref
>
https://chromiumcodereview.appspot.com/22560003/diff/3001/chrome/browser/firs...)

Done.

https://codereview.chromium.org/24345002/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/24345002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/import_data_handler.cc:140:
importer_list_->GetSourceProfileAt(browser_index);
On 2013/09/23 14:39:14, gab wrote:
> This needs the source_profile being imported to be in the importer_list_.
> 
> Perhaps we could add a method to the ImporterList to allow injecting an extra
> profile after it's done initializing the default source profiles?
> 
> Otherwise we could also change ImporterList::DetectSourceProfiles() to take an
> extra argument "bool include_interactive_profiles" which would need to be set
to
> true to get profiles such as bookmarks_file which can't be imported silently.
> 
> I think I prefer the 2nd suggestion as changing the method's signature
> explicitly forces us in this CL to make a call for all call sites that
> DetectSourceProfiles(); making sure we don't forget any (and also making sure
> any new call site has to understand this and make a decision too).

Done.

[gab, 2013-10-01 18:33:58]

lgtm, please update description to reflect latest approach.

Thanks,
Gab

[tfarina, 2013-10-02 01:43:09]

Dan for OWNERS stamp of chrome/browser/ui/webui/options/ changes.

Thanks,

[tfarina, 2013-10-02 01:43:29]

On 2013/10/01 18:33:58, gab wrote:
> lgtm, please update description to reflect latest approach.
> 
thanks! CL description updated.

> Thanks,
> Gab

[Dan Beam, 2013-10-02 09:03:36]

lgtm

[Dan Beam, 2013-10-02 09:05:27]

https://chromiumcodereview.appspot.com/24345002/diff/11001/chrome/browser/imp...
File chrome/browser/importer/importer_list.h (right):

https://chromiumcodereview.appspot.com/24345002/diff/11001/chrome/browser/imp...
chrome/browser/importer/importer_list.h:32: bool include_interactive_prifiles,
nit: s/prifiles/profiles/

[tfarina, 2013-10-02 13:24:31]

https://chromiumcodereview.appspot.com/24345002/diff/11001/chrome/browser/imp...
File chrome/browser/importer/importer_list.h (right):

https://chromiumcodereview.appspot.com/24345002/diff/11001/chrome/browser/imp...
chrome/browser/importer/importer_list.h:32: bool include_interactive_prifiles,
On 2013/10/02 09:05:27, Dan Beam wrote:
> nit: s/prifiles/profiles/

Done.

[commit-bot: I haz the power, 2013-10-02 13:24:42]

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

[commit-bot: I haz the power, 2013-10-02 15:34:42]

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

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

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

[commit-bot: I haz the power, 2013-10-02 16:51:22]

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=win&number...

[commit-bot: I haz the power, 2013-10-03 01:34:15]

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

[commit-bot: I haz the power, 2013-10-03 06:00:07]

Change committed as 226694