Justin Dolske [:Dolske] Assignee
	Description • 18 years ago

Upcoming password manager changes need to observer form submits, but nsIFormSubmitObserver isn't scriptable. There's really no need for this interface, since nsIObserver is a more general way of doing it.

	Justin Dolske [:Dolske] Assignee
	Comment 1 • 18 years ago

I should probably address bug 214249 while I'm in here.

	Justin Dolske [:Dolske] Assignee
	Comment 2 • 18 years ago

Comment on attachment 255276 [details] [diff] [review]
Draft patch. Non-FF code not tested.

Upon further reflection, removing this interface isn't the right approach. Mano had concerns about removing the useful Notify() arguments (even if derivable), and I found that supporting the aCancelSubmit argument with an nsIObserver would be a bit of a mess.

So, Plan B... Don't remove the interface, but just make it scriptable. I should still be able to whack nsIContent while I'm here.

	Justin Dolske [:Dolske] Assignee
	Comment 3 • 18 years ago

This seems to do the trick. Tested with FF and Suite, but I don't know how to build or test the /embedding stuff.

	Justin Dolske [:Dolske] Assignee
	Comment 4 • 17 years ago

Review-ready patch. Tested on Firefox, and made sure Suite/Embedding parts compile as well.

	Johnny Stenback (:jst)
	Comment 5 • 17 years ago

Comment on attachment 260984 [details] [diff] [review]
Final patch.

- In nsIFormSubmitObserver.idl:

+[scriptable, uuid(0787d64a-44bf-4273-8438-61ff13ebec0c)]
+
+interface nsIFormSubmitObserver: nsISupports

Loose the empty line between the [scriptable ...] line and the interface declaration.

- In WLLT_OnSubmit():

-  nsCOMPtr<nsIDocument> doc = currentForm->GetDocument();
-  if (!doc) {
+
+  nsCOMPtr<nsIDOMDocument> aDOMDocument;
+  currentFormNode->GetOwnerDocument(getter_AddRefs(aDOMDocument));
+  if (!aDOMDocument) {
     return;
   }

This is actually a change in behavior. nsIContent::GetDocument() will return null if the node is no longer (or not yet) part of a document where nsIDOMNode::GetOwnerDocument() will return the document regardless of whether the node is in a document or not. Unless that's a desired change here I'd recommend reverting this code to using nsIContent (by QI'ing the currentFormNode) and leaving the code as it was.

- In nsPasswordManager::Notify():

-  if (!GetPasswordRealm(aFormNode->GetOwnerDoc()->GetDocumentURI(), realm))
+  nsCOMPtr<nsIDOMDocument> aDOMDocument;
+  aDOMForm->GetOwnerDocument(getter_AddRefs(aDOMDocument));
+  nsCOMPtr<nsIDocument> doc = do_QueryInterface(aDOMDocument);
+
+  // Check the reject list
+  nsCAutoString realm;
+  if (!GetPasswordRealm(doc->GetDocumentURI(), realm))
     return NS_OK;

Wouldn't this be less code if you'd simply QI aFormNode to nsIContent and left the code as is? Fewer nsCOMPtr's at least.

-  nsCOMPtr<nsIForm> formElement = do_QueryInterface(aFormNode);
+  nsCOMPtr<nsIForm> aForm = do_QueryInterface(aDOMForm);

Names starting with "a" followed by an upper case letter are generally used for argument names, try to avoid such names for non-arguments.

- In nsPasswordManager::FillPassword():

-  nsCOMPtr<nsIContent> fieldContent = do_QueryInterface(userField);
-
   // The document may be null during teardown, for example as Windows
   // sends a blur event as a native widget is destroyed.
-  nsIDocument *doc = fieldContent->GetDocument();
-  if (!doc)
+  nsCOMPtr<nsIDOMDocument> aDOMDocument;
+  userField->GetOwnerDocument(getter_AddRefs(aDOMDocument));
+  if (!aDOMDocument)
     return NS_OK;
+  nsCOMPtr<nsIDocument> doc = do_QueryInterface(aDOMDocument);

Same change in functionality here too, i.e. GetDocument() vs. GetOwnerDocument(). Can't this code remain as is?

r- based on that. Fix the above and I'll happily r+sr this.

	Justin Dolske [:Dolske] Assignee
	Comment 6 • 17 years ago

(In reply to comment #5)

> This is actually a change in behavior. nsIContent::GetDocument() will return
> null if the node is no longer (or not yet) part of a document where
> nsIDOMNode::GetOwnerDocument() will return the document regardless of whether
> the node is in a document or not. Unless that's a desired change here I'd
> recommend reverting this code to using nsIContent (by QI'ing the
> currentFormNode) and leaving the code as it was.

Hmm. I was trying to fix bug 214249 by completely removing usage of nsIContent... I'm not sure if it's possible to submit a form that's not attached to the DOM, so I'll just switch this back.

> - In nsPasswordManager::FillPassword():
> 
> [...]
> Same change in functionality here too, i.e. GetDocument() vs.
> GetOwnerDocument(). Can't this code remain as is?

I suppose, since it won't be around much longer anyway. :-)

	Justin Dolske [:Dolske] Assignee
	Comment 7 • 17 years ago

Addresses review comments.

	Johnny Stenback (:jst)
	Comment 8 • 17 years ago

Comment on attachment 261742 [details] [diff] [review]
Finaler patch.

- In EmbedPasswordMgr::Notify():

-        nsCOMPtr<nsIDOMElement> formDOMEl = do_QueryInterface(aFormNode);
+        nsCOMPtr<nsIDOMElement> formDOMEl = do_QueryInterface(formNode);
         formDOMEl->GetAttribute(NS_LITERAL_STRING("autocomplete"), autocomplete);

Now that this method is passed an nsIDOMHTMLFormElement (as aDOMForm), you can call GetAttribute() directly on that as it inherits nsIDOMElement, so no need for this QI.

r+sr=jst with that fixed.

	Justin Dolske [:Dolske] Assignee
	Comment 9 • 17 years ago

Final patch, with last review comment fixed (no other changes).

	Phil Ringnalda (:philor)
	Comment 10 • 17 years ago

content/html/content/public/Makefile.in 1.37
content/html/content/public/nsIFormSubmitObserver.h delete
content/html/content/public/nsIFormSubmitObserver.idl 1.1
embedding/browser/gtk/src/EmbedPasswordMgr.cpp 1.8
embedding/browser/gtk/src/EmbedPasswordMgr.h 1.5
extensions/wallet/src/nsWalletService.cpp 1.165
extensions/wallet/src/nsWalletService.h 1.63
extensions/wallet/src/wallet.cpp 1.381
extensions/wallet/src/wallet.h 1.51
security/manager/boot/src/nsSecureBrowserUIImpl.cpp 1.63
security/manager/boot/src/nsSecureBrowserUIImpl.h 1.18
toolkit/components/passwordmgr/base/nsPasswordManager.cpp 1.93
toolkit/components/passwordmgr/base/nsPasswordManager.h 1.20
toolkit/components/satchel/src/nsFormHistory.cpp 1.38
toolkit/components/satchel/src/nsFormHistory.h 1.12
toolkit/components/satchel/src/nsStorageFormHistory.cpp 1.15
toolkit/components/satchel/src/nsStorageFormHistory.h 1.7

	Phil Ringnalda (:philor)
	Comment 11 • 17 years ago

Oh, and

camino/src/formfill/KeychainService.h 1.8
camino/src/formfill/KeychainService.mm 1.14
camino/src/formfill/wallet.h 1.2
camino/src/formfill/wallet.mm 1.6

Curse you, bug 367617.