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.

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

Move Stylesheet loading and ownership from the layout task into HTML elements #8039

Merged
merged 4 commits into from Nov 7, 2015

Conversation

@tschneidereit
Copy link
Contributor

tschneidereit commented Oct 15, 2015

Stylesheets for HTMLLinkElements are now loaded by the resource task, triggered by the element in question. Stylesheets are owned by the elements they're associated with, which can be HTMLStyleElement, HTMLLinkElement, and HTMLMetaElement (for `).

Additionally, the quirks mode stylesheet (just as the user and user agent stylesheets a couple of commits ago), is implemented as a lazy static, loaded once per process and shared between all documents.

This all has various nice consequences:

  • Stylesheet loading becomes a non-blocking operation.
  • Stylesheets are removed when the element they're associated with is removed from the document.
  • It'll be possible to implement the CSSOM APIs that require direct access to the stylesheets (i.e., ~ all of them).
  • Various subtle correctness issues are fixed.

One piece of interesting follow-up work would be to move parsing of external stylesheets to the resource task, too. Right now, it happens in the link element once loading is complete, so blocks the script task. Moving it to the resource task would probably be fairly straight-forward as it doesn't require access to any external state.

Depends on #7979 because without that loading stylesheets asynchronously breaks lots of content.

Review on Reviewable

@jdm jdm added the S-needs-rebase label Oct 15, 2015
@tschneidereit tschneidereit force-pushed the tschneidereit:script-owns-stylesheets branch from bb3d6eb to 8060aef Oct 15, 2015
@eefriedman
Copy link
Contributor

eefriedman commented Oct 15, 2015

Review status: 0 of 25 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


components/script/dom/document.rs, line 1318 [r8] (raw file):
Weird indentation.


components/script/dom/htmllinkelement.rs, line 53 [r8] (raw file):
Wrong specification reference? The spec specifically says that only <script> elements are marked parser-inserted. Maybe you meant to refer to https://html.spec.whatwg.org/multipage/semantics.html#a-style-sheet-that-is-blocking-scripts ?


components/script/dom/htmllinkelement.rs, line 213 [r8] (raw file):
I don't think you need to box here.


components/script/dom/htmllinkelement.rs, line 255 [r8] (raw file):
The use of RefCell here is ridiculous... but I guess it's constrained by the interface? I'll propose a patch.


Comments from the review on Reviewable.io

@eefriedman
Copy link
Contributor

eefriedman commented Oct 15, 2015

Filed #8043 for the RefCell bit.

@tschneidereit tschneidereit force-pushed the tschneidereit:script-owns-stylesheets branch from 8060aef to 04406ed Oct 16, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 16, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2015

Trying commit 04406ed with merge 5b7eeb3...

bors-servo pushed a commit that referenced this pull request Oct 16, 2015
Move Stylesheet loading and ownership from the layout task into HTML elements

Stylesheets for `HTMLLinkElement`s are now loaded by the resource task, triggered by the element in question. Stylesheets are owned by the elements they're associated with, which can be `HTMLStyleElement`, `HTMLLinkElement`, and `HTMLMetaElement` (for `<meta name="viewport">).

Additionally, the quirks mode stylesheet (just as the user and user agent stylesheets a couple of commits ago), is implemented as a lazy static, loaded once per process and shared between all documents.

This all has various nice consequences:
 - Stylesheet loading becomes a non-blocking operation.
 - Stylesheets are removed when the element they're associated with is removed from the document.
 - It'll be possible to implement the CSSOM APIs that require direct access to the stylesheets (i.e., ~ all of them).
 - Various subtle correctness issues are fixed.

One piece of interesting follow-up work would be to move parsing of external stylesheets to the resource task, too. Right now, it happens in the link element once loading is complete, so blocks the script task. Moving it to the resource task would probably be fairly straight-forward as it doesn't require access to any external state.

Depends on #7979 because without that loading stylesheets asynchronously breaks lots of content.

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

