stylo: ServoStyleSheet::ParseSheet should disconnect child sheets / mRuleList, and re-use child sheets

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
ASSIGNED
a month ago
2 days ago

People

(Reporter: heycam, Assigned: kuoe0)

Tracking

(Blocks: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

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

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

a month ago
In Gecko's CSSStyleSheet::ReparseSheet, we do some work to detach rules and child sheets from the existing sheet.  We should probably do the same for ServoStyleSheets in ParseSheet, if we already have a sheet, so that the page has a consistent view of the CSSOM if devtools updates the sheet contents.

We also should use LoaderReusableStyleSheets when re-parsing a sheet, to avoid kicking off network loads again for @imported child sheets.
Blocks: 1322657
Priority: -- → P2
(Assignee)

Updated

21 days ago
Assignee: nobody → kuoe0
(Assignee)

Updated

21 days ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

10 days ago
Hi heycam, I think I finished the patches. Can you take a look at them and give me some feedback? I'm not sure it is correct. Do you know how to test this feature without devtools (I think devtools is still broken on stylo)?
Flags: needinfo?(cam)
(Reporter)

Comment 7

9 days ago
mozreview-review
Comment on attachment 8868946 [details]
Bug 1358993 - (Part 4) Make stylo support reusable sheets.

https://reviewboard.mozilla.org/r/140616/#review144452

::: layout/style/ServoStyleSheet.cpp:122
(Diff revision 1)
> +      RefPtr<ServoStyleSheet> sheet = new ServoStyleSheet(css::eAgentSheetFeatures,
> +                                                          CORS_NONE,
> +                                                          mozilla::net::RP_Unset,
> +                                                          dom::SRIMetadata());
> +      sheet->SetSheetForImport(importRules[i]);

I think we should be using the existing ServoStyleSheet object, rather than creating a new wrapper here.

You could probably iterate over the child sheets of the current ServoStyleSheet object and find the one whose Inner()->mSheet matches importRules[i].
(Reporter)

Comment 8

9 days ago
I think this is looking good.

One more thing you'll need to do is make inDOMUtils::ParseStyleSheet call into a function that both CSSStyleSheet and ServoStyleSheet support, for re-parsing the sheet.  Currently it is doing do_QueryObject to dynamically cast from nsIDOMCSStyleSheet (an XPCOM interface, which StyleSheet implements) to a concrete CSSStyleSheet object, and you'll need to change that to cast to StyleSheet.  The way that dynamic casting support is implemented for CSSStyleSheet is this:

  http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/layout/style/CSSStyleSheet.cpp#447-449

and we'll need to have something similar for StyleSheet to make that work.

I think stylo doesn't support @import rules being exposed through CSSOM yet.  layout/inspector/tests/test_parseStyleSheetImport.html is the test for reusable sheets, check whether it will work.  If not, you could manually test by writing a new mochitest that calls parseStyleSheet like that test does, and maybe do some printf logging in Loader to make sure we do re-use the sheet.
Flags: needinfo?(cam)
(Reporter)

Comment 9

9 days ago
Another thing I just realized is that we will get into this second branch of the if-statement in ServoStyleSheet::ParseSheet for some cases other than inDOMUtils::ParseStyleSheet, and in those cases, we don't want to do this sheet re-using.  So I think we'll have to have a separate function ServoStyleSheet::ReparseSheet, that has the sheet re-using behaviour that you've added, and leave ServoStyleSheet::ParseSheet to do Servo_StyleSheet_ClearAndUpdate without sheet re-using.  (Then inDOMUtils::ParseStyleSheet can just always call ReparseSheet.)
Blocks: 1367620
Just a note that it's now possible to open DevTools in a Stylo build without crashing.  If you use the Style Editor tool and edit the CSS there, it will call `inIDOMUtils.parseStyleSheet`.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 days ago
Attachment #8868946 - Attachment is obsolete: true
(Assignee)

Updated

2 days ago
Attachment #8868945 - Flags: review?(cam)
Attachment #8868942 - Flags: review?(cam)
Attachment #8868943 - Flags: review?(cam)
Attachment #8868944 - Flags: review?(cam)
(Reporter)

Comment 15

2 days ago
mozreview-review
Comment on attachment 8868945 [details]
Bug 1358993 - (Part 1) [servo] Store the pointer of reusable sheets in StylesheetLoader.

https://reviewboard.mozilla.org/r/140614/#review146828

r=me with these comments addressed.

::: servo/ports/geckolib/glue.rs:710
(Diff revision 2)
>                                                    loader: *mut Loader,
>                                                    gecko_stylesheet: *mut ServoStyleSheet,
>                                                    data: *const nsACString,
>                                                    extra_data: *mut URLExtraData,
> -                                                  line_number_offset: u32)
> +                                                  line_number_offset: u32,
> +                                                  reusable_sheets: *mut c_void)

I think we should try to be a bit more type safe.  You can add "LoaderReusableStyleSheets" to the structs-type list in layout/style/ServoBindings.toml, so you can reference that type instead of c_void, here and elsewhere in these patches.

::: servo/ports/geckolib/glue.rs:847
(Diff revision 2)
>                                              rule_type: *mut u16) -> nsresult {
>      let sheet = Stylesheet::as_arc(&sheet);
>      let loader = if loader.is_null() {
>          None
>      } else {
> -        Some(StylesheetLoader::new(loader, gecko_stylesheet))
> +        Some(StylesheetLoader::new(loader, gecko_stylesheet, 0 as *mut c_void))

I think the idiomatic way to write a null pointer is:

  use std::ptr;
  ...

  ptr::null_mut()

It will infer the type based on where you use it, so you don't need to mention c_void (or, when you change it, LoaderReusableStyleSheets).
Attachment #8868945 - Flags: review?(cam) → review+
(Reporter)

Comment 16

2 days ago
mozreview-review
Comment on attachment 8868942 [details]
Bug 1358993 - (Part 2) Make stylo support reusable style sheets.

https://reviewboard.mozilla.org/r/140608/#review146830

::: layout/style/ServoBindingList.h:47
(Diff revision 2)
>                     mozilla::css::Loader* loader,
>                     mozilla::ServoStyleSheet* gecko_stylesheet,
>                     const nsACString* data,
>                     RawGeckoURLExtraData* extra_data,
> -                   uint32_t line_number_offset)
> +                   uint32_t line_number_offset,
> +                   void* reusableSheets)

"reusable_sheets" (we should use Rust identifier styles here).

Also this can be updated to use LoaderReusableStyleSheets* instead of void*.

::: layout/style/ServoBindings.h:140
(Diff revision 2)
>  RawGeckoElementBorrowedOrNull Gecko_GetPrevSiblingElement(RawGeckoElementBorrowed element);
>  RawGeckoElementBorrowedOrNull Gecko_GetNextSiblingElement(RawGeckoElementBorrowed element);
>  RawGeckoElementBorrowedOrNull Gecko_GetDocumentElement(RawGeckoDocumentBorrowed document);
>  void Gecko_LoadStyleSheet(mozilla::css::Loader* loader,
>                            mozilla::ServoStyleSheet* parent,
> +                          void* reusableSheets,

Here too.

::: layout/style/ServoBindings.cpp:2061
(Diff revision 2)
>  }
>  
>  void
>  Gecko_LoadStyleSheet(css::Loader* aLoader,
>                       ServoStyleSheet* aParent,
> +                     void* aReusableSheets,

LoaderReusableStyleSheets*

::: layout/style/ServoBindings.cpp:2089
(Diff revision 2)
>      // silently do nothing.  Eventually we should be able to assert that the
>      // NS_NewURI succeeds, here.
>      return;
>    }
>  
> -  aLoader->LoadChildSheet(aParent, uri, media, nullptr, aChildSheet, nullptr);
> +  auto reusableSheets = static_cast<css::LoaderReusableStyleSheets*>(aReusableSheets);

