Make document url mutable and implement location.replace() #13418

stshine · 2016-09-25T22:51:12Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13413 (github issue number if applicable).

There are tests for these changes OR

This change is

highfive · 2016-09-25T22:51:15Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/webidls/Location.webidl, components/script/dom/location.rs

highfive · 2016-09-25T22:51:16Z

Warning

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

stshine · 2016-09-25T22:51:47Z

@bors-servo try


        Auto merge of #13418 - stshine:location-replace, r=<try>

Implement Location.replace()  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13413 (github issue number if applicable).  - [X] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13418)

bors-servo · 2016-09-25T22:51:50Z

⌛ Trying commit 8103751 with merge 8a018c0...

bors-servo · 2016-09-25T22:52:18Z

💔 Test failed - linux-rel

stshine · 2016-09-25T23:02:18Z

@bors-servo try

KiChjang · 2016-09-26T00:56:18Z

@bors-servo retry

bors-servo · 2016-09-26T00:56:21Z

⌛ Trying commit 8103751 with merge d0e866e...


        Auto merge of #13418 - stshine:location-replace, r=<try>

Implement Location.replace()  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13413 (github issue number if applicable).  - [X] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13418)

KiChjang · 2016-09-26T00:58:11Z

r=me once you've updated the test expectations. If there are none, you'll need to write one.

@bors-servo delegate+

bors-servo · 2016-09-26T00:58:12Z

✌️ @stshine can now approve this pull request

bors-servo · 2016-09-26T01:47:36Z

💔 Test failed - linux-rel

jdm · 2016-09-26T02:26:06Z

I would be interested in knowing why https://dxr.mozilla.org/servo/source/tests/wpt/web-platform-tests/html/browsers/history/the-location-interface/location_replace.html still fails.

Also, this isn't ready for merging yet - step 1 of https://html.spec.whatwg.org/multipage/browsers.html#dom-location-replace says that we should throw an exception if parsing fails, but that's not included in this PR.

jdm · 2016-09-26T02:31:43Z

We should add a test to location_replace.html for the parse error exception; we can duplicate the one in location_assign.html for that purpose.

stshine · 2016-09-26T04:00:01Z

Some mysterious fails on local machines disappeared.
And Thank for point out the location of the right tests.

stshine · 2016-09-26T16:44:52Z

Tried to construct a minimal test case:

<!DOCTYPE html>
<html>
    <head>
        <style>
         .blank {
             height: 600px;
         }
        </style>
    </head>
    <body>
        <p>Foo</p>
        <div class="blank"></div>
        <a id="x" href="#x">Bar</a>
        <div class="blank"></div>
    </body>
    <script>
     console.log(location.href);
     location.replace("#x");
     setTimeout("eval('console.log(location.href)')", 1000);
    </script>
</html>

Try to serve this file with python3 -m http.server and visit it.
In firefox the output in console is

http://localhost:8000/new.html
http://localhost:8000/new.html#x

while in servo the output is :

http://localhost:8000/new.html
http://localhost:8000/new.html

Tried to dig into the code of load_url() and it look like for url that only different in fragment with the url of current document, servo eventually called into ScriptThread::check_and_scroll_fragment() in https://github.com/servo/servo/blob/master/components/script/script_thread.rs#L2023 , which only set target state on the target element and scroll to to the frame point.
How am I supposed to fix this?

jdm · 2016-09-26T16:55:38Z

I see what you mean. For the purposes of this PR, I propose we add an API to Document to set the document's URL and call it from load_url before returning when changing the fragment. I've filed #13437 about integrating this with session history, too.

stshine · 2016-09-27T07:28:43Z

So... wrap the url field in Cell? But the Url struct does not implement the Copy trait. And Copy can not be simply derived because it contains a String field.

jdm · 2016-09-27T12:24:21Z

DOMRefCell.

stshine · 2016-09-28T00:30:35Z

Finally managed to make document.url work as intented, but now I got a bunch of erros from wpt runner itself like this:

 ▶ ERROR [expected TIMEOUT] /html/browsers/browsing-the-web/scroll-to-fragid/005.html
  └   → Got results from /html/browsers/browsing-the-web/scroll-to-fragid/005.html#test, expected /html/browsers/browsing
-the-web/scroll-to-fragid/005.html                                                                                      
Traceback (most recent call last):
  File "/home/stshine/Programs/servo/tests/wpt/harness/wptrunner/executors/base.py", line 149, in run_test
    result = self.do_test(test)
  File "/home/stshine/Programs/servo/tests/wpt/harness/wptrunner/executors/executorservo.py", line 119, in do_test
    result = self.convert_result(test, self.result_data)
  File "/home/stshine/Programs/servo/tests/wpt/harness/wptrunner/executors/base.py", line 60, in __call__
    (result_url, test.url))
