Remove Google Toolbar importer (aka google.com/bookmarks importer).

Remove Google Toolbar importer (aka google.com/bookmarks importer).

(see bug for details on decision)

A follow-up CL will get rid of the in-process import code (ImporterHost).

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

[gab, 2013-06-28 02:08:01]

Ilya, PTAL.

TBR jam for OWNER on side-effect in
chrome/browser/automation/testing_automation_provider.cc

TBR jhawkins for OWNER on side-effect in
chrome/browser/ui/webui/options/import_data_handler.cc

I own everything else already :)! 

Cheers!
Gab

[scottmg, 2013-06-28 02:10:27]

Woo!


On Thu, Jun 27, 2013 at 7:08 PM, <gab@chromium.org> wrote:

> Reviewers: Ilya Sherman, jam, James Hawkins,
>
> Message:
> Ilya, PTAL.
>
> TBR jam for OWNER on side-effect in
> chrome/browser/automation/**testing_automation_provider.cc
>
> TBR jhawkins for OWNER on side-effect in
> chrome/browser/ui/webui/**options/import_data_handler.cc
>
> I own everything else already :)!
>
> Cheers!
> Gab
>
> Description:
> Remove Google Toolbar importer (aka google.com/bookmarks importer).
>
> (see bug for details on decision)
>
> BUG=243423
>
> Please review this at
https://codereview.chromium.**org/18120005/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/browser/automation/**testing_automation_provider.cc
>   M chrome/browser/first_run/**first_run.cc
>   M chrome/browser/first_run/**first_run_browsertest.cc
>   M chrome/browser/importer/**importer_data_types.h
>   M chrome/browser/importer/**importer_host.h
>   M chrome/browser/importer/**importer_host.cc
>   M chrome/browser/importer/**importer_list.h
>   M chrome/browser/importer/**importer_list.cc
>   M chrome/browser/importer/**importer_type.h
>   M chrome/browser/importer/**importer_type.cc
>   D chrome/browser/importer/**toolbar_importer.h
>   D chrome/browser/importer/**toolbar_importer.cc
>   D chrome/browser/importer/**toolbar_importer_unittest.cc
>   D chrome/browser/importer/**toolbar_importer_utils.h
>   D chrome/browser/importer/**toolbar_importer_utils.cc
>   D chrome/browser/importer/**toolbar_importer_utils_**browsertest.cc
>   M chrome/browser/ui/webui/**options/import_data_handler.cc
>   M chrome/chrome_browser.gypi
>   M chrome/chrome_tests.gypi
>   M chrome/chrome_tests_unit.gypi
>
>
>

[James Hawkins, 2013-06-28 02:23:16]

I didn't look at any of the other files, but for the file you TBRed for me, I do
not consider it a trivial-enough change to TBR.

[gab, 2013-06-28 02:50:08]

On 2013/06/28 02:23:16, James Hawkins wrote:
> I didn't look at any of the other files, but for the file you TBRed for me, I
do
> not consider it a trivial-enough change to TBR.

Ok, sorry about that, will wait for your review then. That changes in that file
only remove usage of the toolbar importer which is being deleted as part of this
CL.

[James Hawkins, 2013-06-28 03:08:15]

LGTM with nit.

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/import_data_handler.cc:112: importer_host_ = new
ExternalProcessImporterHost;
nit: Host();

[gab, 2013-06-28 04:23:15]

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/import_data_handler.cc:112: importer_host_ = new
ExternalProcessImporterHost;
On 2013/06/28 03:08:15, James Hawkins wrote:
> nit: Host();

What do you mean? You want:
importer_host_ = new ExternalProcessImporterHost();
? (i.e., explicitly adding "()"?)

This is not how any of these are constructed across the codebase. Do we have a
style-guide rule enforcing either?

[Avi (use Gerrit), 2013-06-28 04:31:19]

die importers, die!

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/importer/i...
File chrome/browser/importer/importer_type.h (right):

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/importer/i...
chrome/browser/importer/importer_type.h:30: // Value 4 was the (now deleted)
Firefox 2 profile importer.
"Value 4 was the (now deleted) Google Toolbar importer"

copy/paste typo

[gab, 2013-06-28 04:42:10]

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/importer/i...
File chrome/browser/importer/importer_type.h (right):

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/importer/i...
chrome/browser/importer/importer_type.h:30: // Value 4 was the (now deleted)
Firefox 2 profile importer.
On 2013/06/28 04:31:19, Avi wrote:
> "Value 4 was the (now deleted) Google Toolbar importer"
> 
> copy/paste typo

Whoopsy :), nice catch! Done!

[James Hawkins, 2013-06-28 15:23:58]

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/import_data_handler.cc:112: importer_host_ = new
ExternalProcessImporterHost;
On 2013/06/28 04:23:16, gab wrote:
> On 2013/06/28 03:08:15, James Hawkins wrote:
> > nit: Host();
> 
> What do you mean? You want:
> importer_host_ = new ExternalProcessImporterHost();
> ? (i.e., explicitly adding "()"?)
> 
> This is not how any of these are constructed across the codebase. Do we have a
> style-guide rule enforcing either?

Regardless of the rule, the code is currently internally inconsistent.  Line 67
includes the parentheses while this line does not.  Above all the style guide
requires files to be internally consistent.

[gab, 2013-06-28 15:40:28]

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/18120005/diff/13001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/import_data_handler.cc:112: importer_host_ = new
ExternalProcessImporterHost;
On 2013/06/28 15:23:59, James Hawkins wrote:
> On 2013/06/28 04:23:16, gab wrote:
> > On 2013/06/28 03:08:15, James Hawkins wrote:
> > > nit: Host();
> > 
> > What do you mean? You want:
> > importer_host_ = new ExternalProcessImporterHost();
> > ? (i.e., explicitly adding "()"?)
> > 
> > This is not how any of these are constructed across the codebase. Do we have
a
> > style-guide rule enforcing either?
> 
> Regardless of the rule, the code is currently internally inconsistent.  Line
67
> includes the parentheses while this line does not.  Above all the style guide
> requires files to be internally consistent. 

Done.

[jam, 2013-06-28 16:17:32]

On 2013/06/28 02:08:01, gab wrote:
> Ilya, PTAL.
> 
> TBR jam for OWNER on side-effect in
> chrome/browser/automation/testing_automation_provider.cc

lgtm

very nice :)

> 
> TBR jhawkins for OWNER on side-effect in
> chrome/browser/ui/webui/options/import_data_handler.cc
> 
> I own everything else already :)! 
> 
> Cheers!
> Gab

[James Hawkins, 2013-06-28 17:52:26]

SLGTM

[Ilya Sherman, 2013-06-28 20:35:15]

Thanks!  LGTM with one nit:

https://codereview.chromium.org/18120005/diff/4004/chrome/browser/importer/im...
File chrome/browser/importer/importer_list.h (right):

https://codereview.chromium.org/18120005/diff/4004/chrome/browser/importer/im...
chrome/browser/importer/importer_list.h:24: explicit ImporterList();
nit: No longer a need for "explicit"

[commit-bot: I haz the power, 2013-07-02 01:42:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/18120005/47001

[gab, 2013-07-02 01:43:58]

Thanks, done!

https://codereview.chromium.org/18120005/diff/4004/chrome/browser/importer/im...
File chrome/browser/importer/importer_list.h (right):

https://codereview.chromium.org/18120005/diff/4004/chrome/browser/importer/im...
chrome/browser/importer/importer_list.h:24: explicit ImporterList();
On 2013/06/28 20:35:15, Ilya Sherman wrote:
> nit: No longer a need for "explicit"

Done.

[commit-bot: I haz the power, 2013-07-02 04:18:36]

Change committed as 209610