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

Browser logging implementation and Network Activity logging in HAR format support #275

Closed
wants to merge 11 commits into from

Conversation

dbalabka
Copy link

No description provided.

@dbalabka
Copy link
Author

@detro have you any comment or feedback on my PR?

@detro
Copy link
Owner

detro commented Nov 3, 2013

Overall I think those features are needed and welcome.

But some of this might be clashing with the PR #274.
This logging seems to cover the scenarios available in #274, but I have spent just 30 minutes on this: I leave you guys to decide.

The coding is OK: you have respected the "implicit coding standard" :)

The one issue I can see is that you have a lot of commits that are all related to the features you are adding: you will need to squash this stuff into one (or max 2/3).

@detro
Copy link
Owner

detro commented Nov 26, 2013

So, I'm going to merge this PR but I'll manually integrate the feature of #274 as I think it can still be very useful to optionally output Network Req/Res in the console, if the right Capability is set.

@dbalabka
Copy link
Author

I reviewed related PR about network logging. As I understand you plan that HAR loggin will not enabled by default, but can be enabled using capabilities. Am I right?
I have manually tested all my changes and all works fine. But there are still no any unit tests.
And I have some worries about memory leaks. Do you have any standard approach to tests memory problems issues for JavaScript? Or I just can use built-in WebTools console?

@detro
Copy link
Owner

detro commented Nov 26, 2013

I'm working on merging this but in latest PhantomJS/master the page.onError code seems to have issues. I'm investigating.

}
};
page.onResourceError = function(error) {
page.resources[error.id].error = error;
Copy link
Owner

Choose a reason for hiding this comment

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

page.resources[error.id] object needs to be initialized

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fixing it as part of the merge.

Curiosity: did you run the tests?

Copy link
Author

Choose a reason for hiding this comment

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

After that fixes seems like no. My fault, sorry about this.

Copy link
Author

Choose a reason for hiding this comment

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

Btw You need to add one line:

page.onResourceError = function(error) {
    page.resources[error.id] || (page.resources[error.id] = {});

Copy link
Owner

Choose a reason for hiding this comment

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

yep.

@detro
Copy link
Owner

detro commented Nov 27, 2013

Merged (and added few fixed + test).

Thanks! :)

@detro detro closed this Nov 27, 2013
@sebv sebv mentioned this pull request Dec 4, 2013
AkeemMcLennon pushed a commit to AkeemMcLennon/ghostdriver that referenced this pull request Feb 23, 2015
CHANGELOG for v1.1.0 (https://github.com/detro/ghostdriver/issues?labels=1.1.0&state=closed)

JavaScript Driver (Core)
* ENHANCEMENT: `/maximize` window will set the window size to 1336x768,
currently most common resolution online (see http://gs.statcounter.com/#resolution-ww-monthly-201307-201312)
* ENHANCEMENT detro#275: Implemented Browser and Network (HAR) Logging types
* FIXED #284: Attempt to wait for Page to Load if input causes form submit
* FIXED #291: Throw exception when attempting to set invalid timeout value
* FIXED detro#259: Fix issue regarding mouse clicks
* ENHANCEMENT detro#290: Enabled support for "Keep Alive" HTTP connections
* ENHANCEMENT detro#262: Allow access to PhantomJS API from WebDriver (Driver part)
* ENHANCEMENT #293: Import Selenium 2.39.0 WebDriver Atoms

Java Binding
* MINOR detro#251: Minor compilation issues for Binding
* ENHANCEMENT detro#262: Allow access to PhantomJS API from WebDriver (Java Binding part)

Tested using GhostDriver validation tests (https://github.com/detro/ghostdriver/tree/master/test).

ariya/phantomjs#11877
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.

2 participants