self.attributes is largely an overlap between the data passed in in the form of options.__dict__.keys(). There are three things in self.attributes that aren't in the options: 'logFile', 'masterIniSubpath', 'currentDate'. Really, this is duplication of information. In addition, instead of expecting an options instance and updating from its __dict__, we __init__ should take **kwargs and **options.__dict__ passed in to the constructor. (Pdb) options.__dict__.keys() ['rss', 'verbose', 'test_timeout', 'configPath', 'extraPrefs', 'browserWait', 'noShutdown', 'outputName', 'responsiveness', 'develop', 'title', 'symbolsPath', 'webServer', 'fast', 'testPrefix', 'sampleConfig', 'xperf_path', 'branch', 'useId', 'buildid', 'mozAfterPaint', 'addonID', 'testDate', 'exePath', 'resultsURL', 'logFile', 'extension', 'activeTests', 'noChrome', 'branchName'] (Pdb) self.attributes ['exePath', 'configPath', 'sampleConfig', 'outputName', 'title', 'branch', 'branchName', 'buildid', 'currentDate', 'browserWait', 'verbose', 'testDate', 'useId', 'resultsURL', 'activeTests', 'noChrome', 'fast', 'testPrefix', 'extension', 'masterIniSubpath', 'test_timeout', 'symbolsPath', 'addonID', 'noShutdown', 'extraPrefs', 'xperf_path', 'mozAfterPaint', 'webServer', 'develop', 'responsiveness', 'rss'] (Pdb) set(self.attributes) == set(options.__dict__.keys()) False (Pdb) attributes = set(self.attributes) (Pdb) keys = set(options.__dict__.keys()) (Pdb) attributes.symmetric_difference(keys) set(['logFile', 'masterIniSubpath', 'currentDate'])

It is worth noting that these attributes are used in only one place!: talos)│grep -n 'attributes' *.py PerfConfigurator.py:32: attributes = ['browser_path', 'configPath', 'sampleConfig', 'outputName', 'title', PerfConfigurator.py:47: for i in self.attributes: remotePerfConfigurator.py:20: pc.PerfConfigurator.attributes += ['deviceip', 'deviceport', 'deviceroot', 'nativeUI']

This kills a lot of redundant information. In a sense, the code is more brittle since it depends on the options being passed in being correct. In the longer term, the configuration classes (e.g. PerfConfigurator, remotePerfConfigurator) should generate the parser and options from their configuration.

> In the longer term, the configuration classes (e.g. PerfConfigurator, remotePerfConfigurator) should generate the parser and options from their configuration. see bug 725140

Comment on attachment 595246 [details] [diff] [review] fix a typo and unbitrot Review of attachment 595246 [details] [diff] [review]: ----------------------------------------------------------------- nice!

carrying r+ forward

tested on android with ts and tsvg

Trying: https://tbpl.mozilla.org/?tree=Try&rev=3a52c72ec753 So there are a lot of errors. However, they don't particularly seem related to this patch. Should I retry or push?

Try run for 3a52c72ec753 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=3a52c72ec753 Results (out of 73 total builds): success: 69 failure: 4 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jhammel@mozilla.com-3a52c72ec753

pushed: http://hg.mozilla.org/build/talos/rev/953512de9269 The failures on try seem mostly related to a broken m-c that the patch was pushed along with and the others don't seem to have anything to do with this patch (autolog broken, etc)