Closed Bug 1341721 Opened 7 years ago Closed 7 years ago

stylo: add support for alternate ServoStyleSheets and disabling author styles

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

Bug 1341721 Part 1: ServoStyleSet implementation of SetAuthorStyleDisabled.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341721 Part 2a: Add a bool argument to Servo_StyleSet_NoteStyleSheetsChanged.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341721 Part 4: Re-enable reftest 360746, and remove the assert saying that alternate sheets are not supported.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341721 Part 5: Remove asserts from some unexpected-pass reftests.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Causes asserts in various tests
Assignee: nobody → bwerth
Another one for Brad's list.
Flags: needinfo?(bwerth)
Priority: -- → P1
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet 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 on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet 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 on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet 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 on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet 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: ServoStyleSet implementation of SetAuthorStyleDisabled.

Those previous comments should've gone together, sorry for the bugspam everybody.
Attachment #8848295 - Flags: feedback?(emilio+bugs)
See Also: → 1351996
(Since these are are related but a little different.)
Summary: stylo: add support for alternate ServoStyleSheets → stylo: add support for alternate ServoStyleSheets and disabling author styles
Attachment #8848295 - Flags: review?(cam)
Attachment #8856864 - Flags: review?(cam)
Attachment #8854253 - Flags: review?(cam)
Attachment #8854254 - Flags: review?(cam)
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet implementation of SetAuthorStyleDisabled.

https://reviewboard.mozilla.org/r/121216/#review131342
Attachment #8848295 - Flags: review?(cam) → review+
Comment on attachment 8856864 [details]
Bug 1341721 Part 2a: Add a bool argument to Servo_StyleSet_NoteStyleSheetsChanged.

https://reviewboard.mozilla.org/r/128782/#review131344
Attachment #8856864 - Flags: review?(cam) → review+
Comment on attachment 8854253 [details]
Bug 1341721 Part 2b: Add a bool property to PerDocumentStyleDataImpl.

https://reviewboard.mozilla.org/r/126190/#review131346
Attachment #8854253 - Flags: review?(cam) → review+
Comment on attachment 8854254 [details]
Bug 1341721 Part 3: Pass the author_style_disabled flag through ExtraStyleData and ignore document styles during update, if set.

https://reviewboard.mozilla.org/r/126192/#review131348

::: servo/components/style/stylist.rs:130
(Diff revision 2)
>      #[allow(missing_docs)]
>      #[cfg(feature = "servo")]
>      pub marker: PhantomData<&'a usize>,
> +
> +    /// A parameter to note whether we should ignore author styles.
> +    pub author_style_disabled: bool,

Keep the marker member at the end.

::: servo/components/style/stylist.rs:247
(Diff revision 2)
>                  self.add_stylesheet(&ua_stylesheets.quirks_mode_stylesheet,
>                                      guards.ua_or_user, extra_data);
>              }
>          }
>  
> +        if !extra_data.author_style_disabled {

I guess it would be nice if we could do this at the top of add_stylesheet, but I don't think we know that cascade level the sheet as it in there...
Attachment #8854254 - Flags: review?(cam) → review+
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #0)
> Causes asserts in various tests

Patch gets enable/disable of authored styles working, and alternate stylesheets seemed to me to be working already. Try server doesn't show any fails or unexpected passes. I want to make sure this is done right before I land the patch. Which tests are or were failing?
Flags: needinfo?(bwerth) → needinfo?(bzbarsky)
Attachment #8854253 - Attachment is obsolete: true
Attachment #8854254 - Attachment is obsolete: true
> Which tests are or were failing?

layout/reftests/bugs/360746-1.html has this assertion failure:

###!!! ASSERTION: stylo: can't handle alternate ServoStyleSheets yet: 'Error', file /Users/bzbarsky/mozilla/stylo/mozilla/dom/base/nsDocument.cpp, line 6441
Flags: needinfo?(bzbarsky)
And the patches here don't remove that assert, or the "continue" in that function.

I guess you would need a test that exercises that codepath.  That means either hitting it via SetHeaderData as 360746-1.html does (I can try to come up with a testcase that produces incorrect rendering here, too) or via the selectedStyleSheetSet setter or enableStyleSheetsForSet method.

selectedStyleSheetSet is used by layout/reftests/font-face/sheet-set-switch-1.html which is set as skip-if(stylo) at the moment, possibly because it doesn't perform the switch and hence times out.  It's also used in session restore, looks like; see PageStyleInternal.restoreTree.restoreFrame in browser/components/sessionstore/PageStyle.jsm.

enableStyleSheetsForSet seems to only be used in-tree in sessionstore tests, which stylo doesn't run.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #31)
> I guess you would need a test that exercises that codepath.  That means
> either hitting it via SetHeaderData as 360746-1.html does (I can try to come
> up with a testcase that produces incorrect rendering here, too) or via the
> selectedStyleSheetSet setter or enableStyleSheetsForSet method.
Thank you. Since this path now works, I've removed the assert and turned the reftest back on.

> selectedStyleSheetSet is used by
> layout/reftests/font-face/sheet-set-switch-1.html which is set as
> skip-if(stylo) at the moment, possibly because it doesn't perform the switch
> and hence times out.
That issue is filed as Bug 1290228.
Attachment #8857742 - Flags: review?(cam)
Attachment #8857743 - Flags: review?(cam)
Attachment #8857754 - Flags: review?(cam)
> Since this path now works, I've removed the assert and turned the reftest back on.

I don't understand how this path can work with stylo.  It's quite explicitly doing sheet->AsGecko(), no?

> That issue is filed as Bug 1290228.

That bug is about media queries.  There are no media queries in either layout/reftests/font-face/sheet-set-switch-1.html or layout/reftests/font-face/resize-detector-iframe.html that I can see, so I doubt that test is affected by bug 1290228.
Flags: needinfo?(bwerth)
Comment on attachment 8857742 [details]
Bug 1341721 Part 2b: Add a bool property to PerDocumentStyleDataImpl.

https://reviewboard.mozilla.org/r/129684/#review132332
Attachment #8857742 - Flags: review?(cam) → review+
Comment on attachment 8857743 [details]
Bug 1341721 Part 3: Update Stylist to maintain a state of whether author styles are disabled, and ExtraData parameters to modify that setting.

https://reviewboard.mozilla.org/r/129686/#review132334

::: servo/components/style/stylist.rs:264
(Diff revision 1)
> +        for ref stylesheet in doc_stylesheets.iter()
> +          .filter(|&s| author_style_enabled || s.origin != Origin::Author) {
>              self.add_stylesheet(stylesheet, guards.author, extra_data);
>          }
>  
> +

Nit: probably don't need this extra blank line.
Attachment #8857743 - Flags: review?(cam) → review+
Comment on attachment 8857754 [details]
Bug 1341721 Part 4: Re-enable reftest 360746, and remove the assert saying that alternate sheets are not supported.

https://reviewboard.mozilla.org/r/129720/#review132336
Attachment #8857754 - Flags: review?(cam) → review+
And in particular, applying the "part 4" patch and running layout/reftests/bugs/360746-1.html fails the IsGecko() assertions that AsGecko() does and crashes quite nicely.  As expected.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #40)
> And in particular, applying the "part 4" patch and running
> layout/reftests/bugs/360746-1.html fails the IsGecko() assertions that
> AsGecko() does and crashes quite nicely.  As expected.

Urg, yes, I'm working too fast and misreading the console logs. I've fixed the crash noted in comment 36, and I'm seeing if I can get layout/reftests/font-face/sheet-set-switch-1.html working.
Flags: needinfo?(bwerth)
Blocks: 1356158
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #31)
 
> selectedStyleSheetSet is used by
> layout/reftests/font-face/sheet-set-switch-1.html which is set as
> skip-if(stylo) at the moment, possibly because it doesn't perform the switch
> and hence times out.

The test still doesn't complete, but the stylesheet switch does happen with this patch. Bug 1356158 opened to figure out what's going wrong with the test.
Attachment #8857793 - Flags: review?(cam)
Comment on attachment 8857793 [details]
Bug 1341721 Part 5: Remove asserts from some unexpected-pass reftests.

https://reviewboard.mozilla.org/r/129778/#review132452
Attachment #8857793 - Flags: review?(cam) → review+
Attachment #8857742 - Attachment is obsolete: true
Attachment #8857743 - Attachment is obsolete: true
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95bd9774b6fb
Part 1: ServoStyleSet implementation of SetAuthorStyleDisabled. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a5ed50a011ff
Part 2a: Add a bool argument to Servo_StyleSet_NoteStyleSheetsChanged. r=heycam
https://hg.mozilla.org/integration/autoland/rev/aac6714c518a
Part 4: Re-enable reftest 360746, and remove the assert saying that alternate sheets are not supported. r=heycam
https://hg.mozilla.org/integration/autoland/rev/782e957654a2
Part 5: Remove asserts from some unexpected-pass reftests. r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: