Evaluation scores export library and CSS file for the exported HTML file

Evaluation scores export library and CSS file for the exported HTML file

BUG=webrtc:7218
NOTRY=True

Review-Url: https://codereview.webrtc.org/2723703002
Cr-Commit-Position: refs/heads/master@{#17526}
Committed: https://chromium.googlesource.com/external/webrtc/+/aa7d935cd551b6911cacd459efd5be3b15e7ec6f

[AleBzk, 2017-02-28 16:07:56]

Description was changed from

==========
Evaluation scores export library and CSS file for the exported HTML file

BUG=webrtc:7218
NOTRY=True
==========

to

==========
Evaluation scores export library and CSS file for the exported HTML file

BUG=webrtc:7218
NOTRY=True
==========

[AleBzk, 2017-02-28 16:07:56]

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

[AleBzk, 2017-02-28 16:33:50]

Hi Henrik,

This CL implements the HTML exporter to visualize the evaluation scores
generated by the py_quality_assessment tool. The exported file is stored
locally.

[kjellander_webrtc, 2017-03-03 04:52:21]

I think we could improve the readability and reusability by using jinja2, which
is a Python templating system we can use out of the box: it's located in
src/third_party/jinja2. 
Would you consider adopting using that instead here? See http://jinja.pocoo.org/
for more information.

https://codereview.webrtc.org/2723703002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/export.py
(right):

https://codereview.webrtc.org/2723703002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/export.py:25:
def __init__(self, output_path, output_filename):
I'd suggest just having one argument: output_filename, which will be a full path
to the filename.

You're not using output_path for anything else so I don't think it's worth its
own weight.

https://codereview.webrtc.org/2723703002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/export.py:31:
def export(self, scores):
Chromium has two differences from the Google Python style guide:
* Indentation: use 2, not 4, spaces
* Naming: Functions and Method names use CapWords() style, not
lower_with_under()
   The only exception to this rule is the main() function -- we do not use
Main()
http://dev.chromium.org/chromium-os/python-style-guidelines

So please adjust all the function names to CapWords style.

[AleBzk, 2017-03-03 12:57:52]

Hi Henrik,

I addressed all the comments except for CapWords and switching to jinja2 for
HTML templating.

As you suggested in another CL, fixing to CapWords should be done in a separate
CL. Similarly, I'd prefer to switch to jijna2 with a new CL.

https://codereview.webrtc.org/2723703002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/export.py
(right):

https://codereview.webrtc.org/2723703002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/export.py:25:
def __init__(self, output_path, output_filename):
On 2017/03/03 04:52:21, kjellander_webrtc wrote:
> I'd suggest just having one argument: output_filename, which will be a full
path
> to the filename.
> 
> You're not using output_path for anything else so I don't think it's worth its
> own weight.

Done.

https://codereview.webrtc.org/2723703002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/export.py:31:
def export(self, scores):
On 2017/03/03 04:52:21, kjellander_webrtc wrote:
> Chromium has two differences from the Google Python style guide:
> * Indentation: use 2, not 4, spaces
> * Naming: Functions and Method names use CapWords() style, not
> lower_with_under()
>    The only exception to this rule is the main() function -- we do not use
> Main()
> http://dev.chromium.org/chromium-os/python-style-guidelines
> 
> So please adjust all the function names to CapWords style.

Yup, as you suggested in https://codereview.webrtc.org/2718063003/, I will fix
this in a separate CL.

[kjellander_webrtc, 2017-03-04 21:26:11]

lgtm with comment addressed.

Looking forward to follow-up CL for CapWords style and jinja2 templating. In
case using jinja2 becomes too time-consuming and doesn't improve the code - feel
free to skip it. But in general I think a templating library should be
beneficial for this and jinja2 seems simple enough for this purpose.

https://codereview.webrtc.org/2723703002/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/export.py
(right):

https://codereview.webrtc.org/2723703002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/export.py:197:
def _save(cls, output_filepath, html):
I don't think this method serves any purpose as it's only used in one place.
Please inline the code at line 52 instead.

[AleBzk, 2017-04-04 09:55:26]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-04-04 09:55:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2723703002/#ps80001 (title:
"merge")

[commit-bot: I haz the power, 2017-04-04 09:55:38]

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, 2017-04-04 09:58:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491299726933320,
"parent_rev": "8edb8397f810e10afae9ee4e86bf2cd4a6959a19", "commit_rev":
"aa7d935cd551b6911cacd459efd5be3b15e7ec6f"}

[commit-bot: I haz the power, 2017-04-04 09:58:12]

Description was changed from

==========
Evaluation scores export library and CSS file for the exported HTML file

BUG=webrtc:7218
NOTRY=True
==========

to

==========
Evaluation scores export library and CSS file for the exported HTML file

BUG=webrtc:7218
NOTRY=True

Review-Url: https://codereview.webrtc.org/2723703002
Cr-Commit-Position: refs/heads/master@{#17526}
Committed:
https://chromium.googlesource.com/external/webrtc/+/aa7d935cd551b6911cacd459e...
==========

[commit-bot: I haz the power, 2017-04-04 09:58:13]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/aa7d935cd551b6911cacd459e...