48923 - Prefill Form Data should be disabled if no fields to prefill

Status: VERIFIED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Blake Ross]

Build ID: 2000081404

Sometimes Edit | Prefill Form Data is properly disabled (i.e. when I go to 
mozilla.org).  But when I go to a page with form fields (like 
http://bugzilla.mozilla.org/buglist.cgi?
bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bu
g_status=RESOLVED&email1=&emailtype1=substring&emailassigned_to1=1&email2=&email
type2=substring&emailreporter2=1&bugidtype=include&bug_id=&changedin=1&votes=&ch
field=%5BBug+creation%
5D&chfieldfrom=&chfieldto=Now&chfieldvalue=&product=Browser&product=MailNews&sho
rt_desc=&short_desc_type=substring&long_desc=&long_desc_type=substring&bug_file_
loc=&bug_file_loc_type=substring&status_whiteboard=&status_whiteboard_type=subst
ring&keywords=&keywords_type=anywords&field0-0-0=noop&type0-0-0=noop&value0-0-
0=&cmdtype=doit&newqueryname=&order=Reuse+same+sort+as+last+time), it 
enables...however, if I then click it it gives me an error saying there's no 
fields to prefill.  If possible, some proactive error handling (disabling the 
menu item) would be better than this reactive error handling (alerting the user 
after he/she pressed it).

Comment 1

[Stephen P. Morse]

I worried about that myself when I implemented the wallet commands on the edit 
menu this past week-end.  The problem is that we need to invoke some wallet 
functions in order to determine if there is any data for the current form stored 
in the wallet database.  That's more involved than just determining the current 
page contains a form which is the test I am now doing.  This test is done every 
time the edit menu is opened and I was concerned about introducing a delay each 
time the user does that.

I could do a compromise.  Suppose I don't enable the prefill entry if no 
database exists but I do enable it if the database exists even though there is 
nothing in the database that applies to the current form.  What would you think 
of that?

Comment 2

[Blake Ross]

hmm, yeah, that would at least be a start.  Could we try doing a check every 
time to see if there's forms that can be prefilled, and just seeing how much 
delay it introduces?  (Or is that too much work just for an experiment?)

Comment 3

[sairuh (rarely reading bugmail)]

adding dependency, since this affects the UI.

Comment 4

[sairuh (rarely reading bugmail)]

spam: mass-moving open password manager (single signon) and form manager
(autofill) bugs to Terri for qa contact. unfortunately, i cannot cc myself with
this form, so feel free and add me if you want to keep me in the loop with any
(but, pls not all :) of these... will also go thru 'em meself, a bit later...

Comment 5

[Stephen P. Morse]

Although this bug talks about the edit menu, the same applies to the context 
menu.

Attaching patch to fix both the edit and context menu.  This involved adding 
a new API to the wallet menu to test if a particular form element has a saved 
value.

Notes to reviewers:

1. The following change in wallet.cpp has nothing to do with this bug but it was 
a bug that I uncovered while implementing this so it made sense to fix it at the 
same time.

   -      schema = field;
   +      schema = temp;

2. The test for schema.Length() being zero before getting vcard attribute is an 
optimization.  Such optimization was not needed before because the calling 
routine always passed in a zero-length schema name.  The new code added by this 
patch sometimes passes in a known schema in which case this optimization is 
useful.

Comment 6

[Stephen P. Morse]

Attachment: patch to disable menu items if no saved values exist

Comment 7

[Bill Law]

r=law (and a=law, if I'm component owner) for the context menu changes.

I didn't examine (just glanced at) the Wallet code changes.  Steve, do you want
me to review those more thoroughly?

Comment 8

[Stephen P. Morse]

Yes, thoroughly enough to satisfy the check-in rules.  Thanks.

Comment 9

[Blake Ross]

matthew, back to the old `disable or hide' discussion.  I really hate the idea 
of these two form items permanently taking up space in the context menu.  How 
about showing them only when there's form fields present, and then disabling 
them if there isn't anything to prefill...?

Comment 10

[Stephen P. Morse]

Blake, what you described is exactly what the current behavior is.  These fields 
are present in the context menu only if you are viewing a form.  This patch 
refines the test that determines whether or not to enable the items.

Comment 11

[Alec Flett]

now I understand about passing in an nsIDOMWindow in case you might eventually
need to pop up an alert..

In the rest of the code, the convention is that the dialog is not popped up from
C++ - generally C++ just reports an error (return NS_ERROR_FAILURE) and lets the
caller either handle or propagate the error. When it gets to a layer that
generally deals with UI (i.e. JavaScript) then you pop up an error where
appropriate. (i.e. when an exception is thrown, etc)

I would rather see fewer uses of opening dialogs/alerts from C++ and more uses
of JS for that. It helps keep the various layers cleaner - loosely speaking, the
C++ becomes a presentation-free library of sorts, and the JS becomes the layer
where all the user-interaction like alerts, etc, happen.

Comment 12

[Stephen P. Morse]

I agree with you about having the dialogs coming from js instead of c++.  This 
was just an insurance policy which I hope never to have to use.

Comment 13

[Alec Flett]

but why burden your API if you know you don't need it right now? You don't need
this insurance right now.. the APIs are not frozen yet...

With the 'insurance' rational, we should be adding an nsIDOMWindow* parameter to
every API call we have :)

Comment 14

[Stephen P. Morse]

OK, forget what I said above (I was talking from memory instead of really 
looking at the code).  I definitely do need the window parameter in the API.  
It is used to obtain the nsIDocument which I need in order to initialize 
certain wallet functions.

Comment 15

[Stephen P. Morse]

Attaching new patch for wallet.cpp to address reviewers comments about 
GetPrefills not returning an nsresult.

Comment 16

[Stephen P. Morse]

Attachment: getprefills to return an nsresult

Comment 17

[Alec Flett]

thanks.. using the global convention makes it easier for developers not familiar
with your module to understand your code
sr=alecf

Comment 18

[Stephen P. Morse]

Fix checked in.

Comment 19

[Terri Preston]

Verified fixed