1304792 - stylo: Implement @import

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

from https://public.etherpad-mozilla.org/p/stylo-taipei :

We'll need to call into Gecko's css::Loader when we encounter @import rules, just like nsCSSParser::ParseImportRule does.  Loading the child sheet needs to block loading the rest of the HTML document, but Servo would already be blocking due to the top level <style> or <link rel=stylesheet> parse, we shouldn't need to do anything special to handle blocking.  (css::Loader will already block Gecko's subsequent document parsing.)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I'm working on this, preliminar work for servo support is in https://github.com/servo/servo/pull/14540.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1304792: Minimal Loader and CSSParser cleanup.

Signed-off-by: Emilio Cobos Álvarez <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/99258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99258/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1304792: stylo: Implement @import.

Review commit: https://reviewboard.mozilla.org/r/99260/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99260/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8819521 [details]
Bug 1304792: stylo: Implement @import.

Mostly a lot of plumbing over https://github.com/servo/servo/pull/14540.

I think I still need to do the propagation of ServoStyleSheet::mDocument down, but meanwhile... Does this approach make sense to you Cameron?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Also, I wonder when does it matter to pass the owning node of the parent StyleSheet vs the owning document for the content policy?

I guess a content policy implementation could check that, so I guess I should also fix it, oh well...

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So I did a try run last week passing only the Loader's document to the content policy, and it was green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d10c2633397ffe2b0ce0f3240062784b260d8ed

Cameron, do you know what's the purpose of that walk up for the owning node in the content policy stuff?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8819520 [details]
Bug 1304792: Minimal Loader and CSSParser cleanup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99258/diff/1-2/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8819521 [details]
Bug 1304792: stylo: Implement @import.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99260/diff/1-2/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Given try is green with the loader's document in the content policy, and that Hixies tests are correct (except for the ones with media queries that Servo doesn't support), I'd like to get this landed, then work on Servo-backed nsMediaList if I have the time. I don't think fixing the document propagation should be part of this bug (not that it's specially hard, but it's tricky and these patches are already decent in size).

If you think that makes sense also, I'll file bugs if required for the GetParentSheet bits, and presumably for stylesheet caching too.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8819520 [details]
Bug 1304792: Minimal Loader and CSSParser cleanup.

https://reviewboard.mozilla.org/r/99258/#review101780

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8819521 [details]
Bug 1304792: stylo: Implement @import.

https://reviewboard.mozilla.org/r/99260/#review101782

