Initialize iframe viewport immediately #22395

jdm · 2018-12-09T19:27:35Z

This is probably a significant cause of unstable test results, and it finally bothered me enough to fix it. The solution isn't great for performance (a sync layout query every time a iframe is added to a document), but performance needs to follow correctness.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14364 and fix #15689 and fix #15688.
There are tests for these changes

This change is

highfive · 2018-12-09T19:27:39Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/constellation/pipeline.rs, components/script/dom/windowproxy.rs, components/constellation/constellation.rs and 5 more
@cbrewster: components/constellation/pipeline.rs, components/constellation/constellation.rs, components/constellation/browsingcontext.rs
@paulrouget: components/constellation/pipeline.rs, components/compositing/lib.rs, components/constellation/constellation.rs, components/constellation/browsingcontext.rs
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/script/dom/windowproxy.rs, components/script_traits/lib.rs, components/script/dom/htmlimageelement.rs and 3 more

highfive · 2018-12-09T19:27:41Z

Warning

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

jdm · 2018-12-09T19:27:42Z

@bors-servo try=wpt

bors-servo · 2018-12-09T19:27:47Z

⌛ Trying commit af4cc5c with merge a39bbff...


        Auto merge of #22395 - jdm:initial-iframe-size, r=<try>

WIP initialize iframe viewport immediately This is probably a significant cause of unstable test results, and it finally bothered me enough to fix it. The solution isn't great for performance (a sync layout query every time a iframe is added to a document), but performance needs to follow correctness. --- - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [x] These changes fix #14364 and fix #15689 and fix #15688. - [x] There are tests for these changes

bors-servo · 2018-12-09T19:32:03Z

💔 Test failed - linux-rel-wpt

jdm · 2018-12-09T19:36:21Z

@bors-servo try=wpt

bors-servo · 2018-12-09T19:36:26Z

⌛ Trying commit c8fb961 with merge 4e0f3f7...


        Auto merge of #22395 - jdm:initial-iframe-size, r=<try>

WIP initialize iframe viewport immediately This is probably a significant cause of unstable test results, and it finally bothered me enough to fix it. The solution isn't great for performance (a sync layout query every time a iframe is added to a document), but performance needs to follow correctness. --- - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [x] These changes fix #14364 and fix #15689 and fix #15688. - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22395)

bors-servo · 2018-12-09T20:03:11Z

💔 Test failed - linux-rel-wpt

jdm · 2018-12-10T01:54:25Z

Who could have foreseen that invoking layout inside of a method that can be called while the document is in an unstable state (ie. from the Node::bind_to_tree callback, which is invoked during the Node::insert algorithm) could lead to layout's head exploding?

jdm · 2018-12-10T03:09:14Z

@bors-servo try=wpt

bors-servo · 2018-12-10T03:09:19Z

⌛ Trying commit fc41ddc with merge f9c115f...


        Auto merge of #22395 - jdm:initial-iframe-size, r=<try>

WIP initialize iframe viewport immediately This is probably a significant cause of unstable test results, and it finally bothered me enough to fix it. The solution isn't great for performance (a sync layout query every time a iframe is added to a document), but performance needs to follow correctness. --- - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [x] These changes fix #14364 and fix #15689 and fix #15688. - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22395)

bors-servo · 2018-12-10T03:30:06Z

💔 Test failed - linux-rel-css

jdm · 2018-12-10T04:52:59Z

@bors-servo try=wpt

bors-servo · 2018-12-10T04:53:04Z

⌛ Trying commit 9bee8ea with merge 90234c4...


        Auto merge of #22395 - jdm:initial-iframe-size, r=<try>

WIP initialize iframe viewport immediately This is probably a significant cause of unstable test results, and it finally bothered me enough to fix it. The solution isn't great for performance (a sync layout query every time a iframe is added to a document), but performance needs to follow correctness. --- - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [x] These changes fix #14364 and fix #15689 and fix #15688. - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22395)

bors-servo · 2018-12-10T05:15:40Z

💔 Test failed - linux-rel-css


        Auto merge of #22395 - jdm:initial-iframe-size, r=Manishearth

