Last Comment Bug 686913 - HTMLProgressElement should not be form controls
: HTMLProgressElement should not be form controls
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Matthew Schranz [:mjschranz]
:
Mentors:
Depends on: 762047 762435
Blocks: 633207
  Show dependency treegraph
 
Reported: 2011-09-15 10:52 PDT by Mounir Lamouri (:mounir)
Modified: 2012-06-07 04:33 PDT (History)
6 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch V1 removing form attribute (2.00 KB, patch)
2012-04-04 19:55 PDT, Matthew Schranz [:mjschranz]
no flags Details | Diff | Splinter Review
Patch V2 Changing Inheritance from GenericFormElement to GenericElement (5.30 KB, patch)
2012-04-06 18:12 PDT, Matthew Schranz [:mjschranz]
no flags Details | Diff | Splinter Review
Patch V2 Changing Inheritance from GenericFormElement to GenericElement (5.30 KB, patch)
2012-04-06 18:13 PDT, Matthew Schranz [:mjschranz]
no flags Details | Diff | Splinter Review
Patch V3 Changing Inheritance from GenericFormElement to GenericElement (12.27 KB, patch)
2012-04-19 20:10 PDT, Matthew Schranz [:mjschranz]
mounir: review+
Details | Diff | Splinter Review
Patch V3 Changing Inheritance from GenericFormElement to GenericElement (12.14 KB, patch)
2012-04-20 09:16 PDT, Matthew Schranz [:mjschranz]
mounir: review+
mark.finkle: approval‑mozilla‑central+
Details | Diff | Splinter Review
Fixed up Patch from minor issues with last one (12.20 KB, patch)
2012-04-23 06:37 PDT, Matthew Schranz [:mjschranz]
schranz.m: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-09-15 10:52:40 PDT
Thus, they should not have |.form| attributes.
Comment 1 Matthew Schranz [:mjschranz] 2012-04-04 19:55:39 PDT
Created attachment 612436 [details] [diff] [review]
Patch V1 removing form attribute

Quick fix!
Comment 2 Mounir Lamouri (:mounir) 2012-04-05 00:54:13 PDT
Comment on attachment 612436 [details] [diff] [review]
Patch V1 removing form attribute

Review of attachment 612436 [details] [diff] [review]:
-----------------------------------------------------------------

The class shouldn't inherit from nsGenericHTMLFormElement but from nsGenericHTMLElement.

Could you update the patch with that change?
Comment 3 Matthew Schranz [:mjschranz] 2012-04-06 18:12:47 PDT
Created attachment 613050 [details] [diff] [review]
Patch V2 Changing Inheritance from GenericFormElement to GenericElement

I'm not sure if I missed anything here or not but the patch doesn't seem to be blowing up my try server test of it so that's a decent sign!

https://tbpl.mozilla.org/?tree=Try&rev=1112805beb34
Comment 4 Matthew Schranz [:mjschranz] 2012-04-06 18:13:38 PDT
Created attachment 613051 [details] [diff] [review]
Patch V2 Changing Inheritance from GenericFormElement to GenericElement

Accidentally didn't assign a reviewer!
Comment 5 Mounir Lamouri (:mounir) 2012-04-10 07:00:00 PDT
Comment on attachment 613051 [details] [diff] [review]
Patch V2 Changing Inheritance from GenericFormElement to GenericElement

Review of attachment 613051 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for not noticing that before but you have to remove NS_FORM_PROGRESS from content/html/content/public/nsIFormControl.h and all occurences of that.
You will also have to fix the tests that are failing.

Except that, your patch looks good.

::: content/html/content/src/nsHTMLProgressElement.cpp
@@ +165,1 @@
>                                                    aValue, aResult);

nit: could you re-align this line?
Comment 6 Matthew Schranz [:mjschranz] 2012-04-19 20:10:28 PDT
Created attachment 616860 [details] [diff] [review]
Patch V3 Changing Inheritance from GenericFormElement to GenericElement

