Last Comment Bug 582412 - Implement @formmethod, @formenctype
: Implement @formmethod, @formenctype
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b5
Assigned To: Mounir Lamouri (:mounir)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 582303 787095
Blocks: html5forms 583288
  Show dependency treegraph
 
Reported: 2010-07-27 16:09 PDT by David Zbarsky (:dzbarsky)
Modified: 2012-08-30 08:51 PDT (History)
7 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta5+


Attachments
Part 1: Implement the attributes and use them (914 bytes, patch)
2010-07-27 16:10 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Part 1: Implement the attributes and use them (20.08 KB, patch)
2010-07-27 16:12 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Part 2: Define IDL attributed and implement them (5.53 KB, patch)
2010-07-27 16:49 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Implement only @formmethod and @formenctype (12.11 KB, patch)
2010-08-04 20:23 PDT, David Zbarsky (:dzbarsky)
bugs: review-
Details | Diff | Splinter Review
Patch v2 (38.22 KB, patch)
2010-08-18 16:39 PDT, Mounir Lamouri (:mounir)
bugs: review-
Details | Diff | Splinter Review
Patch v2.1 (40.17 KB, patch)
2010-08-19 19:54 PDT, Mounir Lamouri (:mounir)
jonas: superreview+
Details | Diff | Splinter Review
Patch v2.2 (40.96 KB, patch)
2010-08-19 23:52 PDT, Mounir Lamouri (:mounir)
bugs: review+
Details | Diff | Splinter Review
Patch v2.3 (41.01 KB, patch)
2010-08-20 13:08 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2010-07-27 16:09:09 PDT

    
Comment 1 David Zbarsky (:dzbarsky) 2010-07-27 16:10:38 PDT
Created attachment 460689 [details] [diff] [review]
Part 1: Implement the attributes and use them
Comment 2 David Zbarsky (:dzbarsky) 2010-07-27 16:12:01 PDT
Created attachment 460690 [details] [diff] [review]
Part 1: Implement the attributes and use them

Sorry, the other one simply defined GK_ATOMS.  That doesn't really do much =P
Comment 3 David Zbarsky (:dzbarsky) 2010-07-27 16:49:00 PDT
Created attachment 460700 [details] [diff] [review]
Part 2: Define IDL attributed and implement them
Comment 4 David Zbarsky (:dzbarsky) 2010-07-27 16:55:48 PDT
Of course I forgot to bump the IID. Consider that done.
Comment 5 Mounir Lamouri (:mounir) 2010-07-30 10:10:07 PDT
formaction and formtarget patches are in bug 566160 and bug 566064.
Please, implement only formmethod and formenctype in this bug.
Comment 6 Olli Pettay [:smaug] 2010-08-02 13:48:36 PDT
Comment on attachment 460690 [details] [diff] [review]
Part 1: Implement the attributes and use them

Based on comment 5, these patches need some tweaking (removing some parts).
Comment 7 David Zbarsky (:dzbarsky) 2010-08-04 20:23:11 PDT
Created attachment 463048 [details] [diff] [review]
Implement only @formmethod and @formenctype
Comment 8 Mounir Lamouri (:mounir) 2010-08-05 03:32:47 PDT
Be careful with NS_IMPL_ENUM_ATTR_DEFAULT_VALUE. It is assuming the enumerated value is limited to only known values like input.type. When I wrote this macro I was thinking all enumerated values would have to be limited to known values. However, this is clearly not the case [1]. I had to open a bug to have input.type limited to only known value [2] (for backward compatibility) and someone did that for button.type [3].
I hope most enumerated attributes will be limited to only known values. Anne looks to agree with this idea which lets me think it's not completely non sense.

For this bug, you can use the current macro thus not respecting the specifications. You can also create a new macro or add a new parameter to not limit to only known values like:
#define NS_IMPL_ENUM_ATTR_NOT_LIMITED(_class, _method, _atom) \
  NS_IMETHODIMP                                                           \
  _class::Get##_method(nsAString& aValue)                                 \
  {                                                                       \
    return GetEnumAttr(nsGkAtoms::_atom, nsnull, aValue);               \
  }   
[...]

and in nsGenericHTMLElement::GetEnumAttr, you change:
  if (attrVal && attrVal->Type() == nsAttrValue::eEnum) {
    attrVal->GetEnumString(aResult, PR_TRUE);
  } else {
    AppendASCIItoUTF16(nsDependentCString(aDefault), aResult);
  }
