Added the list of active field groups to NetLog dump

Added the list of active field groups to NetLog dump

This CL adds a list of active field groups to NetLog dump. The string
representation is human readable, in the form of trial_name:group_name.

BUG=426474
Committed: https://crrev.com/00c5180f03ebd1664ca2e0b8b4170b0d30441000
Cr-Commit-Position: refs/heads/master@{#301104}

[xunjieli, 2014-10-23 19:50:34]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org

[xunjieli, 2014-10-23 19:56:25]

https://codereview.chromium.org/671253002/diff/1/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (left):

https://codereview.chromium.org/671253002/diff/1/net/base/net_log_logger.cc#o...
net/base/net_log_logger.cc:107: 
I will revert this edit in the next patch.

[mmenke, 2014-10-23 20:50:23]

Do you want to add the UI in this CL, or add it in the followup?

https://codereview.chromium.org/671253002/diff/20001/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/671253002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:267: for (size_t i = 0; i < active_groups.size();
i++) {
nit:  Should use an iterator (size() can theoretically be expensive to compute)

https://codereview.chromium.org/671253002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:269: active_groups[i].trial_name);
Trial name should probably go first - generally goes less specific before more
specific.  NiftyFeature:enabled, rather than enabled:NiftyFeature.

[xunjieli, 2014-10-23 21:32:39]

I have uploaded a new patch to address the comments. PTAL. Thanks!

https://codereview.chromium.org/671253002/diff/20001/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/671253002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:267: for (size_t i = 0; i < active_groups.size();
i++) {
On 2014/10/23 20:50:23, mmenke wrote:
> nit:  Should use an iterator (size() can theoretically be expensive to
compute)

Done.

https://codereview.chromium.org/671253002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:269: active_groups[i].trial_name);
On 2014/10/23 20:50:23, mmenke wrote:
> Trial name should probably go first - generally goes less specific before more
> specific.  NiftyFeature:enabled, rather than enabled:NiftyFeature.

Done. I see! thanks for the explanation!

https://codereview.chromium.org/671253002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/net_internals/import_view.html (right):

https://codereview.chromium.org/671253002/diff/40001/chrome/browser/resources...
chrome/browser/resources/net_internals/import_view.html:68: <td><p
jsselect="constants.activeFieldTrialGroups" jscontent="$this"></p></td>
Not sure if this is okay, but I used p tag here to introduce a line break after
each group.

[mmenke, 2014-10-23 21:46:46]

https://codereview.chromium.org/671253002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/net_internals/import_view.html (right):

https://codereview.chromium.org/671253002/diff/40001/chrome/browser/resources...
chrome/browser/resources/net_internals/import_view.html:68: <td><p
jsselect="constants.activeFieldTrialGroups" jscontent="$this"></p></td>
On 2014/10/23 21:32:39, xunjieli wrote:
> Not sure if this is okay, but I used p tag here to introduce a line break
after
> each group.

That's fine - how long is the list?  I was thinking that it would be long enough
that it's better to just separate them with spaces.

https://codereview.chromium.org/671253002/diff/40001/chrome/browser/resources...
chrome/browser/resources/net_internals/import_view.html:68: <td><p
jsselect="constants.activeFieldTrialGroups" jscontent="$this"></p></td>
nit:  Can use lines less than 80-chars be doing:
<td>
  <p...></p>
</td>

We aren't strict about line length in HTML, but does make reviewing a little
nicer.

https://codereview.chromium.org/671253002/diff/40001/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/671253002/diff/40001/net/base/net_log_logger....
net/base/net_log_logger.cc:268: active_groups.begin(); it !=
active_groups.end(); ++it) {
optional nit:  Suggest putting a linebreak after the first semicolon, when the
first parameter of a for is split onto multiple lines, though you can find a
bunch of cases that are formatted just like this.

Up to you if you want to break at the second semicolon or not.

[xunjieli, 2014-10-23 22:07:52]

Patchset #4 (id:60001) has been deleted

[xunjieli, 2014-10-23 22:18:38]

Thanks Matt! PTAL

https://codereview.chromium.org/671253002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/net_internals/import_view.html (right):

https://codereview.chromium.org/671253002/diff/40001/chrome/browser/resources...
chrome/browser/resources/net_internals/import_view.html:68: <td><p
jsselect="constants.activeFieldTrialGroups" jscontent="$this"></p></td>
On 2014/10/23 21:46:45, mmenke wrote:
> On 2014/10/23 21:32:39, xunjieli wrote:
> > Not sure if this is okay, but I used p tag here to introduce a line break
> after
> > each group.
> 
> That's fine - how long is the list?  I was thinking that it would be long
enough
> that it's better to just separate them with spaces.

I have 9. With space, the list looks a little crammed, but I think space works
better in case the list grows long. Thanks!

https://codereview.chromium.org/671253002/diff/40001/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/671253002/diff/40001/net/base/net_log_logger....
net/base/net_log_logger.cc:268: active_groups.begin(); it !=
active_groups.end(); ++it) {
On 2014/10/23 21:46:45, mmenke wrote:
> optional nit:  Suggest putting a linebreak after the first semicolon, when the
> first parameter of a for is split onto multiple lines, though you can find a
> bunch of cases that are formatted just like this.
> 
> Up to you if you want to break at the second semicolon or not.

Got it! Done.

[mmenke, 2014-10-24 14:33:08]

LGTM!

[xunjieli, 2014-10-24 14:34:17]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2014-10-24 14:35:14]

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

[commit-bot: I haz the power, 2014-10-24 15:37:24]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-10-24 15:38:03]

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