1016675 - Talos TART - correct profiler pause/resume timing

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Avi Halachmi (:avih)]

Bug 967635 adds profiler support to talos and pauses/resumes the profiler when the tests stop/start. For TART, however, it accidentally also records "warmup" animations which are not measured or reported.

This bug is about:
- Resumes the profiler only on animations which get measured in TART.
- Update TART UI for profiler pause/resume/markers (when used stand-alone).
- Minor TART UI texts cleanups (usage instructions).

Comment 1

[Avi Halachmi (:avih)]

Attachment: bug1016675.v1.patch

Several small modifications:
- Modified Profiler.js to also accept arbitrary marker strings.
- Resumes the profiler only on animations which get measured in TART.
- Update TART UI for profiler control (when used stand-alone).
- Minor TART changes:
  - Clearer instructions texts with link to the talos wiki.
  - Buttons to set ASAP mode on/off.
  - Read URL args also when not starting the test automatically.

Comment 2

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

Comment on attachment 8431197 [details] [diff] [review]
bug1016675.v1.patch

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

This looks like a lot of great cleanup.  A few nits and a couple of questions.

::: talos/page_load_test/tart/addon/content/tart.html
@@ +28,5 @@
> +
> +function unsetASAP() {
> +  prefs.clearUserPref("layout.frame_rate");
> +  prefs.clearUserPref("docshell.event_starvation_delay_hint");
> +}

should we set these prefs to original values and on unset put them to the original values?  If we have NULL for an original value then we call clearUserPref()?

@@ +59,5 @@
>  function doneTest(dispResult) {
>    $("hide-during-run").style.display = "block";
>    $("show-during-run").style.display = "none";
>    if (dispResult) {
> +    // array of test results, each element has .name and .value (test name and test result)

While you are here, s/array/Array/

@@ +74,5 @@
>        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)
> +        dispResult[i] = "<br/>" + dispResult[i]; // add space before raw results (which are the first result of an animation)

nit: s/add/Add/

@@ +189,5 @@
>  }
>  
> +// e.g. returns "world" for key "hello", "2014" for key "year", and "" for key "dummy":
> +// http://localhost/x.html#hello=world&x=12&year=2014
> +function getUriHashValue(key){

nit: /(key){/(key) {/

@@ +202,5 @@
>  
>  // URL e.g. chrome://tart/content/tart.html#auto&tests=["simple","iconFadeDpiCurrent"]
>  function updateOptionsFromUrl() {
> +  var uriTests = getUriHashValue("tests");
> +  var tests = uriTests ? JSON.parse(uriTests) : [];

if JSON.parse fails, then what happens?  do we get an error that makes sense?

@@ +217,5 @@
>    }
> +
> +  var cp = getUriHashValue("controlProfiler");
> +  if (cp.length)
> +    $("controlProfiler").checked = (cp == "1");

will this be 1 or true?

Comment 3

[Avi Halachmi (:avih)]

(In reply to Joel Maher (:jmaher) from comment #2)
> > +function unsetASAP() {
> > +  prefs.clearUserPref("layout.frame_rate");
> > +  prefs.clearUserPref("docshell.event_starvation_delay_hint");
> > +}
> 
> should we set these prefs to original values and on unset put them to the
> original values?

"Original values" is hard to define in this context. I could store these values someplace and then restore them with this button. However, they might have changed since I stored them if the user experimented with them, and restoring the "original" values after at least one restart of the browser is a questionable approach IMO. This is also the reason for the label being "Restore defaults" rather than "restore original".

> If we have NULL for an original value then we call clearUserPref()?

I didn't check specifically for NULL, but both behave correctly even while the frame rate pref exists by default, and the starvation hint pref doesn't exist by default. clearUserPref() restores both to their default state (-1 for the former, and no pref at all for the latter).

> While you are here, s/array/Array/
> nit: s/add/Add/

Thanks, I'll go over the all the comments at these files and make sure the sentences are capitalized, ending with '.' etc :)

> if JSON.parse fails, then what happens?  do we get an error that makes sense?

No. The URL arguments are not an official part of the UI, so while making sure they work when used correctly, I didn't feel it was worth the effort to handle errors gracefully.

The URL arguments are used within talos (e.g. CART specifies which sub-tests should be executed), but this is internal enough to also not _Require_ graceful handling of errors IMO.


> will this be 1 or true?

I considered checking both 1, true and yes, case-insensitive to cover all bases, but concluded that it's not worth the extra lines of code. I don't feel strongly at all with any one of the approaches, so if you prefer another value or checking for different values, just let me know which you prefer and I'll change it.

Comment 4

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

ignoring the *original values* seems just fine, I have no problem with the code that is in this patch for setting and clearing preferences.

I ask that you put a comment in where we are not doing error handing, then we sort of handle it in the code.

also another comment as to the fact that you are checking the value of '1' and not 'true'

Comment 5

[Avi Halachmi (:avih)]

(In reply to Joel Maher (:jmaher) from comment #4)
> also another comment as to the fact that you are checking the value of '1'
> and not 'true'

Yes, I'm well aware of this fact, I wrote the code, and 1 wasn't accidental. Yet I don't really care what value to use.

Just tell me what you think is best and I'll do that, no questions asked.

Comment 6

[Avi Halachmi (:avih)]

Attachment: bug1016675.v2.patch

Addressed comments, carrying r+.
Thanks!

https://hg.mozilla.org/build/talos/rev/1cc819c9de27