Set navigation start value according to navigation timing spec #17735

ferjm · 2017-07-14T18:30:28Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #17651

This change is

highfive · 2017-07-14T18:30:33Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/network_listener.rs, components/constellation/constellation.rs
@fitzgen: components/script/dom/document.rs, components/script/script_thread.rs, components/script/document_loader.rs, components/script/dom/eventsource.rs, components/script/dom/window.rs and 2 more
@KiChjang: components/script/dom/document.rs, components/net/fetch/methods.rs, components/net/resource_thread.rs, components/script/script_thread.rs, components/net_traits/lib.rs and 8 more

highfive · 2017-07-14T18:30:34Z

Warning

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

ferjm · 2017-07-14T18:30:55Z

r? @jdm

jdm · 2017-07-14T19:35:24Z

This seems more complicated than necessary. The value for navigation start would make more sense as the time at which the fetch was initiated by the thread that communicates with the network thread, rather than the time at which the fetch algorithm starts executing on the network thread.

bors-servo · 2017-07-17T16:19:31Z

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

ferjm · 2017-07-17T21:47:21Z

r? @jdm

jdm · 2017-07-17T23:04:47Z

components/script/script_thread.rs

@@ -181,6 +185,8 @@ impl InProgressLoad {
            is_visible: true,
            url: url,
            origin: origin,
+            navigation_start: 0,
+            navigation_start_precise: 0.,


Why don't we store the values here? No need to for any of the constellation to be aware of the timing code at all.

I am not sure what you mean with storing the values here, they are already stored in InProgressLoad during the handling of the constellation message to script that notifies about the start of the navigation. Anyway, I agree that the constellation does not need to be aware of the timing code, so I am removing that part. Thanks!

To rephrase my question - why don't we get the current time here and store it immediately, instead of waiting for the constellation? This both simplifies the code and gives us a more accurate initial time.

Ok, thank you for rephrasing the question. I added the constellation code trying to address your suggestion of setting the navigation start time as the time at which the thread that communicates with the network thread initiates the fetch. Setting navigation start during InProgressLoad construction does not vary the value significantly, but it indeed simplifies the code, so I'll change it as you suggests. Thanks for the feedback and apologies for the back and forth.

ferjm · 2017-07-18T12:53:38Z

r? @jdm

ferjm · 2017-07-18T14:14:35Z

r? @jdm

jdm

This looks fine with a simple change. Sorry my descriptions suggested more complexity than was necessary!

jdm · 2017-07-18T14:27:48Z

components/script/dom/window.rs

-    navigation_start: u64,
-    navigation_start_precise: f64,
+    navigation_start: DOMRefCell<u64>,
+    navigation_start_precise: DOMRefCell<f64>,


These can be Cell instead.


        Set navigation start value according to navigation timing spec

ferjm · 2017-07-18T16:03:44Z

@bors-servo r=jdm

bors-servo · 2017-07-18T16:03:45Z

📌 Commit db044bd has been approved by jdm

bors-servo · 2017-07-18T17:24:46Z

⌛ Testing commit db044bd with merge eb26194...


        Auto merge of #17735 - ferjm:navigationstart, r=jdm

Set navigation start value according to navigation timing spec - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #17651  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17735)

bors-servo · 2017-07-18T18:26:53Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing eb26194 to master...

highfive assigned wafflespeanut Jul 14, 2017

highfive added the S-awaiting-review label Jul 14, 2017

highfive assigned jdm and unassigned wafflespeanut Jul 14, 2017

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 14, 2017

highfive added the S-needs-rebase label Jul 17, 2017

ferjm force-pushed the ferjm:navigationstart branch from 252ea27 to 75e953d Jul 17, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 17, 2017

ferjm force-pushed the ferjm:navigationstart branch from 75e953d to c4c78da Jul 17, 2017

jdm requested changes Jul 17, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Jul 17, 2017

ferjm force-pushed the ferjm:navigationstart branch from c4c78da to 5377b42 Jul 18, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 18, 2017

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 18, 2017

ferjm force-pushed the ferjm:navigationstart branch from 5377b42 to 9ebf234 Jul 18, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 18, 2017

jdm approved these changes Jul 18, 2017

View changes

components/script/dom/window.rs Outdated

navigation_start: u64,

navigation_start_precise: f64,

navigation_start: DOMRefCell<u64>,

navigation_start_precise: DOMRefCell<f64>,

This comment has been minimized.

Sign in to view

jdm Jul 18, 2017
Member

These can be Cell instead.

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 18, 2017

Set navigation start value according to navigation timing spec

Loading status checks…

db044bd

ferjm force-pushed the ferjm:navigationstart branch from 9ebf234 to db044bd Jul 18, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 18, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 18, 2017

bors-servo merged commit db044bd into servo:master Jul 18, 2017
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 Jul 18, 2017

This was referenced Jul 18, 2017

Fix iframe.onload being fired twice on about:blank. #16091

Closed

cleanup embedder/compositor/constellation/script messages #17425

Merged

servo / servo

Set navigation start value according to navigation timing spec #17735

Set navigation start value according to navigation timing spec #17735

ferjm commented Jul 14, 2017 •

edited by larsbergstrom

highfive commented Jul 14, 2017

highfive commented Jul 14, 2017

ferjm commented Jul 14, 2017

jdm commented Jul 14, 2017

bors-servo commented Jul 17, 2017

ferjm commented Jul 17, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Jul 18, 2017

ferjm commented Jul 18, 2017

jdm left a comment

This comment has been minimized.

ferjm commented Jul 18, 2017

bors-servo commented Jul 18, 2017

bors-servo commented Jul 18, 2017

bors-servo commented Jul 18, 2017

servo / servo

Join GitHub today

Set navigation start value according to navigation timing spec #17735

Set navigation start value according to navigation timing spec #17735

Conversation

ferjm commented Jul 14, 2017 • edited by larsbergstrom

highfive commented Jul 14, 2017

highfive commented Jul 14, 2017

ferjm commented Jul 14, 2017

jdm commented Jul 14, 2017

bors-servo commented Jul 17, 2017

ferjm commented Jul 17, 2017

This comment has been minimized.

jdm Jul 17, 2017

This comment has been minimized.

ferjm Jul 18, 2017

This comment has been minimized.

jdm Jul 18, 2017

This comment has been minimized.

ferjm Jul 18, 2017

ferjm commented Jul 18, 2017

ferjm commented Jul 18, 2017

jdm left a comment

This comment has been minimized.

jdm Jul 18, 2017

ferjm commented Jul 18, 2017

bors-servo commented Jul 18, 2017

bors-servo commented Jul 18, 2017

bors-servo commented Jul 18, 2017

ferjm commented Jul 14, 2017 •

edited by larsbergstrom