Brian Grinstead [:bgrins] Assignee
	Description • 9 years ago

When initially loading the devedition theme, there is a brief delay while the stylesheet is loading.  During this time the normal theme is applied, so the flash is especially noticeable when the dark devedition theme is applied.

	Brian Grinstead [:bgrins] Assignee
	Comment 1 • 9 years ago

We could load the stylesheet before onLoad for the browser (with disabled=true) set.  It will take some reorganizing that will be easier after we convert to a lightweight theme in bug 1094821.

	:Gijs (he/him)
	Comment 2 • 9 years ago

Guessing this now depends on bug 1148996, too.

	Brian Grinstead [:bgrins] Assignee
	Comment 3 • 9 years ago

This approach runs some script at the bottom of browser.js to create the processing instruction - that way it loads before gBrowserInit.onLoad fires.

I have another approach that involves adding the stylesheet directly into browser.xul.  I'll upload that next.

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 4 • 9 years ago

Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25

	Brian Grinstead [:bgrins] Assignee
	Comment 5 • 9 years ago

I actually prefer this approach in terms of it simplifying the code and not having to execute code inline in browser.js.  Although it does have a sort-of-hack in which I set media="none" on the stylesheet (since disabled isn't valid in the markup for some reason).

Gijs, what do you think?

	Brian Grinstead [:bgrins] Assignee
	Comment 6 • 9 years ago

Comment on attachment 8589964 [details] [diff] [review]
devedition-fouc-2.patch

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

::: browser/base/content/browser-devedition.js
@@ +23,5 @@
>    init: function () {
> +
> +    for (let i = document.styleSheets.length - 1; i >= 0; i--) {
> +      let sheet = document.styleSheets[i];
> +      if (sheet.href == "chrome://browser/skin/devedition.css") {

Could use this.styleSheetLocation instead of the hardcoded string

	:Gijs (he/him)
	Comment 7 • 9 years ago

Comment on attachment 8589964 [details] [diff] [review]
devedition-fouc-2.patch

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

I'm worried that this will have perf impact everywhere (not just devedition) because of the now-default-loaded stylesheet (at least, I'm assuming the extra load will delay the load event etc.). I think I would prefer not to do that.

Can you do trypushes to quantify this?

If the impact is significant, it might make sense to do this behind an ifdef only on aurora/devedition, where it is more likely to be used? Although maybe the lwt work means that it's possible to use the theme elsewhere and that might mean this isn't a good argument anymore? Not sure...

	Brian Grinstead [:bgrins] Assignee
	Comment 8 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8589964 [details] [diff] [review]
> devedition-fouc-2.patch
> 
> Review of attachment 8589964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm worried that this will have perf impact everywhere (not just devedition)
> because of the now-default-loaded stylesheet (at least, I'm assuming the
> extra load will delay the load event etc.). I think I would prefer not to do
> that.
> 
> Can you do trypushes to quantify this?
> 
Fair enough.. here is an ongoing try push with talos tests enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8285d37ed839

> If the impact is significant, it might make sense to do this behind an ifdef
> only on aurora/devedition, where it is more likely to be used? Although
> maybe the lwt work means that it's possible to use the theme elsewhere and
> that might mean this isn't a good argument anymore? Not sure...

We are already ifdeffing the existence of the lw theme for non-release builds, so we could definitely do the same for the stylesheet [0].  In theory we could support beta/release with the theme, but until we have a strong reason to do so it's not available.

The tricky part if we excluded the stylesheet is that the test will fail in release builds, so maybe approach 1 is easier to deal with as far as having a function that we can call to do this eager loading.  So we could only call it immediately in browser.js only if it looks like the DE theme is going to be applied - otherwise initialize it only when it's about to be enabled (which will handle the case of the theme being applied from addons manager, or during test runs).

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#723

	:Gijs (he/him)
	Comment 9 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > Comment on attachment 8589964 [details] [diff] [review]
> > devedition-fouc-2.patch
> > 
> > Review of attachment 8589964 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm worried that this will have perf impact everywhere (not just devedition)
> > because of the now-default-loaded stylesheet (at least, I'm assuming the
> > extra load will delay the load event etc.). I think I would prefer not to do
> > that.
> > 
> > Can you do trypushes to quantify this?
> > 
> Fair enough.. here is an ongoing try push with talos tests enabled:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8285d37ed839

I pushed a baseline trypush: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=34010f830d1e

We'll still need to retrigger the talos results on it (probably just the 'o' runs, but we could do the 'tp' ones to be sure).


> > If the impact is significant, it might make sense to do this behind an ifdef
> > only on aurora/devedition, where it is more likely to be used? Although
> > maybe the lwt work means that it's possible to use the theme elsewhere and
> > that might mean this isn't a good argument anymore? Not sure...
> 
> We are already ifdeffing the existence of the lw theme for non-release
> builds, so we could definitely do the same for the stylesheet [0].  In
> theory we could support beta/release with the theme, but until we have a
> strong reason to do so it's not available.
> 
> The tricky part if we excluded the stylesheet is that the test will fail in
> release builds, so maybe approach 1 is easier to deal with as far as having
> a function that we can call to do this eager loading. So we could only call
> it immediately in browser.js only if it looks like the DE theme is going to
> be applied

But we do this already, right? DevEdition.init() is called from onLoad. Does the FOUC only happen for loading devedition while the browser is already running?

	Brian Grinstead [:bgrins] Assignee
	Comment 10 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #9)
> > > If the impact is significant, it might make sense to do this behind an ifdef
> > > only on aurora/devedition, where it is more likely to be used? Although
> > > maybe the lwt work means that it's possible to use the theme elsewhere and
> > > that might mean this isn't a good argument anymore? Not sure...
> > 
> > We are already ifdeffing the existence of the lw theme for non-release
> > builds, so we could definitely do the same for the stylesheet [0].  In
> > theory we could support beta/release with the theme, but until we have a
> > strong reason to do so it's not available.
> > 
> > The tricky part if we excluded the stylesheet is that the test will fail in
> > release builds, so maybe approach 1 is easier to deal with as far as having
> > a function that we can call to do this eager loading. So we could only call
> > it immediately in browser.js only if it looks like the DE theme is going to
> > be applied
> 
> But we do this already, right? DevEdition.init() is called from onLoad. Does
> the FOUC only happen for loading devedition while the browser is already
> running?

We currently add the sheet onLoad (which causes the FOUC on startup because we don't start loading the sheet until after the browse has finished loading).  Approach 1 adds the sheet at the bottom of browser.js (before onLoad, just directly executing in the script).  We could modify that approach by only doing it before onLoad if DevEdition._isThemeCurrentlyApplied.

One thing about approach 1 that's weird is that it ends up executing also in the hiddenDOMWindow.  I'm not sure what the best way to differentiate at that point in time is, since gBrowser doesn't yet exist.

	:Gijs (he/him)
	Comment 11 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > > > If the impact is significant, it might make sense to do this behind an ifdef
> > > > only on aurora/devedition, where it is more likely to be used? Although
> > > > maybe the lwt work means that it's possible to use the theme elsewhere and
> > > > that might mean this isn't a good argument anymore? Not sure...
> > > 
> > > We are already ifdeffing the existence of the lw theme for non-release
> > > builds, so we could definitely do the same for the stylesheet [0].  In
> > > theory we could support beta/release with the theme, but until we have a
> > > strong reason to do so it's not available.
> > > 
> > > The tricky part if we excluded the stylesheet is that the test will fail in
> > > release builds, so maybe approach 1 is easier to deal with as far as having
> > > a function that we can call to do this eager loading. So we could only call
> > > it immediately in browser.js only if it looks like the DE theme is going to
> > > be applied
> > 
> > But we do this already, right? DevEdition.init() is called from onLoad. Does
> > the FOUC only happen for loading devedition while the browser is already
> > running?
> 
> We currently add the sheet onLoad (which causes the FOUC on startup because
> we don't start loading the sheet until after the browse has finished
> loading).  Approach 1 adds the sheet at the bottom of browser.js (before
> onLoad, just directly executing in the script).  We could modify that
> approach by only doing it before onLoad if
> DevEdition._isThemeCurrentlyApplied.
> 
> One thing about approach 1 that's weird is that it ends up executing also in
> the hiddenDOMWindow.  I'm not sure what the best way to differentiate at
> that point in time is, since gBrowser doesn't yet exist.

Compare window with Services.appShell.hiddenDOMWindow ? :-)

	Brian Grinstead [:bgrins] Assignee
	Comment 12 • 9 years ago

I think loading the stylesheet in js is going to give us more control over your concerns in comment 7.  I've extracted the stylesheet loading into a separate function, which is called at these times:

* Immediately in browser.js if the DE theme is going to be applied
* Inside _toggleStyleSheet if the sheet is going to be enabled and it hasn't been called yet.  This function is called in a lightweight-theme-styling-update event

It preserves the existing behavior in the case of a non-DE-themed browser.  Which is that the stylesheet never gets created.

Assuming you agree with this plan, my main question/concern has to do with the placement of the call to DevEdition.createStyleSheet in browser.js.  Maybe there is a different callback that happens earlier where it could run, or it could be put into browser-devedition.js and get preprocessed into browser.js at that point.

	:Gijs (he/him)
	Comment 13 • 9 years ago

Yeah, tpaint and ts_paint are sad pretty much everywhere:

http://compare-talos.mattn.ca/?oldRevs=34010f830d1e&newRev=8285d37ed839&server=graphs.mozilla.org&submit=true

:-(

	:Gijs (he/him)
	Comment 14 • 9 years ago

Comment on attachment 8590305 [details] [diff] [review]
devedition-fouc.patch

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

::: browser/base/content/browser-devedition.js
@@ +34,5 @@
>  
> +  createStyleSheet: function() {
> +    let styleSheetAttr = `href="${this.styleSheetLocation}" type="text/css"`;
> +    this.styleSheet = document.createProcessingInstruction(
> +      'xml-stylesheet', styleSheetAttr);

Nit: double quotes.

::: browser/base/content/browser.js
@@ +7808,5 @@
> +// then preload it now.  This prevents a flash of unstyled content where the
> +// normal theme is applied while the DevEdition stylesheet is loading.
> +if (this != Services.appShell.hiddenDOMWindow && DevEdition.isThemeCurrentlyApplied) {
> +  DevEdition.createStyleSheet();
> +}

Right, I think including this toplevel in browser-devedition.js is fine.

Should this be ifdef'd for non-release builds, then, per your earlier comment that the theme is never available on beta/release?

	Brian Grinstead [:bgrins] Assignee
	Comment 15 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #14)
> 
> Should this be ifdef'd for non-release builds, then, per your earlier
> comment that the theme is never available on beta/release?

May as well. *In theory* devedition should never be returned as the LWTM.currentTheme in release, but it doesn't hurt anything to preprocess this out.

	Brian Grinstead [:bgrins] Assignee
	Comment 16 • 9 years ago

Moved the call to createStyleSheet into browser-devedition.js and preprocessed it with #ifndef RELEASE_BUILD.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0a6a2d1c9cd
Along with a reference one for talos comparison: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27c34c547f11.  Although really this shouldn't have an effect on talos since the DE theme is preffed off there.

	:Gijs (he/him)
	Comment 17 • 9 years ago

Comment on attachment 8590479 [details] [diff] [review]
devedition-fouc.patch

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

r=me!

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/484fe7759dd4

	Carsten Book [:Tomcat]
	Comment 19 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/484fe7759dd4