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

Add a new command to get HeapSnapshot for ChromeDriver. #131

Closed
wants to merge 4 commits into from

Conversation

gaoshuotao
Copy link

Main changes:

  1. Add a new command "-chromium-take-heap-snapshot" and a new interface "TakesHeapSnapshot".
  2. Make ChromeDriver.java implement "TakesHeapSnapshot".
  3. Add an augmenter provider "AddTakesHeapSnapshot".

@@ -24,6 +24,7 @@
String PLATFORM = "platform";
String SUPPORTS_JAVASCRIPT = "javascriptEnabled";
String TAKES_SCREENSHOT = "takesScreenshot";
String TAKES_HEAP_SNAPSHOT = "takesHeapSnapshot";
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 a standard capability and should be hidden behind a vendor prefix: -chromium-takesHeapSnapshot

@jleyba
Copy link
Contributor

jleyba commented Oct 30, 2013

I'm not comfortable with extending the core Selenium API for a browser-specific feature. Can this be provided as utility library that uses WebDriver as a collaborator? Maybe something like:

WebDriver driver = // ...
HeapSnapshot snapshot = HeapSnapshotter.for(driver).takeSnapshot();

@klepikov
Copy link
Contributor

Sorry, has this ever made it into the client API?

@ddavison
Copy link
Member

@klepikov no it's not. you can do a File search by pressing T then searching for the class that the OP gave. HeapSnapshot didn't pop up.

@klepikov
Copy link
Contributor

@jleyba @gaoshuotao Is there a disagreement in principle, or is it simply a case of a stalled code review?

@AutomatedTester
Copy link
Member

@klepikov This should be handled as a 3rd party dependency and shouldnt be added to the Selenium project.

This can be in the same library as #168

@andreastt
Copy link
Member

@gaoshuotao, do you want to address @jleyba's issues?

@lukeis
Copy link
Member

lukeis commented Aug 25, 2016

closing, PR now too old, required changes. Submit a new PR if appropriate.

@lukeis lukeis closed this Aug 25, 2016
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.

7 participants