Then you shouldn't need this cast.
Attachment #8868942 - Flags: review?(cam) → review+
(Reporter)

Comment 17

2 days ago
mozreview-review
Comment on attachment 8868943 [details]
Bug 1358993 - (Part 3) Add ServoStyleSheet::ReparseSheet function to collect reusable style sheets and clean up the child sheets of the parent sheet.

https://reviewboard.mozilla.org/r/140610/#review146832

::: layout/style/ServoStyleSheet.cpp:172
(Diff revision 2)
>    Inner()->mSheet = Servo_StyleSheet_Empty(mParsingMode).Consume();
>    Inner()->mURLData = URLExtraData::Dummy();
>  }
>  
> +nsresult
> +ServoStyleSheet::ReparseSheet(const nsAString& aInput) {

Nit: "{" on new line.

Also, can you add some comments in this function that we should be doing the kind of document notifications (calling StyleRuleRemoved and StyleRuleAdded) for the @import rule objects.  I just filed bug 1367996 for not calling them -- please mention the bug number in the comment.

(Since it's so simliar to CSSStyleSheet::ReparseSheet, it would be nice if we could re-use it or have a single definition on StyleSheet.  Maybe that will be easier once we handle the StyleRule{Removed,Added} calls.)
Attachment #8868943 - Flags: review?(cam) → review+
(Reporter)

Comment 18

2 days ago
mozreview-review
Comment on attachment 8868944 [details]
Bug 1358993 - (Part 4) Call ServoStyleSheet::ReparseSheet in inDOMUtils::ParseStyleSheet.

https://reviewboard.mozilla.org/r/140612/#review146840

::: layout/inspector/inDOMUtils.cpp:1372
(Diff revision 2)
>  
>  NS_IMETHODIMP
>  inDOMUtils::ParseStyleSheet(nsIDOMCSSStyleSheet *aSheet,
>                              const nsAString& aInput)
>  {
> -  RefPtr<CSSStyleSheet> sheet = do_QueryObject(aSheet);
> +  RefPtr<StyleSheet> stylesheet = static_cast<StyleSheet*>(aSheet);

static_cast is the wrong thing to do.  In theory, there might be other implementations of nsIDOMCSSStyleSheet, which aren't CSSStyleSheet or ServoStyleSheet.  In general, JS can implement XPCOM interfaces like nsIDOMCSSStyleSheet, too.  In practice people won'd do this, but we need to protect against it.

So instead you could do something like:

  RefPtr<CSSStyleSheet> geckoSheet = do_QueryObject(aSheet);
  if (geckoSheet) {
    ...
  }

  RefPtr<ServoStyleSheet> servoSheet = do_QueryObject(aSheet);
  if (servoSheet) {
    ...
  }

  return NS_ERROR_INVALID_POINTER;

The do_QueryObject to dynamically cast to ServoStyleSheet should work since Brad landed this change:

  https://hg.mozilla.org/integration/autoland/rev/24d9cf9eedae
Attachment #8868944 - Flags: review?(cam) → review-
(Reporter)

Comment 19

2 days ago
mozreview-review
Comment on attachment 8868943 [details]
Bug 1358993 - (Part 3) Add ServoStyleSheet::ReparseSheet function to collect reusable style sheets and clean up the child sheets of the parent sheet.

https://reviewboard.mozilla.org/r/140610/#review146848

::: layout/style/ServoStyleSheet.cpp:193
(Diff revision 2)
> +  // clean up child sheets list
> +  for (StyleSheet* child = GetFirstChild(); child; ) {
> +    StyleSheet* next = child->mNext;
> +    child->mParent = nullptr;
> +    child->mNext = nullptr;
> +    child = next;
> +  }

I think you should call SetAssociatedDocument in here, like we do in CSSStyleSheet::ReparseSheet.
You need to log in before you can comment on or make changes to this bug.