Initialize iframe viewport immediately This is probably a significant cause of unstable test results, and it finally bothered me enough to fix it. The solution isn't great for performance (a sync layout query every time a iframe is added to a document), but performance needs to follow correctness. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14364 and fix #15689 and fix #15688. - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22395)

bors-servo · 2018-12-18T07:40:34Z

💔 Test failed - status-taskcluster

CYBAI · 2018-12-18T08:43:51Z

@bors-servo retry

{
  "status": "TIMEOUT",
  "group": "default",
  "message": null,
  "stack": null,
  "subtest": null,
  "test": "/html/semantics/embedded-content/the-iframe-element/same_origin_parentage.html",
  "line": 135833,
  "action": "test_result",
  "expected": "OK"
}

bors-servo · 2018-12-18T08:52:08Z

💣 Failed to start rebuilding: Unknown error

bors-servo · 2018-12-18T08:52:14Z

⌛ Testing commit 496e2f6 with merge bfdb529...


        Auto merge of #22395 - jdm:initial-iframe-size, r=Manishearth

Initialize iframe viewport immediately This is probably a significant cause of unstable test results, and it finally bothered me enough to fix it. The solution isn't great for performance (a sync layout query every time a iframe is added to a document), but performance needs to follow correctness. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14364 and fix #15689 and fix #15688. - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22395)

bors-servo · 2018-12-18T09:36:56Z

💔 Test failed - status-taskcluster

CYBAI · 2018-12-18T12:41:54Z

@bors-servo retry

{
    "status": "TIMEOUT", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/wasm/jsapi/table/constructor.any.html", 
    "line": 132064, 
    "action": "test_result", 
    "expected": "OK"
}
{
    "status": "TIMEOUT", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/performance-timeline/po-getentries.any.worker.html", 
    "line": 132152, 
    "action": "test_result", 
    "expected": "OK"
}

bors-servo · 2018-12-18T13:51:30Z

⌛ Testing commit 496e2f6 with merge c553c43...


        Auto merge of #22395 - jdm:initial-iframe-size, r=Manishearth

Initialize iframe viewport immediately This is probably a significant cause of unstable test results, and it finally bothered me enough to fix it. The solution isn't great for performance (a sync layout query every time a iframe is added to a document), but performance needs to follow correctness. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14364 and fix #15689 and fix #15688. - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22395)

bors-servo · 2018-12-18T15:23:18Z

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: Manishearth
Pushing c553c43 to master...

nox · 2019-01-31T10:57:23Z

components/script/dom/htmlscriptelement.rs