AssertionError: Got results from /html/browsers/browsing-the-web/scroll-to-fragid/005.html#test, expected /html/browsers/
browsing-the-web/scroll-to-fragid/005.html

Maybe it is not a good idea to set document.url ...

stshine · 2016-09-28T00:35:56Z

Push my changes up for review. The way I appease borrow checker with RefCell feels quite awkward.

KiChjang · 2016-09-28T01:04:51Z

components/script/script_thread.rs

@@ -1195,7 +1195,7 @@ impl ScriptThread {

        if let Some(fragment) = doc.url().fragment() {
            self.check_and_scroll_fragment(fragment, pipeline, doc);
-        }
+        };


nit: Remove this extra ;.

This is to appease borrow checker on RefCell in if let. See rust-lang/rust#22449

KiChjang · 2016-09-28T01:04:51Z

components/script/script_thread.rs

                    return;
-                }
+                } 


nit: Remove extra space after }.

KiChjang · 2016-09-28T01:04:51Z

components/script/script_thread.rs

@@ -1217,7 +1217,8 @@ impl ScriptThread {

        if let Some(root_context) = self.browsing_context.get() {
            for it_context in root_context.iter() {
-                let current_url = it_context.active_document().url().to_string();
+                let doc = it_context.active_document();
+                let current_url = doc.url().to_string();


This looks like a premature optimization - no code is using the doc variable at all, except for current_url, so we can just keep the previous code here (i.e. everything on 1 line).

This is also for appease the borrow checker, we hold the document in temp variable so the Ref from .url() will get out of scope before it. And you just can not place the Ref<T> in the return expression of the function. See rust-lang/rust#23338

KiChjang · 2016-09-28T01:04:51Z

components/script/script_thread.rs

                if &url[..Position::AfterQuery] == &nurl[..Position::AfterQuery] &&
                    load_data.method == Method::Get {
                    self.check_and_scroll_fragment(fragment, parent_pipeline_id, document.r());
+                    document.set_url(nurl);


We need to leave a note here saying that this is wrong, and should be fixed appropriately via #13437.

Well, Eventually I think this commit needs to be reverted because a lot of test failed, and I push this commit mostly to notify what actually happened (and to sync between different computers). I hope to get some design suggestions on how the URL fragment should be stored for both the location API and for navigation management.

KiChjang · 2016-09-28T01:04:51Z

components/script/dom/location.rs

+        let base_url = self.window.get_url();
+        if let Ok(url) = base_url.join(&url.0) {
+            self.window.load_url(url, true, None);
+        }


This code is still not throwing any exceptions.

KiChjang · 2016-09-28T01:04:51Z

components/script/dom/htmlbodyelement.rs

-                AttrValue::from_url(document_from_node(self).url(), value.into())
+                let doc = document_from_node(self);
+                let val = AttrValue::from_url(&*doc.url(), value.into());
+                val 


This creates 2 temporary stack variables for the sake of readability and sacrifices performance. Not a good thing here.

KiChjang · 2016-09-28T01:04:51Z

components/script/dom/document.rs

+    }
+
+    pub fn set_url(&self, url: &Url) {
+        *self.url.borrow_mut() = url.clone();


Why is a DOMRefCell necessary? Does returning a mutable reference to self.url not work?

Also, we need to leave a FIXME here pointing to #13437.

That would need to obtain the document as mutable, which I did not figure out how to do that.

pub fn url_mut(&mut self) { &mut self.url }

No, no, NO. There are no &mut references to DOM objects.

This could really take in a Url instead of a &Url.

KiChjang · 2016-09-28T01:04:51Z

components/script/dom/canvasrenderingcontext2d.rs

-                        url.origin() == node.owner_doc().url().origin()
+                        let doc = node.owner_doc();
+                        let result = url.origin() == doc.url().origin();
+                        result


This is adding 2 temporary stack variables here for no gain in readability and added verbosity. Revert.

Needs comment linking to rust-lang/rust#37785.

bors-servo · 2016-11-19T16:26:51Z

⌛ Testing commit dc4bc99 with merge b8cb92a...


        Auto merge of #13418 - stshine:location-replace, r=KiChjang

Make document url mutable and implement location.replace()  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13413 (github issue number if applicable).  - [X] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13418)

bors-servo · 2016-11-19T17:02:50Z

💔 Test failed - linux-rel-wpt

highfive · 2016-11-19T17:02:52Z

  ▶ FAIL [expected PASS] /_mozilla/css/incremental_trailing_whitespace_a.html
  └   → /_mozilla/css/incremental_t</span><span class="stdout">railing_whitespace_a.html da54041086f2975f8e3c776d2283ad6609e6862a
/_mozilla/css/incremental_trailing_whitespace_ref.html f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b
Testing da54041086f2975f8e3c776d2283ad6609e6862a == f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b


        Minor fixes and update test expectations

stshine · 2016-11-19T22:34:03Z

@bors-servo r=KiChjang
#10473
comment addressed.

bors-servo · 2016-11-19T22:34:05Z

📌 Commit 5287e70 has been approved by KiChjang

bors-servo · 2016-11-20T02:23:03Z

⌛ Testing commit 5287e70 with merge debad8a...


        Auto merge of #13418 - stshine:location-replace, r=KiChjang

Make document url mutable and implement location.replace()  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13413 (github issue number if applicable).  - [X] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13418)

bors-servo · 2016-11-20T03:13:13Z

💔 Test failed - linux-rel-wpt

KiChjang · 2016-11-20T06:58:35Z

@bors-servo retry

#12797

bors-servo · 2016-11-20T08:27:55Z

⌛ Testing commit 5287e70 with merge 72e4c6d...


        Auto merge of #13418 - stshine:location-replace, r=KiChjang

Make document url mutable and implement location.replace()  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13413 (github issue number if applicable).  - [X] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13418)

bors-servo · 2016-11-20T09:26:33Z

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

stshine · 2016-11-20T12:04:46Z

Waaaaoh! Finally this PR got merged before it has 150 comments. Thanks @KiChjang and @jdm for review, please forgive my single-threaded head :)

