Kris Maglione [:kmag] Assignee
	Description • 8 years ago

This is a follow-up to bug 1333990, to load and cache content stylesheets the same way we do content scripts.

	Cameron McCormack (:heycam)
	Comment 9 • 8 years ago
mozreview-review

Comment on attachment 8848685 [details]
Bug 1348442: Part 1 - Allow loading preloaded stylesheets asynchronously.

https://reviewboard.mozilla.org/r/121580/#review123656

This looks generally OK, but some comments to address and one question.

::: layout/base/nsIStyleSheetService.idl:60
(Diff revision 2)
>                                        in unsigned long type);
>  
>    /**
> +   * Asynchronously loads a style sheet from |sheetURI| and returns a Promise
> +   * which resolves to the new style sheet object when it has completed
> +   * loading. Can be used with nsIDOMWindowUtils.addSheet.

s/, Can/, which can/, so that it's clearer that the thing the Promise resolves to can be used with addSheet, not the Promise object itself.

::: layout/base/nsIStyleSheetService.idl:63
(Diff revision 2)
> +   * Asynchronously loads a style sheet from |sheetURI| and returns a Promise
> +   * which resolves to the new style sheet object when it has completed
> +   * loading. Can be used with nsIDOMWindowUtils.addSheet.
> +   */
> +  [implicit_jscontext]
> +  jsval asyncPreloadSheet(in nsIURI sheetURI, in unsigned long type);

Would prefer "preloadSheetAsync", to mirror the naming of some methods on css::Loader, which use "Sync" as a suffix.

::: layout/base/nsStyleSheetService.cpp:308
(Diff revision 2)
> +
> +    default:
> +      NS_WARNING("invalid sheet type argument");
> +      return NS_ERROR_INVALID_ARG;
> +  }
> +

Nit: remove this blank line.

