Tim Taubert [:ttaubert] (inactive) Assignee
	Description • 10 years ago

With bug 1071635 fixed we should add a test to ensure we don't regress this.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 2 • 10 years ago

The test seems to work as it currently shows the two reflows we want to get rid of:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_reflow_load.js | unexpected uninterruptible reflow 'gPage.onPageFirstVisible/checkSizing/<@chrome://browser/content/newtab/newTab.js:506:1|' -
Stack trace:
chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_reflow_load.js:runTests/<:23
chrome://mochitests/content/browser/browser/base/content/test/newtab/content-reflows.js:reflow:15
chrome://browser/content/newtab/newTab.js:gPage.onPageFirstVisible/checkSizing/<:506
null:null:0

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_reflow_load.js | unexpected uninterruptible reflow 'gPage.onPageFirstVisible/checkSizing/<@chrome://browser/content/newtab/newTab.js:506:1|' -
Stack trace:
chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_reflow_load.js:runTests/<:23
chrome://mochitests/content/browser/browser/base/content/test/newtab/content-reflows.js:reflow:15
chrome://browser/content/newtab/newTab.js:gPage.onPageFirstVisible/checkSizing/<:506
null:null:0

	Drew Willcoxon :adw
	Comment 4 • 10 years ago

Comment on attachment 8493853 [details] [diff] [review]
0001-Bug-1071638-Add-test-to-ensure-there-are-no-uninterr.patch, v2

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

Honest question: what are uninterruptible reflows and why are they bad?

::: browser/base/content/test/newtab/browser.ini
@@ +1,5 @@
>  [DEFAULT]
>  skip-if = buildapp == 'mulet' || e10s # Bug ?????? - about:newtab tests don't work in e10s
>  support-files =
>    head.js
> +  content-reflows.js

Nit: I prefer to add support-files inline with the test, keeping only files needed by multiple tests here at the top.

::: browser/base/content/test/newtab/browser_newtab_reflow_load.js
@@ +9,5 @@
> +/*
> + * Ensure that loading about:newtab doesn't cause uninterruptible reflows.
> + */
> +function* runTests() {
> +  Services.prefs.setBoolPref(PREF_INTRO_SHOWN, true);

The testing profile already sets this to true: http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#186

@@ +24,5 @@
> +  });
> +
> +  browser.loadURI("about:newtab");
> +  yield whenBrowserLoaded(browser);
> +  gBrowser.removeCurrentTab({animate: false});

So if an uninterruptible reflow happens, it will always happen before this point?

::: browser/base/content/test/newtab/content-reflows.js
@@ +9,5 @@
> +
> +  docShell.addWeakReflowObserver({
> +    reflow() {
> +      // Gather information about the current code path.
> +      let path = (new Error().stack).split("\n").slice(1).join("|");

Nit: Why not keep the line feeds?  Should be easier to read.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 5 • 10 years ago

(In reply to Drew Willcoxon :adw from comment #4)
> Honest question: what are uninterruptible reflows and why are they bad?

There are two types of reflows. The "bad type" is a reflow that has to happen now or the current operation can't guarantee to return the correct result, e.g. retrieving document.body.offsetWidth. The good reflows are the ones that can happen whenever the layout engine thinks it's best to do them. Another good thing about the second type is that they are "interruptible" which means we can interrupt heavier ones and process some events in between to keep the browser responsive.

> Nit: I prefer to add support-files inline with the test, keeping only files
> needed by multiple tests here at the top.

Yeah, initially thought I'd write a second test that uses this but turns out the test didn't make sense.

> The testing profile already sets this to true:
> http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.
> js#186

Ah, thanks.

> > +  browser.loadURI("about:newtab");
> > +  yield whenBrowserLoaded(browser);
> > +  gBrowser.removeCurrentTab({animate: false});
> 
> So if an uninterruptible reflow happens, it will always happen before this
> point?

Yeah, I checked that with the current code the test fails. There might be some more reflows after the tab has loaded already but they're not as critical to user experience here. I thought about adding a ~2s wait before checking to ensure that no one optimizes for that and just delays reflows by a bit. WDYT?

> > +      // Gather information about the current code path.
> > +      let path = (new Error().stack).split("\n").slice(1).join("|");
> 
> Nit: Why not keep the line feeds?  Should be easier to read.

Yeah, good idea. Stole that from another test where it actually made sense to replace those.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 6 • 10 years ago

r=adw

	Drew Willcoxon :adw
	Comment 7 • 10 years ago

Thanks for the explanation.

(In reply to Tim Taubert [:ttaubert] from comment #5)
> Yeah, I checked that with the current code the test fails. There might be
> some more reflows after the tab has loaded already but they're not as
> critical to user experience here. I thought about adding a ~2s wait before
> checking to ensure that no one optimizes for that and just delays reflows by
> a bit. WDYT?

Is the tab animation done by that point?  If so, then I'd say don't worry about it.  If not, then it might be good to keep listening until it's done.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 8 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=76459c153261

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 9 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/77ae1deb677a

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/77ae1deb677a