848358 - Add talos regression test for tab animation smoothness

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Avi Halachmi (:avih)]

We recently added tab animation smoothness telemetry (bug 828097), and there's a patch (no current intention to land it) which automates and measures a sequence of tab open/close, and is triggered manually (attachment at bug 837885). This patch is used to measure changes in Australis and newtab page performance (bug 837885, bug 843853).

So we might want to make this a proper talos regression test.

Need to consider:
1. It's noisy. It usually takes about 3-4 manual runs of 15x close+open tab sequences to start getting stable results, and even that is not guaranteed.
2. It's long. 4 runs of 15x open+close, with few seconds of wait between, would be towards 1 minute.

The current patch repeats tab open+close 15 times by default, and measures average frame interval and average paint time. ttaubert modified it a bit to (also?) report the median of number of frames during the animation.

Discuss:
1. Do we want this for talos?
2. What should we measure?
3. How do we make the results consistent and with good value?

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Just my 2c - avih suggested that the test only run 5-10 seconds after the main window is first shown so as to allow Firefox to "settle". Not sure if talos already takes that into account, but it's a thing that that we did during our tests.

As for what we should measure, a few ideas:

1) The base case, where a new tab is opened and closed without re-flowing through the rest of the tabstrip

2) Reflow case, where adding and removing a new tab causes the rest of the tabs in the strip to change widths each time

3) We probably should be measuring the performance when opening a new tab with about:newtab as the default new tab page.

Comment 2

[Avi Halachmi (:avih)]

Do we want this as a talos test? does it fit the class of regression tests we're interested in for talos?

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

the duration doesn't seem that long, but the noisy part scares me.  Any test that measures something a user will encounter is valid for atalos test.  We could look at this and see how noisy it is over a hundred runs on each platform to consider if this is somthing that we can rely on for finding regressions and tracking over time.

Comment 4

[Avi Halachmi (:avih)]

(In reply to Joel Maher (:jmaher) from comment #3)
> the duration doesn't seem that long, but the noisy part scares me.  Any test
> that measures something a user will encounter is valid for atalos test.  We
> could look at this and see how noisy it is over a hundred runs on each
> platform to consider if this is somthing that we can rely on for finding
> regressions and tracking over time.

OK, let's talk this over IRC and see how we make this useful.

Comment 5

[:Gijs (he/him)]

What happened with this discussion, and/or is this something we could move forward in the work week?

Comment 6

[Avi Halachmi (:avih)]

Being worked on.

Current status is that we have a preliminary addon which tests tab animation performance on several animation cases and works well, and we also have a preliminary patch to integrate it into talos as a new suite - TART - tab animation regression tests.

TODO:
- Add measurement of tabscrip scroll into view (need some heuristics since no event is generated when this scroll ends).
- Make the newtab page contain 9 preview images (not blanks) while testing.
- Small issue where talos currently only manages to run it once, but we need several runs in succession.
- Cleanups and landing.

jmaher and myself are working on it, and we'll probably continue at the workweek as well.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: initial patch to integrate tart into talos (0.5)

Comment 8

[Avi Halachmi (:avih)]

To get ASAP iterations on OS X (and also for tsvgx, tscrollx), we need bug 888899 (use non blocking swap on OS X) and also prevent paint/present starvation by setting docshell.event_starvation_delay_hint=10 (bug 884955).

Comment 9

[Avi Halachmi (:avih)]

Attachment: v2: TART v1 for talos

r?
ttaubert - the addon part under talos/page_load_test/tart/addon
jmaher   - the talos part

Notes:
- The addon contains some commented out tests which didn't make it into TART v1.
- The addon has some unreachable code (by config) of alternative execution methods.
- jmaher: I was unable to configure tart to use the addon at test.py. However, it does work from CLI.

Comment 10

[Avi Halachmi (:avih)]

TART v1:

Tests tab animation on 3 cases:
1. Simple: single new tab of about:blank open/close without affecting (shrinking/expanding) other tabs.
2. icon: same as above with favicon and long title instead of about:blank.
3. Newtab: newtab open with thumbnails preview - without affecting other tabs, with and without preload.
- Case 1 is tested with DPI scaling of 1.
- Case 2 is tested with DPI scaling of 1.0 and 2.0.
- Case 3 tested with the default scaling of the test system.
- Each animation produces 3 test results:
  - error: difference between the designated duration and the actual completion duration from the trigger.
  - half: average interval over 50% of the designated duration - from the end of the animation backwards.
  - all: average interval over all recorded intervals.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 779728 [details] [diff] [review]
v2: TART v1 for talos

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

it would be nice to mention the addon in the test.py configuration.  I can work on making that work so we don't need to specify it on the commandline.

Comment 12

[Avi Halachmi (:avih)]

(In reply to Joel Maher (:jmaher) from comment #11)
> it would be nice to mention the addon in the test.py configuration.  I can
> work on making that work so we don't need to specify it on the commandline.

(In reply to Avi Halachmi (:avih) from comment #9)
> - jmaher: I was unable to configure tart to use the addon at test.py.
> However, it does work from CLI.

And the patch does include at test.py at class tart:
> extension = '${talos}/page_load_test/tart/addon'

It just doesn't work.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 779728 [details] [diff] [review]
v2: TART v1 for talos

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

::: talos/page_load_test/tart/addon/content/blank.icon.html
@@ +1,4 @@
> +<html>
> +<head>
> +<meta charset="UTF-8"/>
> +<link id="tart-icon" rel="icon" href="tart.ico"/>

That file seems to be missing?

::: talos/page_load_test/tart/addon/content/tart.html
@@ +1,1 @@
> +<html>

This needs a license header.

@@ +4,5 @@
> +<meta charset="UTF-8"/>
> +<link id="tart-icon" rel="icon" href="tart.ico"/>
> +<title>TART - Tab Animation Regression Test</title>
> +
> +<script type="application/x-javascript">

Nit: application/javascript

@@ +14,5 @@
> +function getWin() {
> +  const Ci = Components.interfaces;
> +  const Cc = Components.classes;
> +  var wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
> +  return wm.getMostRecentWindow("navigator:browser");

This is quite dependent on focus and what not. It's better to do the interface dance:

> return window.QueryInterface(Ci.nsIInterfaceRequestor)
>              .getInterface(Ci.nsIWebNavigation)
>              .QueryInterface(Ci.nsIDocShellTreeItem)
>              .rootTreeItem
>              .QueryInterface(Ci.nsIInterfaceRequestor)
>              .getInterface(Ci.nsIDOMWindow);

@@ +24,5 @@
> +  if (dispResult) {
> +    // arry of test results, each element has .name and .value (test name and test result)
> +    // test result may also be an array of numeric values (all the intervals)
> +    for (var i in dispResult) {
> +      var di = dispResult[i];

If you'd use application/javascript;version=1.7 at the top you could also use for (let di of dispResult).

@@ +25,5 @@
> +    // arry of test results, each element has .name and .value (test name and test result)
> +    // test result may also be an array of numeric values (all the intervals)
> +    for (var i in dispResult) {
> +      var di = dispResult[i];
> +      var disp = [].concat(di.value).map(function(a){return " " + (isNaN(a) ? -1 : a.toFixed(1));}).join("&nbsp;&nbsp;");

Why's there so much post-processing to do here? Can't you just return usable data? There's only one consumer anyway.

@@ +26,5 @@
> +    // test result may also be an array of numeric values (all the intervals)
> +    for (var i in dispResult) {
> +      var di = dispResult[i];
> +      var disp = [].concat(di.value).map(function(a){return " " + (isNaN(a) ? -1 : a.toFixed(1));}).join("&nbsp;&nbsp;");
> +      dispResult[i] = String(di.name) + ": " + disp;

di.name should be converted to a string automatically, no?

@@ +29,5 @@
> +      var disp = [].concat(di.value).map(function(a){return " " + (isNaN(a) ? -1 : a.toFixed(1));}).join("&nbsp;&nbsp;");
> +      dispResult[i] = String(di.name) + ": " + disp;
> +      if (di.name.indexOf(".half")>=0 || di.name.indexOf(".all")>=0)
> +        dispResult[i] = "<b>"+dispResult[i]+"</b>";
> +      if (di.name.indexOf(".raw")>=0)

Using indexOf() here seems lame. Can't you include a property that is "half" or "all" or "raw"?

@@ +38,5 @@
> +}
> +
> +function triggerStart() {
> +  $("hide-during-run").style.display = "none";
> +  $("show-during-run").style.display = "block";

If you could add a "running" class to the body element and define rules for show/hide-during-run, I think that'd be a tad more elegant.

@@ +47,5 @@
> +  getWin().dispatchEvent(e);
> +}
> +
> +function init() {
> +  if (document.location.href.split("#")[1] == "auto") {

if (location.hash == "#auto") {

@@ +56,5 @@
> +addEventListener("load", init);
> +
> +</script>
> +</head>
> +<body style="font-family:sans-serif;">

You should put all those inline styles at least in a style tag at the top, if not into a separate css file.

::: talos/page_load_test/tart/addon/content/tart.js
@@ +1,2 @@
> +// TODO:
> +// V - Objectify

This needs a license header.

@@ +20,5 @@
> +//   X - Without add-tab button -> can be hidden while testing manually. in talos always with the button
> +function Tart() {
> +}
> +
> +Tart.prototype = {

Make me a simple object maybe?

@@ +21,5 @@
> +function Tart() {
> +}
> +
> +Tart.prototype = {
> +  clickNewTab: function() {

This doesn't really simulate a click on the new tab button. We should rename the function, it opens a tab and selects it.

@@ +42,5 @@
> +  },
> +
> +  clickCloseCurrentTab: function() {
> +    this._win.BrowserCloseTabOrWindow();
> +    return this._win.gBrowser.selectedTab;

BrowserCloseTabOrWindow() and gBrowser should be defined in the global scope.

@@ +54,5 @@
> +    return this._win.gBrowser.unpinTab(this._tartTab);
> +  },
> +
> +  USE_RECORDING_API: true, // true for Start[/Stop]FrameTimeRecording, otherwise record using rAF - which will also work with OMTC
> +                           // but (currently) also records iterations without paint invalidations

Please define this as a const at the top of the file. This shouldn't be hidden down here.

@@ +58,5 @@
> +                           // but (currently) also records iterations without paint invalidations
> +
> +  _win: undefined,
> +  _tartTab: undefined,
> +  _results: [],

We usually define properties/fields at the top of the object, above all functions.

@@ +60,5 @@
> +  _win: undefined,
> +  _tartTab: undefined,
> +  _results: [],
> +
> +  _animate: function(preWaitMs, triggerFunc, onDoneCallback, isReportResult, name, referenceDuration) {

This function would really benefit from taking an options object, that way I wouldn't have to look up its argument list every time I see a call to it. Also there could be a few specialized versions of it like _animateNoWait() that passes preWaitMs=0 etc. That should make the code a lot more readable.

@@ +66,5 @@
> +    var recordingHandle;
> +    var timeoutId = 0;
> +    var listnerObject;
> +    var rAF = window.requestAnimationFrame || window.mozRequestAnimationFrame;
> +    const Ci = Components.interfaces;

Those constant definition should be moved to the top.

@@ +69,5 @@
> +    var rAF = window.requestAnimationFrame || window.mozRequestAnimationFrame;
> +    const Ci = Components.interfaces;
> +
> +    var _recording = [];
> +    var _abortRecording = false;

Nit: Those variables shouldn't start with an underscore.

@@ +73,5 @@
> +    var _abortRecording = false;
> +    function startRecord() {
> +      if (self.USE_RECORDING_API) {
> +        return window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                     .getInterface(Ci.nsIDOMWindowUtils)

You could use XPCOMUtils to define the nsIDOMWindowUtils as a lazy getter at the top.

@@ +100,5 @@
> +    function stopRecord(Handle) {
> +      if (self.USE_RECORDING_API) {
> +        var paints = {};
> +        return window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                     .getInterface(Ci.nsIDOMWindowUtils)

You could use that lazy getter now again.

@@ +157,5 @@
> +      clearTimeout(timeoutId);
> +      listnerObject.removeEventListener("transitionend", transEnd);
> +
> +      // Get the recorded frame intervals and append result if required
> +      let intervals = stopRecord(recordingHandle);

I like that you're using let. Can you please use this everywhere and get rid of 'var'? Thanks :)

@@ +179,5 @@
> +      var orig = self._win.document.getElementById(id).style.opacity;
> +      var i = 0;
> +
> +      function tickleLoop() {
> +        if (i++ < (isReportResult ? 17 : 0)) {

What kind of magic number is 17? I'll replace that by 42 :) But seriously, that should be a constant with a comment.

@@ +195,5 @@
> +
> +
> +    setTimeout(function() {
> +      trigger(function() {
> +        timeoutId = setTimeout(transEnd, 1000);

That 1000 should go in a constant.

@@ +202,5 @@
> +        // Listen to tabstrip instead of tab because when closing a tab, the listener ends up on an inactive tab,
> +        // thus throttled timers -> inaccurate (possibly late) finish event timing
> +        (listnerObject = self._win.gBrowser.tabContainer).addEventListener("transitionend", transEnd);
> +      });
> +    }, preWaitMs);

This would probably look much nicer when using Task.jsm. triggerFunc() could then return a promise that is resolved on transitionend or rejected when the timeout is fired.

@@ +218,5 @@
> +    }
> +    this._commands[this._nextCommandIx++]();
> +  },
> +  // Each command at the array a function which must call nextCommand once it's done
> +  _doSequence: function(commands, onComplete) {

Instead of using _doSequence() to run a given list of functions you should take a look at Task.jsm, that would make the code much more readable.

@@ +231,5 @@
> +  _log: function(str) {
> +    if (window.MozillaFileLogger && window.MozillaFileLogger.log)
> +      window.MozillaFileLogger.log(str);
> +
> +    window.dump(str);

Calling the global function dump() should probably be enough?

@@ +236,5 @@
> +  },
> +
> +  _logLine: function(str) {
> +    return this._log(str + "\n");
> +  },

This seems unused.

@@ +244,5 @@
> +    var testResults = [];
> +
> +    var out = "";
> +    for (var i in this._results) {
> +      res = this._results[i];

You're defining 'res' as  global variable here. Maybe use for (let res of this._results).

@@ +259,5 @@
> +    if (content && content.tpRecordTime) {
> +      content.tpRecordTime(testResults.join(','), 0, testNames.join(','));
> +    } else {
> +      //alert(out);
> +    }

That looks like debug output you forgot to remove.

@@ +267,5 @@
> +
> +  _doneInternal: function() {
> +    this._reportAllResults();
> +    if (this._onTestComplete) {
> +      this._onTestComplete(JSON.parse(JSON.stringify(this._results))); // clone results

Why is cloning them necessary?

@@ +277,5 @@
> +    // Save prefs and states which will change during the test, to get restored when done.
> +    var origNewtabUrl =     Services.prefs.getCharPref("browser.newtab.url");
> +    var origNewtabEnabled = Services.prefs.getBoolPref("browser.newtabpage.enabled");
> +    var origPreload =       Services.prefs.getBoolPref("browser.newtab.preload");
> +    var origDpi =           Services.prefs.getCharPref("layout.css.devPixelsPerPx");

Do we really need to save those values? We could just reset them using clearUserPref(), I think.

@@ +311,5 @@
> +      function(){animate(0, closeCurrentTab, next);},
> +      function(){animate(rest, addTab, next, true, "simple-open-DPI1", tabRefDuration);},
> +      function(){animate(rest, closeCurrentTab, next, true, "simple-close-DPI1", tabRefDuration);},
> +
> +      function(){Services.prefs.setCharPref("browser.newtab.url", "chrome://tart/content/blank.icon.html"); next();},

The preference name should be a const defined at the top. Same for the URL.

@@ +404,5 @@
> +  startTest: function(doneCallback) {
> +    this._onTestComplete = doneCallback;
> +
> +    const Ci = Components.interfaces;
> +    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);

Services.wm

@@ +405,5 @@
> +    this._onTestComplete = doneCallback;
> +
> +    const Ci = Components.interfaces;
> +    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
> +    this._win = wm.getMostRecentWindow("navigator:browser");

You can replace all "this._win" references by just using "window". The chrome window is the global if tart.js is loaded in an overlay.

@@ +408,5 @@
> +    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
> +    this._win = wm.getMostRecentWindow("navigator:browser");
> +    this._tartTab = this._win.gBrowser.selectedTab;
> +
> +    return this._startTest();

_startTest() doesn't return anything.

::: talos/page_load_test/tart/addon/content/tart.overlay.xul
@@ +1,5 @@
> +<?xml version="1.0"?>
> +<overlay id="Scrapper-Overlay" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +<script type="application/x-javascript" src="tart.js" />
> +<script type="application/x-javascript">

Nit: application/javascript

@@ +4,5 @@
> +<script type="application/x-javascript" src="tart.js" />
> +<script type="application/x-javascript">
> +
> +function start(e) {
> +  (new Tart()).startTest(e.doneCallback);

Do we really need to create a new object every time? This could just be a singleton/a simple object because you're only running one test at a time.

@@ +7,5 @@
> +function start(e) {
> +  (new Tart()).startTest(e.doneCallback);
> +}
> +
> +window.addEventListener("tartstart", start, false);

Instead of dispatching an event I guess you could also just define a function like StartTARTTest() that you then pass a callback. The content page could just call it directly. If you want to support e10s and run your test page in a separate process you should use messages.

::: talos/page_load_test/tart/addon/install.rdf
@@ +1,5 @@
> +<?xml version="1.0"?><RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"     xmlns:em="http://www.mozilla.org/2004/em-rdf#"><Description about="urn:mozilla:install-manifest">
> +
> +<!-- Required Items -->
> +<em:id>[e-mail]">[e-mail]</em:id>
> +<em:name>TART - Tab Animation regression Test</em:name>

Nit: Regression (big R)

Comment 14

[Avi Halachmi (:avih)]

Attachment: V3 (WIP) - Improve stand-alone usage (repeat, subtests selections, other params)

I'm attaching a WIP patch which helps when running manually - just for reference, as well as the actual TART xpi, but I'll mark both as obsolete until I post a cleaned-up patch.

Comment 15

[Avi Halachmi (:avih)]

Attachment: TART-v1.3-WIP.xpi

The addon XPI (zipped directly from the patch) for easier testing.

Comment 16

[:Gijs (he/him)]

(following up on an IRC conversation because I expect avih is no longer there at this hour)

How does this take the easing, duration, and the size of the transition into account (NB: tab sizes and some animation durations are different on Australis)?

If we ease-out a transition of 250ms, at the last 125ms we're going to be further than 50% in terms of accomplishing the transition, right? Which means those frames will be 'closer together' or 'more like each other' than the previous ones.

In the ASAP case, do we still get notified for an rAF even if, from the perspective of the CSS animation, there is 'nothing to do'?

For instance, if I have a transition without easing from width: 10px to width: 20px, over 1100ms, there are logically only 11 frames to be calculated and rendered, so they could render at 100ms intervals.

Is rAF going to be called 11 times, or as many times as possible (1100? Is there any restriction at all?), or...?

If the answer is "as many times as possible", then how does that say anything about the smoothness of the animation in question? If the answer is "11 times", then I think the add-on currently won't be an accurate measure because we do change some of the underlying invariants such as tab sizes and transition durations, compared to m-c.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 786884 [details] [diff] [review]
V3 (WIP) - Improve stand-alone usage (repeat, subtests selections, other params)

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

::: talos/page_load_test/tart/addon/content/tart.js
@@ +13,5 @@
> +// X - have preview images: hard to make work across versions
> +// X - Read transition duration from style instead of hardcoded 250ms
> +// - Tests:
> +//   V - With favicon
> +//   V - Different DPIs

I'm curious to know why TART runs with different DPIs as it seems unrelated to tab animation. Do we have data that shows that tab animation reacts significantly different depending on the DPI?

Comment 18

[Avi Halachmi (:avih)]

(In reply to :Gijs Kruitbosch (PTO Aug 8-Aug 18) from comment #16)
> How does this take the easing, duration, and the size of the transition into
> account (NB: tab sizes and some animation durations are different on
> Australis)?

Right now it doesn't specifically take easing into account.

> If we ease-out a transition of 250ms, at the last 125ms we're going to be
> further than 50% in terms of accomplishing the transition, right? Which
> means those frames will be 'closer together' or 'more like each other' than
> the previous ones.

It might be closer together, but it will still try to paint a frame at every iteration, at whatever position it happens to be at the time of painting.
 
> In the ASAP case, do we still get notified for an rAF even if, from the
> perspective of the CSS animation, there is 'nothing to do'?

I got no straight answer for this. First of all, it uses the Start[/stop]FrameTimeRecording API and not rAF. They should theoretically be the same, but they aren't. rAF appears to return with 0ms intervals sometimes (possibly on cases such as you describe), while this API doesn't do that, but rather returns steady intervals even on a very fast system. So I believe the recording is valid.


(In reply to Matthew N. [:MattN] from comment #17)
> I'm curious to know why TART runs with different DPIs as it seems unrelated
> to tab animation. Do we have data that shows that tab animation reacts
> significantly different depending on the DPI?

Yes, at DPI scaling of 2.0 performance is considerably worse than at DPI scaling of 1.0. Roughly around 50% longer intervals on average. I'll soon post benchmark data over several systems at bug 902024.


In general, measuring performance isn't exact science, and many variable should be taken into account. While I did my best with TART to isolate important aspects of the animation and set it up properly, I very possibly made wrong assumptions, incorrect/incomplete setups, etc, so by all means, if you notice vectors for improving TART, post them here and I'll gladly discuss them and hopefully also implement them.

Comment 19

[Avi Halachmi (:avih)]

Also, as a philosophy, TART's goal within talos isn't to represent real-world performance ("60fps" or some other hard threshold), since these vary a LOT over different systems/platforms/etc. Getting some X ms average interval on some case Y is meaningless as an answer to the question "are we good enough?"

Rather, TART tries to exercise as many aspects of tab animation as possible, while slicing them into different perspectives, where hopefully each perspective (sub-result) isolates and correlates to different variables/factors.

The usefulness of TART appears when it's being used relatively. E.g. the same test on different builds, different platforms, different themes, different hardware, etc, where we could say that X difference between the our contenders results in Y difference on subtest Z of TART.

As such, I hope it gives the most benefit as a talos regression test which we run automatically on every new build.

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 786884 [details] [diff] [review]
V3 (WIP) - Improve stand-alone usage (repeat, subtests selections, other params)

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

One thing I noticed while running this as part of talos is that the caret is blinking in the addressbar for the whole test. I think the focus should probably be on the content area as that seems more common.

::: talos/test.py
@@ +201,5 @@
> +      - half: average interval over 50% of the designated duration - from the end of the animation backwards.
> +      - all: average interval over all recorded intervals.
> +    """
> +    tpmanifest = '${talos}/page_load_test/tart/tart.manifest'
> +    extensions = '${talos}/page_load_test/tart/addon'

Nit: s/addon/extension/

Comment 21

[Avi Halachmi (:avih)]

(In reply to Matthew N. [:MattN] from comment #20)
> One thing I noticed while running this as part of talos is that the caret is
> blinking in the addressbar for the whole test.I think the focus should
> probably be on the content area as that seems more common.

Since the caret changes state every 500ms and tab animation takes less than that, I think that each tab animation includes at most one caret change. Also, I think that the work to update the caret once is negligible compared to the tab animation and whatever else is happening. Also #2, when users open tabs, the URL bar is focused as well so if there's any effect by the caret, it's something which users also notice and hence worth measuring as well IMO. Besides, changing focus while the tab is animating might have a much bigger effect than one caret blink.

Right now tabs are opened using BrowserOpenTab() because it looked to me the correct way. If you think another call or sequence of calls would improve the results in a meaningful way, I'm open for suggestions.

Comment 22

[Matthew N. [:MattN]]

(In reply to Avi Halachmi (:avih) from comment #21)
> (In reply to Matthew N. [:MattN] from comment #20)
> > One thing I noticed while running this as part of talos is that the caret is
> > blinking in the addressbar for the whole test.I think the focus should
> > probably be on the content area as that seems more common.
> 
> Since the caret changes state every 500ms and tab animation takes less than
> that, I think that each tab animation includes at most one caret change.
> Also, I think that the work to update the caret once is negligible compared
> to the tab animation and whatever else is happening.

Sure but if it's trivial to make it more like the real world then we should do so.

> Also #2, when users
> open tabs, the URL bar is focused as well so if there's any effect by the
> caret, it's something which users also notice and hence worth measuring as
> well IMO. Besides, changing focus while the tab is animating might have a
> much bigger effect than one caret blink.

The caret should be in the address bar for pages which we consider blank (e.g. newtab and about:blank) but I'm saying that the test when run as talos has the focus in the address bar for tart.html#auto which is not normal. You can use gBrowser.selectedBrowser.focus(); to address this.

Comment 23

[Avi Halachmi (:avih)]

Attachment: v4 - address fx-team requests (See comment 23)

V3 -> V4 changes:
- Remove caret from tart.html (unfocus url bar).
- Add fade-only tests (idea by mconley, and works great too).
- Output JSON results to the log.
- UI: Add tests descriptions, deselect all button, copy JSON to clipboard button.
- Add profiler markers as start/done:[test-name]
- Add custom subtests for talos by URL (tart.html)
- Change repeat order from column to row major

When running with custom talos build, it's now possible to select specific subtests by changing the test URL at talos/page_load_test/tart/tart.manifest .

E.g. 
All default tests: % chrome://tart/content/tart.html#auto
2 subtests: % chrome://tart/content/tart.html#auto&tests=["simple","iconFadeDpiCurrent"]

Comment 24

[Avi Halachmi (:avih)]

Attachment: TART-v1.4-WIP.xpi

Just adding the XPI for convinience (zipped/xpi directly from the v4 patch)

Comment 25

[Mike Conley (:mconley) (:⚙️)]

During the Australis meeting today, it was emphasized how important landing this test is to the Australis team, since it's our last major performance blocker, and we need a target to aim for and the ability to track our progress.

So, for us, landing this test is pretty critical.

Tim / Gavin: suppose we land this patch in talos more or less the way it is, and have avih refactor and fix the review comments as we work on the tab regression? Because until we have test results coming from a central source, we're kinda flying blind.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

As mentioned on IRC, I'm fine with this as long as someone pinky swears to take care of cleaning up later. It's a talos test and not code we're shipping. Also I didn't see any critical issues, most of my comments were targeted towards easier maintenance in the future.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 794024 [details] [diff] [review]
v4 - address fx-team requests (See comment 23)

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

::: talos/page_load_test/tart/addon/content/tart.js
@@ +393,5 @@
> +
> +      newtabYesPreload: [
> +        function(){Services.prefs.setCharPref("browser.newtab.url", "about:newtab");
> +                   Services.prefs.setCharPref("layout.css.devPixelsPerPx", "-1");
> +                   Services.prefs.setBoolPref("browser.newtab.preload", true);

This seems to work only because you wait longer than 5s before starting these tests and the delay between opening tabs is big enough to preload another one. Actually you should check and wait for an about:newtab page to be preloaded and then run tests. This seems like it should work for now, though.

Comment 28

[Avi Halachmi (:avih)]

Attachment: V5 - (TART v1.5), minor name changes for talos, read duration from style

Carrying r+.

This patch changes (compared to V4):
- TART addon version to 1.5
- Read transitionDuration from style (thanks mconley!) with fallback to 250ms.
- Minor test names changes (.TART at the end of the name instead TART.[name]) because talos crops common prefixes.
- Updated TART description comment at test.py

TODO: once this lands, address tim's original review comments in a followup.

Comment 29

[Avi Halachmi (:avih)]

Attachment: TART-v1.5.xpi (zipped/xpi from patch V5)

Comment 30

[Avi Halachmi (:avih)]

https://hg.mozilla.org/build/talos/rev/9a727942ea62

Comment 32

[Matt Brubeck (:mbrubeck)]

This test is not yet documented at https://wiki.mozilla.org/Buildbot/Talos/Tests

Comment 33

[Matt Brubeck (:mbrubeck)]

avih added this to the documentation page.

Comment 34

[Avi Halachmi (:avih)]

Joel added it actually, but had I added it, it would be identical to the letter since it's copied from the comments within talos code. Thanks joel!