::: layout/style/ServoStyleSheet.cpp:65
(Diff revision 2)
>  }
>  
>  void
>  ServoStyleSheet::AppendStyleSheet(ServoStyleSheet* aSheet)
>  {
> -  // XXXheycam: When we implement support for child sheets, we'll have
> +  aSheet->mDocument = mDocument;

I guess it's at least worth a comment here or in SetOwningDocument that we need to recursively propagate document down to children.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8819521 [details]
Bug 1304792: stylo: Implement @import.

https://reviewboard.mozilla.org/r/99260/#review101784

::: layout/style/Loader.h:296
(Diff revision 2)
> -                          ImportRule* aRule,
> +                          ImportRule* aGeckoParentRule,
> +                          RawServoImportRule* aServoParentRule,

Please update the comment.

::: layout/style/Loader.cpp:1415
(Diff revision 2)
> +                         RawServoImportRule* aServoParentRule)
>  {
>    LOG(("css::Loader::InsertChildSheet"));
> -  NS_PRECONDITION(aSheet, "Nothing to insert");
> -  NS_PRECONDITION(aParentSheet, "Need a parent to insert into");
> -  NS_PRECONDITION(aParentSheet, "How did we get imported?");
> +  MOZ_ASSERT(aSheet, "Nothing to insert");
> +  MOZ_ASSERT(aParentSheet, "Need a parent to insert into");
> +  MOZ_ASSERT(aGeckoParentRule || aServoParentRule, "How did we get imported?");

Please assert that exactly one of aGeckoParentRule and aServoParentRule is non-null, e.g. with |!aGeckoParentRule != !aServoParentRule|.

::: layout/style/Loader.cpp:1417
(Diff revision 2)
>    LOG(("css::Loader::InsertChildSheet"));
> -  NS_PRECONDITION(aSheet, "Nothing to insert");
> -  NS_PRECONDITION(aParentSheet, "Need a parent to insert into");
> -  NS_PRECONDITION(aParentSheet, "How did we get imported?");
> -
> -  // XXXheycam The InsertChildSheet API doesn't work with ServoStyleSheets,
> +  MOZ_ASSERT(aSheet, "Nothing to insert");
> +  MOZ_ASSERT(aParentSheet, "Need a parent to insert into");
> +  MOZ_ASSERT(aGeckoParentRule || aServoParentRule, "How did we get imported?");
> +  if (aSheet->IsGecko()) {
> +    MOZ_ASSERT(aGeckoParentRule, "Really?");

If providing an assertion message, please try to give a useful one. :-)  But in general, for assertions that are just null checks on arguments, no message is needed.

::: layout/style/Loader.cpp:1423
(Diff revision 2)
> -  // child sheets should always start out enabled, even if they got
> +    // child sheets should always start out enabled, even if they got
> -  // cloned off of top-level sheets which were disabled
> +    // cloned off of top-level sheets which were disabled
> -  aSheet->AsGecko()->SetEnabled(true);
> +    aSheet->AsGecko()->SetEnabled(true);
> -
> +    aGeckoParentRule->SetSheet(aSheet->AsGecko()); // This sets the ownerRule on the sheet
> +  } else {
> +    MOZ_ASSERT(aServoParentRule, "Really?");

Same here.

::: layout/style/Loader.cpp:2215
(Diff revision 2)
> +    MOZ_ASSERT(!aServoParentRule, "That makes no sense!");
> +  } else {
> +    MOZ_ASSERT(aServoParentRule, "How did we get imported?");
> +    MOZ_ASSERT(!aGeckoParentRule, "That makes no sense!");
> +  }
> +#endif
> +

You could shorten this to:

MOZ_ASSERT(!aGeckoParentRule && !aServoParentRule,
           "must pass only one type of parent rule");
MOZ_ASSERT(aParentSheet->IsGecko() == !!aGeckoParentRule),
           "rule type does not match sheet type");

::: layout/style/Loader.cpp:2234
(Diff revision 2)
> +  // FIXME(emilio): Figure out whether this walk up is necessary (try seems
> +  // green without it), and fix the parenting of stylesheets in the servo case
> +  // if that's the case.

It's likely we do need to do something here, since this determines the "requesting node" (SheetLoadData::mRequestingNode), without which we can end up doing loads with the system principal rather than the principal of the thing that starting loading the sheets.

If you have time to fix this, please do; otherwise file a followup.

::: layout/style/ServoBindings.cpp:15
(Diff revision 2)
>  #include "StyleStructContext.h"
>  #include "gfxFontFamilyList.h"
>  #include "nsAttrValueInlines.h"
>  #include "nsCSSRuleProcessor.h"
>  #include "nsContentUtils.h"
> +#include "nsCSSParser.h"

Nit: Please move this two lines up to keep this sorted.

::: layout/style/ServoBindings.cpp:23
(Diff revision 2)
>  #include "nsIDOMNode.h"
>  #include "nsIDocument.h"
>  #include "nsIFrame.h"
>  #include "nsINode.h"
>  #include "nsIPrincipal.h"
> +#include "nsNetUtil.h"

Nit: Please move this one line down to keep this sorted.

::: layout/style/ServoBindings.cpp:1035
(Diff revision 2)
> +  MOZ_ASSERT(aLoader, "Should've catched this before");
> +  MOZ_ASSERT(aParent, "Only used for @import, so parent should exist!");
> +  MOZ_ASSERT(aURLString, "Invalid URLs shouldn't be loaded!");

Nit: I'd drop the messages.

::: layout/style/ServoBindings.cpp:1045
(Diff revision 2)
> +    nsDependentCSubstring medium_(reinterpret_cast<const char*>(aMediaString),
> +                                  aMediaStringLength);
> +    nsString medium;
> +    medium_.Assign(medium_);
> +    nsCSSParser mediumParser(aLoader);
> +    mediumParser.ParseMediaList(medium, nullptr, 0, media);

Can do this slightly shorter:

nsDependentCString medium(...);

nsCSSParser parser(aLoader);
parser.ParseMediaList(NS_ConvertUTF8toUTF16(medium),
                      nullptr, 0, media);

::: layout/style/ServoStyleSheet.h:64
(Diff revision 2)
>  #ifdef DEBUG
>    void List(FILE* aOut = stdout, int32_t aIndex = 0) const;
>  #endif
>  
>    RawServoStyleSheet* RawSheet() const { return mSheet; }
> +  void SetSheetForImport(RawServoStyleSheet* aSheet) { mSheet = aSheet; }

Here are we guaranteed that mSheet will be null before we assign to it, because the sheet will have been created not too long ago?  If so, please assert this in here.

::: layout/style/ServoStyleSheet.cpp:65
(Diff revision 2)
> -  // XXXheycam: When we implement support for child sheets, we'll have
> +  aSheet->mDocument = mDocument;
> -  // to fix SetOwningDocument to propagate the owning document down
> -  // to the children.

As above, please try to fix the owning document / owning node stuff if you have time.  Xidorn is right to request we preserve the comment but it looks like we already have such a comment in SetOwningDocument.

::: layout/style/ServoStyleSheet.cpp:86
(Diff revision 2)
> -  mSheet = Servo_StyleSheet_FromUTF8Bytes(&input, mParsingMode, &baseString,
> -                                          base, referrer, principal).Consume();
> +  if (!mSheet) {
> +    mSheet =
> +      Servo_StyleSheet_FromUTF8Bytes(aLoader, this, &input, mParsingMode,
> +                                     &baseString, base, referrer,
> +                                     principal).Consume();
> +  } else {
> +    Servo_StyleSheet_ClearAndUpdate(mSheet, aLoader, this, &input, base,
> +                                    referrer, principal);
> +  }

I guess if we're moving to a model of always keeping the same Servo Stylesheet object around, it might make sense to have this be:

  if (!mSheet) {
    mSheet = create empty sheet
  }
  Servo_StyleSheet_ClearAndUpdate(...)

but that can be a followup.

::: layout/style/nsLayoutStylesheetCache.cpp:980
(Diff revision 2)
> +    Loader* loader = nullptr;
> +    if (nsIDocument* doc = servoSheet->GetOwningDocument()) {
> +      loader = doc->CSSLoader();
> +    }
> +
> +    nsresult rv = servoSheet->ParseSheet(loader, sheetText, uri, uri, nullptr, 0);

If ParseSheet() supports passing null for the loader, then we may as well do that rather than look for one, since the pref sheet will never have an @import rule in it.

::: servo/ports/geckolib/glue.rs:268
(Diff revision 2)
> -    let loader = StylesheetLoader::new();
> +    let loader = if loader.is_null() {
> +        None
> +    } else {
> +        Some(StylesheetLoader::new(loader, stylesheet))
> +    };
> +
> +    // FIXME(emilio): loader.as_ref() doesn't typecheck for some reason?
> +    let loader: Option<&StyleStylesheetLoader> = match loader {
> +        None => None,
> +        Some(ref s) => Some(s),
> +    };

I don't know if this avoids your typechecking issues, but how about:

let loader = loader.as_ref().map(|l| StylesheetLoader::new(l, stylesheet));
let sheet = ... loader.as_ref() ...

::: servo/ports/geckolib/stylesheet_loader.rs:13
(Diff revision 2)
> +use style::gecko_bindings::bindings::Gecko_LoadStyleSheet;
> +use style::gecko_bindings::structs::{Loader, ServoStyleSheet};
>  use style::stylesheets::{ImportRule, StylesheetLoader as StyleStylesheetLoader};
> +use style_traits::ToCss;
>  
> -pub struct StylesheetLoader;
> +pub struct StylesheetLoader(*mut Loader, *mut ServoStyleSheet);

