Send screenshots back to the client for debugging

Send screenshots back to the client for debigging
BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79721

[John Grabowski, 2011-02-14 22:13:59]

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:46: void CaptureEntirePageAsPNG(const
FilePath& path, bool* success);
Return bool?
Presumably this dumps to file on disk.  Document as such.

Is that a good idea when the WebDriver client may be on a different machine?

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/response.h (right):

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.h:29: #define
SET_WEBDRIVER_SCREENSHOT_ERROR(response, msg, err, sesssion) { \
You don't use this macro anywhere.  Delete it perhaps?

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.h:53: inline void set_screenshot(const
std::string* screen) {
2 things:
1. I can't really tell what this does by looking at it.  Is the screen data
somehow encoded in the string?  (I think the answer is that "screen" is a
filename but you'd never know by looking at the name or type.)
2. You implicitly pass ownership of the pointer to set_screenshot.  Seems like a
bad idea.  Caller should own the pointer.

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:232: bool Session::Screenshot(std::string&
screenshot) {
std::string is the wrong type for image data.  std::vector<unsigned char> might
be fine.

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:256: is.open(file.get(), std::ios::binary);
Use of a variable named the same as the verb "is" makes this awkward.  "is
open"?  "is fail"?
How about "ifs"?

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:263: is.seekg(0, std::ios::end);
Use a stat instead?

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:276: base::Base64Encode(*data.get(),
&screenshot);
Once you load the data, should you delete the file?

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.h (right):

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:58: bool Screenshot(std::string& screenshot);
Just by looking at the names and types of these functions the difference or use
case isn't clear.

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_remote_tests.py:17:
sys.path.append("/chromium/src/third_party/webdriver/python")
This line needs a little tweaking

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_remote_tests.py:117: self.assertTrue(len(s) >
1000);
Another idea: navigate to a page like
data:text/html;utf-8,<div style="background-color:#000; height:1000px">

And sanity check that it's all zeros.

[Paweł Hajdan Jr., 2011-02-15 08:54:35]

Drive-by with testing-related comments. Please fix them and let me do another
review before committing.

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:235: scoped_ptr<char>
file(tempnam(screenshots_dir_.path().value().c_str(),
Well, we have some functions for dealing with temporary files in file_util...
like CreateTempFileInDir or something like that. Please use it.

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:250: if (CaptureEntirePageAsPNG(path) == false)
{
nit: !CaptureEntirePageAsPNG(path)

Explicit comparisons with true or false are strongly discouraged.

http://codereview.chromium.org/5572001/diff/12001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:256: is.open(file.get(), std::ios::binary);
Oh, we have functions to deal with that in file_util. Please don't reinvent
them.

[kkania, 2011-02-24 22:05:02]

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/response.cc (right):

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.cc:58: const std::string& file, int
line) {
add a 'const std::string& screenshot' as another arg to this func. If it is not
empty, set it.  Remove the special SetScreenshot func

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/screenshot_command.cc (right):

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/screenshot_command.cc:13: #include
"chrome/test/webdriver/commands/response.h"
this include should go above the error_codes one

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:376: // Generate a temp file name for the
screenshot
we no longer need a temp file name if we are using a temp directory, remove

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_remote_tests.py:100: def
Testcreenshots_dircreenShots(RemoteWebDriverTest):
I think this test name got messed up. But why are you adding a test here? This
will not be run on the bots. Instead, put it in chromedriver_tests.py

[kkania, 2011-02-25 23:56:02]

my comments were on the second patch. Let me know when you fix and upload
another one

[Joe, 2011-03-02 02:21:25]

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/response.cc (right):

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.cc:58: const std::string& file, int
line) {
On 2011/02/24 22:05:02, kkania wrote:
> add a 'const std::string& screenshot' as another arg to this func. If it is
not
> empty, set it.  Remove the special SetScreenshot func

Done.

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/screenshot_command.cc (right):

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/screenshot_command.cc:13: #include
"chrome/test/webdriver/commands/response.h"
On 2011/02/24 22:05:02, kkania wrote:
> this include should go above the error_codes one

Done.

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:376: // Generate a temp file name for the
screenshot
On 2011/02/24 22:05:02, kkania wrote:
> we no longer need a temp file name if we are using a temp directory, remove

Done.

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_remote_tests.py:100: def
Testcreenshots_dircreenShots(RemoteWebDriverTest):
On 2011/02/24 22:05:02, kkania wrote:
> I think this test name got messed up. But why are you adding a test here? This
> will not be run on the bots. Instead, put it in chromedriver_tests.py

Done.

[Joe, 2011-03-02 09:34:17]

the screen commands didn't get uploaded with the patch.  I will re-upload the
patch again and make sure they are added

On 2011/03/02 02:21:25, Joe wrote:
>
http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
> File chrome/test/webdriver/commands/response.cc (right):
> 
>
http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
> chrome/test/webdriver/commands/response.cc:58: const std::string& file, int
> line) {
> On 2011/02/24 22:05:02, kkania wrote:
> > add a 'const std::string& screenshot' as another arg to this func. If it is
> not
> > empty, set it.  Remove the special SetScreenshot func
> 
> Done.
> 
>
http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
> File chrome/test/webdriver/commands/screenshot_command.cc (right):
> 
>
http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/comma...
> chrome/test/webdriver/commands/screenshot_command.cc:13: #include
> "chrome/test/webdriver/commands/response.h"
> On 2011/02/24 22:05:02, kkania wrote:
> > this include should go above the error_codes one
> 
> Done.
> 
>
http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/sessi...
> File chrome/test/webdriver/session.cc (right):
> 
>
http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/sessi...
> chrome/test/webdriver/session.cc:376: // Generate a temp file name for the
> screenshot
> On 2011/02/24 22:05:02, kkania wrote:
> > we no longer need a temp file name if we are using a temp directory, remove
> 
> Done.
> 
>
http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/webdr...
> File chrome/test/webdriver/webdriver_remote_tests.py (right):
> 
>
http://codereview.chromium.org/5572001/diff/28001/chrome/test/webdriver/webdr...
> chrome/test/webdriver/webdriver_remote_tests.py:100: def
> Testcreenshots_dircreenShots(RemoteWebDriverTest):
> On 2011/02/24 22:05:02, kkania wrote:
> > I think this test name got messed up. But why are you adding a test here?
This
> > will not be run on the bots. Instead, put it in chromedriver_tests.py
> 
> Done.

[Paweł Hajdan Jr., 2011-03-03 14:43:42]

Code I commented in the drive-by LGTM, thanks!

http://codereview.chromium.org/5572001/diff/44001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.h (right):

http://codereview.chromium.org/5572001/diff/44001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:154: void SetImplicitWait(const int& timeout);
nit: Just int timeout.

[kkania, 2011-03-07 18:15:17]

couple overall questions:
1) I noticed you de-inlined some stuff, which is good. But why did you change
their names? E.g., you changed current_window_id to CurrentWindowId? Even though
it is not inlined, it is still a simple accessor method and should be
current_window_id style.

2) I noticed you made taking a screenshot a command-line arg. I think we should
start moving away from command-line args to having it configurable per session.

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/chrom...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:133: def testScreenCapture
there are a lot of errors in this script; please fix

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/create_session.cc (right):

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/create_session.cc:33: if
(!session->Init(session_manager->ChromeDir())) {
why'd you change this name?

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/navigate_commands.cc (right):

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/navigate_commands.cc:27:
session_->SetCurrentFrameXPath("");
if you want to change this, really I think the best way is to remove this
function, and change the frame xpath in the Session methods

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/response.h (right):

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.h:30: if (sm->ScreenshotOnError()) { \
why not screenshot_on_error

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.h:78: DictionaryValue data_;
i don't really care that much, but I think the norm is to have a newline between
variables and DISALLOW..., but not between private: and DISALLOW

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:383: std::string file =
screenshots_dir.path().value() + "screen";
instead of all this code, you should be able to just do
screenshots_dir.path().AppendASCII("screen")

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:641: void Session::SetCurrentFrameXPath(const
std::string& xpath) {
Why did you change these func names?

[Joe, 2011-03-11 22:12:48]

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/chrom...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:133: def testScreenCapture
refreshed
On 2011/03/07 18:15:17, kkania wrote:
> there are a lot of errors in this script; please fix

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/response.h (right):

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.h:30: if (sm->ScreenshotOnError()) { \
Not every error is a screen shot and since they are large PNG files it puts an
extra unnecessary overhead.

On 2011/03/07 18:15:17, kkania wrote:
> why not screenshot_on_error

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.h:78: DictionaryValue data_;
On 2011/03/07 18:15:17, kkania wrote:
> i don't really care that much, but I think the norm is to have a newline
between
> variables and DISALLOW..., but not between private: and DISALLOW

Done.

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/5572001/diff/45003/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:383: std::string file =
screenshots_dir.path().value() + "screen";
That would keep the file around until the session is closed rather than cleaning
it up once the shot is sent back.  It's probably cleaner to remove the file once
it is not needed anymore.

On 2011/03/07 18:15:17, kkania wrote:
> instead of all this code, you should be able to just do
> screenshots_dir.path().AppendASCII("screen")

[kkania, 2011-03-11 23:43:15]

http://codereview.chromium.org/5572001/diff/53001/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.cc (right):

http://codereview.chromium.org/5572001/diff/53001/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.cc:6: 
include base/file_path

http://codereview.chromium.org/5572001/diff/53001/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/5572001/diff/53001/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:13: #include
"base/path_service.h"
what's this include for? I guess you meant base/file_path.h Instead of either,
just forward declare 'class FilePath;' below.

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/chrom...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:127: self._driver =
WebDriver(self._launcher.GetURL(), 'chrome', 'any')
i just committed the other change you reviewed, so you'll want to change this to
{}

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:134: self._driver_.get(self.SEARCH)
hmm, did you try running these tests? it probably won't even run since
self._driver_ is not defined...; ask me if you don't know how to run them

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:139: self.assertTrue(len(s1) !=
len(s2))
we should test and make sure at least the dimensions are correct and the pixels
are close.  How about we load
'chrome/test/data/automation_proxy_snapshot/set_size.html?2000,2000' and compare
the base64 screenshot with some reference files we save to disk?

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/response.h (right):

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.h:25: // a proper stack trace to the
client
add '.' at end

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.h:25: // a proper stack trace to the
client
include something in the comment about it taking a screenshot

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.h:26: #define
SET_WEBDRIVER_ERROR2(response, session, msg, err) \
i like the previous name you used to have

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.h:32:
response->SetScreenshotAsBase64(__screenshot__); \
bit confused about what this is. Where does this func exist? I don't think you
need it anyways.

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/serve...
File chrome/test/webdriver/server.cc (right):

http://codereview.chromium.org/5572001/diff/53001/chrome/test/webdriver/serve...
chrome/test/webdriver/server.cc:199:
manager->set_screenshot_on_error(cmd_line->HasSwitch("screenshot-on-error"));
change this to be a parameter when you create a session

[kkania, 2011-03-16 18:19:10]

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1678:
use_json_interface_(use_json_interface){}
space before {}

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2054:
handler_map["GetScreenShot"] =
change name here too

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4973: #else
I think you can get rid of the #else part; as you have it it wouldn't compile
anyways for the unknown platform, because path_str isn't defined

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4975: #endif
actually, I think you should be able to replace all the defines with:

FilePath::StringType path_str;

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4988: this,
reply_message, render_view, path, true);
just make the FilePath(path_str) here and get rid of it above

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4991: LOG(ERROR) <<
"Could not get render view for tab handle";
this should be replaced with a AutomationJSONRelpy(this,
reply_message).SendError("Tab contents has no renderer host")

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:866: // Example:  //  
input: { "windex": 1, "tab_index": 1 }
format this like the one above

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:867: //   output: {
"png": raw binary data as a string }
the output does not have a 'png' key. The output refers to what you Send with
the AutomationJSONReply, which comes from PageSnapshotTaker.

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:868: void
GetScreenShotJSON(DictionaryValue* args, IPC::Message* reply_message);
it's not really a screenshot, it is the entire page. I know that's what they
call it in webdriver land, but this is chrome automation code. Can you go back
to the old name CaptureEntirePageJSON?

http://codereview.chromium.org/5572001/diff/56004/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.cc:210: return
SendAutomationJSONRequest(sender, dict, &reply_dict);
you need to check the value of the "success" key here

http://codereview.chromium.org/5572001/diff/56004/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:104: bool
SendGetScreenShotJSONRequest(
change name here

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:20: #include "base/scoped_temp_dir.h"
i don't think you need this here anymore

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/create_session.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/create_session.cc:40: bool screenshot_on_error;

DictionaryValue* capabilities = NULL;
if (GetDictionaryParameter("desiredCapabilities", &capabilities)) {
  capabilities->GetBoolean("takeScreenshotOnError", &screenshot_on_error);
}

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/response.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.cc:81: if (!base::Base64Encode(png,
&base64_png)) {
does this work with the case where |png| is empty?

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.h (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:106: bool CaptureEntirePageAsPNG(std::string*
png);
I see you didn't change this one to GetScreenShot. The idea with the naming is
as following:

Session methods can follow webdriver naming conventions, all Automation methods
do not. That is why Session passes around window_id's everywhere and Automation
passes around tab_id's.

So, it would be ok to use GetScreenshot here, because that is what Webdriver
uses, but better to use CaptureEntirePage in Automation, because that is what
Chrome automation is really doing.

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:190: void set_screenshot_on_error(const bool
error);
get rid of const before bool here.

[Joe, 2011-03-17 00:15:25]

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1678:
use_json_interface_(use_json_interface){}
On 2011/03/16 18:19:10, kkania wrote:
> space before {}

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2054:
handler_map["GetScreenShot"] =
On 2011/03/16 18:19:10, kkania wrote:
> change name here too

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4973: #else
On 2011/03/16 18:19:10, kkania wrote:
> I think you can get rid of the #else part; as you have it it wouldn't compile
> anyways for the unknown platform, because path_str isn't defined

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4975: #endif
On 2011/03/16 18:19:10, kkania wrote:
> actually, I think you should be able to replace all the defines with:
> 
> FilePath::StringType path_str;

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4988: this,
reply_message, render_view, path, true);
On 2011/03/16 18:19:10, kkania wrote:
> just make the FilePath(path_str) here and get rid of it above

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4991: LOG(ERROR) <<
"Could not get render view for tab handle";
On 2011/03/16 18:19:10, kkania wrote:
> this should be replaced with a AutomationJSONRelpy(this,
> reply_message).SendError("Tab contents has no renderer host")

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:866: // Example:  //  
input: { "windex": 1, "tab_index": 1 }
On 2011/03/16 18:19:10, kkania wrote:
> format this like the one above

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:867: //   output: {
"png": raw binary data as a string }
On 2011/03/16 18:19:10, kkania wrote:
> the output does not have a 'png' key. The output refers to what you Send with
> the AutomationJSONReply, which comes from PageSnapshotTaker.

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:868: void
GetScreenShotJSON(DictionaryValue* args, IPC::Message* reply_message);
On 2011/03/16 18:19:10, kkania wrote:
> it's not really a screenshot, it is the entire page. I know that's what they
> call it in webdriver land, but this is chrome automation code. Can you go back
> to the old name CaptureEntirePageJSON?

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.cc:210: return
SendAutomationJSONRequest(sender, dict, &reply_dict);
On 2011/03/16 18:19:10, kkania wrote:
> you need to check the value of the "success" key here

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:104: bool
SendGetScreenShotJSONRequest(
On 2011/03/16 18:19:10, kkania wrote:
> change name here

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:20: #include "base/scoped_temp_dir.h"
On 2011/03/16 18:19:10, kkania wrote:
> i don't think you need this here anymore

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/create_session.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/create_session.cc:40: bool screenshot_on_error;
Shouldn't the bindings roll the dictionary as normal parameters to the url.

On 2011/03/16 18:19:10, kkania wrote:
> 
> DictionaryValue* capabilities = NULL;
> if (GetDictionaryParameter("desiredCapabilities", &capabilities)) {
>   capabilities->GetBoolean("takeScreenshotOnError", &screenshot_on_error);
> }

Done.

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/response.cc (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.cc:81: if (!base::Base64Encode(png,
&base64_png)) {
yes it returns the empty string
On 2011/03/16 18:19:10, kkania wrote:
> does this work with the case where |png| is empty?

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.h (right):

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:106: bool CaptureEntirePageAsPNG(std::string*
png);
Renamed functions
On 2011/03/16 18:19:10, kkania wrote:
> I see you didn't change this one to GetScreenShot. The idea with the naming is
> as following:
> 
> Session methods can follow webdriver naming conventions, all Automation
methods
> do not. That is why Session passes around window_id's everywhere and
Automation
> passes around tab_id's.
> 
> So, it would be ok to use GetScreenshot here, because that is what Webdriver
> uses, but better to use CaptureEntirePage in Automation, because that is what
> Chrome automation is really doing.
>

http://codereview.chromium.org/5572001/diff/56004/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:190: void set_screenshot_on_error(const bool
error);
On 2011/03/16 18:19:10, kkania wrote:
> get rid of const before bool here.

Done.

[kkania, 2011-03-17 18:35:11]

I am almost ready to give approval.

However, since we want to patch this into chromium 11, let's split this change
up. Remove the 4 files in chrome/browser/automation from this CL and upload a
new one.

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1739:
dict.SetBoolean("result", success);
change this to "success" to match what you have in automation_json_requests.cc

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2054:
handler_map["GetScreenShot"] =
change this

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4986:
AutomationJSONReply(this,reply_message)
space after comma

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4988:
AutomationMsg_CaptureEntirePageAsPNG::WriteReplyParams(reply_message,
remove this and the following Send. We are already sending the error above, so
this is not needed. Take a look at how SendError in AutomationJSONReply is
implemented.

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:865: // Get the current
tab view as a PNG file in raw binary format.
change comment to 
// Captures the entire page of the the specified tab, including the non-visible
portions of the page, and saves the PNG to a file.

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:867: //   input: {
"windex": 1, "tab_index": 1 }
you function takes more inputs than this. Add them

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:868: //   output: none
the output is the dictionary you write in PageSnapshotTaker, which has a
'success' key. This should be here { "success": true }

http://codereview.chromium.org/5572001/diff/56005/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.cc:205:
dict.SetString("command", "GetScreenShot");
change name here too

http://codereview.chromium.org/5572001/diff/56005/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.cc:215: if
(reply_dict.GetBoolean("success", &success))
if (!

http://codereview.chromium.org/5572001/diff/56005/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:103: // PNG format.
Add Returns true on success.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:239: 
remove newline here

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:89: def DataDir():
move this out of BasicTest and put the function up top.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:191: self._driver.get(self.SEARCH)
we should not use public sites on the bots unless necessary. Either use a local
file or you can just delete this test, since the one below covers it

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:207: self.assertTrue(h,
'12c0ade27e3875da3d8866f52d2fa84f')
did this work on all platforms?

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/create_session.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/create_session.cc:40: bool screenshot_on_error;
default to false

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/create_session.cc:44: }
I didn't mean for you to delete the part of
session->set_screenshot_on_error(screenshot_on_error). Otherwise this doesn't do
anything.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.h (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:183: Speed speed();
add const;

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:189: bool get_screenshot_on_error() const;
accessor should just be called screenshot_on_error()

[Joe, 2011-03-18 00:33:32]

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1739:
dict.SetBoolean("result", success);
On 2011/03/17 18:35:11, kkania wrote:
> change this to "success" to match what you have in automation_json_requests.cc

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2054:
handler_map["GetScreenShot"] =
On 2011/03/17 18:35:11, kkania wrote:
> change this

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4986:
AutomationJSONReply(this,reply_message)
On 2011/03/17 18:35:11, kkania wrote:
> space after comma

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4988:
AutomationMsg_CaptureEntirePageAsPNG::WriteReplyParams(reply_message,
Is there any documentation with instructions on how to add a command and how all
of this message passing works.  To be honest I can't follow it at all, I am just
hacking from the example code.  What would be the proper way to handle this
particular condition.

On 2011/03/17 18:35:11, kkania wrote:
> remove this and the following Send. We are already sending the error above, so
> this is not needed. Take a look at how SendError in AutomationJSONReply is
> implemented.

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:865: // Get the current
tab view as a PNG file in raw binary format.
On 2011/03/17 18:35:11, kkania wrote:
> change comment to 
> // Captures the entire page of the the specified tab, including the
non-visible
> portions of the page, and saves the PNG to a file.

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:867: //   input: {
"windex": 1, "tab_index": 1 }
On 2011/03/17 18:35:11, kkania wrote:
> you function takes more inputs than this. Add them

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:868: //   output: none
On 2011/03/17 18:35:11, kkania wrote:
> the output is the dictionary you write in PageSnapshotTaker, which has a
> 'success' key. This should be here { "success": true }

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.cc:205:
dict.SetString("command", "GetScreenShot");
On 2011/03/17 18:35:11, kkania wrote:
> change name here too

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.cc:215: if
(reply_dict.GetBoolean("success", &success))
On 2011/03/17 18:35:11, kkania wrote:
> if (!

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:103: // PNG format.
On 2011/03/17 18:35:11, kkania wrote:
> Add Returns true on success.

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:239: 
On 2011/03/17 18:35:11, kkania wrote:
> remove newline here

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:89: def DataDir():
Won't future tests all use reference web files.  As a reference point pyauto
uses functions in the base class to find the data directory and doesn't use a
global function.

On 2011/03/17 18:35:11, kkania wrote:
> move this out of BasicTest and put the function up top.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:191: self._driver.get(self.SEARCH)
On 2011/03/17 18:35:11, kkania wrote:
> we should not use public sites on the bots unless necessary. Either use a
local
> file or you can just delete this test, since the one below covers it

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:207: self.assertTrue(h,
'12c0ade27e3875da3d8866f52d2fa84f')
Yah all platforms gave me the same PNG file with the same hash.  I think it's
because webkit did all of the rendering since it was just a box with no shading
or transparency.

On 2011/03/17 18:35:11, kkania wrote:
> did this work on all platforms?

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/create_session.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/create_session.cc:40: bool screenshot_on_error;
On 2011/03/17 18:35:11, kkania wrote:
> default to false

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/create_session.cc:44: }
Sorry I mistook the comment as to remove the ability for now.
On 2011/03/17 18:35:11, kkania wrote:
> I didn't mean for you to delete the part of
> session->set_screenshot_on_error(screenshot_on_error). Otherwise this doesn't
do
> anything.

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.h (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:183: Speed speed();
On 2011/03/17 18:35:11, kkania wrote:
> add const;

Done.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:189: bool get_screenshot_on_error() const;
On 2011/03/17 18:35:11, kkania wrote:
> accessor should just be called screenshot_on_error()

Done.

[Paweł Hajdan Jr., 2011-03-18 10:51:34]

Driving-by again, it seems there are new changes in chrome/browser/automation
introduced after my last review.

Please fix/answer the comment and wait for another review.

http://codereview.chromium.org/5572001/diff/70002/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/5572001/diff/70002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1737: if
(use_json_interface_) {
Is this duplication needed? Can we switch the sender of
AutomationMsg_CaptureEntirePageAsPNG to the JSON message?

[kkania, 2011-03-18 19:18:22]

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4986:
AutomationJSONReply(this,reply_message)
On 2011/03/18 00:33:32, Joe wrote:
> On 2011/03/17 18:35:11, kkania wrote:
> > space after comma
> 
> Done.

hmm, for some reason there are now two spaces after the comma; just 1 please

http://codereview.chromium.org/5572001/diff/56005/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4988:
AutomationMsg_CaptureEntirePageAsPNG::WriteReplyParams(reply_message,
you can take a look at what Nirnimesh wrote a while back
(http://www.chromium.org/developers/testing/pyauto/contributing). Feel free to
read the code and ask me questions.

Take a look at AutomationJSONReply::SendError. There are two things we need to
do in the automation provider: write the message, and send it.
AutomationMsg_....::WriteReplyParams is generated from the chrome IPC macro, and
it will write the message. Send of course sends it back.

Now there is one IPC message called SendJSONRequest, with the associated
AutomationMsg_SendJSONRequest::WriteReply func.  We want to write the correctly
formatted JSON string, so Nirnimesh added AUtomationJSONReply to easily write
and send an error or a success value.

You do need to remove the lines I told you too though.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:89: def DataDir():
Yes, I know this is from pyauto. However, right now we are set up a bit
differently because all the test fixtures (e.g., ScreenshotTest) inherit from
unittest.TestCase and not from  BasicTest. This is different than pyauto tests,
which inherit from PyUITest, which contains the DataDir funcs. If you want to
rearrange all the tests to inherit from chromedriver tests, that is fine,
although I think it is simpler just to have a standalone function in this case.
You can't pollute the global namespace in python (I think), only the module
namespace.

http://codereview.chromium.org/5572001/diff/56005/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:191: self._driver.get(self.SEARCH)
remove the SEARCH and NEW constants above too

[Joe, 2011-03-22 20:47:15]

With Ken's help I removed the duplication and made some improvements

http://codereview.chromium.org/5572001/diff/70002/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/5572001/diff/70002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1737: if
(use_json_interface_) {
I've switched the function to use the JSON message

On 2011/03/18 10:51:34, Paweł Hajdan Jr. wrote:
> Is this duplication needed? Can we switch the sender of
> AutomationMsg_CaptureEntirePageAsPNG to the JSON message?

[kkania, 2011-03-22 21:08:56]

LGTM; just fix the comments I gave and wait for Pawel

http://codereview.chromium.org/5572001/diff/79004/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4988:
AutomationJSONReply(this, reply_message).SendSuccess(NULL);
this should be SendError("Tab has no associated RenderViewHost")

http://codereview.chromium.org/5572001/diff/79004/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:868: //   input: {
"windex": 1, "tab_index": 1, "path:"file location"}
fix the comment (it is missing a quote and should have a real file path example:
"path": "/Users/temp/sreenshot.png"

http://codereview.chromium.org/5572001/diff/79004/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:102: // Requests a string
containing the raw bytes of the entire tab view in
Requests a snapshot of the entire page to be saved to the given path. Returns
true on success.

http://codereview.chromium.org/5572001/diff/79004/chrome/test/data/reliabilit...
File chrome/test/data/reliability/known_crashes.txt (left):

http://codereview.chromium.org/5572001/diff/79004/chrome/test/data/reliabilit...
chrome/test/data/reliability/known_crashes.txt:175: 
????

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:62: // Returns a string containing the raw
binary data of a PNG image of the
this comment is not accurate, like the other one. It does not return a string.
It just saves the snapshot to disk.  Please fix.

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/chrom...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:32: 
one more newline here

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:37: 
one more newline here (to match python style guide)

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:186: url =
BasicTest.GetFileURLForPath(os.path.join(DataDir(),
try running this test from scratch.  You at least need to remove BasicTest. here

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:188: url = url + "?2000,2000"
url += "?2000,2000"

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/response.cc (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.cc:84: error->SetString(kScreenKey, "");
I think it might be better to just not set kScreenKey in this case

[Joe, 2011-03-22 22:08:19]

http://codereview.chromium.org/5572001/diff/79004/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4988:
AutomationJSONReply(this, reply_message).SendSuccess(NULL);
On 2011/03/22 21:08:56, kkania wrote:
> this should be SendError("Tab has no associated RenderViewHost")

Done.

http://codereview.chromium.org/5572001/diff/79004/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:868: //   input: {
"windex": 1, "tab_index": 1, "path:"file location"}
On 2011/03/22 21:08:56, kkania wrote:
> fix the comment (it is missing a quote and should have a real file path
example:
> "path": "/Users/temp/sreenshot.png"

Done.

http://codereview.chromium.org/5572001/diff/79004/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:102: // Requests a string
containing the raw bytes of the entire tab view in
On 2011/03/22 21:08:56, kkania wrote:
> Requests a snapshot of the entire page to be saved to the given path. Returns
> true on success.

Done.

http://codereview.chromium.org/5572001/diff/79004/chrome/test/data/reliabilit...
File chrome/test/data/reliability/known_crashes.txt (left):

http://codereview.chromium.org/5572001/diff/79004/chrome/test/data/reliabilit...
chrome/test/data/reliability/known_crashes.txt:175: 
I reverted the file but i am not sure why this diff was shown on the checkin.

On 2011/03/22 21:08:56, kkania wrote:
> ????

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:62: // Returns a string containing the raw
binary data of a PNG image of the
On 2011/03/22 21:08:56, kkania wrote:
> this comment is not accurate, like the other one. It does not return a string.
> It just saves the snapshot to disk.  Please fix.

Done.

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/chrom...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:32: 
On 2011/03/22 21:08:56, kkania wrote:
> one more newline here

Done.

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:37: 
On 2011/03/22 21:08:56, kkania wrote:
> one more newline here (to match python style guide)

Done.

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:186: url =
BasicTest.GetFileURLForPath(os.path.join(DataDir(),
On 2011/03/22 21:08:56, kkania wrote:
> try running this test from scratch.  You at least need to remove BasicTest.
here

Done.

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:188: url = url + "?2000,2000"
On 2011/03/22 21:08:56, kkania wrote:
> url += "?2000,2000"

Done.

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/response.cc (right):

http://codereview.chromium.org/5572001/diff/79004/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/response.cc:84: error->SetString(kScreenKey, "");
On 2011/03/22 21:08:56, kkania wrote:
> I think it might be better to just not set kScreenKey in this case

Done.

[kkania, 2011-03-22 22:14:29]

http://codereview.chromium.org/5572001/diff/75016/chrome/test/webdriver/chrom...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/5572001/diff/75016/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:38: 
you forgot to add a newline here

http://codereview.chromium.org/5572001/diff/75016/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:195: self.assertEquals(h,
'12c0ade27e3875da3d8866f52d2fa84f')
add a newline after this too

[Joe, 2011-03-22 22:39:50]

http://codereview.chromium.org/5572001/diff/75016/chrome/test/webdriver/chrom...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/5572001/diff/75016/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:38: 
sorry about that

On 2011/03/22 22:14:29, kkania wrote:
> you forgot to add a newline here

http://codereview.chromium.org/5572001/diff/75016/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:195: self.assertEquals(h,
'12c0ade27e3875da3d8866f52d2fa84f')
On 2011/03/22 22:14:29, kkania wrote:
> add a newline after this too

Done.

[Paweł Hajdan Jr., 2011-03-23 18:01:57]

Thank you, just one more cleanup bit to do.

http://codereview.chromium.org/5572001/diff/76008/chrome/test/automation/tab_...
File chrome/test/automation/tab_proxy.cc (left):

http://codereview.chromium.org/5572001/diff/76008/chrome/test/automation/tab_...
chrome/test/automation/tab_proxy.cc:750: sender_->Send(new
AutomationMsg_CaptureEntirePageAsPNG(handle_, path,
Could you mark that IPC message as deprecated - just add a comment in automation
messages header (we can't remove it because of hardcoded message IDs in the
reference builds), and remove the handler from the automation provider?

[Joe, 2011-03-28 18:45:40]

marked the IPC message as deprecated and removed the handler

On 2011/03/23 18:01:57, Paweł Hajdan Jr. wrote:
> Thank you, just one more cleanup bit to do.
> 
>
http://codereview.chromium.org/5572001/diff/76008/chrome/test/automation/tab_...
> File chrome/test/automation/tab_proxy.cc (left):
> 
>
http://codereview.chromium.org/5572001/diff/76008/chrome/test/automation/tab_...
> chrome/test/automation/tab_proxy.cc:750: sender_->Send(new
> AutomationMsg_CaptureEntirePageAsPNG(handle_, path,
> Could you mark that IPC message as deprecated - just add a comment in
automation
> messages header (we can't remove it because of hardcoded message IDs in the
> reference builds), and remove the handler from the automation provider?

[Paweł Hajdan Jr., 2011-03-28 19:13:10]

LGTM with a comment, thank you!

http://codereview.chromium.org/5572001/diff/86001/chrome/common/automation_me...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/5572001/diff/86001/chrome/common/automation_me...
chrome/common/automation_messages_internal.h:1433: // similar functionality.
nit: Please also add TODO(phajdan.jr) to remove it.