Skip to content
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

Module HAR #263

Merged
merged 16 commits into from
Mar 27, 2014
Merged

Module HAR #263

merged 16 commits into from
Mar 27, 2014

Conversation

william-p
Copy link
Contributor

Add har module for export HAR data in external file.
I use netsniff.js for convert to HAR format.
(#64)

@macbre
Copy link
Owner

macbre commented Mar 25, 2014

@william-p, thanks 👍

Please update README.md file as well

*/

if (!Date.prototype.toISOString) {
Date.prototype.toISOString = function () {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phantomjs> console.log(typeof Date.prototype.toISOString);
function

Polyfill not needed here :)

@william-p
Copy link
Contributor Author

Done ;)

version: '1.2',
creator: {
name: "Phantomas - HAR",
version: VERSION
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use phantomas version here to ease debugging

@macbre
Copy link
Owner

macbre commented Mar 25, 2014

One tiny little detail left and we're ready to merge it :)

@william-p
Copy link
Contributor Author

You are right 😄
How to get Phantomas version properly ?
phantomas.version is undefined :/

macbre added a commit that referenced this pull request Mar 25, 2014
@macbre
Copy link
Owner

macbre commented Mar 25, 2014

@william-p, good point. I've added phantomas.getVersion function in 3ca06c7

@macbre macbre added this to the v1.0 milestone Mar 25, 2014
@william-p
Copy link
Contributor Author

Okay, now i'm first to use it 😄

id: address,
title: title,
pageTimings: {
onLoad: endTime.getTime() - startTime.getTime()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/HAR/Overview.html#sec-object-types-pageTimings

HAR spec mentions the optional onContentLoad property that "represents DOMContentLoad event or document.readyState == interactive". Worth adding?

phantomas.log('Convert HAR to JSON');
var dump = JSON.stringify(har);

phantomas.log('Write HAR in \'%s\'', path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap with double quotes so you don't have to escape single quotes

@cvan
Copy link
Contributor

cvan commented Mar 26, 2014

HAR support is awesome to see! it'd be nice if we could identify which resources are inline à la https://github.com/cvan/phantomHAR/blob/master/phantomhar.js#L232 but that's an enhancement

headers: endReply.headers,
redirectURL: "",
headersSize: -1,
bodySize: startReply.bodySize,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not accurate but that's phantomJS' fault. In a project of mine, I actually request each resource separately to get the accurate number of bytes transferred (and the content of each response). It's a pain.

vgangan pushed a commit to vgangan/phantomas that referenced this pull request Mar 26, 2014
@william-p
Copy link
Contributor Author

@cvan, thanks for your comments, i appreciate.
Sorry for code style, major part of code is a wrapper around original ntsniff but you are right. I will work for improve this.
Maybe the best solution is to use your HAR code ? it seems more efficient.
What do you think ?

@macbre
Copy link
Owner

macbre commented Mar 26, 2014

@william-p, it's actually a good idea to re-use @cvan code given the fact that phantomas already provides information such as type of response's content.

@william-p
Copy link
Contributor Author

@macbre, i'm agree :) you are ok with simple copy/paste with origin in comments ?

@macbre
Copy link
Owner

macbre commented Mar 26, 2014

Copy&paste - no :) As I said previously, phantomas provides most of the information for each request / response that phantom-HAR generates. There's no need to copy & paste the whole code.

@william-p
Copy link
Contributor Author

Of course, I talked about createHAR function and utils if need ...

@macbre
Copy link
Owner

macbre commented Mar 26, 2014

In that case, I'm all for it :)

@william-p
Copy link
Contributor Author

nb: inline is not supported in this commit

@william-p
Copy link
Contributor Author

Humm, module jsErrors raise a error in report ...

"jsErrors": [
    "SECURITY_ERR: DOM Exception 18: An attempt was made to break through the security policy of the user agent. - unknown fn: phantomjs://webpage.evaluate() @ 2 / unknown fn: phantomjs://webpage.evaluate() @ 3 / unknown fn: phantomjs://webpage.evaluate() @ 3"
]

Edit:
Ok, localStorage.clear() is the cause.

@cvan
Copy link
Contributor

cvan commented Mar 26, 2014

Hmm. I understand the reason behind reusing phantomHAR code, but adding "custom" properties to PhantomJS page object is something I'd like to avoid. Can a "local" module variable / variables be used instead?

The issue becomes unless you're keeping track of the rendered DOM, you don't know for example which images were inline and which were defined in stylesheets.

I would recommend skipping on that part; that can be an enhancement.

Ideally, @cvan can publish phantomHAR to npm and expose CommonJS module that can be reused in phantomas code.

I would not recommend reusing phantomHAR code, especially for this part.

I've been reverse-engineering Chrome's HAR builder. And I've been working on a patch for my project (cvan/fastHAR-api) that generates the correct values for the following:

  • response.headersSize (length of raw headers)
  • response.bodySize (transfer size - response.headersSize)
  • response.content.size (length of response body)
  • response.content.compression (response.bodySize - response.content.size)

I'll be sure to open a pull request once I'm done.

Nice work, guys!

@macbre
Copy link
Owner

macbre commented Mar 26, 2014

The issue becomes unless you're keeping track of the rendered DOM, you don't know for example > which images were inline and which were defined in stylesheets.

I would recommend skipping on that part; that can be an enhancement.

@cvan, I do not quite get you here. I was referring to an array that stores a list of requests that is a stored as page object member instead of being a variable in module's scope.

That's my only concern regarding code style and good practices in general. Once this one is solved, we're ready to merge...

@cvan, looking forward to your pull request :)

@william-p
Copy link
Contributor Author

Okay, until something better, tomorrow (France) i will make a patch for use own scope in place of page variable.

@cvan, i'm waiting with impatience your enhancements ;)

Nice work too 😄

@macbre
Copy link
Owner

macbre commented Mar 26, 2014

@william-p, merci :)

@macbre
Copy link
Owner

macbre commented Mar 27, 2014

@william-p, this one is great 👍 Thanks @cvan for the feedback.

macbre added a commit that referenced this pull request Mar 27, 2014
@macbre macbre merged commit 50d980f into macbre:master Mar 27, 2014
@william-p
Copy link
Contributor Author

Nice job guys, thanks for merge :)

@william-p william-p deleted the module-har branch March 27, 2014 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants