Closed Bug 1360488 Opened 7 years ago Closed 7 years ago

stylo: Propagate quirks mode information from Gecko to Servo

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

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

People

(Reporter: xidorn, Assigned: canova)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1360488 - Part 2: stylo: Propagate quirks mode information from Gecko to Servo
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1360488 - Part 1: stylo: Add ServoCSSParsingEnvironment and pass this instead of URLExtraData
59 bytes, text/x-review-board-request
bholley
: review+
Details 
nox added the ability to Servo side in servo/servo#16609, but Gecko currently doesn't provide the quirks mode information to Servo. We need to fix that to eventually fix quirks mode support in Stylo.

The corresponding Servo side issue is servo/servo#15342. nox said he's confused by bindings, so he would like someone else to do this.

I have a feeling that this should be a relatively straightforward change for those who are comfortable with touching bindings.
Blocks: 1341769
Nazim, can you take this one and help out nox? High priority, should fix a bunch of tests. I figured it makes sense since you're very familiar with the glue and are also working on bug 1355724.
Assignee: nobody → canaltinova
Flags: needinfo?(canaltinova)
Priority: -- → P1
Sure! I'll look at this.
Flags: needinfo?(canaltinova)
Comment on attachment 8864363 [details]
Bug 1360488 - Part 2: stylo: Propagate quirks mode information from Gecko to Servo

https://reviewboard.mozilla.org/r/136034/#review139048

nice! r=me with those things fixed and a followup filed for the other NoQuirks stuff.

::: layout/style/ServoBindings.cpp:694
(Diff revision 1)
> +nsCompatibility
> +Gecko_GetCompatibilityMode(RawGeckoPresContextBorrowed aPresContext)
> +{
> +  return aPresContext->CompatibilityMode();
> +}

We should be able to do this in pure rust over bindgen, no?

::: layout/style/nsDOMCSSDeclaration.cpp:142
(Diff revision 1)
> -    newdecl = ServoDeclarationBlock::FromCssText(aCssText, urlData);
> +    // FIXME: We can't access to document from here. Do we need to access it or can
> +    // we ignore and pass it just normal mode here?
> +    newdecl = ServoDeclarationBlock::FromCssText(aCssText, urlData,
> +                                                 eCompatibility_FullStandards);

You should be able to grab it off of the CSSParsingEnv's CSSLoader (see what the Gecko code does above).

::: servo/ports/geckolib/glue.rs:166
(Diff revision 1)
>          // FIXME Find the real QuirksMode information for this document
> -        quirks_mode: QuirksMode::NoQuirks,
> +        quirks_mode: per_doc_data.stylist.quirks_mode,

You can remove the fixme.
Attachment #8864363 - Flags: review?(bobbyholley) → review+
Fixed these and triggered a try build. Let's see.
Comment on attachment 8864363 [details]
Bug 1360488 - Part 2: stylo: Propagate quirks mode information from Gecko to Servo

https://reviewboard.mozilla.org/r/136034/#review139048

> You should be able to grab it off of the CSSParsingEnv's CSSLoader (see what the Gecko code does above).

I can't move the `GetCSSParsingEnvironment(env);` line outside of the `IsGecko` if. When I try to do that, it fails at this assert: http://searchfox.org/mozilla-central/source/layout/style/ServoStyleRule.cpp#100 I don't think I'm able to get that without `GetCSSParsingEnvironment` method, right?
Well, looking at that function, you should be able to do Rule()->GetStyleSheet()->GetAssociatedDocument() (or ->CSSLoader()),
I think we can create another struct in parallel to CSSParsingEnvironment (maybe called ServoCSSParsingEnv or something) which includes URLData and the quirks mode, and change all the GetURLData to get that struct instead.
bz gave me the same advice on IRC :) Pushed a new patch.
(I'm not happy about namings. They are too lengthy, I'm open to suggestions :) )
Comment on attachment 8864694 [details]
Bug 1360488 - Part 1: stylo: Add ServoCSSParsingEnvironment and pass this instead of URLExtraData

https://reviewboard.mozilla.org/r/136336/#review139414

::: layout/style/StyleRule.cpp:1065
(Diff revision 1)
>  
>    NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override;
>    virtual DeclarationBlock* GetCSSDeclaration(Operation aOperation) override;
>    virtual nsresult SetCSSDeclaration(DeclarationBlock* aDecl) override;
>    virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) override;
> -  URLExtraData* GetURLData() const final;
> +  virtual void GetServoCSSParsingEnvironment(ServoCSSParsingEnvironment& aCSSParseEnv) override;

We don't need `virtual` when we have `override`. Actually we only need a `final` here.