bors-servo commented Oct 16, 2015

💔 Test failed - linux-dev

@tschneidereit tschneidereit force-pushed the tschneidereit:script-owns-stylesheets branch from 04406ed to 310a44f Oct 16, 2015
@tschneidereit tschneidereit force-pushed the tschneidereit:script-owns-stylesheets branch from 310a44f to 3af5916 Oct 16, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 16, 2015

@bors-servo retry

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 16, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2015

Trying commit 3af5916 with merge 0d6cfb1...

bors-servo pushed a commit that referenced this pull request Oct 16, 2015
Move Stylesheet loading and ownership from the layout task into HTML elements

Stylesheets for `HTMLLinkElement`s are now loaded by the resource task, triggered by the element in question. Stylesheets are owned by the elements they're associated with, which can be `HTMLStyleElement`, `HTMLLinkElement`, and `HTMLMetaElement` (for `<meta name="viewport">).

Additionally, the quirks mode stylesheet (just as the user and user agent stylesheets a couple of commits ago), is implemented as a lazy static, loaded once per process and shared between all documents.

This all has various nice consequences:
 - Stylesheet loading becomes a non-blocking operation.
 - Stylesheets are removed when the element they're associated with is removed from the document.
 - It'll be possible to implement the CSSOM APIs that require direct access to the stylesheets (i.e., ~ all of them).
 - Various subtle correctness issues are fixed.

One piece of interesting follow-up work would be to move parsing of external stylesheets to the resource task, too. Right now, it happens in the link element once loading is complete, so blocks the script task. Moving it to the resource task would probably be fairly straight-forward as it doesn't require access to any external state.

Depends on #7979 because without that loading stylesheets asynchronously breaks lots of content.

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

bors-servo commented Oct 16, 2015

💔 Test failed - linux-dev

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2015

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

@tschneidereit tschneidereit force-pushed the tschneidereit:script-owns-stylesheets branch from 3f60346 to 35cfe44 Oct 16, 2015
@SimonSapin
Copy link
Member

SimonSapin commented Oct 16, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2015

🔒 Merge conflict

@bors-servo
Copy link
Contributor

bors-servo commented Nov 7, 2015

…elements

Stylesheets for `HTMLLinkElement`s are now loaded by the resource task, triggered by the element in question. Stylesheets are owned by the elements they're associated with, which can be `HTMLStyleElement`, `HTMLLinkElement`, and `HTMLMetaElement` (for `<meta name="viewport">).

Additionally, the quirks mode stylesheet (just as the user and user agent stylesheets a couple of commits ago), is implemented as a lazy static, loaded once per process and shared between all documents.

This all has various nice consequences:
 - Stylesheet loading becomes a non-blocking operation.
 - Stylesheets are removed when the element they're associated with is removed from the document.
 - It'll be possible to implement the CSSOM APIs that require direct access to the stylesheets (i.e., ~ all of them).
 - Various subtle correctness issues are fixed.

One piece of interesting follow-up work would be to move parsing of external stylesheets to the resource task, too. Right now, it happens in the link element once loading is complete, so blocks the script task. Moving it to the resource task would probably be fairly straight-forward as it doesn't require access to any external state.
@tschneidereit tschneidereit force-pushed the tschneidereit:script-owns-stylesheets branch from 1ffcf3b to 543703e Nov 7, 2015
@jdm
Copy link
Member

jdm commented Nov 7, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 7, 2015

📌 Commit 543703e has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Nov 7, 2015

Testing commit 543703e with merge 85b6421...

bors-servo added a commit that referenced this pull request Nov 7, 2015
Move Stylesheet loading and ownership from the layout task into HTML elements

Stylesheets for `HTMLLinkElement`s are now loaded by the resource task, triggered by the element in question. Stylesheets are owned by the elements they're associated with, which can be `HTMLStyleElement`, `HTMLLinkElement`, and `HTMLMetaElement` (for `<meta name="viewport">).

