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

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Cameron McCormack (:heycam)]

There are a few users of nsIStyleSheetService::preloadSheet, and layout/style/test/test_addSheet.html fails without it.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1328546 - stylo: Support ServoStyleSheets in nsIStyleSheetService::PreloadSheet and nsIDOMWindowUtils::AddSheet.

Review commit: https://reviewboard.mozilla.org/r/102098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102098/

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 6

[Cameron McCormack (:heycam)]

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.

Comment 7

[Bobby Holley (:bholley)]

(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 8

[Cameron McCormack (:heycam)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102098/diff/1-2/

Comment 9

[Bobby Holley (:bholley)]

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);

Comment 10

[Cameron McCormack (:heycam)]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c069e5e1e63f
stylo: Support ServoStyleSheets in nsIStyleSheetService::PreloadSheet and nsIDOMWindowUtils::AddSheet. r=bholley

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/c069e5e1e63f