Move forms/ out of webkit/.

Move forms/ out of webkit/.

The motivation for this CL is to move PasswordForm and friends (which are totally unrelated to WebKit) out of webkit/ and into a target that platforms that do not use WebKit (such as iOS) can logically depend on.

As such, this CL does three things:

1. Separates the WebKit-related code in webkit/forms from the
non-WebKit-related code. Concretely, this means having the WebKit::WebFormElement->PasswordForm conversion function in its own file.

2. Moves the core, non-WebKit-related forms code to chrome/common and content/public/common depending on where its usage points are.

3. Moves the above-mentioned conversion function to content/public/renderer. It cannot stay in webkit/ as it (now) has a dependency on content/, and as it is used only in chrome/renderer and content/renderer, this is a good place for it.

The rest of this CL is churn due to namespace, file location, and GYP target changes.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160280

[blundell, 2012-09-27 13:40:04]

Stuart,

The non-trivial parts of this CL are:
1. The extraction of password_form_fill_data from password_form_dom_manager.
2. The removal of the "forms" target, replaced by building the non-WebKit
related forms files as part of content_common and the WebKit-related files as
part of content_renderer.

It is hard to be sure that in doing (2), I have preserved all of the
dependencies exactly as they should be. A component build is successful. Tests
are passing except for browser_tests failures on mac_asan and chromeos, which
I'm trying to figure out if they are related or not.

[stuartmorgan, 2012-10-02 12:17:51]

LGTM with nits as a first pass.
+jam for full review and content/ and chrome/ OWNERS, +darin for webkit/ OWNERS
(and to make sure this matches what you had in mind from our discussions)

+dhollowa and isherman for the WATCHLIST question

