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.

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

Insert adjacent #11668

Merged
merged 3 commits into from Jun 8, 2016
Merged

Insert adjacent #11668

merged 3 commits into from Jun 8, 2016

Conversation

@GuillaumeGomez
Copy link
Contributor

GuillaumeGomez commented Jun 7, 2016

Fixes #11622.

r? @nox


This change is Reviewable

@highfive
Copy link

highfive commented Jun 7, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/element.rs, components/script/dom/webidls/Element.webidl
@highfive
Copy link

highfive commented Jun 7, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@nox
Copy link
Member

nox commented Jun 7, 2016

This just requires small changes and a few tests.

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

Previously, highfive wrote…

warning Warning warning

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

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


a discussion (no related file):
Second commit needs tests, I think. We will see when we try.


components/script/dom/element.rs, line 132 [r2] (raw file):

impl AdjacentPosition {
    pub fn parse(position: &str) -> Fallible<AdjacentPosition> {
        match &*position.to_ascii_lowercase() {

Use match_ignore_ascii_case!.


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

    // https://w3c.github.io/DOM-Parsing/#dom-element-insertadjacenthtml
    fn InsertAdjacentHTML(&self, where_: DOMString, html: DOMString)

Nit: in that method, the parameters are called position and text .


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

    fn InsertAdjacentHTML(&self, where_: DOMString, html: DOMString)
                          -> ErrorResult {
        fn step3(context: &Element, where_: AdjacentPosition, html: DOMString)

Inline that, no need to do a step3 function.


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

        }

        fn step2(context: &Element, where_: AdjacentPosition, html: DOMString)

Inline that and just do like for Range.createContextualFragment.


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

        // Step 1.
        let where_ = try!(AdjacentPosition::parse(&*where_));

What about:

let context = match position {
    AdjacentPosition::BeforeBegin | AdjacentPosition::AfterEnd => {
        match self.upcast::<Node>().GetParentNode() {
            Some(parent) if !parent.is::<Document>() => parent,
            _ => return Err(Error::NoModificationAllowed),
        }
    },
    AdjacentPosition::AfterBegin | AdjacentPosition::BeforeEnd => {
        Root::from_ref(self.upcast::<Node>())
    },
};

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

        match where_ {
            AdjacentPosition::BeforeBegin | AdjacentPosition::AfterEnd => {
                match self.clone().upcast::<Node>().GetParentElement() {

GetParentElement() returns an element, which should be a hint that it is not the right method to use here. How could an Element be a Document? Use GetParentNode().


Comments from Reviewable

@jdm
Copy link
Member

jdm commented Jun 7, 2016

Sorry my IRC suggestion of element.document() didn't work. I should have said document_from_node(element) instead, which is different than calling GetParentNode.

@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Jun 7, 2016

No worries, I found it so everything's fine! :)

@highfive
Copy link

highfive commented Jun 7, 2016

New code was committed to pull request.

@nox
Copy link
Member

nox commented Jun 8, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 8, 2016

Trying commit 7542fed with merge 6948504...

bors-servo added a commit that referenced this pull request Jun 8, 2016
Insert adjacent

Fixes #11622.

r? @nox

<!-- 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/11668)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 8, 2016

💔 Test failed - linux-dev

@nox
Copy link
Member

nox commented Jun 8, 2016

-S-awaiting-review -S-tests-failed +S-needs-code-changes +S-fails-tidy

Previously, bors-servo wrote…

💔 Test failed - linux-dev


Reviewed 3 of 3 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


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

Previously, nox (Anthony Ramine) wrote…

What about:

let context = match position {
    AdjacentPosition::BeforeBegin | AdjacentPosition::AfterEnd => {
        match self.upcast::<Node>().GetParentNode() {
            Some(parent) if !parent.is::<Document>() => parent,
            _ => return Err(Error::NoModificationAllowed),
        }
    },
    AdjacentPosition::AfterBegin | AdjacentPosition::BeforeEnd => {
        Root::from_ref(self.upcast::<Node>())
    },
};
Did you have lifetime issues with that way?

components/script/dom/element.rs, line 134 [r5] (raw file):

        match_ignore_ascii_case! { &*position,
            "beforebegin" => Ok(AdjacentPosition::BeforeBegin),
            "afterbegin"  => Ok(AdjacentPosition::AfterEnd),

AfterBegin


components/script/dom/element.rs, line 135 [r5] (raw file):

            "beforebegin" => Ok(AdjacentPosition::BeforeBegin),
            "afterbegin"  => Ok(AdjacentPosition::AfterEnd),
            "beforeend"   => Ok(AdjacentPosition::AfterBegin),

BeforeEnd


components/script/dom/element.rs, line 136 [r5] (raw file):

            "afterbegin"  => Ok(AdjacentPosition::AfterEnd),
            "beforeend"   => Ok(AdjacentPosition::AfterBegin),
            "afterend"    => Ok(AdjacentPosition::BeforeEnd),

AfterEnd


components/script/dom/element.rs, line 2051 [r5] (raw file):

            }
            AdjacentPosition::AfterBegin | AdjacentPosition::BeforeEnd => Root::from_ref(self.upcast::<Node>()),
        };

Do you have a lifetime issue with this?

let context = match position {
    AdjacentPosition::BeforeBegin | AdjacentPosition::AfterEnd => {
        match self.upcast::<Node>().GetParentNode() {
            Some(parent) if !parent.is::<Document>() => parent,
            _ => return Err(Error::NoModificationAllowed),
        }
    },
    AdjacentPosition::AfterBegin | AdjacentPosition::BeforeEnd => {
        Root::from_ref(self.upcast::<Node>())
    },
};

components/script/dom/webidls/Element.webidl, line 79 [r5] (raw file):

  void insertAdjacentText(DOMString where_, DOMString data);
  [Throws]
  void insertAdjacentHTML(DOMString psoition, DOMString html);

Nit: position.


Comments from Reviewable

@nox
Copy link
Member

nox commented Jun 8, 2016

-S-needs-code-changes

Previously, nox (Anthony Ramine) wrote…

-S-awaiting-review -S-tests-failed +S-needs-code-changes +S-fails-tidy


Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


components/script/dom/element.rs, line 2051 [r5] (raw file):

Previously, nox (Anthony Ramine) wrote…

Do you have a lifetime issue with this?

let context = match position {
    AdjacentPosition::BeforeBegin | AdjacentPosition::AfterEnd => {
        match self.upcast::<Node>().GetParentNode() {
            Some(parent) if !parent.is::<Document>() => parent,
            _ => return Err(Error::NoModificationAllowed),
        }
    },
    AdjacentPosition::AfterBegin | AdjacentPosition::BeforeEnd => {
        Root::from_ref(self.upcast::<Node>())
    },
};
Ok, that can't work because "cannot bind by-move into a pattern guard".

Comments from Reviewable

@highfive
Copy link

highfive commented Jun 8, 2016

New code was committed to pull request.

@highfive
Copy link

highfive commented Jun 8, 2016

  ▶ CRASH [expected PASS] /_mozilla/css/submit_focus_a.html
  │ 
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ thread &#39;ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }&#39; panicked at &#39;index out of bounds: the len is 0 but the index is 0&#39;, ../src/libcollections/vec.rs:1167
  │ stack backtrace:
  │    1:        0x10f8f2ac8 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x10f8f8e75 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x10f8f8a8e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x10f6d3a32 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │    5:        0x10f8e0aa2 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
  │    6:        0x10f8f9436 - std::panicking::begin_panic::h4889569716505182
  │    7:        0x10f8e2308 - std::panicking::begin_panic_fmt::h484cd47786497f03
  │    8:        0x10f8f908f - rust_begin_unwind
  │    9:        0x10f91ff10 - core::panicking::panic_fmt::h257ceb0aa351d801
  │   10:        0x10f920090 - core::panicking::panic_bounds_check::h3956fdcea61ff421
  │   11:        0x10e60eb3e - script::dom::browsingcontext::BrowsingContext::active_document::h226f24d08ec44427
  │   12:        0x10e61385a - script::dom::browsingcontext::BrowsingContext::active_window::h2ba128d21d9d19cf
  │   13:        0x10ec2777c - _&lt;script..script_thread..ScriptMemoryFailsafe&lt;&#39;a&gt; as core..ops..Drop&gt;::drop::h4fd0a77cbabe1834
  │   14:        0x10ec2b9ee - std::panicking::try::call::hc9179c9e89d6d659
  │   15:        0x10f8fc11b - __rust_try
  │   16:        0x10f8fc0b5 - __rust_maybe_catch_panic
  │   17:        0x10ec2c1b4 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::hbef70c0af4cb429b
  │   18:        0x10f8f7dd8 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   19:     0x7fff8e158059 - _pthread_body
  │   20:     0x7fff8e157fd6 - _pthread_start
  └ thread panicked while panicking. aborting.

  ▶ Unexpected subtest result in /domparsing/insert-adjacent.html:
  └ PASS [expected FAIL] insertAdjacentHTML(afterbegin, &lt;h3&gt;afterbegin&lt;/h3&gt; )

  ▶ Unexpected subtest result in /domparsing/insert-adjacent.html:
  └ PASS [expected FAIL] insertAdjacentHTML(beforeend, &lt;h3&gt;beforeend&lt;/h3&gt; )

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] Afterbegin content without next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] BeforeEnd content without next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] Afterbegin content again, with next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] BeforeEnd content again, with next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] Should throw when inserting with invalid position string

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] When the parent node is null, insertAdjacentHTML should throw for beforebegin and afterend (text)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] When the parent node is null, insertAdjacentHTML should throw for beforebegin and afterend (comments)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] When the parent node is </span><span class="stdout">null, insertAdjacentHTML should throw for beforebegin and afterend (elements)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] When the parent node is a document, insertAdjacentHTML should throw for beforebegin and afterend (text)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] When the parent node is a document, insertAdjacentHTML should throw for beforebegin and afterend (comments)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] When the parent node is a document, insertAdjacentHTML should throw for beforebegin and afterend (elements)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] Inserting after being and before end should order things correctly

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] Afterbegin child node not in tree but has parent

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] BeforeEnd child node not in tree but has parent

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] Afterbegin content2 without next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] BeforeEnd content2 without next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] Afterbegin content2 test again, now that there&#39;s a next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.xhtml:
  └ PASS [expected FAIL] BeforeEnd content2 test again, now that there&#39;s a next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] Afterbegin content without next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] BeforeEnd content without next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] Afterbegin content again, with next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] BeforeEnd content again, with next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] Should throw when inserting with invalid position string

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] When the parent node is null, insertAdjacentHTML should throw for beforebegin and afterend (text)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] When the parent node is null, insertAdjacentHTML should throw for beforebegin and afterend (comments)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] When the parent node is null, insertAdjacentHTML should throw for beforebegin and afterend (elements)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] When the parent node is a document, insertAdjacentHTML should throw for beforebegin and afterend (text)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] When the parent node is a document, insertAdjacentHTML should throw for beforebegin and afterend (comments)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] When the parent node is a document, insertAdjacentHTML should throw for beforebegin and afterend (elements)

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] Inserting after being and before end should order things correctly

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] Afterbegin child node not in tree but has parent

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] BeforeEnd child node not in tree but has parent

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] Afterbegin content2 without next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] BeforeEnd content2 without next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] Afterbegin content2 test again, now that there&#39;s a next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] BeforeEnd content2 test again, now that there&#39;s a next sibling

  ▶ Unexpected subtest result in /domparsing/insert_adjacent_html.html:
  └ PASS [expected FAIL] Inserting kids of the &lt;html&gt; element should not do weird things with implied &lt;body&gt;/&lt;head&gt; tags
