Fix for: Autofill on Windows does not work for the first autofillable page en...

Fix for: Autofill on Windows does not work for the first autofillable page encountered.
BUG=51831
TEST=In the bug.

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

[GeorgeY, 2010-08-11 20:46:24]

Bug, I was telling you about yesterday.

[dhollowa, 2010-08-16 22:04:03]

http://codereview.chromium.org/3149005/diff/9001/10002
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/3149005/diff/9001/10002#newcode39
chrome/browser/autofill/autofill_manager.h:39: public
PersonalDataManager::Observer {
It'd be better to land the delayed |FormsSeen| fix separately from the disabled
downloading stuff.

http://codereview.chromium.org/3149005/diff/9001/10002#newcode107
chrome/browser/autofill/autofill_manager.h:107: void
set_disable_download_manager_requests(bool value) {
Please move this into private section and use FRIEND_TEST... to access.  Also,
please add to comment information about why this is here, i.e. to disable
network access for non-branded builds and unit tests alike.

http://codereview.chromium.org/3149005/diff/9001/10002#newcode200
chrome/browser/autofill/autofill_manager.h:200: // Should be set to true in
AutoFillManagerTest and other tests, false in
nit: Newline before comment.

http://codereview.chromium.org/3149005/diff/9001/10002#newcode203
chrome/browser/autofill/autofill_manager.h:203: bool
disable_download_manager_requests_;
I was expecting the addition of this code to lead to the removal of
DISABLED_REQUEST_URL in autofill_download.cc:18.  Is there a reason why we're
not using this new mechanism for non-branded builds as well?

http://codereview.chromium.org/3149005/diff/9001/10003
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/3149005/diff/9001/10003#newcode243
chrome/browser/autofill/autofill_manager_unittest.cc:243: test_personal_data_ =
NULL;
Please change to scoped_ptr<> (below), and use reset() here as well.

http://codereview.chromium.org/3149005/diff/9001/10003#newcode292
chrome/browser/autofill/autofill_manager_unittest.cc:292:
scoped_refptr<TestPersonalDataManager> test_personal_data_;
Please change to scoped_ptr<>.

[GeorgeY, 2010-08-17 17:44:36]

Separate cl follows, this will be updated when other is submitted.

http://codereview.chromium.org/3149005/diff/9001/10002
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/3149005/diff/9001/10002#newcode39
chrome/browser/autofill/autofill_manager.h:39: public
PersonalDataManager::Observer {
On 2010/08/16 22:04:03, dhollowa wrote:
> It'd be better to land the delayed |FormsSeen| fix separately from the
disabled
> downloading stuff.

Done.

http://codereview.chromium.org/3149005/diff/9001/10002#newcode107
chrome/browser/autofill/autofill_manager.h:107: void
set_disable_download_manager_requests(bool value) {
On 2010/08/16 22:04:03, dhollowa wrote:
> Please move this into private section and use FRIEND_TEST... to access.  Also,
> please add to comment information about why this is here, i.e. to disable
> network access for non-branded builds and unit tests alike.

Done, though reluctantly: that way everybody who add relevant test will need to
add FRIEND_TEST line here.

http://codereview.chromium.org/3149005/diff/9001/10002#newcode200
chrome/browser/autofill/autofill_manager.h:200: // Should be set to true in
AutoFillManagerTest and other tests, false in
On 2010/08/16 22:04:03, dhollowa wrote:
> nit: Newline before comment.

Done.

http://codereview.chromium.org/3149005/diff/9001/10002#newcode203
chrome/browser/autofill/autofill_manager.h:203: bool
disable_download_manager_requests_;
On 2010/08/16 22:04:03, dhollowa wrote:
> I was expecting the addition of this code to lead to the removal of
> DISABLED_REQUEST_URL in autofill_download.cc:18.  Is there a reason why we're
> not using this new mechanism for non-branded builds as well?
No the other code is related to actual URL, which is either internal or disabled
(but could be changed in other people implementations if they conform to data
format)

http://codereview.chromium.org/3149005/diff/9001/10003
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/3149005/diff/9001/10003#newcode243
chrome/browser/autofill/autofill_manager_unittest.cc:243: test_personal_data_ =
NULL;
On 2010/08/16 22:04:03, dhollowa wrote:
> Please change to scoped_ptr<> (below), and use reset() here as well.

Must be refcounted - see below.

http://codereview.chromium.org/3149005/diff/9001/10003#newcode292
chrome/browser/autofill/autofill_manager_unittest.cc:292:
scoped_refptr<TestPersonalDataManager> test_personal_data_;
On 2010/08/16 22:04:03, dhollowa wrote:
> Please change to scoped_ptr<>.

No, RenderViewHostTestHarness is refcounted and should be used through the
scoped_refptr<>

[dhollowa, 2010-08-25 18:12:32]

LGTM.

[James Hawkins, 2010-08-25 21:16:35]

This change is incorrect and needs to be reverted. Please include me on any
design-changing AF CL in the future.

a) Parsing forms has nothing to do with profile data.
b) autofill_manager.cc:163-165 need to be removed.  I added those checks in an
effort to not hit the autofill server in the case where a user has no profiles,
thus we wouldn't be able to fill the form anyway. This assumption is incorrect
as the user can add a profile after the page has loaded.

[GeorgeY, 2010-08-25 23:41:47]

On 2010/08/25 21:16:35, James Hawkins wrote:
> This change is incorrect and needs to be reverted. Please include me on any
> design-changing AF CL in the future.
> 
> a) Parsing forms has nothing to do with profile data.
> b) autofill_manager.cc:163-165 need to be removed.  I added those checks in an
> effort to not hit the autofill server in the case where a user has no
profiles,
> thus we wouldn't be able to fill the form anyway. This assumption is incorrect
> as the user can add a profile after the page has loaded.
I do not agree:
1) This is a not design changing CL.
2) You were included on this cl for the last 2 weeks, I removed you today.
3) We initiate load of the data when we encounter first page with parsable
forms.
4) By that time is too late for forms on the page, so unless we delay parsing
they will not be processed.
5) As an alternative we could *always* load profile data on startup, not on the
first encounter, but that has its own negative consequences too (such as slower
startup).

[James Hawkins, 2010-08-25 23:49:53]

On 2010/08/25 23:41:47, GeorgeY wrote:
> On 2010/08/25 21:16:35, James Hawkins wrote:
> > This change is incorrect and needs to be reverted. Please include me on any
> > design-changing AF CL in the future.
> > 
> > a) Parsing forms has nothing to do with profile data.
> > b) autofill_manager.cc:163-165 need to be removed.  I added those checks in
an
> > effort to not hit the autofill server in the case where a user has no
> profiles,
> > thus we wouldn't be able to fill the form anyway. This assumption is
incorrect
> > as the user can add a profile after the page has loaded.
> I do not agree:
> 1) This is a not design changing CL.

I'm not going to argue about this.

> 2) You were included on this cl for the last 2 weeks, I removed you today.
> 3) We initiate load of the data when we encounter first page with parsable
> forms.
> 4) By that time is too late for forms on the page, so unless we delay parsing
> they will not be processed.

I don't follow your logic here.  Profile data is not used to parse forms on a
page.

> 5) As an alternative we could *always* load profile data on startup, not on
the
> first encounter, but that has its own negative consequences too (such as
slower
> startup).

We don't need profile data for FormsSeen; we need profile data for
GetAutoFillSuggestions.

We already load profile data on startup:

AutoFillManager() -> Profile::GetPersonalDataManager() ->
PersonalDataManager::Init() -> loads profiles.

Yes it's possible that profile data might not be available for
GetAutoFillSuggestions, but there's nothing we can do about that anyway.

[GeorgeY, 2010-08-25 23:54:22]

On 2010/08/25 23:49:53, James Hawkins wrote:
> On 2010/08/25 23:41:47, GeorgeY wrote:
> > On 2010/08/25 21:16:35, James Hawkins wrote:
> > > This change is incorrect and needs to be reverted. Please include me on
any
> > > design-changing AF CL in the future.
> > > 
> > > a) Parsing forms has nothing to do with profile data.
> > > b) autofill_manager.cc:163-165 need to be removed.  I added those checks
in
> an
> > > effort to not hit the autofill server in the case where a user has no
> > profiles,
> > > thus we wouldn't be able to fill the form anyway. This assumption is
> incorrect
> > > as the user can add a profile after the page has loaded.
> > I do not agree:
> > 1) This is a not design changing CL.
> 
> I'm not going to argue about this.
> 
> > 2) You were included on this cl for the last 2 weeks, I removed you today.
> > 3) We initiate load of the data when we encounter first page with parsable
> > forms.
> > 4) By that time is too late for forms on the page, so unless we delay
parsing
> > they will not be processed.
> 
> I don't follow your logic here.  Profile data is not used to parse forms on a
> page.
> 
> > 5) As an alternative we could *always* load profile data on startup, not on
> the
> > first encounter, but that has its own negative consequences too (such as
> slower
> > startup).
> 
> We don't need profile data for FormsSeen; we need profile data for
> GetAutoFillSuggestions.
> 
> We already load profile data on startup:
> 
> AutoFillManager() -> Profile::GetPersonalDataManager() ->
> PersonalDataManager::Init() -> loads profiles.
> 
> Yes it's possible that profile data might not be available for
> GetAutoFillSuggestions, but there's nothing we can do about that anyway.
Oh, Sorry misread your previous e-mail, yes deleting the lines checking for
profile should work fine if profiles are not needed for parsing.