Skip to content

/ servo

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.

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

script: Reflow 200 milliseconds after the `<body>` is parsed, like Gecko does. #6028

Merged
merged 2 commits into from May 26, 2015
Merged

script: Reflow 200 milliseconds after the `<body>` is parsed, like Gecko does. #6028

merged 2 commits into from May 26, 2015
+67 −7

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented May 13, 2015

It would be nice if HTML parsing didn't have to hog the event loop, so I didn't have to do this polling in content_changed(), but maybe the way we do it is unavoidable.

r? @jdm

Review on Reviewable
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented May 13, 2015

Critic review: https://critic.hoppipolla.co.uk/r/4981

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.
@pcwalton
Copy link
Contributor Author

pcwalton commented May 13, 2015

cc #4201
@jdm
Copy link
Member

jdm commented May 13, 2015

#5727 will avoid the main loop hogging. It just needs @Ms2ger to actually review it.
@pcwalton
Copy link
Contributor Author

pcwalton commented May 19, 2015

I think this is effectively blocked on #5727.
@jdm jdm assigned jdm and unassigned Ms2ger May 20, 2015
@bors-servo
Copy link
Contributor

bors-servo commented May 21, 2015

☔ The latest upstream changes (presumably #6156) made this pull request unmergeable. Please resolve the merge conflicts.
@pcwalton pcwalton force-pushed the pcwalton:reflow-timer branch from 702331c to 7527504 May 22, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented May 22, 2015

Updated to take advantage of the new parse_sync driver method for the HTML parser. r? @jdm
@bors-servo
Copy link
Contributor

bors-servo commented May 25, 2015

☔ The latest upstream changes (presumably #6125) made this pull request unmergeable. Please resolve the merge conflicts.
@jdm jdm added the S-needs-rebase label May 25, 2015
@jdm
Copy link
Member

jdm commented May 25, 2015

-S-awaiting-review +S-needs-code-changes

Review status: all files reviewed, 3 unresolved discussions, some commit checks failed.
Reviewed files:

  • components/script/dom/document.rs @ r2
  • components/script/dom/htmlbodyelement.rs @ r2
  • components/script/dom/servohtmlparser.rs @ r2
  • components/script/dom/window.rs @ r2
  • components/script/script_task.rs @ r2

components/script/dom/document.rs, line 372 [r1] (raw file):
Do we not want to set a shorter timeout if requested?

components/script/dom/htmlbodyelement.rs, line 103 [r2] (raw file):
Check tree_in_doc here.

components/script/dom/servohtmlparser.rs, line 309 [r2] (raw file):
Why this extra block?

Comments from the review on Reviewable.io
@pcwalton
script: Reflow 200 milliseconds after the `<body>` is parsed, like Gecko
9e34e82 
does.
@pcwalton
Copy link
Contributor Author

pcwalton commented May 26, 2015

Review status: all files reviewed, 3 unresolved discussions, some commit checks failed.

components/script/dom/servohtmlparser.rs, line 309 [r2] (raw file):
I guess I was thinking that it might keep the document alive too long if I didn't add the block, but that's a silly thing to worry about in retrospect. I'll remove it.

Comments from the review on Reviewable.io
@pcwalton
script: Address review comments
Loading status checks…
05bf2ff
@pcwalton pcwalton force-pushed the pcwalton:reflow-timer branch from 7527504 to 05bf2ff May 26, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented May 26, 2015

Review comments addressed. r? @jdm
@jdm
Copy link
Member

jdm commented May 26, 2015

Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.
Reviewed files:

  • components/script/dom/document.rs @ r3
  • components/script/dom/htmlbodyelement.rs @ r3
  • components/script/dom/servohtmlparser.rs @ r3
  • components/script/dom/window.rs @ r3
  • components/script/script_task.rs @ r3

Comments from the review on Reviewable.io
@jdm
Copy link
Member

jdm commented May 26, 2015

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2015

📌 Commit 05bf2ff has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2015

⌛ Testing commit 05bf2ff with merge e604b66...
bors-servo pushed a commit that referenced this pull request May 26, 2015
bors-servo
Auto merge of #6028 - pcwalton:reflow-timer, r=jdm
Loading status checks…
e604b66 
It would be nice if HTML parsing didn't have to hog the event loop, so I didn't have to do this polling in `content_changed()`, but maybe the way we do it is unavoidable.

r? @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6028)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2
@bors-servo bors-servo merged commit 05bf2ff into servo:master May 26, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nox nox mentioned this pull request Sep 30, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@pcwalton @hoppipolla-critic-bot @jdm @bors-servo @Ms2ger @highfive
You can’t perform that action at this time.