@@ -777,7 +777,10 @@ impl VirtualMethods for HTMLScriptElement {
        }

        if tree_in_doc && !self.parser_inserted.get() {
-            self.prepare();
+            let script = Trusted::new(self);
+            document_from_node(self).add_delayed_task(task!(ScriptDelayedInitialize: move || {


Where is it specified that scripts shouldn't be immediately prepared?

This seems to specify the opposite.

https://html.spec.whatwg.org/multipage/scripting.html#script-processing-model:prepare-a-script-4

It still is immeditate, but it waits until the current DOM modification is complete, so no intermediate state can be observed by script or layout.


        Auto merge of #24677 - jdm:iframe-sizing-cleanup, r=<try>

Iframe sizing cleanup Following in the path of #22395, these commits serve several purposes: * prevent layout instability early during iframe loads caused by a succession of resize events * reduce the complexity of determining what actual DPI and initial window size values are being used at startup * ensure that all documents have a correct initial viewport size at creation These changes fix problems that were exposed by the changes in #24462 but are independent of that PR. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes


        Auto merge of #24677 - jdm:iframe-sizing-cleanup, r=asajeffrey

Ensure all iframes are sized correctly at creation Following in the path of #22395, these commits serve several purposes: * prevent layout instability early during iframe loads caused by a succession of resize events * reduce the complexity of determining what actual DPI and initial window size values are being used at startup * ensure that all documents have a correct initial viewport size at creation These changes fix problems that were exposed by the changes in #24462 but are independent of that PR. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes


        Auto merge of #24677 - jdm:iframe-sizing-cleanup, r=asajeffrey

Ensure all iframes are sized correctly at creation Following in the path of #22395, these commits serve several purposes: * prevent layout instability early during iframe loads caused by a succession of resize events * reduce the complexity of determining what actual DPI and initial window size values are being used at startup * ensure that all documents have a correct initial viewport size at creation These changes fix problems that were exposed by the changes in #24462 but are independent of that PR. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes

highfive assigned Manishearth Dec 9, 2018

highfive added the S-awaiting-review label Dec 9, 2018

highfive added the S-tests-failed label Dec 9, 2018

jdm force-pushed the jdm:initial-iframe-size branch from af4cc5c to c8fb961 Dec 9, 2018

highfive removed the S-tests-failed label Dec 9, 2018

highfive added the S-tests-failed label Dec 9, 2018

highfive removed the S-tests-failed label Dec 10, 2018

highfive added S-tests-failed and removed S-tests-failed labels Dec 10, 2018

highfive added the S-tests-failed label Dec 10, 2018

jdm force-pushed the jdm:initial-iframe-size branch from 9bee8ea to 1655386 Dec 10, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 18, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 18, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 18, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 18, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 18, 2018

bors-servo merged commit 496e2f6 into servo:master Dec 18, 2018
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 18, 2018

This was referenced Dec 18, 2018

Upgrade to rustc 1.33.0-nightly (fb86d604b 2018-12-27) #22385

Merged

style: Make Servo use a single thread-pool for layout-related tasks per process. #22487

Merged

nox reviewed Jan 31, 2019

View changes

jdm mentioned this pull request Nov 6, 2019

Ensure all iframes are sized correctly at creation #24677

Merged

3 of 3 tasks complete

servo / servo

Initialize iframe viewport immediately #22395

Initialize iframe viewport immediately #22395

jdm commented Dec 9, 2018 •

edited

highfive commented Dec 9, 2018

highfive commented Dec 9, 2018

jdm commented Dec 9, 2018

bors-servo commented Dec 9, 2018

bors-servo commented Dec 9, 2018

jdm commented Dec 9, 2018

bors-servo commented Dec 9, 2018

bors-servo commented Dec 9, 2018

jdm commented Dec 10, 2018 •

edited

jdm commented Dec 10, 2018

bors-servo commented Dec 10, 2018

bors-servo commented Dec 10, 2018

jdm commented Dec 10, 2018

bors-servo commented Dec 10, 2018

bors-servo commented Dec 10, 2018

bors-servo commented Dec 18, 2018

CYBAI commented Dec 18, 2018

bors-servo commented Dec 18, 2018

bors-servo commented Dec 18, 2018

bors-servo commented Dec 18, 2018

CYBAI commented Dec 18, 2018

bors-servo commented Dec 18, 2018

bors-servo commented Dec 18, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

servo / servo

Join GitHub today

Initialize iframe viewport immediately #22395

Initialize iframe viewport immediately #22395

Conversation

jdm commented Dec 9, 2018 • edited

highfive commented Dec 9, 2018

highfive commented Dec 9, 2018

jdm commented Dec 9, 2018

bors-servo commented Dec 9, 2018

bors-servo commented Dec 9, 2018

jdm commented Dec 9, 2018

bors-servo commented Dec 9, 2018

bors-servo commented Dec 9, 2018

jdm commented Dec 10, 2018 • edited

jdm commented Dec 10, 2018

bors-servo commented Dec 10, 2018

bors-servo commented Dec 10, 2018

jdm commented Dec 10, 2018

bors-servo commented Dec 10, 2018

bors-servo commented Dec 10, 2018

bors-servo commented Dec 18, 2018

CYBAI commented Dec 18, 2018

bors-servo commented Dec 18, 2018

bors-servo commented Dec 18, 2018

bors-servo commented Dec 18, 2018

CYBAI commented Dec 18, 2018

bors-servo commented Dec 18, 2018

bors-servo commented Dec 18, 2018

This comment has been minimized.

nox Jan 31, 2019

This comment has been minimized.

nox Jan 31, 2019

This comment has been minimized.

jdm Jan 31, 2019

jdm commented Dec 9, 2018 •

edited

jdm commented Dec 10, 2018 •

edited