The default bug view has changed. See this FAQ.

stylo: add support for alternate ServoStyleSheets

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
28 days ago
6 days ago

People

(Reporter: bz, Assigned: bradwerth, NeedInfo)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Causes asserts in various tests
(Assignee)

Updated

28 days ago
Assignee: nobody → bwerth
Another one for Brad's list.
Flags: needinfo?(bwerth)
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 3

6 days ago
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: Minimal do-nothing implementation of SetAuthorStyleDisabled.

I'm testing with /testing/web-platform/tests/html/semantics/links/linktypes/alternate-css.html where the two Page Styles work, but the "No Style" choice does not trigger a restyle. I've traced it to the point that I can see that ServoRestyleManager::ProcessPostTraversal does not get a nsChangeHint from Servo. https://dxr.mozilla.org/mozilla-central/source/layout/base/ServoRestyleManager.cpp#149 .

It appears that no damage is being assigned to the relevant elements (the body element and the sole div element). I had hoped that the call to Servo_StyleSet_FlushStyleSheets at https://dxr.mozilla.org/mozilla-central/source/layout/style/ServoStyleSet.cpp#116 would globally assign damage to give me a starting point, but that's not even happening. What is the appropriate trigger to get Servo to assign damage to the restyled elements?
Attachment #8848295 - Flags: feedback?(emilio+bugs)

Comment 4

6 days ago
mozreview-review
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: Minimal do-nothing implementation of SetAuthorStyleDisabled.

https://reviewboard.mozilla.org/r/121216/#review123208

So, this is right now doing nothing. And it makes sense. If we're arriving to ProcessPostTraversal it means that we have actually restyled the content (that's because we end up calling `RestyleForCSSRuleChanges()` here: http://searchfox.org/mozilla-central/source/layout/base/PresShell.cpp#1440).

But in servo the styles doesn't really change, we shouldn't do more work in the post-traversal, which is what you're seeing.

Note that what Gecko does is effectively setting dirty "stylesheet level" bits, and at the end of the update it will check them and call GatherRuleProcessors() on all the dirty stylesheets, skipping the relevant levels: http://searchfox.org/mozilla-central/source/layout/style/nsStyleSet.cpp#452.

Now, the interesting part I guess this bug is about is figuring out how to tell Servo:

 1) We don't want to apply author stylesheets.
 2) The styles have changed, so you need to rebuild the internal data structures.

We have infra in place for 2), as you've seen above.

I believe the easiest way to do 1) is propagating the "author style disabled" bit up to Stylist. There, we can just skip the relevant stylesheets in `Stylist::update` depending on the origin of the stylesheet. Also, we need to avoid style attributes in `push_applicable_declarations` in the servo side, if I'm reading the Gecko code right.

There are even trickier semantics for presentational hints, too, it seems, where we skip them for ones, but not others. Manish could be able to help here faster than I I guess, but I guess that can be an entire followup bug.

::: layout/style/ServoStyleSet.cpp:100
(Diff revision 1)
>  }
>  
>  nsresult
>  ServoStyleSet::SetAuthorStyleDisabled(bool aStyleDisabled)
>  {
> -  MOZ_CRASH("stylo: not implemented");
> +  if (aStyleDisabled == !mAuthorStyleDisabled) {

nit: I think it's clearer to early-return:

```
if (aStyleDisabled == mAuthorStyleDisabled) {
    return NS_OK;
}
// Do work.
```

Comment 5

6 days ago
mozreview-review
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: Minimal do-nothing implementation of SetAuthorStyleDisabled.

https://reviewboard.mozilla.org/r/121216/#review123224

Feel free to ask for more feedback as you see fit, happy to help! :)

Comment 6

6 days ago
mozreview-review-reply
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: Minimal do-nothing implementation of SetAuthorStyleDisabled.

https://reviewboard.mozilla.org/r/121216/#review123208

> nit: I think it's clearer to early-return:
> 
> ```
> if (aStyleDisabled == mAuthorStyleDisabled) {
>     return NS_OK;
> }
> // Do work.
> ```

Though I guess this is completely unreachable, given the check I pointed out in my previous comment near RestyleForCSSRuleChanges().

So I think you should be able to `MOZ_ASSERT(aStyleDisabled != mAuthorStyleDisabled)`.
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: Minimal do-nothing implementation of SetAuthorStyleDisabled.

Those previous comments should've gone together, sorry for the bugspam everybody.
Attachment #8848295 - Flags: feedback?(emilio+bugs)
You need to log in before you can comment on or make changes to this bug.