Add Results class to hold structured data with details about each of the child processes.

Add Results class to hold structured data with details about each of the child processes.

This permits to know which process opened which file, and permits to add more
metadata in the future.

For example with this, it's possible to know which child process opened which
files. Eventually, executable name, command arguments and timestamp data will
be added to make it even more useful.

NOTRY=true
R=mad@chromium.org
BUG=98636
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139786

[MAD, 2012-05-30 20:29:35]

Have fun!

:-)

BYE
MAD...

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:289: self.root().processes[c].to_results_process()
for c in self.children
We usually prefer not doing in an list construction like that if it can't fit on
a single line... I would prefer

children = []
for child in self.children:
  children.append(
      self.root().processes[child].to_results_process())

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:291: # When resolving files, it's normal to get
dupe because of a file could
"because of a" -> "because a"

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:417: self.path = self.path + os.path.sep
maybe add an assert that path doesn't already ends with a separator?

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:537: for f in files:
I prefer for file in files:

for a one line list construction fine, but for a >10 lines loop... please... :-)

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:540: x = buckets.setdefault(directory, {})
what's 'x'?

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:541: x[path[len(directory)+1:]] = f
really readable... :-P

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:546: if directory + os.path.sep == root or
directory == old_d:
You assume root ends with a path.sep but you don't assert on it here, and I
don't think all cases validate that the file object will always return a root
which ends with a path.sep... I would add an assert at least...

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
File tools/isolate/trace_inputs_smoke_test.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs_smoke_test.py:162: # and no child .
and no child?

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
File tools/isolate/trace_inputs_test.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs_test.py:113: command = command or
['../out/unittests']
You don't actually use command... I don't get it...

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs_test.py:121: 'pid': 27,
27?

A _CONST maybe? Or at least a comment... :-)

[M-A Ruel, 2012-05-30 22:34:10]

ptal

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:289: self.root().processes[c].to_results_process()
for c in self.children
On 2012/05/30 20:29:36, MAD wrote:
> We usually prefer not doing in an list construction like that if it can't fit
on
> a single line... I would prefer
> 
> children = []
> for child in self.children:
>   children.append(
>       self.root().processes[child].to_results_process())

Why? It's much slower. I agree for this line in particular speed it not a
problem but I don't think it increases readability.

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:291: # When resolving files, it's normal to get
dupe because of a file could
On 2012/05/30 20:29:36, MAD wrote:
> "because of a" -> "because a"

done.

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:417: self.path = self.path + os.path.sep
On 2012/05/30 20:29:36, MAD wrote:
> maybe add an assert that path doesn't already ends with a separator?

done

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:537: for f in files:
On 2012/05/30 20:29:36, MAD wrote:
> I prefer for file in files:
> 
> for a one line list construction fine, but for a >10 lines loop... please...
:-)

'file' is a poor choice because it is a builtin, so I'll use fileobj.

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:540: x = buckets.setdefault(directory, {})
On 2012/05/30 20:29:36, MAD wrote:
> what's 'x'?

I didn't recall, fixed.

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:541: x[path[len(directory)+1:]] = f
On 2012/05/30 20:29:36, MAD wrote:
> really readable... :-P

Rewrote.

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:546: if directory + os.path.sep == root or
directory == old_d:
On 2012/05/30 20:29:36, MAD wrote:
> You assume root ends with a path.sep but you don't assert on it here, and I
> don't think all cases validate that the file object will always return a root
> which ends with a path.sep... I would add an assert at least...

Agreed, added assert.

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
File tools/isolate/trace_inputs_smoke_test.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs_smoke_test.py:162: # and no child .
On 2012/05/30 20:29:36, MAD wrote:
> and no child?

No child is left behind, finished sentence.

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
File tools/isolate/trace_inputs_test.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs_test.py:113: command = command or
['../out/unittests']
On 2012/05/30 20:29:36, MAD wrote:
> You don't actually use command... I don't get it...

Oops, but it's used in the next CL;
https://chromiumcodereview.appspot.com/10446052/diff/1/tools/isolate/trace_in...

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs_test.py:121: 'pid': 27,
On 2012/05/30 20:29:36, MAD wrote:
> 27?
> 
> A _CONST maybe? Or at least a comment... :-)

Did both.

[MAD, 2012-05-31 14:00:57]

OK, LGTM with a few more nits...

Thanks!

BYE
MAD...

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
File tools/isolate/trace_inputs.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs.py:289: self.root().processes[c].to_results_process()
for c in self.children
On 2012/05/30 22:34:10, Marc-Antoine Ruel wrote:
> On 2012/05/30 20:29:36, MAD wrote:
> > We usually prefer not doing in an list construction like that if it can't
fit
> on
> > a single line... I would prefer
> > 
> > children = []
> > for child in self.children:
> >   children.append(
> >       self.root().processes[child].to_results_process())
> 
> Why? It's much slower. I agree for this line in particular speed it not a
> problem but I don't think it increases readability.

OK, then...  I would find it more readable, but fine...

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
File tools/isolate/trace_inputs_smoke_test.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs_smoke_test.py:162: # and no child .
On 2012/05/30 22:34:10, Marc-Antoine Ruel wrote:
> On 2012/05/30 20:29:36, MAD wrote:
> > and no child?
> 
> No child is left behind, finished sentence.

You end previous line with a "." and start this one with a lower case, can you
fix one of the two please? :-)

https://chromiumcodereview.appspot.com/10444052/diff/5002/tools/isolate/trace...
File tools/isolate/trace_inputs_test.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/5002/tools/isolate/trace...
tools/isolate/trace_inputs_test.py:175: (14, 'clone(child_stack=0,
flags=CLONE_CHILD_CLEARTID'
Why not do it for _CHILD_PID and _GRAND_CHILD_PID too?

[M-A Ruel, 2012-05-31 14:56:45]

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
File tools/isolate/trace_inputs_smoke_test.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/4001/tools/isolate/trace...
tools/isolate/trace_inputs_smoke_test.py:162: # and no child .
On 2012/05/31 14:00:57, MAD wrote:
> You end previous line with a "." and start this one with a lower case, can you
> fix one of the two please? :-)

I can't type. Hopefully fixed.

https://chromiumcodereview.appspot.com/10444052/diff/5002/tools/isolate/trace...
File tools/isolate/trace_inputs_test.py (right):

https://chromiumcodereview.appspot.com/10444052/diff/5002/tools/isolate/trace...
tools/isolate/trace_inputs_test.py:175: (14, 'clone(child_stack=0,
flags=CLONE_CHILD_CLEARTID'
On 2012/05/31 14:00:57, MAD wrote:
> Why not do it for _CHILD_PID and _GRAND_CHILD_PID too?

done.

[commit-bot: I haz the power, 2012-05-31 14:56:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/10444052/1006

[commit-bot: I haz the power, 2012-05-31 14:58:07]

Change committed as 139786