Robert Wood [:rwood] Assignee
	Description • 7 years ago

The talos pageloader add-on currently receives a bunch of options via command line [1]. Find a better way to pass this info to pageloader and remove these command line options.

[1] http://searchfox.org/mozilla-central/source/testing/talos/talos/pageloader/components/tp-cmdline.js

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 2 • 7 years ago
mozreview-review

Comment on attachment 8926623 [details]
Bug 1410210 - Remove talos pageloader command line options;

https://reviewboard.mozilla.org/r/197850/#review203236

while I have some comments of future work, I would like to see some tests written for this- testing invalid values and how we error out in pageloader when receiving these.  The main difference here is the commandline would not accept many values, now we are passing in strings, ints, bools

::: testing/talos/talos/pageloader/chrome/pageloader.js:25
(Diff revision 1)
>  
>  var numRetries = 0;
>  var maxRetries = 3;
>  
>  var pageFilterRegexp = null;
>  var useBrowser = true;

how is useBrowser  different from the new variable useBrowserChrome added below?

::: testing/talos/talos/pageloader/chrome/pageloader.js:141
(Diff revision 1)
>  
> -    if (profilingInfo) {
> -      TalosParentProfiler.initFromObject(profilingInfo);
> +    NUM_CYCLES = Services.prefs.getIntPref("talos.tpcycles", 1);
> +    numPageCycles = Services.prefs.getIntPref("talos.tppagecycles", 1);
> +    noisy = Services.prefs.getBoolPref("talos.tpnoisy", false)
> +    timeout = Services.prefs.getIntPref("talos.tptimeout", -1)
> +    delay = Services.prefs.getIntPref("talos.tpdelay", 250)

can we get rid of:
* noisy
* timeout
* delay

::: testing/talos/talos/pageloader/chrome/pageloader.js:150
(Diff revision 1)
> +    scrollTest = Services.prefs.getBoolPref("talos.tpscrolltest", false)
> +    useBrowserChrome = Services.prefs.getBoolPref("talos.tpchrome", false)
> +
> +    // for pageloader tests the profiling info is found in an env variable
> +    // because it is not available early enough to set it as a browser pref
> +    var env = Components.classes["@mozilla.org/process/environment;1"].

interesting solution here, glad there was a way to solve this

::: testing/talos/talos/pageloader/chrome/pageloader.js:161
(Diff revision 1)
> +        TalosParentProfiler.initFromObject(JSON.parse(profilingInfo));
> +      }
>      }
>  
> -    forceCC = !args.noForceCC;
> -    doRenderTest = args.doRender;
> +    var tpNoForcecc = Services.prefs.getBoolPref("talos.tpnoforcecc", false);
> +    forceCC = !tpNoForcecc;

can we get rid of tpnoforcecc ?

::: testing/talos/talos/pageloader/chrome/pageloader.js:188
(Diff revision 1)
> -      dumpLine("tp: error: startIndex >= endIndex");
> -      plStop(true);
> -    }
>  
> +    var startIndex = 0;
> +    var endIndex = pages.length - 1;

do we need startIndex and endIndex as variables?  It sounds like we just need to delete these lines and the following line where we call pages.slice()

