Convenience functions to set axis properties in visualization tool.

Convenience functions to set axis properties in visualization tool.

Also makes the properties protected, as previously requested by Stefan.

Committed: https://crrev.com/dc35dcd4dce9d6e3737434aed57ec3b78b0feba0
Cr-Commit-Position: refs/heads/master@{#13593}

[terelius, 2016-07-26 09:18:16]

terelius@webrtc.org changed reviewers:
+ danilchap@webrtc.org, stefan@webrtc.org

[terelius, 2016-07-26 09:18:17]

Please take a look.

[danilchap, 2016-07-26 10:52:47]

nice to see more common style in tools.
few suggestions:

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
File webrtc/tools/event_log_visualizer/analyzer.cc (right):

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:31: namespace {
it is recommended to have unnamed namespace inside named namespaces,
i.e. consider moving (may be in a different CL)
namespace webrtc {
namespace plotting {
above this line.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:79: const float kLeftMargin =
0.01;
can be constexpr

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
File webrtc/tools/event_log_visualizer/plot_base.h (right):

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:51: // title and one or more data
series. We make it a class only to document that
Comment probably need an update too: This struct become almost normal class, not
just a struct & interface.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:53: class Plot {
may be add plot_base.cc and move implementation there to make this more proper
class.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:61: float left_margin = 0,
it doesn't look like you use default arguments (i.e. didn't notice calls to
these functions with just 3 parameters). May be remove them.
or move Margin constants into this class and use them as default parameters
(since it will be uncommon for margins to be different)

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:62: float right_margin = 0) {
margin doesn't use same units as min/max. It might help to hint that.
E.g. by renaming parameters to right_margin_percent (and then divide it by
100.0)

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:73: for (auto& series :
series_list_) {
const auto& here and next line

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:75: min = std::min(min, point.x);
std::min as function and min as variable in same line hints that may be
better to rename min to e.g. min_value.
It would also hint min/max are not pixels.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:79: RTC_DCHECK(max >= min);
RTC_DCHECK_LE(min, max);
and have this check in SetXAxis

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:80: xaxis_min_ = min - left_margin
* (max - min);
may be call SetXAxis instead of duplicating the code.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:114: std::vector<TimeSeries>
series_list_;
Why rename this variable to _list when it is not even a list?
generic name 'series' looks better.

Probably better to hide this member into protected section too and add a
function similar to one in PlotCollection:
TimeSeries* AddNewSeries();
or
AddNewSeries(TimeSeries value); // Move constructor can be used then.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:116: protected:
either make it private, or update CL description that you didn't

[terelius, 2016-07-26 15:38:35]

Description was changed from

==========
Convenience functions to set axis properties in visualization tool.

Also makes the properties private, as previously requested by Stefan.
==========

to

==========
Convenience functions to set axis properties in visualization tool.

Also makes the properties protected, as previously requested by Stefan.
==========

[terelius, 2016-07-26 17:37:39]

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
File webrtc/tools/event_log_visualizer/analyzer.cc (right):

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:31: namespace {
On 2016/07/26 10:52:46, danilchap wrote:
> it is recommended to have unnamed namespace inside named namespaces,
> i.e. consider moving (may be in a different CL)
> namespace webrtc {
> namespace plotting {
> above this line.

Done.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:79: const float kLeftMargin =
0.01;
On 2016/07/26 10:52:46, danilchap wrote:
> can be constexpr

Done.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
File webrtc/tools/event_log_visualizer/plot_base.h (right):

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:51: // title and one or more data
series. We make it a class only to document that
On 2016/07/26 10:52:46, danilchap wrote:
> Comment probably need an update too: This struct become almost normal class,
not
> just a struct & interface.

Done.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:53: class Plot {
On 2016/07/26 10:52:46, danilchap wrote:
> may be add plot_base.cc and move implementation there to make this more proper
> class.

Done.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:61: float left_margin = 0,
On 2016/07/26 10:52:46, danilchap wrote:
> it doesn't look like you use default arguments (i.e. didn't notice calls to
> these functions with just 3 parameters). May be remove them.
> or move Margin constants into this class and use them as default parameters
> (since it will be uncommon for margins to be different)

They are not used right now, but just setting the axis limits (without adding
any extra margins) seems like a very natural case.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:62: float right_margin = 0) {
On 2016/07/26 10:52:46, danilchap wrote:
> margin doesn't use same units as min/max. It might help to hint that.
> E.g. by renaming parameters to right_margin_percent (and then divide it by
> 100.0)

I added comments indicating that the margins are measured as fractions of the
interval.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:73: for (auto& series :
series_list_) {
On 2016/07/26 10:52:46, danilchap wrote:
> const auto& here and next line

Done.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:75: min = std::min(min, point.x);
On 2016/07/26 10:52:46, danilchap wrote:
> std::min as function and min as variable in same line hints that may be
> better to rename min to e.g. min_value.
> It would also hint min/max are not pixels.

Done.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:79: RTC_DCHECK(max >= min);
On 2016/07/26 10:52:46, danilchap wrote:
> RTC_DCHECK_LE(min, max);
> and have this check in SetXAxis

Done.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:80: xaxis_min_ = min - left_margin
* (max - min);
On 2016/07/26 10:52:46, danilchap wrote:
> may be call SetXAxis instead of duplicating the code.

Done.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:114: std::vector<TimeSeries>
series_list_;
On 2016/07/26 10:52:46, danilchap wrote:
> Why rename this variable to _list when it is not even a list?
> generic name 'series' looks better.
> 
> Probably better to hide this member into protected section too and add a
> function similar to one in PlotCollection:
> TimeSeries* AddNewSeries();
> or
> AddNewSeries(TimeSeries value); // Move constructor can be used then.

This variable is a collection of TimeSeries objects. The problem with the name
series is that there is no difference between the singular and plural form, so
we would get strange-looking code like
for (auto& series : series_)

I will consider adding an AddNewSeries()-function in a follow-up CL.

https://codereview.webrtc.org/2179223003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/plot_base.h:116: protected:
On 2016/07/26 10:52:46, danilchap wrote:
> either make it private, or update CL description that you didn't

Can't be private. Updated CL description.

[danilchap, 2016-07-27 08:39:07]

lgtm % nit

https://codereview.webrtc.org/2179223003/diff/20001/webrtc/tools/event_log_vi...
File webrtc/tools/event_log_visualizer/plot_base.h (right):

https://codereview.webrtc.org/2179223003/diff/20001/webrtc/tools/event_log_vi...
webrtc/tools/event_log_visualizer/plot_base.h:13: #include <algorithm>
with implementation moved to .cc this include can be moved too.

https://codereview.webrtc.org/2179223003/diff/20001/webrtc/tools/event_log_vi...
webrtc/tools/event_log_visualizer/plot_base.h:19: #include
"webrtc/base/checks.h"
and this one too

[terelius, 2016-07-28 15:23:04]

https://codereview.webrtc.org/2179223003/diff/20001/webrtc/tools/event_log_vi...
File webrtc/tools/event_log_visualizer/plot_base.h (right):

https://codereview.webrtc.org/2179223003/diff/20001/webrtc/tools/event_log_vi...
webrtc/tools/event_log_visualizer/plot_base.h:13: #include <algorithm>
On 2016/07/27 08:39:07, danilchap wrote:
> with implementation moved to .cc this include can be moved too.

Done.

https://codereview.webrtc.org/2179223003/diff/20001/webrtc/tools/event_log_vi...
webrtc/tools/event_log_visualizer/plot_base.h:19: #include
"webrtc/base/checks.h"
On 2016/07/27 08:39:07, danilchap wrote:
> and this one too

Done.

[terelius, 2016-07-29 15:33:46]

ping

[stefan-webrtc, 2016-08-01 11:08:22]

lgtm

[terelius, 2016-08-01 13:04:58]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2016-08-01 13:04:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2179223003/#ps100001 (title:
"Change literal type to avoid VC++ compile warning")

[commit-bot: I haz the power, 2016-08-01 13:05:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-01 13:23:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 13:23:55]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7095)

[terelius, 2016-08-01 18:00:07]

terelius@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[terelius, 2016-08-01 18:00:08]

Henrik, I need owner approval for BUILD.gn. Could you take a look?

[kjellander_webrtc, 2016-08-01 18:57:13]

The CQ bit was checked by kjellander@webrtc.org

[kjellander_webrtc, 2016-08-01 18:57:14]

lgtm

[commit-bot: I haz the power, 2016-08-01 18:57:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-01 19:03:30]

Description was changed from

==========
Convenience functions to set axis properties in visualization tool.

Also makes the properties protected, as previously requested by Stefan.
==========

to

==========
Convenience functions to set axis properties in visualization tool.

Also makes the properties protected, as previously requested by Stefan.
==========

[commit-bot: I haz the power, 2016-08-01 19:03:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-01 19:03:41]

Description was changed from

==========
Convenience functions to set axis properties in visualization tool.

Also makes the properties protected, as previously requested by Stefan.
==========

to

==========
Convenience functions to set axis properties in visualization tool.

Also makes the properties protected, as previously requested by Stefan.

Committed: https://crrev.com/dc35dcd4dce9d6e3737434aed57ec3b78b0feba0
Cr-Commit-Position: refs/heads/master@{#13593}
==========

[commit-bot: I haz the power, 2016-08-01 19:03:42]

Patchset 6 (id:??) landed as
https://crrev.com/dc35dcd4dce9d6e3737434aed57ec3b78b0feba0
Cr-Commit-Position: refs/heads/master@{#13593}