https://chromiumcodereview.appspot.com/11000016/diff/5004/WATCHLISTS
File WATCHLISTS (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/WATCHLISTS#newcode54
WATCHLISTS:54: 'content/public/renderer/',
I suspect that people watching autofill don't want all the noise from other
changes to these directories.

dhollowa, isherman: How do you want to handle this?

https://chromiumcodereview.appspot.com/11000016/diff/5004/chrome/browser/auto...
File chrome/browser/autofill/form_structure_unittest.cc (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/chrome/browser/auto...
chrome/browser/autofill/form_structure_unittest.cc:31: for
(std::vector<content::FormField>::const_iterator iter =
Since we have a using directive, and this is in the content namespace anyway, we
shouldn't need the content:: here.

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
File content/public/common/password_form_fill_data.cc (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
content/public/common/password_form_fill_data.cc:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
Keep 2011

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
content/public/common/password_form_fill_data.cc:18: void
initPasswordFormFillData(const PasswordForm& form_on_page,
s/init/Init/

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
File content/public/common/password_form_fill_data.h (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
content/public/common/password_form_fill_data.h:42: CONTENT_EXPORT void
initPasswordFormFillData(
s/init/Init/

Should this be a static method on PasswordFormFillData instead?

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/rend...
File content/public/renderer/password_form_dom_manager.cc (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/rend...
content/public/renderer/password_form_dom_manager.cc:16:
scoped_ptr<content::PasswordForm> initPasswordFormFromWebPasswordForm(
s/init/Init/

[jam, 2012-10-02 16:18:13]

(didn't have time to scan everything, went through content\public first). please
make sure everything in content\ is used by code in content

http://codereview.chromium.org/11000016/diff/5004/content/public/common/form_...
File content/public/common/form_data_predictions.h (right):

http://codereview.chromium.org/11000016/diff/5004/content/public/common/form_...
content/public/common/form_data_predictions.h:18: struct CONTENT_EXPORT
FormDataPredictions {
doesn't look like this is used in content, so it should be in chrome/common

http://codereview.chromium.org/11000016/diff/5004/content/public/common/form_...
File content/public/common/form_field_predictions.cc (right):

http://codereview.chromium.org/11000016/diff/5004/content/public/common/form_...
content/public/common/form_field_predictions.cc:5: #include
"content/public/common/form_field_predictions.h"
it doesn't look like this is used in content, so it should be in chrome/common

http://codereview.chromium.org/11000016/diff/5004/content/public/common/passw...
File content/public/common/password_form_fill_data.cc (right):

http://codereview.chromium.org/11000016/diff/5004/content/public/common/passw...
content/public/common/password_form_fill_data.cc:18: void
initPasswordFormFillData(const PasswordForm& form_on_page,
static methods defined in headers in content/public should be implemented in
content/common (just like the webkit api)

if you absolutely need to have a cc file in
content/public/common/password_form_fill_data.cc for the constructor/destructor
above (to avoid a clang error), then put the static method in
content/common/password_form_fill_data_impl.cc

http://codereview.chromium.org/11000016/diff/5004/content/public/common/passw...
File content/public/common/password_form_fill_data.h (right):

http://codereview.chromium.org/11000016/diff/5004/content/public/common/passw...
content/public/common/password_form_fill_data.h:42: CONTENT_EXPORT void
initPasswordFormFillData(
On 2012/10/02 12:17:51, stuartmorgan wrote:
> s/init/Init/
> 
> Should this be a static method on PasswordFormFillData instead?

structs don't have methods per style guide, so you'd need to make it a class
instead which would be cumbersome. having this be a static method is fine.

http://codereview.chromium.org/11000016/diff/5004/content/public/renderer/pas...
File content/public/renderer/password_form_dom_manager.h (right):

http://codereview.chromium.org/11000016/diff/5004/content/public/renderer/pas...
content/public/renderer/password_form_dom_manager.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
looks like svn history is lost?

http://codereview.chromium.org/11000016/diff/5004/content/public/renderer/pas...
content/public/renderer/password_form_dom_manager.h:14: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebPasswordFormData.h"
please forward declare as much as possible

http://codereview.chromium.org/11000016/diff/5004/content/public/renderer/pas...
content/public/renderer/password_form_dom_manager.h:25: const
WebKit::WebFormElement& form);
the content api, like the webkit api, doesn't use a class for scoping a static
method. just keep this as a static method in the content namespace. the cc file
should be in content/renderer/password_form_dom_manager.h

[Ilya Sherman, 2012-10-02 20:12:29]

http://codereview.chromium.org/11000016/diff/5004/WATCHLISTS
File WATCHLISTS (right):

http://codereview.chromium.org/11000016/diff/5004/WATCHLISTS#newcode54
WATCHLISTS:54: 'content/public/renderer/',
On 2012/10/02 12:17:51, stuartmorgan wrote:
> I suspect that people watching autofill don't want all the noise from other
> changes to these directories.
> 
> dhollowa, isherman: How do you want to handle this?

I think these lines could just be:
content/public/common/form_
content/public/common/password_
content/public/renderer/password_

[darin (slow to review), 2012-10-02 21:08:34]

LGTM for webkit/

[blundell, 2012-10-03 15:06:07]

Thanks for the reviews, everyone. PTAL!

https://chromiumcodereview.appspot.com/11000016/diff/5004/WATCHLISTS
File WATCHLISTS (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/WATCHLISTS#newcode54
WATCHLISTS:54: 'content/public/renderer/',
Great, thanks. Done.

On 2012/10/02 20:12:29, Ilya Sherman wrote:
> On 2012/10/02 12:17:51, stuartmorgan wrote:
> > I suspect that people watching autofill don't want all the noise from other
> > changes to these directories.
> > 
> > dhollowa, isherman: How do you want to handle this?
> 
> I think these lines could just be:
> content/public/common/form_
> content/public/common/password_
> content/public/renderer/password_

https://chromiumcodereview.appspot.com/11000016/diff/5004/chrome/browser/auto...
File chrome/browser/autofill/form_structure_unittest.cc (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/chrome/browser/auto...
chrome/browser/autofill/form_structure_unittest.cc:31: for
(std::vector<content::FormField>::const_iterator iter =
On 2012/10/02 12:17:51, stuartmorgan wrote:
> Since we have a using directive, and this is in the content namespace anyway,
we
> shouldn't need the content:: here.

Done.

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
File content/public/common/form_data_predictions.h (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
content/public/common/form_data_predictions.h:18: struct CONTENT_EXPORT
FormDataPredictions {
On 2012/10/02 16:18:14, John Abd-El-Malek wrote:
> doesn't look like this is used in content, so it should be in chrome/common

Done.

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
File content/public/common/form_field_predictions.cc (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
content/public/common/form_field_predictions.cc:5: #include
"content/public/common/form_field_predictions.h"
On 2012/10/02 16:18:14, John Abd-El-Malek wrote:
> it doesn't look like this is used in content, so it should be in chrome/common

Done.

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
File content/public/common/password_form_fill_data.cc (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
content/public/common/password_form_fill_data.cc:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
On 2012/10/02 12:17:51, stuartmorgan wrote:
> Keep 2011

Done.

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
content/public/common/password_form_fill_data.cc:18: void
initPasswordFormFillData(const PasswordForm& form_on_page,
Noticed on re-inspection that this code is also used only in chrome/. Moved to
chrome/common.

On 2012/10/02 16:18:14, John Abd-El-Malek wrote:
> static methods defined in headers in content/public should be implemented in
> content/common (just like the webkit api)
> 
> if you absolutely need to have a cc file in
> content/public/common/password_form_fill_data.cc for the
constructor/destructor
> above (to avoid a clang error), then put the static method in
> content/common/password_form_fill_data_impl.cc

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
File content/public/common/password_form_fill_data.h (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/comm...
content/public/common/password_form_fill_data.h:42: CONTENT_EXPORT void
initPasswordFormFillData(
On 2012/10/02 12:17:51, stuartmorgan wrote:
> s/init/Init/
> 
> Should this be a static method on PasswordFormFillData instead?

Done.

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/rend...
File content/public/renderer/password_form_dom_manager.cc (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/rend...
content/public/renderer/password_form_dom_manager.cc:16:
scoped_ptr<content::PasswordForm> initPasswordFormFromWebPasswordForm(
On 2012/10/02 12:17:51, stuartmorgan wrote:
> s/init/Init/

Done.

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/rend...
File content/public/renderer/password_form_dom_manager.h (right):

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/rend...
content/public/renderer/password_form_dom_manager.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Fixed. Note that the history of password_form_dom_manager is attached to
password_form_fill_data, as those files are more similar content-wise to the old
password_form_dom_manager files than these files are.

On 2012/10/02 16:18:14, John Abd-El-Malek wrote:
> looks like svn history is lost?

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/rend...
content/public/renderer/password_form_dom_manager.h:14: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebPasswordFormData.h"
On 2012/10/02 16:18:14, John Abd-El-Malek wrote:
> please forward declare as much as possible

Done.

https://chromiumcodereview.appspot.com/11000016/diff/5004/content/public/rend...
content/public/renderer/password_form_dom_manager.h:25: const
WebKit::WebFormElement& form);
On 2012/10/02 16:18:14, John Abd-El-Malek wrote:
> the content api, like the webkit api, doesn't use a class for scoping a static
> method. just keep this as a static method in the content namespace. the cc
file
> should be in content/renderer/password_form_dom_manager.h

Done. Also gave the file what seems like a more appropriate name at this point.

[jam, 2012-10-03 16:43:49]

looking closer, it seems that form_data.h and form_field.h aren't used in
content at all? I see they're included in render_view_impl.cc but they're not
actually used...

https://codereview.chromium.org/11000016/diff/36211/content/public/renderer/d...
File content/public/renderer/document_state.cc (right):

https://codereview.chromium.org/11000016/diff/36211/content/public/renderer/d...
content/public/renderer/document_state.cc:36: scoped_ptr<content::PasswordForm>
data) {
ditto

https://codereview.chromium.org/11000016/diff/36211/content/public/renderer/d...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/11000016/diff/36211/content/public/renderer/d...
content/public/renderer/document_state.h:172: content::PasswordForm*
password_form_data() const {
nit: get rid of content:: here and below

[blundell, 2012-10-03 21:17:54]

You are correct: form_field and form_data are used only in chrome/, and are now
in chrome/common/. PTAL!

https://chromiumcodereview.appspot.com/11000016/diff/36211/content/public/ren...
File content/public/renderer/document_state.cc (right):

https://chromiumcodereview.appspot.com/11000016/diff/36211/content/public/ren...
content/public/renderer/document_state.cc:36: scoped_ptr<content::PasswordForm>
data) {
On 2012/10/03 16:43:49, John Abd-El-Malek wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/11000016/diff/36211/content/public/ren...
File content/public/renderer/document_state.h (right):

https://chromiumcodereview.appspot.com/11000016/diff/36211/content/public/ren...
content/public/renderer/document_state.h:172: content::PasswordForm*
password_form_data() const {
On 2012/10/03 16:43:49, John Abd-El-Malek wrote:
> nit: get rid of content:: here and below

Done.

[jam, 2012-10-04 06:18:11]

https://codereview.chromium.org/11000016/diff/41208/chrome/common/form_field_...
File chrome/common/form_field_predictions.h (right):

https://codereview.chromium.org/11000016/diff/41208/chrome/common/form_field_...
chrome/common/form_field_predictions.h:13: namespace chrome {
nit: no need for chrome namespace. code in src/chrome generally doesn't need to
be in a namespace; only code in modules which chrome depends on need to be in a
namespace (this was discussed on chromium-dev a while back)

https://codereview.chromium.org/11000016/diff/41208/content/browser/renderer_...
File content/browser/renderer_host/test_render_view_host.cc (right):

https://codereview.chromium.org/11000016/diff/41208/content/browser/renderer_...
content/browser/renderer_host/test_render_view_host.cc:24: using
content::PasswordForm;
nit: not needed (can also take the one above out)

[blundell, 2012-10-04 16:23:21]

Thanks for the review. PTAL!

http://codereview.chromium.org/11000016/diff/41208/chrome/common/form_field_p...
File chrome/common/form_field_predictions.h (right):

http://codereview.chromium.org/11000016/diff/41208/chrome/common/form_field_p...
chrome/common/form_field_predictions.h:13: namespace chrome {
Thanks. Done for all of the files that I brought into chrome/. I had to rename
form_field.*, as chrome/browser/autofill/form_field.* already exists. Chose
form_field_data to parallel form_data.

On 2012/10/04 06:18:12, John Abd-El-Malek wrote:
> nit: no need for chrome namespace. code in src/chrome generally doesn't need
to
> be in a namespace; only code in modules which chrome depends on need to be in
a
> namespace (this was discussed on chromium-dev a while back)

http://codereview.chromium.org/11000016/diff/41208/content/browser/renderer_h...
File content/browser/renderer_host/test_render_view_host.cc (right):

http://codereview.chromium.org/11000016/diff/41208/content/browser/renderer_h...
content/browser/renderer_host/test_render_view_host.cc:24: using
content::PasswordForm;
On 2012/10/04 06:18:12, John Abd-El-Malek wrote:
> nit: not needed (can also take the one above out)

Done.

[jam, 2012-10-04 19:19:12]

lgtm

[commit-bot: I haz the power, 2012-10-04 20:02:53]

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

[commit-bot: I haz the power, 2012-10-05 01:29:19]

Change committed as 160280