Checking the browsing_context before change title #10821

askeing · 2016-04-23T12:25:10Z

fix #10782

This change is

highfive · 2016-04-23T12:25:13Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/document.rs

highfive · 2016-04-23T12:25:14Z

Warning

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

askeing · 2016-04-23T12:28:11Z

Testing HTML:

<!DOCTYPE html>
<title>Foo</title>
<script>
setTimeout(() => {
  var text =
    '<html xmlns="http://www.w3.org/1999/xhtml">' +
    '  <head>' +
    '    <title>Virtual Library</title>' +
    '  </head>' +
    '</html>';

  var parser = new DOMParser();
  var doc = parser.parseFromString(text, "text/xml");
}, 100);
</script>

The title keep Foo - servo.

shinglyu · 2016-04-23T13:49:56Z

@askeing You can probably add the test HTML into WPT, see https://github.com/servo/servo/blob/master/tests/wpt/README.md#writing-new-tests

jdm · 2016-04-23T14:08:41Z

Unfortunately this change can't be verified by automated tests, since it's only the window frame title that changes, which isn't visible to JS.

Ms2ger · 2016-04-24T11:25:25Z

R? @jdm

notriddle · 2016-04-24T15:35:09Z

Isn't the window title exposed through the <iframe mozbrowser> API? I think you could automatically test it by running this test code in a mozbrower and doing the verification with a fake on the outside.

nox · 2016-04-26T12:47:37Z

What about just calling title_changed when the title actually changed instead of calling it so eagerly?

jdm · 2016-04-26T13:26:44Z

@nox I'm not sure what you're proposing. That's what the code tries to do?

jdm · 2016-04-29T19:07:42Z

@notriddle is absolutely right. We can use a test like tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsershowmodalprompt_event.html to load an HTML file that creates that DOMParser from the example in #10782. After it's finished parsing it can use alert("done") to signal that the test is complete. If the parent page has not observed a mozbrowsertitlechange event on the iframe, the test can pass.

askeing · 2016-04-30T15:52:44Z

@jdm, should I open another PR for the test? Or just add a commit here?

jdm · 2016-04-30T16:00:28Z

Adding the commit here would be appropriate.

askeing · 2016-04-30T16:28:12Z

I found the parent page will get mozbrowsertitlechange at beginning with e.detail = "", and then get real title e.detail = "Foo".
So I call assert_not_equals for checking e.detail cannot be Bar, which is the title in DOMParser.

Origin build:

$ ./mach test-wpt --debug-build tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event.html
Running 1 tests in web-platform-tests

Unsupported test type wdspec for product servo
  ▶ Unexpected subtest result in /_mozilla/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event.html:
  │ FAIL [expected PASS] Check if title_changed is too eager (issue #10782)
  │   → assert_not_equals: got disallowed value "Bar"
  │
  │ @http://web-platform.test:8000/_mozilla/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event.html:7:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1421:1

Ran 1 tests finished in 14.0 seconds.
  • 0 ran as expected. 0 tests skipped.
  • 1 tests had unexpected subtest results

After modified build:

 ./mach test-wpt --debug-build tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event.html
Running 1 tests in web-platform-tests

Unsupported test type wdspec for product servo
Ran 1 tests finished in 5.0 seconds.
  • 1 ran as expected. 0 tests skipped.

jdm · 2016-05-03T16:24:59Z

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

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

components/script/dom/document.rs, line 621 [r1] (raw file):
if self.browsing_context().is_some()

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event.html, line 16 [r2] (raw file):
We should be able to use the regular iframe load event here instead, and use t.step_func_done().

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event_iframe.html, line 5 [r2] (raw file):
Let's get rid of this timeout.

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event_iframe.html, line 14 [r2] (raw file):
No need for this.

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event_iframe.html, line 16 [r2] (raw file):
We don't need this if we use the load event.

Comments from Reviewable

askeing · 2016-05-04T07:53:05Z

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event.html, line 16 [r2] (raw file):
Does the iframe load event mean listening on mozbrowserloadend event?

Comments from Reviewable

jdm · 2016-05-04T15:35:46Z

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

tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event.html, line 16 [r2] (raw file):
No, just the load event.

Comments from Reviewable

highfive · 2016-05-04T16:07:27Z

New code was committed to pull request.

jdm · 2016-05-04T18:40:59Z

@bors-servo: delegate+
Looks good! Squash the commits together into one and go ahead and merge with r=jdm.
-S-awaiting-review +S-needs-squash

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

bors-servo · 2016-05-06T17:05:40Z

💔 Test failed - windows

mbrubeck · 2016-05-06T17:56:45Z

@bors-servo retry

servo/saltfs#363

bors-servo · 2016-05-06T18:41:43Z

⌛ Testing commit 3c56d41 with merge 597a5f1...


        Auto merge of #10821 - askeing:fix_10782, r=jdm

Checking the browsing_context before change title fix #10782  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10821)

bors-servo · 2016-05-06T18:43:33Z

💔 Test failed - windows

larsbergstrom · 2016-05-06T19:41:22Z

@bors-servo retry

servo/saltfs#365

bors-servo · 2016-05-06T21:20:46Z

⌛ Testing commit 3c56d41 with merge 17ba9fb...


        Auto merge of #10821 - askeing:fix_10782, r=jdm

Checking the browsing_context before change title fix #10782  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10821)

bors-servo · 2016-05-06T21:51:36Z

💔 Test failed - mac-rel-css

highfive · 2016-05-06T21:51:40Z

  ▶ CRASH [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-063.htm
  │ 
  │ 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;called `Option::unwrap()` on a `None` value&#39;, ../src/libcore/option.rs:325
  │ stack backtrace:
  │    1:        0x110034d18 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x11003aff5 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x11003ac0e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x10f790242 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::heda6930908f06acc
  │    5:        0x1100224e8 - std::sys_common::unwind::begin_unwind_inner::h30e12d15ce2b2e25
  │    6:        0x110023e1e - std::sys_common::unwind::begin_unwind_fmt::hb2de8a9968d38523
  │    7:        0x110033fe7 - rust_begin_unwind
  │    8:        0x110062db0 - core::panicking::panic_fmt::h257ceb0aa351d801
  │    9:        0x1100630ac - core::panicking::panic::h4bb1497076d04ab9
  │   10:        0x10f160b70 - script::page::Page::window::h010a05681584eec2
  │   11:        0x10f003fcc - std::sys_common::unwind::try::try_fn::hd12b2d76c81a0e30
  │   12:        0x110033f7b - __rust_try
  │   13:        0x110033f03 - std::sys_common::unwind::inner_try::h47a4d9cd4a369dcd
  │   14:        0x10f0042ca - _&lt;F as std..boxed..FnBox&lt;A&gt;&gt;::call_box::he24f36d07ed1c7fd
  │   15:        0x11003a078 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   16:     0x7fff8c3c2059 - _pthread_body
  │   17:     0x7fff8c3c1fd6 - _pthread_start
  └ thread panicked while panicking. aborting.

  ▶ CRASH [expected FAIL] /css-text-3_dev/html/text-align-justify-001.htm
  │ 
  │ 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;called `Option::unwrap()` on a `None` value&#39;, ../src/libcore/option.rs:325
  │ stack backtrace:
  │    1:        0x106359d18 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x10635fff5 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x10635fc0e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x105ab5242 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::heda6930908f06acc
  │    5:        0x1063474e8 - std::sys_common::unwind::begin_unwind_inner::h30e12d15ce2b2e25
  │    6:        0x106348e1e - std::sys_common::unwind::begin_unwind_fmt::hb2de8a9968d38523
  │    7:        0x106358fe7 - rust_begin_unwind
  │    8:        0x106387db0 - core::panicking::panic_fmt::h257ceb0aa351d801
  │    9:        0x1063880ac - core::panicking::panic::h4bb1497076d04ab9
  │   10:        0x105485b70 - script::page::Page::window::h010a05681584eec2
  │   11:        0x105328fcc - std::sys_common::unwind::try::try_fn::hd12b2d76c81a0e30
  │   12:        0x106358f7b - __rust_try
  │   13:        0x106358f03 - std::sys_common::unwind::inner_try::h47a4d9cd4a369dcd
  │   14:        0x1053292ca - _&lt;F as std..boxed..FnBox&lt;A&gt;&gt;::call_box::he24f36d07ed1c7fd
  │   15:        0x10635f078 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   16:     0x7fff8c3c2059 - _pthread_body
  │   17:     0x7fff8c3c1fd6 - _pthread_start
  └ thread panicked while panicking. aborting.

cbrewster · 2016-05-06T21:57:42Z

@bors-servo retry

Checking for intermittent

bors-servo · 2016-05-06T22:29:30Z

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

mbrubeck · 2016-05-06T23:21:15Z

Checking for intermittent

Filed #11059

bors-servo · 2016-05-06T23:28:46Z

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

highfive assigned wafflespeanut Apr 23, 2016

highfive added the S-awaiting-review label Apr 23, 2016

wafflespeanut assigned jdm and unassigned wafflespeanut Apr 23, 2016

jdm added S-needs-tests and removed S-awaiting-review labels Apr 29, 2016

highfive added the S-awaiting-review label Apr 30, 2016

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-tests labels May 3, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels May 4, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels May 6, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels May 6, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels May 6, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels May 6, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels May 6, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels May 6, 2016

bors-servo merged commit 3c56d41 into servo:master May 6, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label May 6, 2016

bors-servo mentioned this pull request May 6, 2016

Private browsing student project #10690

Closed

askeing deleted the askeing:fix_10782 branch May 7, 2016

servo / servo

Checking the browsing_context before change title #10821

Checking the browsing_context before change title #10821

askeing commented Apr 23, 2016

highfive commented Apr 23, 2016

highfive commented Apr 23, 2016

askeing commented Apr 23, 2016

shinglyu commented Apr 23, 2016

jdm commented Apr 23, 2016

Ms2ger commented Apr 24, 2016

notriddle commented Apr 24, 2016 •

edited

nox commented Apr 26, 2016

jdm commented Apr 26, 2016

jdm commented Apr 29, 2016

askeing commented Apr 30, 2016 •

edited

jdm commented Apr 30, 2016

askeing commented Apr 30, 2016 •

edited

jdm commented May 3, 2016

askeing commented May 4, 2016

jdm commented May 4, 2016

highfive commented May 4, 2016

jdm commented May 4, 2016

bors-servo commented May 6, 2016

mbrubeck commented May 6, 2016

bors-servo commented May 6, 2016

bors-servo commented May 6, 2016

larsbergstrom commented May 6, 2016

bors-servo commented May 6, 2016

bors-servo commented May 6, 2016

highfive commented May 6, 2016

cbrewster commented May 6, 2016

bors-servo commented May 6, 2016

mbrubeck commented May 6, 2016 •

edited

bors-servo commented May 6, 2016

servo / servo

Join GitHub today

Checking the browsing_context before change title #10821

Checking the browsing_context before change title #10821

Conversation

askeing commented Apr 23, 2016

highfive commented Apr 23, 2016

highfive commented Apr 23, 2016

askeing commented Apr 23, 2016

shinglyu commented Apr 23, 2016

jdm commented Apr 23, 2016

Ms2ger commented Apr 24, 2016

notriddle commented Apr 24, 2016 • edited

nox commented Apr 26, 2016

jdm commented Apr 26, 2016

jdm commented Apr 29, 2016

askeing commented Apr 30, 2016 • edited

jdm commented Apr 30, 2016

askeing commented Apr 30, 2016 • edited

jdm commented May 3, 2016

askeing commented May 4, 2016

jdm commented May 4, 2016

highfive commented May 4, 2016

jdm commented May 4, 2016

bors-servo commented May 6, 2016

mbrubeck commented May 6, 2016

bors-servo commented May 6, 2016

bors-servo commented May 6, 2016

larsbergstrom commented May 6, 2016

bors-servo commented May 6, 2016

bors-servo commented May 6, 2016

highfive commented May 6, 2016

cbrewster commented May 6, 2016

bors-servo commented May 6, 2016

mbrubeck commented May 6, 2016 • edited

bors-servo commented May 6, 2016

notriddle commented Apr 24, 2016 •

edited

askeing commented Apr 30, 2016 •

edited

askeing commented Apr 30, 2016 •

edited

mbrubeck commented May 6, 2016 •

edited