Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mark the page source as loaded only after parsing is done #15098

Merged
merged 3 commits into from Jan 19, 2017
Merged

Mark the page source as loaded only after parsing is done #15098

merged 3 commits into from Jan 19, 2017

Conversation

nox
Copy link
Contributor

@nox nox commented Jan 18, 2017 •
edited by larsbergstrom

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/document.rs, components/script/dom/servoparser/html.rs, components/script/dom/servoparser/mod.rs, components/script/dom/servoparser/xml.rs, components/script/document_loader.rs, components/script/script_thread.rs
  • @KiChjang: components/script/dom/document.rs, components/script/dom/servoparser/html.rs, components/script/dom/servoparser/mod.rs, components/script/dom/servoparser/xml.rs, components/script/document_loader.rs, components/script/script_thread.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 18, 2017
@nox
Copy link
Contributor Author

nox commented Jan 18, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 2f02b68 with merge 8d7369d...

bors-servo pushed a commit that referenced this pull request Jan 18, 2017
Auto merge of #15098 - nox:load-fixes, r=<try>
8d7369d 
Mark the page source as loaded only after parsing is done

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15098)
<!-- Reviewable:end -->
jdm
jdm requested changes Jan 18, 2017
View reviewed changes
components/script/dom/document.rs
@@ -1572,6 +1572,7 @@ impl Document {
}

if !self.loader.borrow().is_blocked() && !self.loader.borrow().events_inhibited() {
self.loader.borrow_mut().inhibit_events();
// Schedule a task to fire a "load" event (if no blocking loads have arrived in the mean time)
// NOTE: we can end up executing this code more than once, in case more blocking loads arrive.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is no longer true.

components/script/dom/servoparser/mod.rs
self.document.disarm_reflow_timeout();
self.document.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
let window = self.document.window();
window.reflow(ReflowGoal::ForDisplay, ReflowQueryType::NoQuery, ReflowReason::FirstLoad);
}

// Step 3.
// Steps 3-12 are in another castle.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cute, but this neither explains what part of the spec this code implements nor where to find steps 3-12.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 18, 2017
@jdm jdm assigned jdm and unassigned KiChjang Jan 18, 2017
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 18, 2017
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jan 18, 2017
@nox
Copy link
Contributor Author

nox commented Jan 18, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit bc62404 with merge 92d2e91...

bors-servo pushed a commit that referenced this pull request Jan 18, 2017
Auto merge of #15098 - nox:load-fixes, r=<try>
92d2e91 
Mark the page source as loaded only after parsing is done

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15098)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-dev

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 18, 2017
@jdm jdm added the S-fails-tidy `./mach test-tidy` reported errors. label Jan 18, 2017
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Jan 18, 2017
@nox
Copy link
Contributor Author

nox commented Jan 18, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 84845d2 with merge 807aff4...

bors-servo pushed a commit that referenced this pull request Jan 18, 2017
Auto merge of #15098 - nox:load-fixes, r=<try>
807aff4 
Mark the page source as loaded only after parsing is done

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15098)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this pull request Jan 19, 2017
Auto merge of #15098 - nox:load-fixes, r=<try>
69cc001 
Mark the page source as loaded only after parsing is done

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15098)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 19, 2017
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Jan 19, 2017
jdm
jdm requested changes Jan 19, 2017
View reviewed changes
components/script/dom/document.rs
debug!("Document loads are complete.");
let win = self.window();
let msg = MainThreadScriptMsg::DocumentLoadsComplete(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove this variant and the code that handles it.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. S-fails-tidy `./mach test-tidy` reported errors. labels Jan 19, 2017
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jan 19, 2017
@nox
Copy link
Contributor Author

nox commented Jan 19, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 847ff7e with merge 787aaa5...

bors-servo pushed a commit that referenced this pull request Jan 19, 2017
Auto merge of #15098 - nox:load-fixes, r=<try>
787aaa5 
Mark the page source as loaded only after parsing is done

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15098)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 19, 2017
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Jan 19, 2017
@jdm
Copy link
Member

jdm commented Jan 19, 2017

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 0f244d6 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 19, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 0f244d6 with merge 6272cb5...

bors-servo pushed a commit that referenced this pull request Jan 19, 2017
Auto merge of #15098 - nox:load-fixes, r=jdm
6272cb5 
Mark the page source as loaded only after parsing is done

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15098)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

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

@bors-servo bors-servo merged commit 0f244d6 into servo:master Jan 19, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm jdm requested changes

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nox @highfive @bors-servo @jdm @KiChjang