Last Comment Bug 839171 - Update formMethod reflection to have the empty string as default value (and 'get' as invalid value)
: Update formMethod reflection to have the empty string as default value (and '...
Status: RESOLVED FIXED
[mentor=mounir][lang=c++]
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Sunny [:darkowlzz]
:
: Andrew Overholt [:overholt]
Mentors:
http://html5.org/tools/web-apps-track...
Depends on:
Blocks: 787095
  Show dependency treegraph
 
Reported: 2013-02-07 11:16 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2013-02-23 23:30 PST (History)
8 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Applied the missed changes to button from Bug 787095 (1.72 KB, patch)
2013-02-10 06:09 PST, Sunny [:darkowlzz]
mounir: review-
Details | Diff | Splinter Review
Applied the missed changes to button from Bug 787095 (2.92 KB, patch)
2013-02-11 15:51 PST, Sunny [:darkowlzz]
mounir: review+
Details | Diff | Splinter Review
Applied the missed changes to button from Bug 787095 (3.11 KB, patch)
2013-02-12 08:09 PST, Sunny [:darkowlzz]
mounir: review+
Details | Diff | Splinter Review
Applied the missed changes to button from Bug 787095 (6.14 KB, patch)
2013-02-12 15:00 PST, Sunny [:darkowlzz]
no flags Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2013-02-07 11:16:06 PST
+++ This bug was initially created as a clone of Bug #787095 +++

See $URL for the HTML spec change. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=17185 for the reasons. I think it makes sense.

Bug 787095 changed the implementation of input.form{Enctype,Method}, but forgot to make the same changes to the button implementation.
Comment 1 Julian Viereck 2013-02-10 04:52:09 PST
Someone pinged me on IRC about how to fix this. I guess it's about chaning 

  https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLButtonElement.cpp#122

in the same way the marco was changed in

  https://bug787095.bugzilla.mozilla.org/attachment.cgi?id=705182

and update the tests.
Comment 2 Sunny [:darkowlzz] 2013-02-10 06:09:00 PST
Created attachment 712255 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

Hi,
I would like to work on this bug. 

Here is my first patch.
Comment 3 Julian Viereck 2013-02-10 07:02:58 PST
(In reply to darkowlzz from comment #2)
> Created attachment 712255 [details] [diff] [review]
> Applied the missed changes to button from Bug 787095

Have you run the test suite locally? There were some changes to the tests going with the patch in bug 787095 - that's why I assume some tests need to be adjusted for this patch as well.
Comment 4 Benedict Singer 2013-02-10 11:06:12 PST
Yes, you'll want to add tests to content/html/content/test/forms/test_button_attributes_reflection.html Also make sure to run the test suite, as this change may impact other tests.
Comment 5 Mounir Lamouri (:mounir) 2013-02-10 12:08:53 PST
Comment on attachment 712255 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

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

Your changes you look but as Benedict pointed out, you need to update the tests. Could you attach a new patch with those tests updated. Also, run the tests in content/html/content/test/ (it should take a few minutes) to make sure everything is green.
The comment to run the test is, when you are inside the objdir:
TEST_PATH=content/html/content/test/ make mochitest-plain
Comment 6 Sunny [:darkowlzz] 2013-02-11 15:51:06 PST
Created attachment 712674 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

Updated tests.
Ran mochitest and got:
INFO Passed: 86100
INFO Failed: 0
INFO Todo: 247
everything was green.
Comment 7 Sunny [:darkowlzz] 2013-02-11 16:24:49 PST
Whoops!! sorry for the comment #6 where I ran mochitest without applying the patch.
After applying the patch results were like this:

INFO Passed: 86098
INFO Failed: 2
INFO Todo:   244

mochitest-plain failed:
266 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | When no attribute is set, the value should be the default value. - got application/x-www-form-urlencoded, expected
305 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | When no attribute is set, the value should be the default value. - got get, expected

What went wrong and how do I fix it?
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2013-02-12 02:25:08 PST
Are you sure you tested on the right build? The patch looks correct on first sight.
Comment 9 Mounir Lamouri (:mounir) 2013-02-12 02:33:18 PST
Comment on attachment 712674 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

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

r=me with the tests cleaned-up.

::: content/html/content/test/forms/test_button_attributes_reflection.html
@@ +44,5 @@
>  //  defaultValue: {
>  //    invalid: "application/x-www-form-urlencoded",
>  //    missing: "",
>  //  }
> +  defaultValue: { invalid: "application/x-www-form-urlencoded", missing: "" }

Please, uncomment the lines above and remove this one.

@@ +59,5 @@
>  //  defaultValue: {
>  //    invalid: "get",
>  //    missing: "",
>  //  }
> +  defaultValue: { invalid: "get", missing: "" }

ditto
Comment 10 Mounir Lamouri (:mounir) 2013-02-12 02:33:49 PST
Comment on attachment 712674 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

As Ms2ger pointed out, your patch looks good.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2013-02-12 05:51:17 PST
https://tbpl.mozilla.org/?tree=Try&rev=692073e531e0
Comment 12 Sunny [:darkowlzz] 2013-02-12 08:09:23 PST
Created attachment 712920 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

Uncommented and removed the lines as suggested in Comment #9
Comment 13 Mounir Lamouri (:mounir) 2013-02-12 12:26:54 PST
Comment on attachment 712920 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

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

r=me.

Note that generally speaking, when we say "r=me with something changes", you don't have to ask for another review if you change what has been asked ;)

So, could you `hg rm content/html/content/test/test_bug585508.html` and modify content/html/content/test/Makefile.in appropriately? test_bug585508.html duplicates tests already in test_button_attributes_reflection.html and is buggy now ;)
Comment 14 Sunny [:darkowlzz] 2013-02-12 15:00:29 PST
Created attachment 713134 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

Deleted the test file and modified Makefile.in accordingly.
Comment 15 Sunny [:darkowlzz] 2013-02-12 15:03:27 PST
The bug has not been assigned to me yet. I can't add new keywords for check-in.
Comment 16 Mounir Lamouri (:mounir) 2013-02-13 03:55:32 PST
We have some people who regularly push patches with 'checkin-needed'. If your patch isn't push in the next 48 hours, comment here, I will push it myself. (It's better to have patches pushed in batches because it reduces the number of cycles we use to test them and I don't have any patch to push right now.)
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-02-13 07:41:31 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8479bdaf0c8
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-02-13 09:41:30 PST
Backed out for mochitest-browser-chrome failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b87f9a587e86

https://tbpl.mozilla.org/php/getParsedLog.php?id=19703949&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_propertyview-11.js | 'formMethod' in buttonProtoNode should have the right value. - Got "", expected "get"
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_propertyview-11.js | 'formMethod' in buttonAsProtoProtoProtoNode should have the right value. - Got "", expected "get"
Comment 19 Mounir Lamouri (:mounir) 2013-02-15 03:21:47 PST
(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Backed out for mochitest-browser-chrome failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b87f9a587e86
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=19703949&tree=Mozilla-
> Inbound
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/debugger/test/
> browser_dbg_propertyview-11.js | 'formMethod' in buttonProtoNode should have
> the right value. - Got "", expected "get"
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/debugger/test/
> browser_dbg_propertyview-11.js | 'formMethod' in buttonAsProtoProtoProtoNode
> should have the right value. - Got "", expected "get"

Gasp... this is not tested on linux. This is what happens when we want to reduce charge on our infrastructure and only run try on linux64...
Comment 20 Sunny [:darkowlzz] 2013-02-15 03:58:50 PST
So what do we do now?
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2013-02-15 04:11:23 PST
Er... what's not tested on linux?
Comment 22 Mounir Lamouri (:mounir) 2013-02-15 04:12:51 PST
(In reply to darkowlzz from comment #20)
> So what do we do now?

I sent to try your patch with a fix for the failing test:
https://tbpl.mozilla.org/?tree=Try&rev=fa6468b19010

If it succeed, I will land those patches.
Comment 23 Mounir Lamouri (:mounir) 2013-02-15 04:14:26 PST
(In reply to Boris Zbarsky (:bz) from comment #21)
> Er... what's not tested on linux?

devtools
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2013-02-15 04:15:19 PST
Any idea why the heck not?  Can we get a bug filed on that?
Comment 25 Mounir Lamouri (:mounir) 2013-02-15 06:21:36 PST
I pushed that patch again with the devtools tests fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d999c65753ea
Comment 26 Mounir Lamouri (:mounir) 2013-02-15 06:34:15 PST
(In reply to Boris Zbarsky (:bz) from comment #24)
> Any idea why the heck not?  Can we get a bug filed on that?

I've opened bug 841731.
Comment 27 Ryan VanderMeulen [:RyanVM] 2013-02-15 13:40:06 PST
https://hg.mozilla.org/mozilla-central/rev/d999c65753ea
Comment 28 Kohei Yoshino [:kohei] 2013-02-23 23:30:25 PST
I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21

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