Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Include <button type=submit> data whilst constructing the form dataset #11333

Merged
merged 2 commits into from
May 23, 2016
Merged

Include <button type=submit> data whilst constructing the form dataset #11333

merged 2 commits into from
May 23, 2016

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented May 23, 2016 •
edited
Loading

This makes it possible to close things in github (see
Manishearth/mitochondria#1)

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors

Either:

  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/htmlformelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/htmlbuttonelement.rs

@highfive
Copy link

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 23, 2016
@Manishearth
Copy link
Member Author

r? @nox

@highfive highfive assigned nox and unassigned mbrubeck May 23, 2016
@Manishearth Manishearth mentioned this pull request May 23, 2016
Open
24 tasks
Manishearth
Manishearth reviewed May 23, 2016
View reviewed changes
components/script/dom/htmlbuttonelement.rs
if name == "" {
// unspecced, however all the browsers do it
// and it could break urlencoded parsing if
// there are random equal signs floating around
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will file a spec bug on this in a while

@Manishearth
Copy link
Member Author

@bors try

@highfive
Copy link

New code was committed to pull request.

@Manishearth
Copy link
Member Author

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit aaa85de with merge b74cb98...

bors-servo pushed a commit that referenced this pull request May 23, 2016
Auto merge of #11333 - Manishearth:submit-submit-button, r=<try>
b74cb98 
Include <button type=submit> data whilst constructing the form dataset

This makes it possible to close things in github (see
Manishearth/mitochondria#1)

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

Either:
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

Don't merge yet, I need to handle tests and work out a possible spec bug
regarding no-name form elements

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11333)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

@highfive
Copy link

New code was committed to pull request.

@nox
Copy link
Contributor

nox commented May 23, 2016

Thanks for your contribution, just some nits to address.

-S-awaiting-review +S-needs-code-changes

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 3 of 3 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

components/script/dom/htmlbuttonelement.rs, line 145 [r1] (raw file):

    /// Steps range from 3.1 to 3.7 (specific to HTMLButtonElement)
    pub fn form_datum(&self, submitter: Option<FormSubmitter>) -> Option<FormDatum> {
        // 3.1: disabled state check is in get_unclean_dataset

Nit: "Step 3.1"

components/script/dom/htmlbuttonelement.rs, line 147 [r1] (raw file):

        // 3.1: disabled state check is in get_unclean_dataset

        // 3.1: only run steps if this is the submitter

Nit: "Step 3.1"

components/script/dom/htmlbuttonelement.rs, line 160 [r1] (raw file):

        let name = self.Name();

        if name == "" {

Nit: name.is_empty().

components/script/dom/htmlbuttonelement.rs, line 167 [r1] (raw file):

        }

        // Step 3.6

AFAICT, this is step 3.9. not 3.6..

components/script/dom/htmlbuttonelement.rs, line 161 [r3] (raw file):

        if name == "" {
            // 3.1: Must have a name

Nit: "Step 3.1"

Comments from Reviewable

@nox nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 23, 2016
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels May 23, 2016
@highfive
Copy link

New code was committed to pull request.

@highfive
Copy link

New code was committed to pull request.

@nox
Copy link
Contributor

nox commented May 23, 2016

@bors-servo r+

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 2 of 2 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@bors-servo
Copy link
Contributor

📌 Commit 90d72c5 has been approved by nox

@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. labels May 23, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 90d72c5 with merge de79f96...

bors-servo pushed a commit that referenced this pull request May 23, 2016
Auto merge of #11333 - Manishearth:submit-submit-button, r=nox
de79f96 
Include <button type=submit> data whilst constructing the form dataset

This makes it possible to close things in github (see
Manishearth/mitochondria#1)

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

Either:
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11333)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

@bors-servo bors-servo merged commit 90d72c5 into servo:master May 23, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 23, 2016
@Manishearth Manishearth deleted the submit-submit-button branch May 7, 2019 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Manishearth @highfive @bors-servo @nox @mbrubeck