New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add binding for metric #33

Merged
merged 3 commits into from Mar 15, 2018

Conversation

Projects
None yet
2 participants
@jihchi
Copy link
Collaborator

jihchi commented Mar 14, 2018

Fix #28

Archie Lee
@zploskey

This comment has been minimized.

Copy link
Owner

zploskey commented Mar 14, 2018

Some of these values should probably be typed as int, don't you think?

@jihchi

This comment has been minimized.

Copy link
Collaborator Author

jihchi commented Mar 14, 2018

I'm not sure which of metrics should be int type?

https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pagemetrics

Timestamp <number> The timestamp when the metrics sample was taken.
Documents <number> Number of documents in the page.
Frames <number> Number of frames in the page.
JSEventListeners <number> Number of events in the page.
Nodes <number> Number of DOM nodes in the page.
LayoutCount <number> Total number of full or partial page layout.
RecalcStyleCount <number> Total number of page style recalculations.
LayoutDuration <number> Combined durations of all page layouts.
RecalcStyleDuration <number> Combined duration of all page style recalculations.
ScriptDuration <number> Combined duration of JavaScript execution.
TaskDuration <number> Combined duration of all tasks performed by the browser.
JSHeapUsedSize <number> Used JavaScript heap size.
JSHeapTotalSize <number> Total JavaScript heap size.

How about change all metric's type to int?

@zploskey

This comment has been minimized.

Copy link
Owner

zploskey commented Mar 14, 2018

JavaScript does not distinguish between integers and floats so they will show everything as Number. Some of these are naturally integers because they are counts. I think the following should be integers:

Documents <number> Number of documents in the page.
Frames <number> Number of frames in the page.
JSEventListeners <number> Number of events in the page.
Nodes <number> Number of DOM nodes in the page.
LayoutCount <number> Total number of full or partial page layout.
RecalcStyleCount <number> Total number of page style recalculations.
JSHeapUsedSize <number> Used JavaScript heap size.
JSHeapTotalSize <number> Total JavaScript heap size.

As a general rule if there is any chance something might overflow a 32 bit int we should type it as float. I don't think the above fields will but please point out if you don't think that is true for any of them.

Archie Lee
@jihchi

This comment has been minimized.

Copy link
Collaborator Author

jihchi commented Mar 15, 2018

Thank you and fixed.

"LayoutCount": int,
"RecalcStyleCount": int,
"LayoutDuration": int,
"RecalcStyleDuration": int,

This comment has been minimized.

@zploskey

zploskey Mar 15, 2018

Owner

LayoutDuration and RecalcStyleDuration should be floats still. I assume they need to be sub-second timings.

This comment has been minimized.

@jihchi

jihchi Mar 15, 2018

Author Collaborator

Fixed in commit e35add0

@zploskey zploskey merged commit 393ce20 into zploskey:master Mar 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jihchi jihchi deleted the jihchi:binding_for_metrics branch Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment