Closed Bug 1005675 Opened 10 years ago Closed 10 years ago

Wrong input type "telephone" instead of "tel" in some tests

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: arnaud.bienner, Assigned: arnaud.bienner)

References

Details

(Whiteboard: [needs review])

Attachments

(1 file, 3 obsolete files)

Attached patch fix_tel.patch (obsolete) — Splinter Review
Some tests still use "telephone" instead of "tel" input type.
Patch provided to change this.
Pushed to try, everything is still green: https://tbpl.mozilla.org/?tree=Try&rev=d9958f1c3320
Depends on: 637321
Whiteboard: [needs review]
Looks like this had some Mochitest-1 failures. (That's the test suite where these tests live, it looks like.)

(Once you've addressed that, maybe request review from someone who wrote or reviewed changes to these tests? Or failing that, any content peer would probably be a reasonable fallback reviewer, since this is in /content.)
Assignee: nobody → arnaud.bienner
Attached patch fix_tel.patch (obsolete) — Splinter Review
Oops, indeed, thanks for noticing.
This patch should be better.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=4c3f3ca65c8a
let's see if it's OK on all platforms this time.
Attachment #8417059 - Attachment is obsolete: true
Attached patch fix_tel.patch (obsolete) — Splinter Review
(wrong patch)
Attachment #8417133 - Attachment is obsolete: true
In the future, please try to scope your Try runs a bit more tightly. Note that the only files touched here are a few mochitests, so there's no reason to do "-u all".  ("-u mochitests" should be sufficient)

A full try run eats up days of compute-time (in aggregate), so it's worth cutting down on clearly-unnecessary chunks, when it's reasonably-easy to do so.
(For reference, for my first-pass try runs, I usually pick the tests I'm interested in & only ask for 2-3 platforms (2 desktop, and one mobile).  Assuming that goes well, I'll occasionally do an all-platforms try run before landing, but that's often unlikely enough to turn up new issues that I'll skip it & take my chances on being backed out.  It's pretty rare that anyone should actually need a "-u all -p all" Try run.)
On the bright side, though, your Try run is looking quite green. :) (Also, thanks for catching & working on this test-bug.)
Indeed, I didn't pay a lot of attention to this, sorry.
I promise I will try to take care of this next time ;)
Status: NEW → ASSIGNED
Comment on attachment 8417135 [details] [diff] [review]
fix_tel.patch

Boris, do you think confident to review this patch?
As you reviewed patch in bug 637321 and this sounds like a follow up of this bug to me, I thought you might be able to have a look.
If you're too busy, do you know someone who could?
Attachment #8417135 - Flags: review?(bzbarsky)
Comment on attachment 8417135 [details] [diff] [review]
fix_tel.patch

r=me.  Thanks for fixing this!
Attachment #8417135 - Flags: review?(bzbarsky) → review+
Attached patch fix_tel.patchSplinter Review
Updated commit message.
Attachment #8417135 - Attachment is obsolete: true
Attachment #8417653 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9b35779771b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: