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

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(8 attachments, 2 obsolete attachments)

1.78 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.56 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.70 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.53 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.82 KB, patch
bholley
: review+
Details | Diff | Splinter Review
11.38 KB, patch
bholley
: review+
Details | Diff | Splinter Review
10.26 KB, patch
bholley
: review+
Details | Diff | Splinter Review
7.51 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Assignee

Description

3 years ago
No description provided.
Assignee

Comment 1

3 years ago
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8722860 - Flags: review?(bobbyholley)
Assignee

Comment 2

3 years ago
Attachment #8722861 - Flags: review?(bobbyholley)
Assignee

Comment 7

3 years ago
Fix the mPrincipalSet problem.
Attachment #8722862 - Attachment is obsolete: true
Attachment #8722862 - Flags: review?(bobbyholley)
Attachment #8722868 - Flags: review?(bobbyholley)
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.
Attachment #8722860 - Flags: review?(bobbyholley) → review+
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?
Attachment #8722861 - Flags: review?(bobbyholley) → review+
Attachment #8722863 - Flags: review?(bobbyholley) → review+
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.
Attachment #8722868 - Flags: review?(bobbyholley) → review-
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?
Attachment #8722865 - Flags: review?(bobbyholley) → review+
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?
Attachment #8722866 - Flags: review?(bobbyholley) → review+
Assignee

Comment 14

3 years ago
(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...
Assignee

Comment 15

3 years ago
(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 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
Attachment #8723405 - Flags: review?(bobbyholley) → review+
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?
Attachment #8723409 - Flags: review?(bobbyholley) → review+
Assignee

Comment 22

3 years ago
(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 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.
Attachment #8723410 - Flags: review?(bobbyholley) → review+
You need to log in before you can comment on or make changes to this bug.