Additionally, the quirks mode stylesheet (just as the user and user agent stylesheets a couple of commits ago), is implemented as a lazy static, loaded once per process and shared between all documents.

This all has various nice consequences:
 - Stylesheet loading becomes a non-blocking operation.
 - Stylesheets are removed when the element they're associated with is removed from the document.
 - It'll be possible to implement the CSSOM APIs that require direct access to the stylesheets (i.e., ~ all of them).
 - Various subtle correctness issues are fixed.

One piece of interesting follow-up work would be to move parsing of external stylesheets to the resource task, too. Right now, it happens in the link element once loading is complete, so blocks the script task. Moving it to the resource task would probably be fairly straight-forward as it doesn't require access to any external state.

Depends on #7979 because without that loading stylesheets asynchronously breaks lots of content.

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

bors-servo commented Nov 7, 2015

💔 Test failed - linux-rel

@eefriedman
Copy link
Contributor

eefriedman commented Nov 7, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 7, 2015

Testing commit 543703e with merge 7ff3a17...

@highfive highfive removed the S-tests-failed label Nov 7, 2015
bors-servo added a commit that referenced this pull request Nov 7, 2015
Move Stylesheet loading and ownership from the layout task into HTML elements

Stylesheets for `HTMLLinkElement`s are now loaded by the resource task, triggered by the element in question. Stylesheets are owned by the elements they're associated with, which can be `HTMLStyleElement`, `HTMLLinkElement`, and `HTMLMetaElement` (for `<meta name="viewport">).

Additionally, the quirks mode stylesheet (just as the user and user agent stylesheets a couple of commits ago), is implemented as a lazy static, loaded once per process and shared between all documents.

This all has various nice consequences:
 - Stylesheet loading becomes a non-blocking operation.
 - Stylesheets are removed when the element they're associated with is removed from the document.
 - It'll be possible to implement the CSSOM APIs that require direct access to the stylesheets (i.e., ~ all of them).
 - Various subtle correctness issues are fixed.

One piece of interesting follow-up work would be to move parsing of external stylesheets to the resource task, too. Right now, it happens in the link element once loading is complete, so blocks the script task. Moving it to the resource task would probably be fairly straight-forward as it doesn't require access to any external state.

Depends on #7979 because without that loading stylesheets asynchronously breaks lots of content.

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

bors-servo commented Nov 7, 2015

@bors-servo bors-servo merged commit 543703e into servo:master Nov 7, 2015
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable Review in progress: 6 of 18 files reviewed, 2 unresolved discussions
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@frewsxcv
Copy link
Member

frewsxcv commented Nov 10, 2015

This commit from this PR caused a regression that results in some websites not resizing when when the browser window resizes: #8443

@SimonSapin
Copy link
Member

SimonSapin commented Nov 11, 2015

I’ll look into a fix, but we should also have tests to make sure this doesn’t regress. Is implementing window.resizeTo() a good way to support that kind of test? If not, some other servo-specific API? WebDriver? CC @jgraham @gsnedders

@jgraham
Copy link
Contributor

jgraham commented Nov 11, 2015

I'm not sure, really. resizeTo only applies to auxillary browsing contexts, so you would also have to implement multi-window support. WebDriver can theoretically support this use case, but that's a little way off. A servo-specific pref for window size would work for the servo backend, but not for servodriver because there's no support for restarting the browser, and no notification system for subscribing to pref changes.

@jdm
Copy link
Member

jdm commented Nov 11, 2015

Is there any way to trigger this problem with iframes?

@SimonSapin
Copy link
Member

SimonSapin commented Nov 11, 2015

@jgraham I think prefs are not enough in this case as we want to tests dynamic changes

@jdm Ooh, that sounds promising, i’ll look into it.

@SimonSapin
Copy link
Member

SimonSapin commented Nov 12, 2015

Edit: moving discussion to #8443.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.