::: layout/base/nsStyleSheetService.cpp:334
(Diff revision 2)
>  
> -    default:
> -      NS_WARNING("invalid sheet type argument");
> -      return NS_ERROR_INVALID_ARG;
> +  sheet.forget(aSheet);
> +  return NS_OK;
> +}
> +
> +class StylesheetPreloadObserver final : public nsICSSLoaderObserver {

Nit: "{" on new line.

::: layout/base/nsStyleSheetService.cpp:338
(Diff revision 2)
> +
> +class StylesheetPreloadObserver final : public nsICSSLoaderObserver {
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  explicit StylesheetPreloadObserver(dom::Promise* aPromise, nsIPreloadedStyleSheet* aSheet)

Nit: please wrap at 80 columns.

::: layout/base/nsStyleSheetService.cpp:343
(Diff revision 2)
> +  explicit StylesheetPreloadObserver(dom::Promise* aPromise, nsIPreloadedStyleSheet* aSheet)
> +    : mPromise(aPromise)
> +    , mPreloadedSheet(aSheet)
> +  {}
> +
> +  NS_IMETHOD StyleSheetLoaded(mozilla::StyleSheet* aSheet,

Nit: no need to use "mozilla::".

::: layout/base/nsStyleSheetService.cpp:358
(Diff revision 2)
> +};
> +
> +NS_IMPL_ISUPPORTS(StylesheetPreloadObserver, nsICSSLoaderObserver)
> +
> +NS_IMETHODIMP
> +StylesheetPreloadObserver::StyleSheetLoaded(mozilla::StyleSheet* aSheet,

Nit: or here.

::: layout/base/nsStyleSheetService.cpp:401
(Diff revision 2)
> -  sheet.forget(aSheet);
> +  RefPtr<StylesheetPreloadObserver> obs = new StylesheetPreloadObserver(promise, sheet);
> +  sheet->PreloadAsync(obs);
> +
> +  if (!ToJSValue(aCx, promise, aRval)) {
> +    return NS_ERROR_FAILURE;
> +  };

Nit: no trailing ';'.

::: layout/style/Loader.h:342
(Diff revision 2)
>     * sheets not associated with a document.  This method cannot be used to
>     * load user or agent sheets.

The presence of the aParsingMode argument contradicts this last sentence of the comment.  I guess you want to adjust it to be more like LoadSheetSync's comment.

::: layout/style/Loader.cpp:2351
(Diff revision 2)
> +                  SheetParsingMode aParsingMode,
> +                  bool aUseSystemPrincipal,
> +                  nsICSSLoaderObserver* aObserver,
> +                  RefPtr<StyleSheet>* aSheet)
> +{
> +  LOG(("css::Loader::LoadSheet"));

Please use something like LOG(("css::Loader::LoadSheet(aURL, aParsingMode, aObserver, aSheet)")) to distinguish this from the other LoadSheet methods.

::: layout/style/PreloadedStyleSheet.cpp:88
(Diff revision 2)
> +// Note: After calling this method, the preloaded sheet *must not* be used
> +// until the observer is notified that the sheet has finished loading.

Rather than just a comment, I think we need some sort of protection against trying to async load a sheet multiple times at once.  Can you add something that makes PreloadAsync fail if we've already called it?  We should also protect against calling Preload after PreloadAsync, or vice versa.

Also please make nsIDOMWindowUtils.addSheet fail if passed a PreloadedStyleSheet that has had PreloadAsync called on it but which hasn't finished loading yet.

The comment here would be better placed in nsIStyleSheetService.idl, where users of it are more likely to see it.

::: layout/style/PreloadedStyleSheet.cpp:99
(Diff revision 2)
> +  RefPtr<css::Loader> loader = new css::Loader(type);
> +  return loader->LoadSheet(mURI, mParsingMode, true, aObserver, &sheet);

What happens if we incorrectly guess the StyleBackendType that the caller wants?  It seems like we would then end up loading the sheet sync (assuming the assertions you add don't get in the way of that).  Is that the behaviour you want?

	Josh Matthews [:jdm]
	Comment 10 • 8 years ago
mozreview-review

Comment on attachment 8848688 [details]
Bug 1348442: Part 4 - Remove support for synchronously loading localized extension CSS.

https://reviewboard.mozilla.org/r/121586/#review123666

	Kris Maglione [:kmag] Assignee
	Comment 11 • 8 years ago
mozreview-review-reply

Comment on attachment 8848685 [details]
Bug 1348442: Part 1 - Allow loading preloaded stylesheets asynchronously.

https://reviewboard.mozilla.org/r/121580/#review123656

> Rather than just a comment, I think we need some sort of protection against trying to async load a sheet multiple times at once.  Can you add something that makes PreloadAsync fail if we've already called it?  We should also protect against calling Preload after PreloadAsync, or vice versa.
> 
> Also please make nsIDOMWindowUtils.addSheet fail if passed a PreloadedStyleSheet that has had PreloadAsync called on it but which hasn't finished loading yet.
> 
> The comment here would be better placed in nsIStyleSheetService.idl, where users of it are more likely to see it.

A comment in nsIStyleSheetService.idl shouldn't be necessary, since consumers of that API will never get a reference to the preloaded sheet until it's completely loaded.

I suppose I could move the load observer to this module and pass in the promise to resolve rather than the load observer, and have that set the appropriate flags on the instance.

> What happens if we incorrectly guess the StyleBackendType that the caller wants?  It seems like we would then end up loading the sheet sync (assuming the assertions you add don't get in the way of that).  Is that the behaviour you want?

It's not necessarily the behavior I want, but I can't think of a better solution, so it's behavior that I'm willing to accept.

	Cameron McCormack (:heycam)
	Comment 23 • 8 years ago
mozreview-review-reply

Comment on attachment 8848685 [details]
Bug 1348442: Part 1 - Allow loading preloaded stylesheets asynchronously.

https://reviewboard.mozilla.org/r/121580/#review123656

> A comment in nsIStyleSheetService.idl shouldn't be necessary, since consumers of that API will never get a reference to the preloaded sheet until it's completely loaded.
> 
> I suppose I could move the load observer to this module and pass in the promise to resolve rather than the load observer, and have that set the appropriate flags on the instance.

OK, that's a good point.  The new arrangement looks good.

	Cameron McCormack (:heycam)
	Comment 24 • 8 years ago
mozreview-review

Comment on attachment 8848685 [details]
Bug 1348442: Part 1 - Allow loading preloaded stylesheets asynchronously.

https://reviewboard.mozilla.org/r/121580/#review123694

::: layout/style/PreloadedStyleSheet.h:24
(Diff revision 3)
>  #include "nsIPreloadedStyleSheet.h"
>  
>  class nsIURI;
>  
>  namespace mozilla {
> +namespace dom{

Nit: space before "{".

::: layout/style/PreloadedStyleSheet.h:50
(Diff revision 3)
> +  bool mLoaded;
> +

Nit: I guess I'd move that down next to the other member variables.  (Don't think there's any advantage to explicitly making it protected.)

::: layout/style/PreloadedStyleSheet.cpp:123
(Diff revision 3)
> +  auto type = nsLayoutUtils::StyloEnabled() ? StyleBackendType::Servo
> +                                            : StyleBackendType::Gecko;

Can you add a small comment in here saying that similarly to Preload(), we guess what StyleBackendType to use, but that if we get it wrong, we will end up loading the style sheet sync when it gets added to the document.

	Kris Maglione [:kmag] Assignee
	Comment 25 • 8 years ago
mozreview-review-reply

Comment on attachment 8848685 [details]
Bug 1348442: Part 1 - Allow loading preloaded stylesheets asynchronously.

https://reviewboard.mozilla.org/r/121580/#review123694

> Can you add a small comment in here saying that similarly to Preload(), we guess what StyleBackendType to use, but that if we get it wrong, we will end up loading the style sheet sync when it gets added to the document.

Done. Thanks

	Andrew Swan [:aswan]
	Comment 32 • 8 years ago
mozreview-review

Comment on attachment 8848685 [details]
Bug 1348442: Part 1 - Allow loading preloaded stylesheets asynchronously.

https://reviewboard.mozilla.org/r/121580/#review124226

::: commit-message-d70f8:5
(Diff revision 4)
> +Bug 1348442: Part 1 - Allow loading preloaded stylesheets asynchronously. r=heycam
> +
> +These changes allow us to asynchronously load pre-loaded stylesheets, in a way
> +that's similar to ChromeUtils.compileScript. The new method returns a Promise
> +which resolves to the preloaded wheet once it's finished loading.

nit: sheet

	Andrew Swan [:aswan]
	Comment 33 • 8 years ago
mozreview-review

Comment on attachment 8848818 [details]
Bug 1348442: Part 2b - Add getWinUtils helper.

https://reviewboard.mozilla.org/r/121700/#review124230

::: toolkit/components/extensions/ExtensionUtils.jsm:294
(Diff revision 2)
> +const _winUtils = new DefaultWeakMap(win => {
> +  return win.QueryInterface(Ci.nsIInterfaceRequestor)
> +            .getInterface(Ci.nsIDOMWindowUtils);
> +});
> +const getWinUtils = win => _winUtils.get(win);

What's the workload where this is a meaningful optimization?

	Andrew Swan [:aswan]
	Comment 34 • 8 years ago
mozreview-review

Comment on attachment 8848686 [details]
Bug 1348442: Part 2a - Asynchronously load and cache content script CSS.

https://reviewboard.mozilla.org/r/121582/#review124228

::: toolkit/components/extensions/ExtensionContent.jsm:115
(Diff revision 5)
>  }();
>  
> -const SCRIPT_EXPIRY_TIMEOUT_MS = 300000;
> -const SCRIPT_CLEAR_TIMEOUT_MS = 5000;
> +const SCRIPT_EXPIRY_TIMEOUT_MS = 5 * 60 * 1000;
> +const SCRIPT_CLEAR_TIMEOUT_MS = 5 * 1000;
> +
> +const CSS_EXPIRY_TIMEOUT_MS = 30 * 60 * 1000;

Can you add comments explaining these constants?  What's the rationale for a longer expiration time for css than for js?

	Andrew Swan [:aswan]
	Comment 35 • 8 years ago
mozreview-review

Comment on attachment 8848819 [details]
Bug 1348442: Part 2c - Refactor Script class into ES6 class.

https://reviewboard.mozilla.org/r/121702/#review124232

	Kris Maglione [:kmag] Assignee
	Comment 36 • 8 years ago
mozreview-review-reply

Comment on attachment 8848818 [details]
Bug 1348442: Part 2b - Add getWinUtils helper.

https://reviewboard.mozilla.org/r/121700/#review124230

> What's the workload where this is a meaningful optimization?

We use nsIDOMWindowUtils a lot, and XPConnect calls are extremely expensive. Every time we get the cached version of this, we save 4 XPConnect calls, and avoid getting kicked out of the JIT until we touch the result object.

	Kris Maglione [:kmag] Assignee
	Comment 37 • 8 years ago
mozreview-review-reply

Comment on attachment 8848686 [details]
Bug 1348442: Part 2a - Asynchronously load and cache content script CSS.

https://reviewboard.mozilla.org/r/121582/#review124228

> Can you add comments explaining these constants?  What's the rationale for a longer expiration time for css than for js?

The fact that we never evict cached stylesheets that are still in use means that there's a much better chance that they'll still be in use after 5 minutes than there is for scripts.

	Andrew Swan [:aswan]
	Comment 38 • 8 years ago
mozreview-review

Comment on attachment 8848687 [details]
Bug 1348442: Part 3 - Do not evict cached stylesheets while they're still being used by extant documents.

https://reviewboard.mozilla.org/r/121584/#review125014

(whoops sorry this lived in reviewboard limbo for a few days)
The mechanism here seems fine, but I don't fully understand the rationale and the comment inside delete() isn't all that helpful.  An explanation more accessible to somebody who, for instance, doesn't know exactly what a "rule processor" is would be appreciated.

	Kris Maglione [:kmag] Assignee
	Comment 39 • 8 years ago

(In reply to Andrew Swan [:aswan] from comment #38)
> (whoops sorry this lived in reviewboard limbo for a few days)
> The mechanism here seems fine, but I don't fully understand the rationale
> and the comment inside delete() isn't all that helpful.  An explanation more
> accessible to somebody who, for instance, doesn't know exactly what a "rule
> processor" is would be appreciated.

Think of it like a pre-compiled regular expression.

	Andrew Swan [:aswan]
	Comment 40 • 8 years ago

(In reply to Kris Maglione [:kmag] from comment #39)
> (In reply to Andrew Swan [:aswan] from comment #38)
> > (whoops sorry this lived in reviewboard limbo for a few days)
> > The mechanism here seems fine, but I don't fully understand the rationale
> > and the comment inside delete() isn't all that helpful.  An explanation more
> > accessible to somebody who, for instance, doesn't know exactly what a "rule
> > processor" is would be appreciated.
> 
> Think of it like a pre-compiled regular expression.

I wasn't clear but I was asking for a more detailed explanation in the comments in the code.

	Kris Maglione [:kmag] Assignee
	Comment 41 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5b7156ec45f277a225608c1e719cbb946850527
Bug 1348442: Part 1 - Allow loading preloaded stylesheets asynchronously. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b89dcab1adaaf2fdd7a4d1d3a83c0ca58620e5e
Bug 1348442: Part 2a - Asynchronously load and cache content script CSS. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/963bf8bab6e76ac7993af424088d8d7315475fa6
Bug 1348442: Part 2b - Add getWinUtils helper. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/02ffd84b3a0bc6f250eae27af8ec3e67b92113b5
Bug 1348442: Part 2c - Refactor Script class into ES6 class. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0898770b8f174f9edeca40267348801fa23a02
Bug 1348442: Part 3 - Do not evict cached stylesheets while they're still being used by extant documents. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/dda3d09783a5b5fe61c6591aa53d77105147b123
Bug 1348442: Part 4 - Remove support for synchronously loading localized extension CSS. r=jdm

	Carsten Book [:Tomcat]
	Comment 42 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d5b7156ec45f
https://hg.mozilla.org/mozilla-central/rev/0b89dcab1ada
https://hg.mozilla.org/mozilla-central/rev/963bf8bab6e7
https://hg.mozilla.org/mozilla-central/rev/02ffd84b3a0b
https://hg.mozilla.org/mozilla-central/rev/9b0898770b8f
https://hg.mozilla.org/mozilla-central/rev/dda3d09783a5