1250788 - support user agent style sheets being parsed as ServoStyleSheets and being added to the style set

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add interface to create and parse Servo style sheets.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 2: Expose IsCSSSheetType method.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Part 3: Implement enough of ServoStyleSheet for Loader to be able to create and parse one.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 4: Implement enough of ServoStyleSet to handle the UA style sheets being added to it.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 5: Create and parse ServoStyleSheets in css::Loader.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Part 6: Create ServoStyleSheets for the pref style sheet.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 3: Implement enough of ServoStyleSheet for Loader to be able to create and parse one. (v1.1)

Fix the mPrincipalSet problem.

Comment 8

[Cameron McCormack (:heycam)]

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

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8722860 [details] [diff] [review]
Part 1: Add interface to create and parse Servo style sheets.

Review of attachment 8722860 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that.

::: layout/style/ServoBindings.h
@@ +25,5 @@
>  typedef mozilla::dom::Element RawGeckoElement;
>  class nsIDocument;
>  typedef nsIDocument RawGeckoDocument;
>  struct ServoNodeData;
> +struct ServoArcStyleSheet;

I think we should avoid embedding the name 'Arc' in the servo type, just like we don't embed refcountability into gecko type names. In truth the 'Arc' is the thing that we transmute into the pointer, so the pointer type would be the Arc, whereas the actual struct would just be a stylesheet.


So let's just call this ServoStyleSheet.

@@ +73,5 @@
>  void Gecko_SetNodeData(RawGeckoNode* node, ServoNodeData* data);
>  void Servo_DropNodeData(ServoNodeData* data);
>  
> +// Stylesheet management.
> +ServoArcStyleSheet* Servo_StylesheetFromUTF8Bytes(const uint8_t* bytes, uint32_t length);

Once we get C++ parsing of this file working I'd like to make this return an already_AddRefed. Can you add a comment to that effect?

@@ +74,5 @@
>  void Servo_DropNodeData(ServoNodeData* data);
>  
> +// Stylesheet management.
> +ServoArcStyleSheet* Servo_StylesheetFromUTF8Bytes(const uint8_t* bytes, uint32_t length);
> +void Servo_DropStylesheet(ServoArcStyleSheet* sheet);

In keeping with refcounting terminology, I think we should call this Servo_ReleaseStylesheet.

Don't worry about fixing this up on the servo side - I'll take care of that when I refresh all the bindings.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8722861 [details] [diff] [review]
Part 2: Expose IsCSSSheetType method.

Review of attachment 8722861 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleSet.h
@@ +396,5 @@
>    // Tells the RestyleManager for the document using this style set
>    // to drop any nsCSSSelector pointers it has.
>    void ClearSelectors();
>  
> +  static bool IsCSSSheetType(mozilla::SheetType aSheetType);

I don't really understand what this does. Can you add a comment?

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8722868 [details] [diff] [review]
Part 3: Implement enough of ServoStyleSheet for Loader to be able to create and parse one. (v1.1)

Review of attachment 8722868 [details] [diff] [review]:
-----------------------------------------------------------------

Per IRC discussion, I think we should make a shared superclass between ServoStyleSheet and CSSStyleSheetInner to handle the management of all these random members.

::: layout/style/ServoStyleSheet.h
@@ +74,5 @@
> +  nsCOMPtr<nsIURI> mSheetURI;
> +  nsCOMPtr<nsIURI> mOriginalURI;
> +  nsCOMPtr<nsIURI> mBaseURI;
> +  nsCOMPtr<nsIPrincipal> mPrincipal;
> +  nsIDocument* mDocument; // weak ref; parents maintain this for their children

We never set this anywhere, so I'd rather just omit the member for the time being until we add support for it.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8722865 [details] [diff] [review]
Part 5: Create and parse ServoStyleSheets in css::Loader.

Review of attachment 8722865 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/Loader.cpp
@@ +1760,5 @@
> +    aLoadData->mSheet->AsServo()->ParseSheet(aInput, sheetURI, baseURI,
> +                                             aLoadData->mSheet->Principal(),
> +                                             aLoadData->mLineNumber,
> +                                             aLoadData->mParsingMode);
> +    rv = NS_OK;

Do we not want to use this to surface parsing errors? Or is that done some other way?

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8722866 [details] [diff] [review]
Part 6: Create ServoStyleSheets for the pref style sheet.

