683440 - Form fill fails to fill homepage/URL from Me cards created on/after 10.4

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[philippe (part-time)]

Attachment: zipped test 'me' card created with AB on 10.6

With a 'Me' card created on 10.5 or 10.6, the URL field on webpage forms remains blank. The URL is seen correctly in the Bookmark Manager however.

--
Inserting  NSLog(@"webPage: %@", webPage);
before
http://mxr.mozilla.org/camino/source/camino/src/formfill/wallet.mm#1487

logs: 8/31/11 4:16:32 PM	Camino[71820]	webPage: (null)
--
further debugging on irc:
[3:54pm] phiw: I'll try to find old backups of AB
[3:55pm] sauron|ouch: right before http://mxr.mozilla.org/camino/source/camino/src/formfill/wallet.mm#1487
[3:55pm] sauron|ouch: NSLog(webPage);
[3:57pm] sauron|ouch: or, better:
[3:57pm] sauron|ouch: NSLog(@"webPage: %@", webPage);
[3:57pm] sauron|ouch: so you'll see something in the console log all the time, even if webPage is empty
...
[4:07pm] sauron|ouch: the AB APIs *should* smooth out any differences that we see in a .vcf export, since that's an "export" format
[4:07pm] sauron|ouch: but.
[4:08pm] sauron|ouch: heh
[4:08pm] sauron|ouch: http://mxr.mozilla.org/camino/source/camino/src/bookmarks/AddressBookManager.m#83
[4:08pm] sauron|ouch: wallet needs that
[4:09pm] sauron|ouch: someone (cl?) updated the one usage but not the other, i bet

Comment 1

[Chris Lawson (gone)]

Wasn't me, but that's definitely the problem. While we're fixing this, for future maintenance purposes, it might be a good idea to stick a reminder somewhere that we need to look at both files.

Comment 2

[Chris Lawson (gone)]

And we should also make sure there isn't anything else that got deprecated since then.

The code fix looks like it can be a straight copy-and-paste of the AddressBookManager code, replacing line 1486 in wallet.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Quick fix

Yeah, it was pink; I mis-remembered what you had changed in ABManager, Chris.  Sorry for casting aspersions on your coding :-(

This is the quick fix, which is the (almost) copy-paste from ABManager to Wallet.  There's no reminder because I just now saw your comments in the bug ;-) (but, really, when fixing deprecation warnings and whatnot, developers should remember to use MXR :-P That's how I found the ABManager code, by MXRing for the constant we were using in Wallet ;-) ).

The rest of the properties we use in Wallet look fine, though.

There's a "better" way to fix this, though, per the documentation of the AB constants[1]: we should get the value for the identifier/index whose label is "home page" (kABHomePageLabel), since that label still exists in new cards and appears to be the default.

That currently requires iterating through all the items, calling |labelAtIndex:| and then comparing that to kABHomePageLabel, and then getting the |valueAtIndex:| for the index that has labelAtIndex == kABHomePageLabel.  And then we'd have to fall back to the primary identifier if "home page" didn't exist, and finally to the old property.

That seems like a lot of work for little gain, particularly for this code.

[1] http://developer.apple.com/library/mac/documentation/UserExperience/Reference/AddressBook/Miscellaneous/AddressBook_Constants/Reference/reference.html#//apple_ref/doc/c_ref/kABHomePageProperty

--

This patch works for me with the card philippe linked to last night; I probably won't be able to get back to this until next week, so any feedback on the results or the approach is welcome in the meantime.

Comment 4

[philippe (part-time)]

Comment on attachment 557428 [details] [diff] [review]
Quick fix

That works pretty well - the url is filled in correctly for those sites that use 'homepage', 'url', 'uri' and 'webpage' (Typepad, Blogger, Drupal powered commenting systems, Even some WP powered sites). For non-English sites, it remains a hit and miss. It worked on a couple of Japanese and French sites that happen to use 'url' as a label.
Tested with a 10.6 AB card, and an old 10.3 AB file imported in 10.6 AB with a new user account.

- hmm me wonders if we should add 'Website' to the list in SchemaStrings.tbl. It is is widely used - it is the default for Textpattern commenting forms, I've seen it on some WP sites (*) and used in the registration forms for some forums, and I use it on my contact form(s)… But that is for another bug eventually. 

(*) e.g. http://blog.gerv.net/2011/09/blog-moved/

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to philippe from comment #4)
> - hmm me wonders if we should add 'Website' to the list in
> SchemaStrings.tbl. It is is widely used - it is the default for Textpattern
> commenting forms, I've seen it on some WP sites (*) and used in the
> registration forms for some forums, and I use it on my contact form(s)… But
> that is for another bug eventually. 

Yeah, anything else that's obvious can easily be added and will produce a nice win in this feature for little effort (and I probably should have thought of "website" in the other bug).

If you have some time and want to round up a list of suggestions+testcases and do a megabug for "improve Wallet form fill text keywords" or something, feel free.  (Just remember we only fill in a limited set of those 13 things/parts, and I don't really want to get into localization, or otherwise to sink too much time/effort into fixing this particular sinking ship.)  Otherwise, just file a bug on "website".

Comment 6

[philippe (part-time)]

(In reply to Smokey Ardisson (offline for a while; no bugmail - do not email) from comment #5)

I filed bug 684567 for the 'website' keyword. An eventual meta bug will need some more time to research - note that such things as 'name', 'email', 'address' are quite common. Beyond that, labels become more application specific.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to philippe from comment #6)
> 
> I filed bug 684567 for the 'website' keyword. An eventual meta bug will need
> some more time to research - note that such things as 'name', 'email',

I really did mean "megabug" not "metabug" ;-)  But if you do get the time to research more, I know users will appreciate any word-matching improvements we can provide.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 557428 [details] [diff] [review]
Quick fix

Stuart, see comment 3 for discussion on this patch; I think this (quick fix) is the right fix for now.  I'm happy to add cross-reference comments "Don't forget wallet.mm also uses AddressBook APIs!" here and in ABMgr if you think that would be helpful.

I'd like to see us land this and philippe's bug 684567 for 2.1; together they'll make form fill a lot less broken and useful for our users.  (I'll also try to fix bug 407563 if I get a chance soon, to make form fill slightly less dumb, but it's less important than this bug and phiw's.)

Comment 9

[Stuart Morgan]

Comment on attachment 557428 [details] [diff] [review]
Quick fix

sr=smorgan

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/fe94c9afbe3e