[Cleanup] List trace categories through script as shown by trace record dialog

[Cleanup] List trace categories through script as shown by trace record dialog

Script just dumps the categories as received from trace buffer.
It includes group of categories clubbed. A minor cleanup to present
each individual category actually present in the code and list them
as seen by the user when trace RecordDialog is created in the UI.

BUG=None

R=nduca,dsinclair

Committed: https://crrev.com/14935a5313c7fe81cdf169e86ee0e81e26649c56
Cr-Commit-Position: refs/heads/master@{#303645}

[r.kasibhatla, 2014-11-06 10:45:04]

PTAL!

Just a minor cleanup to list categories individually instead of a grouped
categories (if any) as listed by trace class.

[r.kasibhatla, 2014-11-06 10:49:32]

r.kasibhatla@samsung.com changed reviewers:
+ skyostil@chromium.org
- nduca@chromium.org

[dsinclair, 2014-11-07 00:05:25]

https://codereview.chromium.org/690103005/diff/1/tools/profile_chrome/chrome_...
File tools/profile_chrome/chrome_controller.py (right):

https://codereview.chromium.org/690103005/diff/1/tools/profile_chrome/chrome_...
tools/profile_chrome/chrome_controller.py:56: if
item.startswith('disabled-by-default'):
Don't we want to do this check against each item in categories? If the second
category is disabled by default we'd put it in the wrong list?

It seems like like we want to add a loop over categories and do the original if
statement on each of the items in that list?

[r.kasibhatla, 2014-11-07 09:58:47]

Done. Thanks for catching the error. :)

https://codereview.chromium.org/690103005/diff/1/tools/profile_chrome/chrome_...
File tools/profile_chrome/chrome_controller.py (right):

https://codereview.chromium.org/690103005/diff/1/tools/profile_chrome/chrome_...
tools/profile_chrome/chrome_controller.py:56: if
item.startswith('disabled-by-default'):
On 2014/11/07 00:05:25, dsinclair wrote:
> Don't we want to do this check against each item in categories? If the second
> category is disabled by default we'd put it in the wrong list?
> 
> It seems like like we want to add a loop over categories and do the original
if
> statement on each of the items in that list?

Correct. I was trying to club too many conditions in single statement. I have
corrected it now. :)

[Sami, 2014-11-07 18:58:14]

https://codereview.chromium.org/690103005/diff/20001/tools/profile_chrome/chr...
File tools/profile_chrome/chrome_controller.py (right):

https://codereview.chromium.org/690103005/diff/20001/tools/profile_chrome/chr...
tools/profile_chrome/chrome_controller.py:50: record_categories = []
Could you turn both of these lists into sets please? That would simplify the
logic below since we wouldn't need to check for duplicates.

[r.kasibhatla, 2014-11-10 06:42:17]

Done. PTAL!

https://codereview.chromium.org/690103005/diff/20001/tools/profile_chrome/chr...
File tools/profile_chrome/chrome_controller.py (right):

https://codereview.chromium.org/690103005/diff/20001/tools/profile_chrome/chr...
tools/profile_chrome/chrome_controller.py:50: record_categories = []
On 2014/11/07 18:58:14, Sami wrote:
> Could you turn both of these lists into sets please? That would simplify the
> logic below since we wouldn't need to check for duplicates.

Done.

[dsinclair, 2014-11-10 14:10:14]

https://codereview.chromium.org/690103005/diff/40001/tools/profile_chrome/chr...
File tools/profile_chrome/chrome_controller.py (right):

https://codereview.chromium.org/690103005/diff/40001/tools/profile_chrome/chr...
tools/profile_chrome/chrome_controller.py:50: record_categories = set()
Does a set() work the same way as [] for access? Will the callers of
GetCategories be expecting an array and having a set instead cause issues?

[Sami, 2014-11-10 14:39:45]

https://codereview.chromium.org/690103005/diff/40001/tools/profile_chrome/chr...
File tools/profile_chrome/chrome_controller.py (right):

https://codereview.chromium.org/690103005/diff/40001/tools/profile_chrome/chr...
tools/profile_chrome/chrome_controller.py:50: record_categories = set()
On 2014/11/10 14:10:13, dsinclair wrote:
> Does a set() work the same way as [] for access? Will the callers of
> GetCategories be expecting an array and having a set instead cause issues?