Review of attachment 8722866 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsLayoutStylesheetCache.cpp
@@ +940,5 @@
> +  if (aSheet->IsGecko()) {
> +    aSheet->AsGecko()->ReparseSheet(sheetText);
> +  } else {
> +    aSheet->AsServo()->ParseSheet(sheetText, uri, uri, nullptr, 0,
> +                                  SheetParsingMode::eAuthorSheetFeatures);

What's the difference between ReparseSheet and doing ParseSheet with an nsCSSParser?

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #12)
> Do we not want to use this to surface parsing errors? Or is that done some
> other way?

Yeah, nsCSSParser surfaces parsing errors through the css::ErrorReporter it creates (which routes the error to the appropriate window's web console).  We'll eventually need something similar for ServoStyleSheet parsing.  Actually, it looks like nsCSSParser::ParseStyleSheet always returns NS_OK, anyway...

Comment 15

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #13)
> What's the difference between ReparseSheet and doing ParseSheet with an
> nsCSSParser?

For a newly created sheet and no associated document, it will be the same.  (I think it was just shorter to type than using nsCSSParser directly, when I originally changed InvalidatePreferenceSheets in bug 1234773.)

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Part 3.1: Factor out CSSStyleSheetInner members so they can be used by ServoStyleSheet.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 3.2: Factor out CSSStyleSheet members so they can be used by ServoStyleSheet.

Comment 18

[Cameron McCormack (:heycam)]

Attachment: Part 3.3: Implement enough of ServoStyleSheet for Loader to be able to create and parse one.

Comment 19

[Cameron McCormack (:heycam)]

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

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8723405 [details] [diff] [review]
Part 3.1: Factor out CSSStyleSheetInner members so they can be used by ServoStyleSheet.

Review of attachment 8723405 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/moz.build
@@ +186,5 @@
>      'ServoStyleSet.cpp',
>      'ServoStyleSheet.cpp',
>      'StyleAnimationValue.cpp',
>      'StyleRule.cpp',
> +    'StyleSheetInfo.cpp',

Beware unified bustage. :-P

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8723409 [details] [diff] [review]
Part 3.2: Factor out CSSStyleSheet members so they can be used by ServoStyleSheet.

Review of attachment 8723409 [details] [diff] [review]:
-----------------------------------------------------------------

This superclass is pretty sparse, but I guess the idea is that we'll eventually hoist more things into it, right?

Comment 22

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #21)
> This superclass is pretty sparse, but I guess the idea is that we'll
> eventually hoist more things into it, right?

Yes, I hope so!

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 8723410 [details] [diff] [review]
Part 3.3: Implement enough of ServoStyleSheet for Loader to be able to create and parse one.

Review of attachment 8723410 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSheet.cpp
@@ +11,5 @@
> +ServoStyleSheet::ServoStyleSheet(CORSMode aCORSMode,
> +                                 net::ReferrerPolicy aReferrerPolicy,
> +                                 const dom::SRIMetadata& aIntegrity)
> +  : StyleSheetInfo(aCORSMode, aReferrerPolicy, aIntegrity)
> +  , mSheet(nullptr)

If we use a RefPtr, the nullptr-init here can go.

@@ +80,5 @@
>  {
> +  DropSheet();
> +
> +  NS_ConvertUTF16toUTF8 input(aInput);
> +#ifdef MOZ_STYLO

This #ifdeffing is for the linkage, right?

I think a better solution would be to add MOZ_CRASHing stubs for the servo routines in ServoBindings.cpp behind and #ifndef MOZ_STYLO. That way we can use a single preprocessor conditional and avoid scattering them everywhere.

Same with DropSheet below.

@@ +98,5 @@
> +    Servo_ReleaseStylesheet(mSheet);
> +#else
> +    MOZ_CRASH("stylo: should not be using ServoStyleSheet objects in a non-"
> +              "MOZ_STYLO build");
> +#endif

ideally the logic here would move into the RefPtrTraits, and DropSheet calls can be replaced with |mSheet = nullptr;|

::: layout/style/ServoStyleSheet.h
@@ +30,5 @@
>    NS_INLINE_DECL_REFCOUNTING(ServoStyleSheet)
>  
> +  nsIURI* GetSheetURI() const { return mSheetURI; }
> +  nsIURI* GetOriginalURI() const { return mOriginalSheetURI; }
> +  nsIURI* GetBaseURI() const { return mBaseURI; }

It seems like we might as well hoist these into StyleSheetInfo.

@@ +39,5 @@
>  
>    nsIDocument* GetOwningDocument() const;
>    void SetOwningDocument(nsIDocument* aDocument);
>  
> +  nsINode* GetOwnerNode() const { return mOwningNode; }

And this.

@@ +44,5 @@
>  
>    StyleSheetHandle GetParentSheet() const;
>    void AppendStyleSheet(StyleSheetHandle aSheet);
>  
> +  nsIPrincipal* Principal() const { return mPrincipal; }

And this.

@@ +49,4 @@
>  
> +  CORSMode GetCORSMode() const { return mCORSMode; }
> +  net::ReferrerPolicy GetReferrerPolicy() const { return mReferrerPolicy; }
> +  void GetIntegrity(dom::SRIMetadata& aResult) const { aResult = mIntegrity; }

And these.

@@ +69,5 @@
> +
> +private:
> +  void DropSheet();
> +
> +  RawServoStyleSheet* mSheet;

Can we implement RefPtrTraits for RawServoStyleSheet and make this a RefPtr? Followup is fine if the patch ends up nontrivial enough to need review and that review would delay landing. I think it's probably trivial though.

In one of my patches upstream, I have a ServoBindingHelpers.h that looks like this: https://pastebin.mozilla.org/8861102

Feel free to add something similar here, r=me.

Comment 24

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cfd5df65247

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9da0d93d757d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9471c9a842ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5003168dd68
https://hg.mozilla.org/integration/mozilla-inbound/rev/a576bae69137
https://hg.mozilla.org/integration/mozilla-inbound/rev/51fba56fe26c
https://hg.mozilla.org/integration/mozilla-inbound/rev/13ec36aaffcf
https://hg.mozilla.org/integration/mozilla-inbound/rev/e787e35539bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/76673f4686dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd451c2bc80

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/9da0d93d757d
https://hg.mozilla.org/mozilla-central/rev/9471c9a842ef
https://hg.mozilla.org/mozilla-central/rev/a5003168dd68
https://hg.mozilla.org/mozilla-central/rev/a576bae69137
https://hg.mozilla.org/mozilla-central/rev/51fba56fe26c
https://hg.mozilla.org/mozilla-central/rev/13ec36aaffcf
https://hg.mozilla.org/mozilla-central/rev/e787e35539bc
https://hg.mozilla.org/mozilla-central/rev/76673f4686dd
https://hg.mozilla.org/mozilla-central/rev/dbd451c2bc80