::: testing/talos/talos/pageloader/chrome/pageloader.js:206
(Diff revision 1)
>      var blank = Cc["@mozilla.org/supports-string;1"]
>        .createInstance(Ci.nsISupportsString);
>      blank.data = "about:blank";
>  
>      let toolbars = "all";
> -    if (!args.useBrowserChrome) {
> +    if (!useBrowserChrome) {

can we get rid of tpChrome and assume userBrowserChrome=False to simplify code?

::: testing/talos/talos/pageloader/components/tp-cmdline.js:80
(Diff revision 1)
> -      args.numCycles = cmdLine.handleFlagWithParam("tpcycles", false);
> -      args.numPageCycles = cmdLine.handleFlagWithParam("tppagecycles", false);
> -      args.startIndex = cmdLine.handleFlagWithParam("tpstart", false);
> -      args.endIndex = cmdLine.handleFlagWithParam("tpend", false);
> -      args.filter = cmdLine.handleFlagWithParam("tpfilter", false);
> -      args.useBrowserChrome = cmdLine.handleFlag("tpchrome", false);
> +    var tpmanifest = Services.prefs.getCharPref("talos.tpmanifest", null);
> +    if (tpmanifest == null) {
> +      // pageloader tests as well as startup tests use this pageloader add-on; pageloader tests
> +      // pass in a 'tpmanifest' which has been set to the talos.tpmanifest pref; startup tests
> +      // don't use 'tpmanifest' but pass in the test page which is forwarded onto the Firefox cmd
> +      // line. If this is a startup test exit here as we don't need a pageloader window etc.

possibly we can consider making pageloader an overlay that loads on start?  Also we could have a 'start' page we always load by default which could jumpstart pageloader and tell it to load prefs and start.

	Robert Wood [:rwood] Assignee
	Comment 3 • 7 years ago
mozreview-review-reply

Comment on attachment 8926623 [details]
Bug 1410210 - Remove talos pageloader command line options;

https://reviewboard.mozilla.org/r/197850/#review203236

Thanks for the review, good call re: unit tests, I'll add those also

> how is useBrowser  different from the new variable useBrowserChrome added below?

Ah, removed

> can we get rid of:
> * noisy
> * timeout
> * delay

Noisy: tpnoisy is set to true by default in config.py, but I see that no tests change that setting. Ah yes, in the code if noisy is true it prints the "Cycle 1(X)..." line, I see no need to NOT have that line so yes I'll remove the option thx.

Delay: delay (between pageloads) wasn't even enabled on the cmd line to begin with but I wondered if it would be useful to keep. However you're right let's remove it since it's not actually being used.

Timeout: We need to keep, as you noted earlier on IRC tptimeout is used for ccov runs and added inside a tc transform.

> can we get rid of tpnoforcecc ?

Yes good call, I thought it might be a good feature to keep however it's not used anywhere and I see it's not even supported as a cmd line arg hah, I'll remove it.

> do we need startIndex and endIndex as variables?  It sounds like we just need to delete these lines and the following line where we call pages.slice()

Yes good call I'll remove them thanks

> can we get rid of tpChrome and assume userBrowserChrome=False to simplify code?

No, tpchrome is true by default and only *some* of the talos tests change it to false, so we need to keep it.

> possibly we can consider making pageloader an overlay that loads on start?  Also we could have a 'start' page we always load by default which could jumpstart pageloader and tell it to load prefs and start.

This might be an option except pageloader is used by startup tests (that just directly load a test page html and don't use pageloader manifests etc) as well as pageloader tests. I think we should consider this though but I'd like to land this cmd line cleanup initially and then consider further improvements down the road. :)

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 4 • 7 years ago

lets solve the removing of the tp-cmdline.js in a different bug and use this to cleanup the options, use prefs, and remove a lot of code :)

	Robert Wood [:rwood] Assignee
	Comment 7 • 7 years ago

(In reply to Robert Wood [:rwood] from comment #6)
> Comment on attachment 8926623 [details]
> Bug 1410210 - Remove talos pageloader command line options;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/197850/diff/1-2/

Update for review comments except the talos unit tests.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8926623 [details]
Bug 1410210 - Remove talos pageloader command line options;

https://reviewboard.mozilla.org/r/197850/#review203460

if you want to do unittests as a followup.. :)

	Robert Wood [:rwood] Assignee
	Comment 9 • 7 years ago

Thanks! I may add the unit tests with this patch also, but initial try run anyway:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d8ad1d7ebf00a6a278fed75a49da51f2414deca

	Robert Wood [:rwood] Assignee
	Comment 12 • 7 years ago

More extensive try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=785e40bcb21198a604025fe4480499aa6d4462d5

	Robert Wood [:rwood] Assignee
	Comment 15 • 7 years ago

Rebased patch (and some minor updates; carrying fw the r+). Landed on try again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8081d1fe74762cde4859859af23946fb9e5e1a8

	Pulsebot
	Comment 16 • 7 years ago

Pushed by rwood@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a40ffd441b9a
Remove talos pageloader command line options; r=jmaher

	Stefan Hindli [:stefan_hindli]
	Comment 17 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a40ffd441b9a