Fixed up the tests I was causing to fail with this patch. Or at least, removed them since they were checking against a now non existent form attribute.

Try server results of this are here https://tbpl.mozilla.org/?tree=Try&rev=874fc96bb753

Let me know if there's something else I've missed.
Comment 7 Mounir Lamouri (:mounir) 2012-04-20 06:56:29 PDT
Comment on attachment 616860 [details] [diff] [review]
Patch V3 Changing Inheritance from GenericFormElement to GenericElement

Review of attachment 616860 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the two changes applied.

::: content/html/content/src/nsHTMLProgressElement.cpp
@@ -68,2 @@
>    NS_IMETHOD Reset();
>    NS_IMETHOD SubmitNamesValues(nsFormSubmission* aFormSubmission);

You should remove Reset() and SubmitNamesValues() too. nsHTMLProgressElement doesn't inherit from nsIFormControl anymore.

::: content/html/content/test/test_bug588683-1.html
@@ -453,5 @@
>            is(formsList[i].elements[j], elementsList[j],
>               "The form should contain " + elementsList[j]);
>          }
> -        is(elementsList[j].form, formsList[i],
> -           "The form owner should be the form associated to the list");

Keep that. You should only remove "progress" from the array |elementNames|.
Comment 8 Matthew Schranz [:mjschranz] 2012-04-20 09:16:23 PDT
Created attachment 616992 [details] [diff] [review]
Patch V3 Changing Inheritance from GenericFormElement to GenericElement

Two minor changes made. Carrying over the review+ mentioned above by mounir.
Comment 9 Mounir Lamouri (:mounir) 2012-04-20 09:25:47 PDT
Comment on attachment 616992 [details] [diff] [review]
Patch V3 Changing Inheritance from GenericFormElement to GenericElement

Review of attachment 616992 [details] [diff] [review]:
-----------------------------------------------------------------

For a few days, patches need to be approved to go to mozilla-central.
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-04-21 14:36:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/149a80dafcb8
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-04-21 14:51:22 PDT
Backed out due to build failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f41d05f51c6

Logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=11096252&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=11096228&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=11096257&tree=Mozilla-Inbound

../../../../../content/html/content/src/nsHTMLProgressElement.cpp:124: error: no 'nsresult nsHTMLProgressElement::Reset()' member function declared in class 'nsHTMLProgressElement'
../../../../../content/html/content/src/nsHTMLProgressElement.cpp:131: error: no 'nsresult nsHTMLProgressElement::SubmitNamesValues(nsFormSubmission*)' member function declared in class 'nsHTMLProgressElement'
make[8]: *** [nsHTMLProgressElement.o] Error 1

http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound
Comment 12 Matthew Schranz [:mjschranz] 2012-04-21 15:34:45 PDT
http://i0.kym-cdn.com/entries/icons/original/000/000/554/facepalm.jpg

Man do I feel dumb right now.

I pushed it up to try during, but didn't on the final one...

Anyway, fixed and going through try right now!
Comment 13 Mounir Lamouri (:mounir) 2012-04-23 01:29:24 PDT
Could you attach the updated patch so I can push it to mozilla-inbound?
Comment 14 Matthew Schranz [:mjschranz] 2012-04-23 06:37:16 PDT
Created attachment 617458 [details] [diff] [review]
Fixed up Patch from minor issues with last one

Yeah, last one was me just being silly. As you can see in this try run, it's actually just fine now!

https://tbpl.mozilla.org/?tree=Try&rev=c18e66d1a153

Most of the fails I see are with fullscreen tests.
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-23 22:39:11 PDT
*** Bug 748238 has been marked as a duplicate of this bug. ***
Comment 16 Mounir Lamouri (:mounir) 2012-04-24 03:05:36 PDT
https://hg.mozilla.org/mozilla-central/rev/f946709d44e1
Comment 17 Eric Shepherd [:sheppy] 2012-05-17 07:41:46 PDT
Docs updated:

https://developer.mozilla.org/en/HTML/Element/progress

And listed on Firefox 14 for developers.

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