Skip to content

Comments

Parse size attribute of HTMLInputElemnt correctly#9119

Merged
bors-servo merged 1 commit intoservo:masterfrom
TheKK:input_element_size
Jan 6, 2016
Merged

Parse size attribute of HTMLInputElemnt correctly#9119
bors-servo merged 1 commit intoservo:masterfrom
TheKK:input_element_size

Conversation

@TheKK
Copy link
Contributor

@TheKK TheKK commented Jan 2, 2016

Should fix #8773

Review on Reviewable

@highfive
Copy link

highfive commented Jan 2, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 2, 2016
@eefriedman
Copy link
Contributor

eefriedman commented Jan 3, 2016

You can rearrange the order of the match statement to avoid duplicated code.

A reftest would be nice.

@eefriedman eefriedman self-assigned this Jan 3, 2016
@TheKK
Copy link
Contributor Author

TheKK commented Jan 3, 2016

Sure, I'll try and learn how testing works.

@jdm
Copy link
Member

jdm commented Jan 3, 2016

@TheKK http://testthewebforward.org/docs/reftests.html and http://mxr.mozilla.org/servo/source/tests/wpt/README.md#99

@eefriedman eefriedman added S-needs-tests New tests have been requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 3, 2016
@frewsxcv
Copy link
Contributor

frewsxcv commented Jan 4, 2016

Feel free to take the tests I mentioned in the original issue: #8773

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 4, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jan 4, 2016

☔ The latest upstream changes (presumably #9076) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jan 4, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 4, 2016

Sorry, we're no longer accepting new tests under tests/ref. See tests/wpt/README.md.

@TheKK TheKK force-pushed the input_element_size branch from 239d02b to 5559555 Compare January 4, 2016 12:30
@eefriedman
Copy link
Contributor

eefriedman commented Jan 4, 2016

@TheKK Please squash the commits. Make sure the commit message for the squashed commit contains "fixes #8773".

@Ms2ger this seems to be hitting #8535 (weird manifest changes); is it okay to merge as is? If not, suggestions?

@jdm jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. S-needs-tests New tests have been requested by a reviewer. labels Jan 4, 2016
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 5, 2016
@frewsxcv frewsxcv removed the S-needs-squash Some (or all) of the commits in the PR should be combined. label Jan 5, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 5, 2016

Ugh. Just land the manifest changes, I guess.

Do remove my FIXME comment though, please.

@TheKK TheKK force-pushed the input_element_size branch from c4bff7e to 24c4e95 Compare January 5, 2016 16:38
@eefriedman
Copy link
Contributor

eefriedman commented Jan 5, 2016

@TheKK As Ms2ger said, you can also delete the comment referring to this bug. (// FIXME(Ms2ger): this is nonsense! Invalid values also end up as a text field)

@TheKK
Copy link
Contributor Author

TheKK commented Jan 6, 2016

Oh, my bad. I read it as "Do not remove" :p

@TheKK TheKK force-pushed the input_element_size branch from 24c4e95 to 74905f0 Compare January 6, 2016 02:56
@KiChjang
Copy link
Contributor

KiChjang commented Jan 6, 2016

@bors-servo r=eefriedman

@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2016

📌 Commit 74905f0 has been approved by eefriedman

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-awaiting-answer Someone asked a question that requires an answer. labels Jan 6, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2016

⌛ Testing commit 74905f0 with merge 64e968d...

bors-servo pushed a commit that referenced this pull request Jan 6, 2016
Auto merge of #9119 - TheKK:input_element_size, r=eefriedman
64e968d 
Parse size attribute of HTMLInputElemnt correctly

Should fix #8773

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9119)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2016

💔 Test failed - linux-rel

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 6, 2016
@KiChjang
Copy link
Contributor

KiChjang commented Jan 6, 2016

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2016

⚡ Previous build results for android, gonk, mac-dev-ref-unit, mac-rel-wpt are reusable. Rebuilding only linux-dev, linux-rel, mac-rel-css...

@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2016

💔 Test failed - linux-rel

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 6, 2016
@KiChjang
Copy link
Contributor

KiChjang commented Jan 6, 2016

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2016

⚡ Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

@TheKK
Copy link
Contributor Author

TheKK commented Jan 6, 2016

Just wondering. What happened after you tell the bot to rebuild with a certain issue number?

@KiChjang
Copy link
Contributor

KiChjang commented Jan 6, 2016

@TheKK it's simply a way for us to track intermittent errors. We link it to an existing issue so that we know all of the instances where the intermittent errors happened.

@TheKK
Copy link
Contributor Author

TheKK commented Jan 6, 2016

Thanks for explain. I thought the bot will magically skip the tests mentioned by these issues.

@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit 74905f0 into servo:master Jan 6, 2016
@TheKK TheKK deleted the input_element_size branch January 6, 2016 10:25
@frewsxcv frewsxcv mentioned this pull request Jan 15, 2016
Closed
@SimonSapin SimonSapin removed the S-tests-failed The changes caused existing tests to fail. label Jan 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@eefriedman eefriedman

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<input> with no specified value or size has incorrect width

9 participants

@TheKK @highfive @eefriedman @jdm @frewsxcv @bors-servo @Ms2ger @KiChjang @SimonSapin