emilio · 2016-11-20T13:16:57Z

Thank you for being so persistent :)

On Sun, Nov 20, 2016 at 04:04:52AM -0800, stshine wrote:

Waaaaoh! Finally this PR got merged before it has 150 comments. Thanks @KiChjang and @jdm for review, please forgive my single-threaded head :)

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#13418 (comment)

highfive assigned mbrubeck Sep 25, 2016

highfive added the S-awaiting-review label Sep 25, 2016

highfive added the S-tests-failed label Sep 25, 2016

highfive added the S-tests-failed label Sep 26, 2016

mbrubeck removed the S-awaiting-review label Sep 26, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Sep 28, 2016

KiChjang requested changes Sep 28, 2016

View changes

highfive removed the S-awaiting-merge label Nov 19, 2016

highfive added the S-tests-failed label Nov 19, 2016

Minor fixes and update test expectations

Loading status checks…

5287e70

stshine force-pushed the stshine:location-replace branch from dc4bc99 to 5287e70 Nov 19, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Nov 19, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 19, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 20, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 20, 2016

bors-servo merged commit 5287e70 into servo:master Nov 20, 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 Nov 20, 2016

stshine deleted the stshine:location-replace branch Nov 20, 2016

servo / servo

Join GitHub today

Make document url mutable and implement location.replace() #13418

Make document url mutable and implement location.replace() #13418

Conversation

stshine commented Sep 25, 2016 • edited by larsbergstrom

highfive commented Sep 25, 2016

highfive commented Sep 25, 2016

stshine commented Sep 25, 2016

bors-servo commented Sep 25, 2016

bors-servo commented Sep 25, 2016

stshine commented Sep 25, 2016

KiChjang commented Sep 26, 2016

bors-servo commented Sep 26, 2016

KiChjang commented Sep 26, 2016

bors-servo commented Sep 26, 2016

bors-servo commented Sep 26, 2016

jdm commented Sep 26, 2016

jdm commented Sep 26, 2016

stshine commented Sep 26, 2016

stshine commented Sep 26, 2016

jdm commented Sep 26, 2016

stshine commented Sep 27, 2016 • edited

jdm commented Sep 27, 2016

stshine commented Sep 28, 2016

stshine commented Sep 28, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

stshine Sep 28, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang Sep 28, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 19, 2016

bors-servo commented Nov 19, 2016

highfive commented Nov 19, 2016

stshine commented Nov 19, 2016

bors-servo commented Nov 19, 2016

bors-servo commented Nov 20, 2016

bors-servo commented Nov 20, 2016

KiChjang commented Nov 20, 2016

bors-servo commented Nov 20, 2016

bors-servo commented Nov 20, 2016

stshine commented Nov 20, 2016

emilio commented Nov 20, 2016

stshine commented Sep 25, 2016 •

edited by larsbergstrom

stshine commented Sep 27, 2016 •

edited

stshine Sep 28, 2016 •

edited

KiChjang Sep 28, 2016 •

edited