Initialize iframe viewport immediately by jdm · Pull Request #22395 · servo/servo

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 Transitory initial width and height for windows in iframes #14364 and fix Transparent Rects demo is confined to top left corner #15689 and fix Iframe's window size is not available synchronously in about:blank #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...

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...

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...

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...

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

bors-servo · 2018-12-18T01:43:27Z

⌛ Testing commit 496e2f6 with merge 5eb3d27...

bors-servo · 2018-12-18T01:44:49Z

💔 Test failed - status-taskcluster

CYBAI · 2018-12-18T06:24:17Z

@bors-servo retry

bors-servo · 2018-12-18T07:09:31Z

⌛ Testing commit 496e2f6 with merge 0d2e811...

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...

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...

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

        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.

highfive assigned Manishearth Dec 9, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 9, 2018

highfive added the S-tests-failed The changes caused existing tests to fail. label Dec 9, 2018

jdm force-pushed the initial-iframe-size branch from af4cc5c to c8fb961 Compare December 9, 2018 19:36

highfive removed the S-tests-failed The changes caused existing tests to fail. label Dec 9, 2018

highfive added the S-tests-failed The changes caused existing tests to fail. label Dec 9, 2018

highfive removed the S-tests-failed The changes caused existing tests to fail. label Dec 10, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 10, 2018

highfive added the S-tests-failed The changes caused existing tests to fail. label Dec 10, 2018

jdm force-pushed the initial-iframe-size branch from 9bee8ea to 1655386 Compare December 10, 2018 06:24

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 18, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Dec 18, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 18, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Dec 18, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 18, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels 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 reviewed changes

jdm mentioned this pull request Nov 6, 2019

Ensure all iframes are sized correctly at creation #24677

Merged

3 tasks

Uh oh!

Conversation

jdm commented Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Dec 9, 2018

Uh oh!

highfive commented Dec 9, 2018

Uh oh!

jdm commented Dec 9, 2018

Uh oh!

bors-servo commented Dec 9, 2018

Uh oh!

bors-servo commented Dec 9, 2018

Uh oh!

jdm commented Dec 9, 2018

Uh oh!

bors-servo commented Dec 9, 2018

Uh oh!

bors-servo commented Dec 9, 2018

Uh oh!

jdm commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdm commented Dec 10, 2018

Uh oh!

bors-servo commented Dec 10, 2018

Uh oh!

bors-servo commented Dec 10, 2018

Uh oh!

jdm commented Dec 10, 2018

Uh oh!

bors-servo commented Dec 10, 2018

Uh oh!

bors-servo commented Dec 10, 2018

Uh oh!

bors-servo commented Dec 18, 2018

Uh oh!

bors-servo commented Dec 18, 2018

Uh oh!

CYBAI commented Dec 18, 2018

Uh oh!

bors-servo commented Dec 18, 2018

Uh oh!

bors-servo commented Dec 18, 2018

Uh oh!

CYBAI commented Dec 18, 2018

Uh oh!

bors-servo commented Dec 18, 2018

Uh oh!

bors-servo commented Dec 18, 2018

Uh oh!

bors-servo commented Dec 18, 2018

Uh oh!

CYBAI commented Dec 18, 2018

Uh oh!

bors-servo commented Dec 18, 2018

Uh oh!

bors-servo commented Dec 18, 2018

Uh oh!

nox Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

nox Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

jdm Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jdm commented Dec 9, 2018 •

edited

Loading

jdm commented Dec 10, 2018 •

edited

Loading