@highfive
Copy link

highfive commented Jun 8, 2016

New code was committed to pull request.

@nox
Copy link
Member

nox commented Jun 8, 2016

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

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 4 of 4 files at r9.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


a discussion (no related file):

Previously, nox (Anthony Ramine) wrote…

Second commit needs tests, I think. We will see when we try.

Seems like there are some already.

a discussion (no related file):
Please just update expectations for the unexpected PASS, not timeouts that occur locally because you are using a debug build.


Comments from Reviewable

@nox
Copy link
Member

nox commented Jun 8, 2016

The crash is #11671.

@highfive
Copy link

highfive commented Jun 8, 2016

New code was committed to pull request.

@nox
Copy link
Member

nox commented Jun 8, 2016

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

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 3 of 3 files at r10.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


a discussion (no related file):

Previously, nox (Anthony Ramine) wrote…

Please just update expectations for the unexpected PASS, not timeouts that occur locally because you are using a debug build.

There are still wrong expectations for createContextualFragment.html and you didn't remove the failing expectations for insertAdjacentHTML, the unexpected `PASS` tests on Buildbot.

Comments from Reviewable

@highfive
Copy link

highfive commented Jun 8, 2016

New code was committed to pull request.

@nox
Copy link
Member

nox commented Jun 8, 2016

-S-awaiting-review

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 5 of 5 files at r11.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


a discussion (no related file):

Previously, nox (Anthony Ramine) wrote…

There are still wrong expectations for createContextualFragment.html and you didn't remove the failing expectations for insertAdjacentHTML, the unexpected PASS tests on Buildbot.

The expectations are now ok.

a discussion (no related file):
That's a lot of still failing tests. This requires further enquiry.


Comments from Reviewable

@nox
Copy link
Member

nox commented Jun 8, 2016

@bors-servo r+

Previously, nox (Anthony Ramine) wrote…

-S-awaiting-review


Review status: all files reviewed at latest revision, all discussions resolved.


a discussion (no related file):

Previously, nox (Anthony Ramine) wrote…

That's a lot of still failing tests. This requires further enquiry.

The tests look broken, I'll study that myself.

Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Jun 8, 2016

📌 Commit 8c57208 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 8, 2016

Testing commit 8c57208 with merge da5007e...

bors-servo added a commit that referenced this pull request Jun 8, 2016
Insert adjacent

Fixes #11622.

r? @nox

<!-- 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/11668)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 8, 2016

@bors-servo bors-servo merged commit 8c57208 into servo:master Jun 8, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:insert_adjacent branch Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.