Closed Bug 370561 Opened 15 years ago Closed 15 years ago

Make nsIFormSubmitObserver scriptable

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
I should probably address bug 214249 while I'm in here.
Assignee: dolske → general
Component: Password Manager → DOM
Product: Firefox → Core
QA Contact: password.manager → ian
Target Milestone: Firefox 3 → ---
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.
Attachment #255276 - Attachment is obsolete: true
Summary: Remove nsIFormSubmitObserver (just use nsIObserver) → Make nsIFormSubmitObserver scriptable
This seems to do the trick. Tested with FF and Suite, but I don't know how to build or test the /embedding stuff.
Blocks: 374723
Attached patch Final patch. (obsolete) — Splinter Review
Review-ready patch. Tested on Firefox, and made sure Suite/Embedding parts compile as well.
Attachment #256332 - Attachment is obsolete: true
Attachment #260984 - Flags: review?(jst)
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.
Attachment #260984 - Flags: review?(jst) → review-
(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. :-)
Attached patch Finaler patch. (obsolete) — Splinter Review
Addresses review comments.
Attachment #260984 - Attachment is obsolete: true
Attachment #261742 - Flags: review?(jst)
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.
Attachment #261742 - Flags: superreview+
Attachment #261742 - Flags: review?(jst)
Attachment #261742 - Flags: review+
Attached patch Finalest patch.Splinter Review
Final patch, with last review comment fixed (no other changes).
Attachment #261742 - Attachment is obsolete: true
Whiteboard: [needs checkin]
Whiteboard: [needs checkin] → [checkin needed]
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.