Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 3149005: Fix for: Autofill on Windows does not work for the first autofillable page en... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 9 months ago by GeorgeY
Modified:
3 years, 11 months ago
Reviewers:
James Hawkins, dhollowa
CC:
chromium-reviews, ben+cc_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 12

Patch Set 4 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -2 lines) Patch
M chrome/browser/autofill/autofill_manager.h View 1 2 3 4 chunks +10 lines, -2 lines 0 comments Download
M chrome/browser/autofill/autofill_manager.cc View 1 2 3 4 chunks +20 lines, -0 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 9 (0 generated)
GeorgeY
Bug, I was telling you about yesterday.
4 years, 9 months ago (2010-08-11 20:46:24 UTC) #1
GeorgeY
4 years, 9 months ago (2010-08-16 21:26:32 UTC) #2
dhollowa
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 ...
4 years, 9 months ago (2010-08-16 22:04:03 UTC) #3
GeorgeY
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): ...
4 years, 9 months ago (2010-08-17 17:44:36 UTC) #4
dhollowa
LGTM.
4 years, 9 months ago (2010-08-25 18:12:32 UTC) #5
James Hawkins
This change is incorrect and needs to be reverted. Please include me on any design-changing ...
4 years, 9 months ago (2010-08-25 21:16:35 UTC) #6
GeorgeY
On 2010/08/25 21:16:35, James Hawkins wrote: > This change is incorrect and needs to be ...
4 years, 9 months ago (2010-08-25 23:41:47 UTC) #7
James Hawkins
On 2010/08/25 23:41:47, GeorgeY wrote: > On 2010/08/25 21:16:35, James Hawkins wrote: > > This ...
4 years, 9 months ago (2010-08-25 23:49:53 UTC) #8
GeorgeY
4 years, 9 months ago (2010-08-25 23:54:22 UTC) #9
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld ec887be