::: layout/style/nsComputedDOMStyle.h:129
(Diff revision 1)
>    // compile errors.
>    virtual mozilla::DeclarationBlock* GetCSSDeclaration(Operation) override;
>    virtual nsresult SetCSSDeclaration(mozilla::DeclarationBlock*) override;
>    virtual nsIDocument* DocToUpdate() override;
>    virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) override;
> -  mozilla::URLExtraData* GetURLData() const final;
> +  virtual void GetServoCSSParsingEnvironment(ServoCSSParsingEnvironment& aCSSParseEnv) override;

Ditto.

::: layout/style/nsDOMCSSAttrDeclaration.h:35
(Diff revision 1)
>  
>    // If GetCSSDeclaration returns non-null, then the decl it returns
>    // is owned by our current style rule.
>    virtual mozilla::DeclarationBlock* GetCSSDeclaration(Operation aOperation) override;
>    virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) override;
> -  mozilla::URLExtraData* GetURLData() const final;
> +  virtual void GetServoCSSParsingEnvironment(ServoCSSParsingEnvironment& aCSSParseEnv) override;

Ditto.

::: layout/style/nsDOMCSSDeclaration.h:151
(Diff revision 1)
>                                           "performance overhead (see bug 649163)") mCSSLoader;
>    };
>  
> +  // Information neded to parse a declaration for Servo side.
> +  struct ServoCSSParsingEnvironment {
> +    mozilla::URLExtraData* MOZ_UNSAFE_REF("user of CSSParsingEnviroment must hold an owning "

I somehow suspect whether we really need `MOZ_UNSAFE_REF`... I guess we can mark this struct `MOZ_STACK_CLASS` and remove this annotation.

::: layout/style/nsDOMCSSDeclaration.h:165
(Diff revision 1)
>    virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) = 0;
>  
> +  // mUrlExtraData returns URL data for parsing url values in
> +  // CSS. Returns nullptr on failure. If mUrlExtraData is nullptr,
> +  // mCompatMode may not be set to anything meaningful.
> +  virtual void GetServoCSSParsingEnvironment(ServoCSSParsingEnvironment& aCSSParseEnv) = 0;

I think you can make this method return `ServoCSSParsingEnvironment` directly, rather than taking a reference.

The struct is small, and the compiler should be able to optimize it to whatever the optimal way to pass the return value.

::: layout/style/nsDOMCSSDeclaration.h:165
(Diff revision 1)
>    virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) = 0;
>  
> +  // mUrlExtraData returns URL data for parsing url values in
> +  // CSS. Returns nullptr on failure. If mUrlExtraData is nullptr,
> +  // mCompatMode may not be set to anything meaningful.
> +  virtual void GetServoCSSParsingEnvironment(ServoCSSParsingEnvironment& aCSSParseEnv) = 0;

Can we mark it `const`? I don't see why it needs to mutate anything.
Comment on attachment 8864694 [details]
Bug 1360488 - Part 1: stylo: Add ServoCSSParsingEnvironment and pass this instead of URLExtraData

https://reviewboard.mozilla.org/r/136336/#review139416
Attachment #8864694 - Flags: review?(bobbyholley) → review+
Comment on attachment 8864363 [details]
Bug 1360488 - Part 2: stylo: Propagate quirks mode information from Gecko to Servo

https://reviewboard.mozilla.org/r/136034/#review139736

::: servo/components/style/stylist.rs:167
(Diff revision 6)
>  
>  impl Stylist {
>      /// Construct a new `Stylist`, using a given `Device`.
>      #[inline]
>      pub fn new(device: Device) -> Self {
> +        let quirks = unsafe { (*(*device.pres_context).mDocument.raw()).mCompatMode };

This is not going to work for servo. We can make it work passing the `QuirksMode` directly though, I guess.
Comment on attachment 8864694 [details]
Bug 1360488 - Part 1: stylo: Add ServoCSSParsingEnvironment and pass this instead of URLExtraData

https://reviewboard.mozilla.org/r/136336/#review139746

::: layout/style/nsCSSRules.cpp:1607
(Diff revision 2)
>    MOZ_ASSERT_UNREACHABLE("GetURLData shouldn't be calling on a Gecko rule");
> -  return GetURLDataForRule(mRule);
> +  return GetServoCSSParsingEnvironmentForRule(mRule);

Just replace all these with MOZ_CRASH(). There's no sense in trying to recover.

::: layout/style/nsDOMCSSDeclaration.h:149
(Diff revision 2)
> +  // Information neded to parse a declaration for Servo side.
> +  struct MOZ_STACK_CLASS ServoCSSParsingEnvironment {
> +    mozilla::URLExtraData* mUrlExtraData;
> +    nsCompatibility mCompatMode;
> +  };

Give this a 2-arg constructor to prevent it from getting default-constructed, and ensure that the values are always explicit?
Comment on attachment 8864363 [details]
Bug 1360488 - Part 2: stylo: Propagate quirks mode information from Gecko to Servo

https://reviewboard.mozilla.org/r/136034/#review139750

::: layout/style/nsDOMCSSDeclaration.cpp:119
(Diff revisions 2 - 6)
> -  CSSParsingEnvironment env;
> -  URLExtraData* urlData = nullptr;
> +  CSSParsingEnvironment geckoEnv;
> +  ServoCSSParsingEnvironment servoEnv;
>    if (olddecl->IsGecko()) {
> -    GetCSSParsingEnvironment(env);
> -    if (!env.mPrincipal) {
> +    GetCSSParsingEnvironment(geckoEnv);
> +    if (!geckoEnv.mPrincipal) {
>        return NS_ERROR_NOT_AVAILABLE;
>      }
>    } else {
> -    urlData = GetURLData();
> -    if (!urlData) {
> +    servoEnv = GetServoCSSParsingEnvironment();
> +    if (!servoEnv.mUrlExtraData) {
>        return NS_ERROR_NOT_AVAILABLE;
>      }
>    }

Along with the previous comment, the lack of a default constructor here is going to be a problem. Can you just move the initializaion of these things into the branches where they're actually needed? That'll make the code cleaner too.

::: layout/style/nsDOMCSSDeclaration.cpp:290
(Diff revisions 2 - 6)
>  
> -/* static */ URLExtraData*
> -nsDOMCSSDeclaration::GetURLDataForRule(const css::Rule* aRule)
> +/* static */ nsDOMCSSDeclaration::ServoCSSParsingEnvironment
> +nsDOMCSSDeclaration::GetServoCSSParsingEnvironmentForRule(const css::Rule* aRule)
>  {
> -  if (StyleSheet* sheet = aRule ? aRule->GetStyleSheet() : nullptr) {
> -    return sheet->AsServo()->URLData();
> +  StyleSheet* sheet = aRule ? aRule->GetStyleSheet() : nullptr;
> +  ServoCSSParsingEnvironment parsinEnv;

nit: parsingEnv (missing g).

::: servo/components/style/stylist.rs
(Diff revisions 2 - 6)
> -    #[allow(non_snake_case)]
>      pub fn new(device: Device) -> Self {
> -        let quirks = unsafe { Gecko_GetCompatibilityMode(&*device.pres_context) };
> +        let quirks = unsafe { (*(*device.pres_context).mDocument.raw()).mCompatMode };

Yeah, we'll want to pass this as an arg.
Comment on attachment 8864694 [details]
Bug 1360488 - Part 1: stylo: Add ServoCSSParsingEnvironment and pass this instead of URLExtraData

https://reviewboard.mozilla.org/r/136032/#review139776

::: layout/style/nsComputedDOMStyle.cpp:722
(Diff revisions 6 - 7)
>  
>  nsDOMCSSDeclaration::ServoCSSParsingEnvironment
>  nsComputedDOMStyle::GetServoCSSParsingEnvironment() const
>  {
>    NS_RUNTIMEABORT("called nsComputedDOMStyle::GetServoCSSParsingEnvironment");
> -  ServoCSSParsingEnvironment parsingEnv;
> +  ServoCSSParsingEnvironment parsingEnv(nullptr, eCompatibility_FullStandards);

I passed Standard mode here but it's probably not important since mUrlExtraData will be null

::: layout/style/nsDOMCSSDeclaration.h:155
(Diff revisions 6 - 7)
>    struct MOZ_STACK_CLASS ServoCSSParsingEnvironment {
>      mozilla::URLExtraData* mUrlExtraData;
>      nsCompatibility mCompatMode;
> +
> +    ServoCSSParsingEnvironment(mozilla::URLExtraData* aUrlData, nsCompatibility aCompatMode)
> +      : mUrlExtraData(aUrlData), mCompatMode(aCompatMode) {}

I'm not sure whether this follows gecko style guides :) I can get some indentation and style recommendation for this if it's not right.
Attachment #8864694 - Flags: review+ → review?(bobbyholley)
Comment on attachment 8864694 [details]
Bug 1360488 - Part 1: stylo: Add ServoCSSParsingEnvironment and pass this instead of URLExtraData

https://reviewboard.mozilla.org/r/136336/#review140570

::: layout/style/nsComputedDOMStyle.cpp:721
(Diff revision 4)
> -  NS_RUNTIMEABORT("called nsComputedDOMStyle::GetURLData");
> -  return nullptr;
> +  NS_RUNTIMEABORT("called nsComputedDOMStyle::GetServoCSSParsingEnvironment");
> +  ServoCSSParsingEnvironment parsingEnv(nullptr, eCompatibility_FullStandards);
> +  return parsingEnv;

This can MOZ_CRASH too.
Attachment #8864694 - Flags: review?(bobbyholley) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2c21526308b
Part 1: stylo: Add ServoCSSParsingEnvironment and pass this instead of URLExtraData r=bholley
https://hg.mozilla.org/integration/autoland/rev/27ea73043eaa
Part 2: stylo: Propagate quirks mode information from Gecko to Servo r=bholley
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: