Move password_form_conversion_utils out of //content into //autofill.

Move password_form_conversion_utils out of //content into //autofill.

This is a necessary step in moving PasswordForm from //content to //autofill.

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

[blundell, 2013-08-30 14:03:23]

Jói,

Before I send this CL out for full review, I'd appreciate your input on what to
do re: moving a browsertest to live in //components. My thinking is to build it
as part of browser_tests in the short term coupled with a bug to bring up a
components_browsertests target (see the comment). Do you agree?

Thanks,

Colin

https://codereview.chromium.org/23622013/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/23622013/diff/1/chrome/chrome_tests.gypi#newc...
chrome/chrome_tests.gypi:1168: # TODO(blundell): Is this the right place to put
this browsertest in
Jói, I'd love your input here.

[Jói, 2013-08-30 14:08:16]

LGTM

https://codereview.chromium.org/23622013/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/23622013/diff/1/chrome/chrome_tests.gypi#newc...
chrome/chrome_tests.gypi:1168: # TODO(blundell): Is this the right place to put
this browsertest in
On 2013/08/30 14:03:23, blundell wrote:
> Jói, I'd love your input here.

Yes, this seems like the right way to do it.

[blundell, 2013-08-30 15:44:28]

Thanks, Jói! I'll create that bug and reference it in chrome_tests.gypi.

jam: for //content
isherman: for //components/autofill

This CL is dependent on https://codereview.chromium.org/23564006/.

[jam, 2013-08-30 16:30:48]

lgtm

[Ilya Sherman, 2013-08-30 21:52:13]

Hmm, should this perhaps be moved into something //components/passwords instead?
 This isn't really directly related to Autofill, unless we want to completely
merge the passwords and Autofill components throughout.

[Ilya Sherman, 2013-08-30 21:55:47]

https://codereview.chromium.org/23622013/diff/1/components/autofill/content/DEPS
File components/autofill/content/DEPS (right):

https://codereview.chromium.org/23622013/diff/1/components/autofill/content/D...
components/autofill/content/DEPS:9: '.*_[a-z]*test\.cc': [
Could you narrow this rule to allow only the specific tests that currently have
this dependency, and annotate with a TODO + bug to remove this?

[blundell, 2013-09-02 10:51:56]

On 2013/08/30 21:52:13, Ilya Sherman wrote:
> Hmm, should this perhaps be moved into something //components/passwords
instead?
>  This isn't really directly related to Autofill, unless we want to completely
> merge the passwords and Autofill components throughout.

Ilya, I think that what you're suggesting is the right way to go. I hadn't
before checked for all the places that content::PasswordForm is used, and it
turns out that it's used from code that has nothing at all to do with autofill
(e.g., //chrome/browser/importer).

Jói, are you OK with adding a new component to hold PasswordForm and its
conversion utils? This could be //components/password or //components/passwords.

[blundell, 2013-09-02 10:52:56]

https://codereview.chromium.org/23622013/diff/1/components/autofill/content/DEPS
File components/autofill/content/DEPS (right):

https://codereview.chromium.org/23622013/diff/1/components/autofill/content/D...
components/autofill/content/DEPS:9: '.*_[a-z]*test\.cc': [
This is the DEPS file for //components/autofill/content, so there is no problem
with its test code using //content/public/test unless there is something I'm
missing.

On 2013/08/30 21:55:47, Ilya Sherman wrote:
> Could you narrow this rule to allow only the specific tests that currently
have
> this dependency, and annotate with a TODO + bug to remove this?

[Jói, 2013-09-02 10:58:25]

> Jói, are you OK with adding a new component to hold PasswordForm and its
> conversion utils? This could be //components/password or
> //components/passwords.

Yes, sounds good to me.

On Mon, Sep 2, 2013 at 12:51 PM,  <blundell@chromium.org> wrote:
> On 2013/08/30 21:52:13, Ilya Sherman wrote:
>>
>> Hmm, should this perhaps be moved into something //components/passwords
>
> instead?
>>
>>   This isn't really directly related to Autofill, unless we want to
>> completely
>> merge the passwords and Autofill components throughout.
>
>
> Ilya, I think that what you're suggesting is the right way to go. I hadn't
> before checked for all the places that content::PasswordForm is used, and it
> turns out that it's used from code that has nothing at all to do with
> autofill
> (e.g., //chrome/browser/importer).
>
> Jói, are you OK with adding a new component to hold PasswordForm and its
> conversion utils? This could be //components/password or
> //components/passwords.
>
> https://codereview.chromium.org/23622013/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2013-09-02 12:26:58]

On 2013/09/02 10:51:56, blundell wrote:
> On 2013/08/30 21:52:13, Ilya Sherman wrote:
> > Hmm, should this perhaps be moved into something //components/passwords
> instead?
> >  This isn't really directly related to Autofill, unless we want to
completely
> > merge the passwords and Autofill components throughout.
> 
> Ilya, I think that what you're suggesting is the right way to go. I hadn't
> before checked for all the places that content::PasswordForm is used, and it
> turns out that it's used from code that has nothing at all to do with autofill
> (e.g., //chrome/browser/importer).
> 
> Jói, are you OK with adding a new component to hold PasswordForm and its
> conversion utils? This could be //components/password or
//components/passwords.

+gcasto@ as reviewer

On reflection, I think that this organization would be unsatisfactory for
gcasto@: crbug.com/263121 states that one goal of the work is that "For password
generation and eventually password autofill, we want to be able to depend on
autofill::FormData so that we can send pings to the autofill servers." If
PasswordForm lived in its own //components/passwords component that
//components/autofill would necessarily depend on, then //components/passwords
(and specifically PasswordForm) could not reference //components/autofill.

Garrett, can you confirm that this organization would not satisfy your
objectives?

[Garrett Casto, 2013-09-03 16:19:32]

On 2013/09/02 12:26:58, blundell wrote:
> On 2013/09/02 10:51:56, blundell wrote:
> > On 2013/08/30 21:52:13, Ilya Sherman wrote:
> > > Hmm, should this perhaps be moved into something //components/passwords
> > instead?
> > >  This isn't really directly related to Autofill, unless we want to
> completely
> > > merge the passwords and Autofill components throughout.
> > 
> > Ilya, I think that what you're suggesting is the right way to go. I hadn't
> > before checked for all the places that content::PasswordForm is used, and it
> > turns out that it's used from code that has nothing at all to do with
autofill
> > (e.g., //chrome/browser/importer).
> > 
> > Jói, are you OK with adding a new component to hold PasswordForm and its
> > conversion utils? This could be //components/password or
> //components/passwords.
> 
> +gcasto@ as reviewer
> 
> On reflection, I think that this organization would be unsatisfactory for
> gcasto@: crbug.com/263121 states that one goal of the work is that "For
password
> generation and eventually password autofill, we want to be able to depend on
> autofill::FormData so that we can send pings to the autofill servers." If
> PasswordForm lived in its own //components/passwords component that
> //components/autofill would necessarily depend on, then //components/passwords
> (and specifically PasswordForm) could not reference //components/autofill.
> 
> Garrett, can you confirm that this organization would not satisfy your
> objectives?

Yeah, being able to depend on //components/autofill from wherever PasswordForm
resides was the reason I started trying to move PasswordForm in the first place.
Password autofill and autofill are tied together in strange ways currently, and
it would be nice to separate them out in some way eventually, but it will
probably take some effort to determine how to split them appropriately. I'd vote
for putting everything in //components/autofill for now and file a bug to
refactor.

[Ilya Sherman, 2013-09-03 22:39:17]

Hokay, LGTM.

https://codereview.chromium.org/23622013/diff/1/components/autofill/content/DEPS
File components/autofill/content/DEPS (right):

https://codereview.chromium.org/23622013/diff/1/components/autofill/content/D...
components/autofill/content/DEPS:9: '.*_[a-z]*test\.cc': [
Ah, I mistook this for a temporary exception.  Never mind :)

On 2013/09/02 10:52:56, blundell wrote:
> This is the DEPS file for //components/autofill/content, so there is no
problem
> with its test code using //content/public/test unless there is something I'm
> missing.
> 
> On 2013/08/30 21:55:47, Ilya Sherman wrote:
> > Could you narrow this rule to allow only the specific tests that currently
> have
> > this dependency, and annotate with a TODO + bug to remove this?
>

[Garrett Casto, 2013-09-04 01:03:54]

lgtm

[blundell, 2013-09-04 06:06:54]

+thakis for chrome/

[Nico, 2013-09-04 15:32:40]

lgtm

[commit-bot: I haz the power, 2013-09-04 16:36:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/23622013/13001

[commit-bot: I haz the power, 2013-09-04 18:31:56]

Change committed as 221229

[blundell, 2013-09-05 08:49:51]

On 2013/09/03 16:19:32, Garrett Casto wrote:
> On 2013/09/02 12:26:58, blundell wrote:
> > On 2013/09/02 10:51:56, blundell wrote:
> > > On 2013/08/30 21:52:13, Ilya Sherman wrote:
> > > > Hmm, should this perhaps be moved into something //components/passwords
> > > instead?
> > > >  This isn't really directly related to Autofill, unless we want to
> > completely
> > > > merge the passwords and Autofill components throughout.
> > > 
> > > Ilya, I think that what you're suggesting is the right way to go. I hadn't
> > > before checked for all the places that content::PasswordForm is used, and
it
> > > turns out that it's used from code that has nothing at all to do with
> autofill
> > > (e.g., //chrome/browser/importer).
> > > 
> > > Jói, are you OK with adding a new component to hold PasswordForm and its
> > > conversion utils? This could be //components/password or
> > //components/passwords.
> > 
> > +gcasto@ as reviewer
> > 
> > On reflection, I think that this organization would be unsatisfactory for
> > gcasto@: crbug.com/263121 states that one goal of the work is that "For
> password
> > generation and eventually password autofill, we want to be able to depend on
> > autofill::FormData so that we can send pings to the autofill servers." If
> > PasswordForm lived in its own //components/passwords component that
> > //components/autofill would necessarily depend on, then
//components/passwords
> > (and specifically PasswordForm) could not reference //components/autofill.
> > 
> > Garrett, can you confirm that this organization would not satisfy your
> > objectives?
> 
> Yeah, being able to depend on //components/autofill from wherever PasswordForm
> resides was the reason I started trying to move PasswordForm in the first
place.
> Password autofill and autofill are tied together in strange ways currently,
and
> it would be nice to separate them out in some way eventually, but it will
> probably take some effort to determine how to split them appropriately. I'd
vote
> for putting everything in //components/autofill for now and file a bug to
> refactor.

Filed crbug.com/285683.