Would prefer named fields here.

::: servo/ports/geckolib/stylesheet_loader.rs:40
(Diff revision 2)
> +        let media = import.stylesheet.media.read().to_css_string();
> +
> +        unsafe {
> +            Gecko_LoadStyleSheet(self.0,
> +                                 self.1,
> +                                 mem::transmute(import_rule.clone()),

Why isn't this argument a RawServoImportRuleBorrowed?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8819521 [details]
Bug 1304792: stylo: Implement @import.

https://reviewboard.mozilla.org/r/99260/#review101784

> You could shorten this to:
> 
> MOZ_ASSERT(!aGeckoParentRule && !aServoParentRule,
>            "must pass only one type of parent rule");
> MOZ_ASSERT(aParentSheet->IsGecko() == !!aGeckoParentRule),
>            "rule type does not match sheet type");

I ended up doing:

```
  MOZ_ASSERT_IF(aSheet->IsGecko(), aGeckoParentRule && !aServoParentRule);
  MOZ_ASSERT_IF(aSheet->IsServo(), aServoParentRule && !aGeckoParentRule);
```

Which I think is cleaner.

> It's likely we do need to do something here, since this determines the "requesting node" (SheetLoadData::mRequestingNode), without which we can end up doing loads with the system principal rather than the principal of the thing that starting loading the sheets.
> 
> If you have time to fix this, please do; otherwise file a followup.

Filed bug 1326242.

> As above, please try to fix the owning document / owning node stuff if you have time.  Xidorn is right to request we preserve the comment but it looks like we already have such a comment in SetOwningDocument.

Seems unrelated enough to this patch. I'll file a followup for this and I'll address it when I have the time, probably this weekend or next week.

> I don't know if this avoids your typechecking issues, but how about:
> 
> let loader = loader.as_ref().map(|l| StylesheetLoader::new(l, stylesheet));
> let sheet = ... loader.as_ref() ...

The first line is a different way to put the first `let loader`. The second line will fail type-checking the same way.

> Why isn't this argument a RawServoImportRuleBorrowed?

There wasn't any mechanism to convert from &Arc to a borrowed thingie. Will push a patch for it.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1304792: Use borrowed types for ServoImportRule.

Review commit: https://reviewboard.mozilla.org/r/101386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101386/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8819520 [details]
Bug 1304792: Minimal Loader and CSSParser cleanup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99258/diff/2-3/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8819521 [details]
Bug 1304792: stylo: Implement @import.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99260/diff/2-3/

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8822472 [details]
Bug 1304792: Use borrowed types for ServoImportRule.

https://reviewboard.mozilla.org/r/101386/#review101960

Looks OK to me; defer to Manish for the sugar changes.

Comment 18

[Manish Goregaokar [:manishearth]]

Comment on attachment 8822472 [details]
Bug 1304792: Use borrowed types for ServoImportRule.

https://reviewboard.mozilla.org/r/101386/#review101986

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7947d53b3587
Minimal Loader and CSSParser cleanup. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2e9b88f958
stylo: Implement @import. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/82fe5697e8fa
Use borrowed types for ServoImportRule. r=Manishearth r=heycam

Comment 20

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45b02f5095c5bdba3a7137d217c7ec7ca101e4a6

Comment 21

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #12)
> It's likely we do need to do something here, since this determines the "requesting node" (SheetLoadData::mRequestingNode), without which we can end up doing loads with the system principal rather than the principal of the thing that starting loading the sheets.

After looking a bit more into it, I'm not so sure about whether we can end up with a system principal load. Note that we should pretty much always have a node, since we use the loader's document if we haven't an owner node.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7947d53b3587
https://hg.mozilla.org/mozilla-central/rev/ab2e9b88f958
https://hg.mozilla.org/mozilla-central/rev/82fe5697e8fa