686913 - HTMLProgressElement should not be form controls

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Thus, they should not have |.form| attributes.

Comment 1

[Matthew Schranz [:mjschranz]]

Attachment: Patch V1 removing form attribute

Quick fix!

Comment 2

[Mounir Lamouri (:mounir)]

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]]

Attachment: 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]]

Attachment: Patch V2 Changing Inheritance from GenericFormElement to GenericElement

Accidentally didn't assign a reviewer!

Comment 5

[Mounir Lamouri (:mounir)]

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]]

Attachment: 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)]

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]]

Attachment: 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)]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/149a80dafcb8

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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]]

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)]

Could you attach the updated patch so I can push it to mozilla-inbound?

Comment 14

[Matthew Schranz [:mjschranz]]

Attachment: 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 16

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/f946709d44e1

Comment 17

[Eric Shepherd [:sheppy]]

Docs updated:

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

And listed on Firefox 14 for developers.