Note: There are a few cases of duplicates in user autocompletion which are being worked on.

stylo: support ServoStyleSheets in nsIStyleSheetService::PreloadSheet and nsIDOMWindowUtils::AddSheet

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: CSS Object Model
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

({addon-compat})

unspecified
mozilla53
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
There are a few users of nsIStyleSheetService::preloadSheet, and layout/style/test/test_addSheet.html fails without it.
Comment hidden (mozreview-request)
Comment on attachment 8823563 [details]
Bug 1328546 - stylo: Support ServoStyleSheets in nsIStyleSheetService::PreloadSheet and nsIDOMWindowUtils::AddSheet.

It looks fine to me, but I'm not familiar with XPIDL stuff, so I'd prefer bholley to review this.
Attachment #8823563 - Flags: review?(xidorn+moz) → review?(bobbyholley)
So, this could break addons if they were using the result of preloadSheet for something other than passing it to addSheet, or if they were passing things other than the result of preloadSheet into addSheet.

I've checked addons DXR though, and I don't think anyone's doing either of those things. So this should be ok, but tagging for addon-compat to be safe.
Keywords: addon-compat

Comment 4

7 months ago
mozreview-review
Comment on attachment 8823563 [details]
Bug 1328546 - stylo: Support ServoStyleSheets in nsIStyleSheetService::PreloadSheet and nsIDOMWindowUtils::AddSheet.

https://reviewboard.mozilla.org/r/102098/#review102650

::: layout/style/PreloadedStyleSheet.h:32
(Diff revision 1)
> +  RefPtr<StyleSheet> mGeckoSheet;
> +  RefPtr<StyleSheet> mServoSheet;
> +
> +  nsresult mGeckoLoadStatus;
> +  nsresult mServoLoadStatus;

It seems to me like we should group these into a struct containing both called SheetAndStatus, and have two Maybe<SheetAndStatus> members.

::: layout/style/PreloadedStyleSheet.cpp:20
(Diff revision 1)
> +  : mGeckoLoadStatus(NS_OK)
> +  , mServoLoadStatus(NS_OK)
> +  , mURI(aURI)
> +  , mParsingMode(aParsingMode)
> +{
> +  // Preload according to whether the stylo pref is enabled.

I'd expand this comment more, explaining that the API doesn't tell us which backend the sheet will be used for (and in fact, it can be used for both), but that we can make a pretty good guess based on the pref, and so we use that to eagerly load the stylesheet and provide the latency characteristics that the caller expects.

::: layout/style/PreloadedStyleSheet.cpp:35
(Diff revision 1)
> +NS_INTERFACE_MAP_END
> +
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(PreloadedStyleSheet)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(PreloadedStyleSheet)
> +
> +NS_IMPL_CYCLE_COLLECTION_0(PreloadedStyleSheet)

Hm, is this really what we want here? Do StyleSheets participate in cycle collection? If so, we need to note that edge here. If not, I don't see why we need to make this class cycle-collected.
Attachment #8823563 - Flags: review?(bobbyholley) → review-
See Also: → bug 1290224

Comment 5

7 months ago
mozreview-review-reply
Comment on attachment 8823563 [details]
Bug 1328546 - stylo: Support ServoStyleSheets in nsIStyleSheetService::PreloadSheet and nsIDOMWindowUtils::AddSheet.

https://reviewboard.mozilla.org/r/102098/#review102650

> Hm, is this really what we want here? Do StyleSheets participate in cycle collection? If so, we need to note that edge here. If not, I don't see why we need to make this class cycle-collected.

I guess this class could involve in cycle if, e.g. someone assigns an instance to an expando of some descendant of the style sheet object. So I guess we need to note the edge.
(Assignee)

Comment 6

7 months ago
I wonder whether we should fetch the contents of the URL eagerly and store that in the PreloadedStyleSheet.  Perhaps the URL's contents change by the time we load the sheet later for the unexpected StyleBackendType.  Also it would help with the latency if we need to load the sheet later.
(In reply to Cameron McCormack (:heycam) from comment #6)
> I wonder whether we should fetch the contents of the URL eagerly and store
> that in the PreloadedStyleSheet.  Perhaps the URL's contents change by the
> time we load the sheet later for the unexpected StyleBackendType.  Also it
> would help with the latency if we need to load the sheet later.

I don't think we should worry about it. I don't think toggling the pref dynamically is enough of a supported  configuration to justify this level of effort.
Comment hidden (mozreview-request)
Attachment #8823563 - Flags: review?(xidorn+moz) → review?(bobbyholley)

Comment 9

7 months ago
mozreview-review
Comment on attachment 8823563 [details]
Bug 1328546 - stylo: Support ServoStyleSheets in nsIStyleSheetService::PreloadSheet and nsIDOMWindowUtils::AddSheet.

https://reviewboard.mozilla.org/r/102098/#review102762

We could make this code nicer (see comments), but the existing patch is correct, so I'm also ok with landing as-is if you're in a hurry and don't want to mess around with it.

::: layout/base/nsStyleSheetService.cpp:290
(Diff revision 2)
> -  // XXXheycam PreloadSheet can't support ServoStyleSheets until they implement
> -  // nsIDOMStyleSheet.
> +  RefPtr<PreloadedStyleSheet> sheet =
> +    new PreloadedStyleSheet(aSheetURI, parsingMode);
>  
> -  RefPtr<css::Loader> loader = new css::Loader(StyleBackendType::Gecko);
> -
> -  RefPtr<StyleSheet> sheet;
> -  nsresult rv = loader->LoadSheetSync(aSheetURI, parsingMode, true, &sheet);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  nsresult rv = sheet->GetPreloadResult();
> +  if (NS_FAILED(rv)) {
> +    *aSheet = nullptr;
> +    return rv;
> +  }

This API is kinda unintuitive. It would probably be nicer to refactor things such that we call a factory function instead, which first performs  the preload and then only returns a PreloadedStyleSheet in the success case.

This, in turn, would actually let us get rid of SheetAndStatus entirely, which unlocks more cleanups (see below).

::: layout/style/PreloadedStyleSheet.cpp:34
(Diff revision 2)
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(PreloadedStyleSheet)
> +  NS_INTERFACE_MAP_ENTRY(nsIPreloadedStyleSheet)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(PreloadedStyleSheet)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(PreloadedStyleSheet)
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(PreloadedStyleSheet)
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(PreloadedStyleSheet)
> +  if (tmp->mGecko) {
> +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mGecko->mSheet);
> +  }
> +  if (tmp->mServo) {
> +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mServo->mSheet);
> +  }
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(PreloadedStyleSheet)
> +  if (tmp->mGecko) {
> +    NS_IMPL_CYCLE_COLLECTION_UNLINK(mGecko->mSheet);
> +  }
> +  if (tmp->mServo) {
> +    NS_IMPL_CYCLE_COLLECTION_UNLINK(mServo->mSheet);
> +  }
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END

If we get rid of SheetAndStatus as suggested in the other comment, then this whole block of goop could be replaced with:

NS_IMPL_CYCLE_COLLECTION(mGecko, mServo);
Attachment #8823563 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 10

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0e25a70099c23a9a2cbc2d4915db5a9ad552a4b

Comment 11

7 months ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c069e5e1e63f
stylo: Support ServoStyleSheets in nsIStyleSheetService::PreloadSheet and nsIDOMWindowUtils::AddSheet. r=bholley

Comment 12

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c069e5e1e63f
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee: nobody → cam
Depends on: 1336223
You need to log in before you can comment on or make changes to this bug.