You can iterate over sets the same way as lists (for x in s), but indexing isn't
supported so they're not exactly compatible. We could hide the fact that we're
dealing with sets by converting them to lists instead:

    return list(record_categories), list(disabled_by_default_categories)

[r.kasibhatla, 2014-11-11 04:26:58]

> You can iterate over sets the same way as lists (for x in s), but indexing
isn't
> supported so they're not exactly compatible. We could hide the fact that we're
> dealing with sets by converting them to lists instead:
> 
>     return list(record_categories), list(disabled_by_default_categories)

It doesn't matter whether we return set or list, since the callee side is just
iterating through the return object (be it list or set) and  printing the
members in sorted manner. Hence, I didn't modified return value. Do we need to
change it?

[Sami, 2014-11-11 12:47:53]

On 2014/11/11 04:26:58, r.kasibhatla wrote:
> > You can iterate over sets the same way as lists (for x in s), but indexing
> isn't
> > supported so they're not exactly compatible. We could hide the fact that
we're
> > dealing with sets by converting them to lists instead:
> > 
> >     return list(record_categories), list(disabled_by_default_categories)
> 
> It doesn't matter whether we return set or list, since the callee side is just
> iterating through the return object (be it list or set) and  printing the
> members in sorted manner. Hence, I didn't modified return value. Do we need to
> change it?

I guess Dan's point was that it leaks the implementation of this method out to
the caller. I'd be happy either way, but let's coerce it into a list since
that's the way it was before.

[dsinclair, 2014-11-11 13:59:16]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2014-11-11 13:59:16]

lgtm

[dsinclair, 2014-11-11 14:00:19]

On 2014/11/11 12:47:53, Sami wrote:
> On 2014/11/11 04:26:58, r.kasibhatla wrote:
> > > You can iterate over sets the same way as lists (for x in s), but indexing
> > isn't
> > > supported so they're not exactly compatible. We could hide the fact that
> we're
> > > dealing with sets by converting them to lists instead:
> > > 
> > >     return list(record_categories), list(disabled_by_default_categories)
> > 
> > It doesn't matter whether we return set or list, since the callee side is
just
> > iterating through the return object (be it list or set) and  printing the
> > members in sorted manner. Hence, I didn't modified return value. Do we need
to
> > change it?
> 
> I guess Dan's point was that it leaks the implementation of this method out to
> the caller. I'd be happy either way, but let's coerce it into a list since
> that's the way it was before.


Mostly, I don't know python, and didn't know how the callers used the data. If
we access it such that set is fine, then set is fine by me.

[commit-bot: I haz the power, 2014-11-11 14:00:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/690103005/40001

[r.kasibhatla, 2014-11-11 14:02:08]

On 2014/11/11 14:00:19, dsinclair wrote:
> On 2014/11/11 12:47:53, Sami wrote:
> > On 2014/11/11 04:26:58, r.kasibhatla wrote:
> > > > You can iterate over sets the same way as lists (for x in s), but
indexing
> > > isn't
> > > > supported so they're not exactly compatible. We could hide the fact that
> > we're
> > > > dealing with sets by converting them to lists instead:
> > > > 
> > > >     return list(record_categories), list(disabled_by_default_categories)
> > > 
> > > It doesn't matter whether we return set or list, since the callee side is
> just
> > > iterating through the return object (be it list or set) and  printing the
> > > members in sorted manner. Hence, I didn't modified return value. Do we
need
> to
> > > change it?
> > 
> > I guess Dan's point was that it leaks the implementation of this method out
to
> > the caller. I'd be happy either way, but let's coerce it into a list since
> > that's the way it was before.
> 
> 
> Mostly, I don't know python, and didn't know how the callers used the data. If
> we access it such that set is fine, then set is fine by me.

Anyways, as suggested by Sami, I am now returning list instead of set. :)

[r.kasibhatla, 2014-11-11 14:03:13]

The CQ bit was checked by r.kasibhatla@samsung.com

[commit-bot: I haz the power, 2014-11-11 14:04:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/690103005/60001

[Sami, 2014-11-11 14:04:13]

lgtm

[commit-bot: I haz the power, 2014-11-11 14:41:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-11 14:42:15]

Patchset 4 (id:??) landed as
https://crrev.com/14935a5313c7fe81cdf169e86ee0e81e26649c56
Cr-Commit-Position: refs/heads/master@{#303645}