Closed Bug 1304792 Opened 8 years ago Closed 7 years ago

stylo: Implement @import

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: bholley, Assigned: emilio)

References

Details

Attachments

(3 files)

Bug 1304792: Minimal Loader and CSSParser cleanup.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1304792: stylo: Implement @import.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1304792: Use borrowed types for ServoImportRule.
59 bytes, text/x-review-board-request
heycam
: review+
manishearth
: review+
Details 
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.)
I'm working on this, preliminar work for servo support is in https://github.com/servo/servo/pull/14540.
Assignee: nobody → ecoal95
Status: NEW → ASSIGNED
Depends on: 1029867
No longer blocks: 1323145, 1323146
Depends on: 1323145
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?
Attachment #8819521 - Flags: feedback?(cam)
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...
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?
Flags: needinfo?(cam)
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 on attachment 8819520 [details]
Bug 1304792: Minimal Loader and CSSParser cleanup.

https://reviewboard.mozilla.org/r/99258/#review101780
Attachment #8819520 - Flags: review?(cam) → review+
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 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?
Attachment #8819521 - Flags: review?(cam) → review+
See Also: → 1326242
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 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.
Attachment #8822472 - Flags: review?(cam) → review+
Comment on attachment 8822472 [details]
Bug 1304792: Use borrowed types for ServoImportRule.

https://reviewboard.mozilla.org/r/101386/#review101986
Attachment #8822472 - Flags: review?(manishearth) → review+
Pushed by ecoal95@gmail.com:
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
(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.
Depends on: 1331291
Depends on: 1335987
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: