Never allow empty preview entries. As some data is always present (we drop co...

Never allow empty preview entries. As some data is always present (we drop completely empty profiles),
display this data in preview if name and first line of address is not available.
TEST=in the bug + unit-test
BUG=50320
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=63806

[James Hawkins, 2010-10-15 07:06:18]

AutoFillProfile::PreviewSummary should be removed and the platform-specific
dialog implementations should just call
AutoFillProfile::CreateInferredLabels(profiles), where |profiles| is the entire
list of stored profiles.  The mac implementation (somewhat) already does this.
AutoFillProfile::AdjustInferredLabels really needs to go in favor of using
CreateInferredLabels() directly.

[GeorgeY, 2010-10-15 17:22:49]

On 2010/10/15 07:06:18, James Hawkins wrote:
> AutoFillProfile::PreviewSummary should be removed and the platform-specific
> dialog implementations should just call
> AutoFillProfile::CreateInferredLabels(profiles), where |profiles| is the
entire
> list of stored profiles.  The mac implementation (somewhat) already does this.
> AutoFillProfile::AdjustInferredLabels really needs to go in favor of using
> CreateInferredLabels() directly.

Yes, I thought about this, but in current implementation you need to have a list
of the items to create inferred labels, what more if items somewhat similar
there could be a situation when editing of one item updates all labels, which is
jarring to the user. So I think we need to wait on this change until we solve
similar profiles problem.

[James Hawkins, 2010-10-15 18:20:31]

On 2010/10/15 17:22:49, GeorgeY wrote:
> On 2010/10/15 07:06:18, James Hawkins wrote:
> > AutoFillProfile::PreviewSummary should be removed and the platform-specific
> > dialog implementations should just call
> > AutoFillProfile::CreateInferredLabels(profiles), where |profiles| is the
> entire
> > list of stored profiles.  The mac implementation (somewhat) already does
this.
> > AutoFillProfile::AdjustInferredLabels really needs to go in favor of using
> > CreateInferredLabels() directly.
> 
> Yes, I thought about this, but in current implementation you need to have a
list
> of the items to create inferred labels, what more if items somewhat similar
> there could be a situation when editing of one item updates all labels, which
is
> jarring to the user. So I think we need to wait on this change until we solve
> similar profiles problem.

I don't understand the problem with the first issue; we do have the list of
items: profiles(), or however you're storing the list of profiles in the dialog.

We designed inferred labels so that this method would be used in the dialog as
well; we just never got around to making the switch for Linux/Win. Do you have
any evidence that users will find the summaries changing jarring? Mac code has
been running this for a while and there have been no bug reports.

[dhollowa, 2010-10-15 18:22:27]

On 2010/10/15 17:22:49, GeorgeY wrote:
> On 2010/10/15 07:06:18, James Hawkins wrote:
> > AutoFillProfile::PreviewSummary should be removed and the platform-specific
> > dialog implementations should just call
> > AutoFillProfile::CreateInferredLabels(profiles), where |profiles| is the
> entire
> > list of stored profiles.  The mac implementation (somewhat) already does
this.
> > AutoFillProfile::AdjustInferredLabels really needs to go in favor of using
> > CreateInferredLabels() directly.
> 
> Yes, I thought about this, but in current implementation you need to have a
list
> of the items to create inferred labels, what more if items somewhat similar
> there could be a situation when editing of one item updates all labels, which
is
> jarring to the user. So I think we need to wait on this change until we solve
> similar profiles problem.

George it is fine to update the other labels when a single profile (or cc) is
edited.   We should deprecate AutoFillProfile::PreviewSummary, and refresh all
the labels in the UI after an item is added, edited, or deleted.

[GeorgeY, 2010-10-15 18:23:22]

On 2010/10/15 18:22:27, dhollowa wrote:
> On 2010/10/15 17:22:49, GeorgeY wrote:
> > On 2010/10/15 07:06:18, James Hawkins wrote:
> > > AutoFillProfile::PreviewSummary should be removed and the
platform-specific
> > > dialog implementations should just call
> > > AutoFillProfile::CreateInferredLabels(profiles), where |profiles| is the
> > entire
> > > list of stored profiles.  The mac implementation (somewhat) already does
> this.
> > > AutoFillProfile::AdjustInferredLabels really needs to go in favor of using
> > > CreateInferredLabels() directly.
> > 
> > Yes, I thought about this, but in current implementation you need to have a
> list
> > of the items to create inferred labels, what more if items somewhat similar
> > there could be a situation when editing of one item updates all labels,
which
> is
> > jarring to the user. So I think we need to wait on this change until we
solve
> > similar profiles problem.
> 
> George it is fine to update the other labels when a single profile (or cc) is
> edited.   We should deprecate AutoFillProfile::PreviewSummary, and refresh all
> the labels in the UI after an item is added, edited, or deleted.

'k, give me an hour...

[GeorgeY, 2010-10-15 18:30:14]

On 2010/10/15 18:20:31, James Hawkins wrote:
> On 2010/10/15 17:22:49, GeorgeY wrote:
> > On 2010/10/15 07:06:18, James Hawkins wrote:
> > > AutoFillProfile::PreviewSummary should be removed and the
platform-specific
> > > dialog implementations should just call
> > > AutoFillProfile::CreateInferredLabels(profiles), where |profiles| is the
> > entire
> > > list of stored profiles.  The mac implementation (somewhat) already does
> this.
> > > AutoFillProfile::AdjustInferredLabels really needs to go in favor of using
> > > CreateInferredLabels() directly.
> > 
> > Yes, I thought about this, but in current implementation you need to have a
> list
> > of the items to create inferred labels, what more if items somewhat similar
> > there could be a situation when editing of one item updates all labels,
which
> is
> > jarring to the user. So I think we need to wait on this change until we
solve
> > similar profiles problem.
> 
> I don't understand the problem with the first issue; we do have the list of
> items: profiles(), or however you're storing the list of profiles in the
dialog.
> 
> We designed inferred labels so that this method would be used in the dialog as
> well; we just never got around to making the switch for Linux/Win. Do you have
> any evidence that users will find the summaries changing jarring? Mac code has
> been running this for a while and there have been no bug reports.

I just explained to David how change could potentially change all (or almost
all) the labels, it could happen *only* if you have a lot of *almost* matching
profiles. The QA probably are not testing for it, but in real life it tends to
happen with current implementation (see Ben's bug).

[James Hawkins, 2010-10-15 18:35:49]

On 2010/10/15 18:30:14, GeorgeY wrote:
> On 2010/10/15 18:20:31, James Hawkins wrote:
> > On 2010/10/15 17:22:49, GeorgeY wrote:
> > > On 2010/10/15 07:06:18, James Hawkins wrote:
> > > > AutoFillProfile::PreviewSummary should be removed and the
> platform-specific
> > > > dialog implementations should just call
> > > > AutoFillProfile::CreateInferredLabels(profiles), where |profiles| is the
> > > entire
> > > > list of stored profiles.  The mac implementation (somewhat) already does
> > this.
> > > > AutoFillProfile::AdjustInferredLabels really needs to go in favor of
using
> > > > CreateInferredLabels() directly.
> > > 
> > > Yes, I thought about this, but in current implementation you need to have
a
> > list
> > > of the items to create inferred labels, what more if items somewhat
similar
> > > there could be a situation when editing of one item updates all labels,
> which
> > is
> > > jarring to the user. So I think we need to wait on this change until we
> solve
> > > similar profiles problem.
> > 
> > I don't understand the problem with the first issue; we do have the list of
> > items: profiles(), or however you're storing the list of profiles in the
> dialog.
> > 
> > We designed inferred labels so that this method would be used in the dialog
as
> > well; we just never got around to making the switch for Linux/Win. Do you
have
> > any evidence that users will find the summaries changing jarring? Mac code
has
> > been running this for a while and there have been no bug reports.
> 
> I just explained to David how change could potentially change all (or almost
> all) the labels, it could happen *only* if you have a lot of *almost* matching
> profiles. The QA probably are not testing for it, but in real life it tends to
> happen with current implementation (see Ben's bug).

I understand that. My argument is that users will not notice or care if they do
change all at once. We designed the feature to have this flow, and we didn't see
it as a problem them. As I said before, Mac users haven't noticed, so I don't
see what will be different for Win/Linux.

[GeorgeY, 2010-10-20 00:20:31]

done

[dhollowa, 2010-10-20 19:29:59]

http://codereview.chromium.org/3806008/diff/21001/22001
File chrome/browser/autofill/auto_fill_editor_gtk.cc (right):

http://codereview.chromium.org/3806008/diff/21001/22001#newcode683
chrome/browser/autofill/auto_fill_editor_gtk.cc:683: COL_TITLE,
UTF16ToUTF8((*i)->Label()).c_str(),
I would expect to see a call to |AdjustInferredLabels| here somewhere.  Am I
missing something?

http://codereview.chromium.org/3806008/diff/21001/22002
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/3806008/diff/21001/22002#newcode615
chrome/browser/autofill/autofill_dialog_gtk.cc:615: COL_TITLE,
UTF16ToUTF8(profile.Label()).c_str(),
I would expect to see a call to |AdjustInferredLabels| here somewhere.  Am I
missing something?

http://codereview.chromium.org/3806008/diff/21001/22006
File chrome/browser/dom_ui/options/autofill_options_handler.cc (right):

http://codereview.chromium.org/3806008/diff/21001/22006#newcode164
chrome/browser/dom_ui/options/autofill_options_handler.cc:164:
address->SetString("label", (*i)->Label());
I would expect to see a call to |AdjustInferredLabels| here somewhere.  Am I
missing something?

http://codereview.chromium.org/3806008/diff/21001/22007
File chrome/browser/views/autofill_profiles_view_win.cc (right):

http://codereview.chromium.org/3806008/diff/21001/22007#newcode1426
chrome/browser/views/autofill_profiles_view_win.cc:1426: return
profiles_->at(row).address.Label();
I would expect to see a call to |AdjustInferredLabels| here somewhere.  Am I
missing something?

[GeorgeY, 2010-10-20 22:59:00]

http://codereview.chromium.org/3806008/diff/21001/22001
File chrome/browser/autofill/auto_fill_editor_gtk.cc (right):

http://codereview.chromium.org/3806008/diff/21001/22001#newcode683
chrome/browser/autofill/auto_fill_editor_gtk.cc:683: COL_TITLE,
UTF16ToUTF8((*i)->Label()).c_str(),
On 2010/10/20 19:29:59, dhollowa wrote:
> I would expect to see a call to |AdjustInferredLabels| here somewhere.  Am I
> missing something?

It is in PersonalDataManager::SetProfiles(), missed the load (as it could be
synced with incorrect labels, and we should drop the label altogether) so added
it to PersonalDataManager::OnWebDataServiceRequestDone as well, thus simplifying
the code.

http://codereview.chromium.org/3806008/diff/21001/22002
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/3806008/diff/21001/22002#newcode615
chrome/browser/autofill/autofill_dialog_gtk.cc:615: COL_TITLE,
UTF16ToUTF8(profile.Label()).c_str(),
On 2010/10/20 19:29:59, dhollowa wrote:
> I would expect to see a call to |AdjustInferredLabels| here somewhere.  Am I
> missing something?

See previous comment.

http://codereview.chromium.org/3806008/diff/21001/22006
File chrome/browser/dom_ui/options/autofill_options_handler.cc (right):

http://codereview.chromium.org/3806008/diff/21001/22006#newcode164
chrome/browser/dom_ui/options/autofill_options_handler.cc:164:
address->SetString("label", (*i)->Label());
On 2010/10/20 19:29:59, dhollowa wrote:
> I would expect to see a call to |AdjustInferredLabels| here somewhere.  Am I
> missing something?

See previous comment.

http://codereview.chromium.org/3806008/diff/21001/22007
File chrome/browser/views/autofill_profiles_view_win.cc (right):

http://codereview.chromium.org/3806008/diff/21001/22007#newcode1426
chrome/browser/views/autofill_profiles_view_win.cc:1426: return
profiles_->at(row).address.Label();
On 2010/10/20 19:29:59, dhollowa wrote:
> I would expect to see a call to |AdjustInferredLabels| here somewhere.  Am I
> missing something?

On Labels are cached, and rebuilt on each change. Just in case we're going to
remove it from DB, I'll add Label rebuild on each load as well.

[GeorgeY, 2010-10-22 00:32:22]

Fixing broken unit-test...

[dhollowa, 2010-10-22 20:37:36]

We should make the Mac side consistent too then.  Please remove
|UpdateProfileLabels| and calls to it, see:
autofill_dialog_controller_mac.mm:46.

[GeorgeY, 2010-10-22 20:44:45]

On 2010/10/22 20:37:36, dhollowa wrote:
> We should make the Mac side consistent too then.  Please remove
> |UpdateProfileLabels| and calls to it, see:
> autofill_dialog_controller_mac.mm:46.

done :)

[dhollowa, 2010-10-22 20:51:46]

LGTM.  Thanks.

[James Hawkins, 2010-10-25 20:26:42]

LGTM, thanks for making the changes.