for:
  if (attrVal && attrVal->Type() == nsAttrValue::eEnum) {
    attrVal->GetEnumString(aResult, PR_TRUE);
  } else if (aDefault) {
    AppendASCIItoUTF16(nsDependentCString(aDefault), aResult);
  } else if (attrVal) { // when not limited to only known value, there is no default value
    attrVal->ToString(aResult);
  }


[1] http://www.w3.org/Bugs/Public/show_bug.cgi?id=9634
[2] http://www.w3.org/Bugs/Public/show_bug.cgi?id=9635
[3] http://www.w3.org/Bugs/Public/show_bug.cgi?id=10290
Comment 9 Mounir Lamouri (:mounir) 2010-08-06 15:43:10 PDT
Good news, you can forget comment 8, the specs have changed and formmethod, formenctype, method and enctype are now limited to known values.
Comment 10 Olli Pettay [:smaug] 2010-08-11 09:05:45 PDT
Comment on attachment 463048 [details] [diff] [review]
Implement only @formmethod and @formenctype

>diff --git a/content/html/content/public/nsFormSubmission.h b/content/html/content/public/nsFormSubmission.h
>--- a/content/html/content/public/nsFormSubmission.h
>+++ b/content/html/content/public/nsFormSubmission.h
>@@ -221,11 +221,12 @@ private:
> 
> /**
>  * Get a submission object based on attributes in the form (ENCTYPE and METHOD)
>  *
>  * @param aForm the form to get a submission object based on
>  * @param aFormSubmission the form submission object (out param)
Please document aOrigin param.
 
> nsresult
> GetSubmissionFromForm(nsGenericHTMLElement* aForm,
>+                      nsGenericHTMLElement* aOrigin,
>                       nsFormSubmission** aFormSubmission)
> {
>   // Get all the information necessary to encode the form data
>   NS_ASSERTION(aForm->GetCurrentDoc(),
>                "Should have doc if we're building submission!");
> 
>   // Get encoding type (default: urlencoded)
>   PRInt32 enctype = NS_FORM_ENCTYPE_URLENCODED;
>-  GetEnumAttr(aForm, nsGkAtoms::enctype, &enctype);
>+  bool hasEnctype = GetEnumAttr(aOrigin, nsGkAtoms::formenctype, &enctype);
>+  if (!hasEnctype)
>+    GetEnumAttr(aForm, nsGkAtoms::enctype, &enctype);
> 
>   // Get method (default: GET)
>   PRInt32 method = NS_FORM_METHOD_GET;
>-  GetEnumAttr(aForm, nsGkAtoms::method, &method);
>+  if (!GetEnumAttr(aOrigin, nsGkAtoms::formmethod, &method))
>+    GetEnumAttr(aForm, nsGkAtoms::method, &method);
You need to handle the case when aOrigin is null.
And also add testcases for that.

>-nsHTMLFormElement::BuildSubmission(nsFormSubmission** aFormSubmission, 
>+nsHTMLFormElement::BuildSubmission(nsFormSubmission** aFormSubmission,
>                                    nsEvent* aEvent)
> {
>   NS_ASSERTION(!mPendingSubmission, "tried to build two submissions!");
> 
>   // Get the originating frame (failure is non-fatal)
>-  nsIContent *originatingElement = nsnull;
>+  nsGenericHTMLElement *originatingElement = nsnull;
>   if (aEvent) {
>     if (NS_FORM_EVENT == aEvent->eventStructType) {
>-      originatingElement = ((nsFormEvent *)aEvent)->originator;
>+      originatingElement = static_cast<nsGenericHTMLElement*>(((nsFormEvent *)aEvent)->originator);
This is a bit evil. nsFormEvent has nsIContent* originator.
Could you just change that to nsGenericHTMLElement, and then there is no need 
for the extra static_cast

>@@ -1318,17 +1306,16 @@ nsHTMLFormElement::OnSubmitClickEnd()
> 
> void
> nsHTMLFormElement::FlushPendingSubmission()
> {
>   if (mPendingSubmission) {
>     // Transfer owning reference so that the submissioin doesn't get deleted
>     // if we reenter
>     nsAutoPtr<nsFormSubmission> submission = mPendingSubmission;
>-
No need for this
Comment 11 Mounir Lamouri (:mounir) 2010-08-18 11:10:40 PDT
(In reply to comment #10)
> >-nsHTMLFormElement::BuildSubmission(nsFormSubmission** aFormSubmission, 
> >+nsHTMLFormElement::BuildSubmission(nsFormSubmission** aFormSubmission,
> >                                    nsEvent* aEvent)
> > {
> >   NS_ASSERTION(!mPendingSubmission, "tried to build two submissions!");
> > 
> >   // Get the originating frame (failure is non-fatal)
> >-  nsIContent *originatingElement = nsnull;
> >+  nsGenericHTMLElement *originatingElement = nsnull;
> >   if (aEvent) {
> >     if (NS_FORM_EVENT == aEvent->eventStructType) {
> >-      originatingElement = ((nsFormEvent *)aEvent)->originator;
> >+      originatingElement = static_cast<nsGenericHTMLElement*>(((nsFormEvent *)aEvent)->originator);
> This is a bit evil. nsFormEvent has nsIContent* originator.
> Could you just change that to nsGenericHTMLElement, and then there is no need 
> for the extra static_cast

I agree that originator will always be nsGenericHTMLElement* but I think making originator nsGenericHTMLElement* wouldn't be better because we will have to include nsGenericHTMLElement.h in widget/public/nsGUIEvent.h or force all cpp files including nsGUIEVent.h to include nsGenericHTMLELement.h. Both solutions are not doable, are they? Maybe we could create a nsFormEvent.h file which will need cpp files including it to include nsGenericHTMLElement.h but that may be a bit too much just to prevent a static_cast, right?
Comment 12 Mounir Lamouri (:mounir) 2010-08-18 16:39:59 PDT
Created attachment 467228 [details] [diff] [review]
Patch v2

I'm taking the ownership of this bug considering we want it for ff4 and David finished his internship and, according to jst, he may be on vacation now.
Comment 13 Mounir Lamouri (:mounir) 2010-08-18 16:41:38 PDT
Bug 566160 and bug 566064 may be landed before landing this one. There is no hard dependency but the patch may not apply easily.
Comment 14 Olli Pettay [:smaug] 2010-08-19 03:09:12 PDT
You don't need to #include anything to be able to use it in nsGUIEvent.h
Just have class nsGenericHTMLElement; somewhere in the nsGUIEvent.h
Comment 15 Olli Pettay [:smaug] 2010-08-19 03:13:34 PDT
Comment on attachment 467228 [details] [diff] [review]
Patch v2

Could you change the type of the originator.
static_cast from nsIContent without any checks is error prone.
What if some code later starts to use  .originator with non-html elements?
If we change its type, whoever needs it for non-html will have to
change the type and check also other usage.
Comment 16 Mounir Lamouri (:mounir) 2010-08-19 11:01:13 PDT
(In reply to comment #14)
> You don't need to #include anything to be able to use it in nsGUIEvent.h
> Just have class nsGenericHTMLElement; somewhere in the nsGUIEvent.h

This line can be changed only by adding 'class nsGenericHTMLElement':
  nsIContent *originator;\

But this one needs the class to be defined AFAIK:
  originator(nsnull)
Comment 17 Mounir Lamouri (:mounir) 2010-08-19 19:54:57 PDT
Created attachment 467645 [details] [diff] [review]
Patch v2.1

Jonas, just cancel the review if it seems not needed.
Comment 18 Mounir Lamouri (:mounir) 2010-08-19 19:55:28 PDT
(In reply to comment #17)
> Created attachment 467645 [details] [diff] [review]
> Patch v2.1
> 
> Jonas, just cancel the review if it seems not needed.

s/review/super-review/
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-19 20:07:42 PDT
Comment on attachment 467645 [details] [diff] [review]
Patch v2.1

sr=me
Comment 20 Mounir Lamouri (:mounir) 2010-08-19 23:52:34 PDT
Created attachment 467694 [details] [diff] [review]
Patch v2.2

sr=sicking

This should be more error-proof.
Comment 21 Mounir Lamouri (:mounir) 2010-08-20 13:08:58 PDT
Created attachment 467861 [details] [diff] [review]
Patch v2.3

Patch ready to be landed.
Comment 22 Mounir Lamouri (:mounir) 2010-08-20 19:25:08 PDT
In the tree:
http://hg.mozilla.org/mozilla-central/rev/7a5dfaf19c26

Note You need to log in before you can comment on or make changes to this bug.