File upload API in chromedriver

File upload API in chromedriver

Added support for file upload API in /session/:sessionId/element/:id/value. If the given element is a file upload control, the file paths are set to the file upload control using drag and drop emulation.

BUG=none
TEST=none


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

[kkania, 2011-05-23 18:43:48]

We need some tests for this too.

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1095:
!args->GetInteger("y", &y)) {
put this on the line before

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1112: if
(!paths->GetString(path_index, &path)) {
make path a string16 and GetString will do the UTF8->16 conversion for you

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1122: // Currently
DragTarget*** ignore the screen argument.
do you think it will always remain like that? do the other clients of this
function provide a real coordinate? If you think eventually we need to add the
screen coordinate, can you do that now instead of a TODO.

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1133:
(WebKit::WebDragOperationsMask)operations);
don't use c-style casting

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1135:
AutomationJSONReply(this, reply_message).SendSuccess(NULL);
we should wait till the renderer processes the drag operation.

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.h:1079: void
SetFilePathsToFileUploadControl(DictionaryValue* args,
I think this name should be more generic. How about DragAndDropFilePaths?

http://codereview.chromium.org/7055004/diff/4001/chrome/test/automation/autom...
File chrome/test/automation/automation_json_requests.cc (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/test/automation/autom...
chrome/test/automation/automation_json_requests.cc:569: const std::string& path
= paths[path_index];
merge this and the next line

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/automa...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.h:80: const std::vector<std::string>& path,
paths

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:517: if (tag_name ==
"INPUT" && type == "file") {
do case insensitive comparison

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:526: ListValue* key_list;
rename key_list

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:536:
key_list->GetString(i, &path);
handle errors

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:536:
key_list->GetString(i, &path);
what do you think about checking that the path exists?

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:541: Error* error =
session_->IsElementDisplayed(
I think this code and the following bit is in quite a few places. Can we share
it instead?

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:609: "return
(%s).apply(null, arguments);", atoms::GET_ATTRIBUTE);
can we share this code with the attribute command

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:622:
result->GetAsString(value);
return an error if not a string

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:623: delete result;
use scoped_ptr

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/webelement_commands.h (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.h:253: void
SetFilePathsToFileUploadControl(Response* const response) const;
how about return Error* for these instead of passing response

[nodchip, 2011-05-26 01:14:53]

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1095:
!args->GetInteger("y", &y)) {
On 2011/05/23 18:43:49, kkania wrote:
> put this on the line before

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1112: if
(!paths->GetString(path_index, &path)) {
On 2011/05/23 18:43:49, kkania wrote:
> make path a string16 and GetString will do the UTF8->16 conversion for you

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1122: // Currently
DragTarget*** ignore the screen argument.
I think it will always remain because it does not seem to need a screen
coordinate in drag and drop handling for WebKit. I will change the comment.

On 2011/05/23 18:43:49, kkania wrote:
> do you think it will always remain like that? do the other clients of this
> function provide a real coordinate? If you think eventually we need to add the
> screen coordinate, can you do that now instead of a TODO.

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1133:
(WebKit::WebDragOperationsMask)operations);
On 2011/05/23 18:43:49, kkania wrote:
> don't use c-style casting

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1135:
AutomationJSONReply(this, reply_message).SendSuccess(NULL);
On 2011/05/23 18:43:49, kkania wrote:
> we should wait till the renderer processes the drag operation.

Done. But we have to add additional IPC Message.

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.h:1079: void
SetFilePathsToFileUploadControl(DictionaryValue* args,
On 2011/05/23 18:43:49, kkania wrote:
> I think this name should be more generic. How about DragAndDropFilePaths?

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/automation/autom...
File chrome/test/automation/automation_json_requests.cc (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/test/automation/autom...
chrome/test/automation/automation_json_requests.cc:569: const std::string& path
= paths[path_index];
On 2011/05/23 18:43:49, kkania wrote:
> merge this and the next line

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/automa...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.h:80: const std::vector<std::string>& path,
On 2011/05/23 18:43:49, kkania wrote:
> paths

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:517: if (tag_name ==
"INPUT" && type == "file") {
On 2011/05/23 18:43:49, kkania wrote:
> do case insensitive comparison

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:526: ListValue* key_list;
On 2011/05/23 18:43:49, kkania wrote:
> rename key_list

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:536:
key_list->GetString(i, &path);
On 2011/05/23 18:43:49, kkania wrote:
> handle errors

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:536:
key_list->GetString(i, &path);
On 2011/05/23 18:43:49, kkania wrote:
> handle errors

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:541: Error* error =
session_->IsElementDisplayed(
On 2011/05/23 18:43:49, kkania wrote:
> I think this code and the following bit is in quite a few places. Can we share
> it instead?

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:609: "return
(%s).apply(null, arguments);", atoms::GET_ATTRIBUTE);
On 2011/05/23 18:43:49, kkania wrote:
> can we share this code with the attribute command

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:622:
result->GetAsString(value);
On 2011/05/23 18:43:49, kkania wrote:
> return an error if not a string

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:623: delete result;
On 2011/05/23 18:43:49, kkania wrote:
> use scoped_ptr

Done.

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/webelement_commands.h (right):

http://codereview.chromium.org/7055004/diff/4001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.h:253: void
SetFilePathsToFileUploadControl(Response* const response) const;
On 2011/05/23 18:43:49, kkania wrote:
> how about return Error* for these instead of passing response

Done.

[kkania, 2011-05-27 20:48:47]

You still need to add some tests

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

http://codereview.chromium.org/7055004/diff/12001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:1113: .SendError("path
missing or invalid");
error is a bit off here. How about:
"'paths' contains a non-string type"

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:54: scoped_ptr<ListValue>
args(new ListValue);
don't use scoped_ptr here; just allocate it on the stack

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:566: Error*
ElementValueCommand::HasAttribute(const std::string& key,
HasAttribute sounds a bit misleading to me. How about
HasAttributeWithLowerCaseValueASCII?

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:578: if
(!scoped_value.get()) {
can this ever happen? I thought ExecuteScript never returns a NULL value if
there's no error.

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:584: if
(!scoped_value->GetAsString(&actual_value)) {
could scoped_value be TYPE_NULL?

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:605: if
(!file_util::PathExists(FilePath(path))) {
you can't make a file path with a std::string on windows. Change 'std::string
path' to 'FilePath::StringType path'.

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

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.h:36: Error*
GetAttribute(const std::string& key, Value** value) const;
put this and the following function in Session instead

http://codereview.chromium.org/7055004/diff/12001/content/common/drag_messages.h
File content/common/drag_messages.h (right):

http://codereview.chromium.org/7055004/diff/12001/content/common/drag_message...
content/common/drag_messages.h:79: // Norifies the host that the renderer
finished a drop operation.
Notifies

http://codereview.chromium.org/7055004/diff/12001/content/common/drag_message...
content/common/drag_messages.h:80: IPC_MESSAGE_ROUTED0(DragHostMsg_TargetDrop)
put an ack on the end of the message name

http://codereview.chromium.org/7055004/diff/12001/content/common/notification...
File content/common/notification_type.h (right):

http://codereview.chromium.org/7055004/diff/12001/content/common/notification...
content/common/notification_type.h:484: // This notifies the observer that a
drag operation ack was received. The
specify that it is specifically a drop operation that causes this, not just any
drag operation

http://codereview.chromium.org/7055004/diff/12001/content/common/notification...
content/common/notification_type.h:486: // Note: The RenderViewHost may be
deallocated at this point.
I think the Note: doesn't apply for this case.

[nodchip, 2011-05-30 04:51:45]

I submitted a patch which enables fileupload tests to selenium.

Issue 1725 - selenium - A patch for fileupload API for Google Chrome - Browser
automation framework - Google Project Hosting
http://code.google.com/p/selenium/issues/detail?id=1725

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

http://codereview.chromium.org/7055004/diff/12001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:1113: .SendError("path
missing or invalid");
On 2011/05/27 20:48:47, kkania wrote:
> error is a bit off here. How about:
> "'paths' contains a non-string type"

Done.

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:54: scoped_ptr<ListValue>
args(new ListValue);
On 2011/05/27 20:48:47, kkania wrote:
> don't use scoped_ptr here; just allocate it on the stack

Done.

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:566: Error*
ElementValueCommand::HasAttribute(const std::string& key,
On 2011/05/27 20:48:47, kkania wrote:
> HasAttribute sounds a bit misleading to me. How about
> HasAttributeWithLowerCaseValueASCII?

Done.

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:578: if
(!scoped_value.get()) {
On 2011/05/27 20:48:47, kkania wrote:
> can this ever happen? I thought ExecuteScript never returns a NULL value if
> there's no error.

Done.

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:584: if
(!scoped_value->GetAsString(&actual_value)) {
On 2011/05/27 20:48:47, kkania wrote:
> could scoped_value be TYPE_NULL?

Done. I added a type check.

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:605: if
(!file_util::PathExists(FilePath(path))) {
On 2011/05/27 20:48:47, kkania wrote:
> you can't make a file path with a std::string on windows. Change 'std::string
> path' to 'FilePath::StringType path'.

Done.

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

http://codereview.chromium.org/7055004/diff/12001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.h:36: Error*
GetAttribute(const std::string& key, Value** value) const;
On 2011/05/27 20:48:47, kkania wrote:
> put this and the following function in Session instead

Done.

http://codereview.chromium.org/7055004/diff/12001/content/common/drag_messages.h
File content/common/drag_messages.h (right):

http://codereview.chromium.org/7055004/diff/12001/content/common/drag_message...
content/common/drag_messages.h:79: // Norifies the host that the renderer
finished a drop operation.
On 2011/05/27 20:48:47, kkania wrote:
> Notifies

Done.

http://codereview.chromium.org/7055004/diff/12001/content/common/drag_message...
content/common/drag_messages.h:80: IPC_MESSAGE_ROUTED0(DragHostMsg_TargetDrop)
On 2011/05/27 20:48:47, kkania wrote:
> put an ack on the end of the message name

Done.

http://codereview.chromium.org/7055004/diff/12001/content/common/notification...
File content/common/notification_type.h (right):

http://codereview.chromium.org/7055004/diff/12001/content/common/notification...
content/common/notification_type.h:484: // This notifies the observer that a
drag operation ack was received. The
On 2011/05/27 20:48:47, kkania wrote:
> specify that it is specifically a drop operation that causes this, not just
any
> drag operation

Done.

http://codereview.chromium.org/7055004/diff/12001/content/common/notification...
content/common/notification_type.h:486: // Note: The RenderViewHost may be
deallocated at this point.
On 2011/05/27 20:48:47, kkania wrote:
> I think the Note: doesn't apply for this case.

Done.

[kkania, 2011-05-31 18:50:29]

it looks like a few files did not get changed in the latest patch, although the
comments say 'Done.'. Did your upload finish successfully? For example,
drag_messages.h and the webelement_commands files are unchanged.

Also, we need some tests on our side. We should add some python tests either in
webdriver or in chromedriver_tests.py, your choice.

[nodchip, 2011-06-01 13:22:47]

Sorry for inconvenience. I had been failed to merge my temporary branch to my
main branch. I also added an additional unit test in chromedriver_tests.py
however it might be too simple.

On 2011/05/31 18:50:29, kkania wrote:
> it looks like a few files did not get changed in the latest patch, although
the
> comments say 'Done.'. Did your upload finish successfully? For example,
> drag_messages.h and the webelement_commands files are unchanged.
> 
> Also, we need some tests on our side. We should add some python tests either
in
> webdriver or in chromedriver_tests.py, your choice.

[kkania, 2011-06-01 16:16:41]

http://codereview.chromium.org/7055004/diff/25001/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/7055004/diff/25001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.h:1530: };
newline here

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

http://codereview.chromium.org/7055004/diff/25001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:1104: .SendError("paths
missing or invalid");
'paths'

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

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:618:
self.assertTrue(path.endswith('upload.html'))
can you also have a test for multiple files? Or is that not allowed on the
webdriver side currently?

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:526:
response->SetStatus(kSuccess);
this is the default status of the response, so you can remove this line

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:546: if
(!scoped_value->GetAsString(&actual_value)) {
I believe GetAsString will do the same check as you have above; if so, you can
remove the above check. This will not actually convert a number to a string, but
I think that is ok

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:547: return new
Error(kUnknownError, "Could not get the attribute value");
change error message to "Attribute '%s' did not have a string value",
key.c_str()

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:564: return new
Error(kBadRequest, "Invalid element in 'value' parameter");
this is a bit unclear, how about including something about the supposed path not
being a string in the message

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:568: return new
Error(kBadRequest, "Path does not exist");
put the path in the error msg

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

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:243: // Gets the attribute of the given element.
Mention that this sets |value| on no error and the caller owns it now.

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/uploa...
File chrome/test/webdriver/upload.html (right):

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/uploa...
chrome/test/webdriver/upload.html:4: <input type=file name="fileupload"
id="fileupload"
"file"?

[nodchip, 2011-06-02 07:15:53]

http://codereview.chromium.org/7055004/diff/25001/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/7055004/diff/25001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.h:1530: };
On 2011/06/01 16:16:41, kkania wrote:
> newline here

Done.

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

http://codereview.chromium.org/7055004/diff/25001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:1104: .SendError("paths
missing or invalid");
On 2011/06/01 16:16:41, kkania wrote:
> 'paths'

Done.

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

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:618:
self.assertTrue(path.endswith('upload.html'))
On 2011/06/01 16:16:41, kkania wrote:
> can you also have a test for multiple files? Or is that not allowed on the
> webdriver side currently?

Done.

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:526:
response->SetStatus(kSuccess);
On 2011/06/01 16:16:41, kkania wrote:
> this is the default status of the response, so you can remove this line

Done.

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:546: if
(!scoped_value->GetAsString(&actual_value)) {
On 2011/06/01 16:16:41, kkania wrote:
> I believe GetAsString will do the same check as you have above; if so, you can
> remove the above check. This will not actually convert a number to a string,
but
> I think that is ok

Done.

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:547: return new
Error(kUnknownError, "Could not get the attribute value");
On 2011/06/01 16:16:41, kkania wrote:
> change error message to "Attribute '%s' did not have a string value",
> key.c_str()

Done.

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:564: return new
Error(kBadRequest, "Invalid element in 'value' parameter");
On 2011/06/01 16:16:41, kkania wrote:
> this is a bit unclear, how about including something about the supposed path
not
> being a string in the message

Done.

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:568: return new
Error(kBadRequest, "Path does not exist");
On 2011/06/01 16:16:41, kkania wrote:
> put the path in the error msg

Done.

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

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:243: // Gets the attribute of the given element.
On 2011/06/01 16:16:41, kkania wrote:
> Mention that this sets |value| on no error and the caller owns it now.

Done.

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/uploa...
File chrome/test/webdriver/upload.html (right):

http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/uploa...
chrome/test/webdriver/upload.html:4: <input type=file name="fileupload"
id="fileupload"
On 2011/06/01 16:16:41, kkania wrote:
> "file"?

Done.

[nodchip, 2011-06-07 02:18:23]

Hi kkania,

I submitted the patch set last week. Could you review it?

Thanks,

2011年6月2日16:15  <hnoda@google.com>:
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/browser/automation/a...
> File chrome/browser/automation/automation_provider_observers.h (right):
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/browser/automation/a...
> chrome/browser/automation/automation_provider_observers.h:1530: };
> On 2011/06/01 16:16:41, kkania wrote:
>>
>> newline here
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/browser/automation/t...
> File chrome/browser/automation/testing_automation_provider.cc (right):
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/browser/automation/t...
> chrome/browser/automation/testing_automation_provider.cc:1104:
> .SendError("paths missing or invalid");
> On 2011/06/01 16:16:41, kkania wrote:
>>
>> 'paths'
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/chrom...
> File chrome/test/webdriver/chromedriver_tests.py (right):
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/chrom...
> chrome/test/webdriver/chromedriver_tests.py:618:
> self.assertTrue(path.endswith('upload.html'))
> On 2011/06/01 16:16:41, kkania wrote:
>>
>> can you also have a test for multiple files? Or is that not allowed on
>
> the
>>
>> webdriver side currently?
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
> File chrome/test/webdriver/commands/webelement_commands.cc (right):
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
> chrome/test/webdriver/commands/webelement_commands.cc:526:
> response->SetStatus(kSuccess);
> On 2011/06/01 16:16:41, kkania wrote:
>>
>> this is the default status of the response, so you can remove this
>
> line
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
> chrome/test/webdriver/commands/webelement_commands.cc:546: if
> (!scoped_value->GetAsString(&actual_value)) {
> On 2011/06/01 16:16:41, kkania wrote:
>>
>> I believe GetAsString will do the same check as you have above; if so,
>
> you can
>>
>> remove the above check. This will not actually convert a number to a
>
> string, but
>>
>> I think that is ok
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
> chrome/test/webdriver/commands/webelement_commands.cc:547: return new
> Error(kUnknownError, "Could not get the attribute value");
> On 2011/06/01 16:16:41, kkania wrote:
>>
>> change error message to "Attribute '%s' did not have a string value",
>> key.c_str()
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
> chrome/test/webdriver/commands/webelement_commands.cc:564: return new
> Error(kBadRequest, "Invalid element in 'value' parameter");
> On 2011/06/01 16:16:41, kkania wrote:
>>
>> this is a bit unclear, how about including something about the
>
> supposed path not
>>
>> being a string in the message
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/comma...
> chrome/test/webdriver/commands/webelement_commands.cc:568: return new
> Error(kBadRequest, "Path does not exist");
> On 2011/06/01 16:16:41, kkania wrote:
>>
>> put the path in the error msg
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/sessi...
> File chrome/test/webdriver/session.h (right):
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/sessi...
> chrome/test/webdriver/session.h:243: // Gets the attribute of the given
> element.
> On 2011/06/01 16:16:41, kkania wrote:
>>
>> Mention that this sets |value| on no error and the caller owns it now.
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/uploa...
> File chrome/test/webdriver/upload.html (right):
>
>
http://codereview.chromium.org/7055004/diff/25001/chrome/test/webdriver/uploa...
> chrome/test/webdriver/upload.html:4: <input type=file name="fileupload"
> id="fileupload"
> On 2011/06/01 16:16:41, kkania wrote:
>>
>> "file"?
>
> Done.
>
> http://codereview.chromium.org/7055004/
>



-- 
Hisayori NODA

[kkania, 2011-06-07 15:12:04]

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/chrome...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/chrome...
chrome/test/webdriver/chromedriver_tests.py:621:
self.assertTrue(path.endswith(os.path.basename(file.name)))
do you need to close/delete the temp file?

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:541:
StringPrintf("Attribute '%s' did not have a string value",
base::StringPrintf is preferred

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:560: StringPrintf("%zd-th
element in 'value' is not string",
The style guide says use %"PRIuS" for size_t
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...),
include base/format_macros.h

How about base::StringPrintf("'value' list item #%" PRIuS " is not a string", i
+ 1)

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:564: if
(!file_util::PathExists(FilePath(path))) {
Does the spec actually disallow non-existent file paths?

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:566: StringPrintf("'%s'
does not exist on the file system",
base::

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/session.h
File chrome/test/webdriver/session.h (right):

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/sessio...
chrome/test/webdriver/session.h:244: // function sets |value| and the caller
owns the ownership.
owns the -> takes

[nodchip, 2011-06-08 02:37:19]

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/chrome...
File chrome/test/webdriver/chromedriver_tests.py (right):

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/chrome...
chrome/test/webdriver/chromedriver_tests.py:621:
self.assertTrue(path.endswith(os.path.basename(file.name)))
I think we don't need to manually close/delete the temporary files. The files
will be automatically closed at the end of the script process and deleted at
that time.

10.6. tempfile — Generate temporary files and directories — Python v2.7.1
documentation http://docs.python.org/library/tempfile.html


On 2011/06/07 15:12:04, kkania wrote:
> do you need to close/delete the temp file?

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:541:
StringPrintf("Attribute '%s' did not have a string value",
On 2011/06/07 15:12:04, kkania wrote:
> base::StringPrintf is preferred

Done.

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:560: StringPrintf("%zd-th
element in 'value' is not string",
On 2011/06/07 15:12:04, kkania wrote:
> The style guide says use %"PRIuS" for size_t
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...),
> include base/format_macros.h
> 
> How about base::StringPrintf("'value' list item #%" PRIuS " is not a string",
i
> + 1)

Done.

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:564: if
(!file_util::PathExists(FilePath(path))) {
There are no spec about file uploading in the official spec. IEDriver
unofficially supports the stuff. In IEDriver, if the file does not exits, the
file dialog would show an error dialog. (I have not tested, though.) I'm not
sure what is the best behavior when the file does not exist.

On 2011/06/07 15:12:04, kkania wrote:
> Does the spec actually disallow non-existent file paths?

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/webelement_commands.cc:566: StringPrintf("'%s'
does not exist on the file system",
On 2011/06/07 15:12:04, kkania wrote:
> base::

Done.

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/session.h
File chrome/test/webdriver/session.h (right):

http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/sessio...
chrome/test/webdriver/session.h:244: // function sets |value| and the caller
owns the ownership.
On 2011/06/07 15:12:04, kkania wrote:
> owns the -> takes

Done.

[nodchip, 2011-06-09 03:08:11]

I'm sorry. I fixed a compile error in automation.cc. Could you review the code?

On 2011/06/08 02:37:19, nodchip wrote:
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/chrome...
> File chrome/test/webdriver/chromedriver_tests.py (right):
> 
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/chrome...
> chrome/test/webdriver/chromedriver_tests.py:621:
> self.assertTrue(path.endswith(os.path.basename(file.name)))
> I think we don't need to manually close/delete the temporary files. The files
> will be automatically closed at the end of the script process and deleted at
> that time.
> 
> 10.6. tempfile — Generate temporary files and directories — Python v2.7.1
> documentation http://docs.python.org/library/tempfile.html
> 
> 
> On 2011/06/07 15:12:04, kkania wrote:
> > do you need to close/delete the temp file?
> 
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
> File chrome/test/webdriver/commands/webelement_commands.cc (right):
> 
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
> chrome/test/webdriver/commands/webelement_commands.cc:541:
> StringPrintf("Attribute '%s' did not have a string value",
> On 2011/06/07 15:12:04, kkania wrote:
> > base::StringPrintf is preferred
> 
> Done.
> 
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
> chrome/test/webdriver/commands/webelement_commands.cc:560:
StringPrintf("%zd-th
> element in 'value' is not string",
> On 2011/06/07 15:12:04, kkania wrote:
> > The style guide says use %"PRIuS" for size_t
> >
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...),
> > include base/format_macros.h
> > 
> > How about base::StringPrintf("'value' list item #%" PRIuS " is not a
string",
> i
> > + 1)
> 
> Done.
> 
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
> chrome/test/webdriver/commands/webelement_commands.cc:564: if
> (!file_util::PathExists(FilePath(path))) {
> There are no spec about file uploading in the official spec. IEDriver
> unofficially supports the stuff. In IEDriver, if the file does not exits, the
> file dialog would show an error dialog. (I have not tested, though.) I'm not
> sure what is the best behavior when the file does not exist.
> 
> On 2011/06/07 15:12:04, kkania wrote:
> > Does the spec actually disallow non-existent file paths?
> 
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
> chrome/test/webdriver/commands/webelement_commands.cc:566: StringPrintf("'%s'
> does not exist on the file system",
> On 2011/06/07 15:12:04, kkania wrote:
> > base::
> 
> Done.
> 
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/session.h
> File chrome/test/webdriver/session.h (right):
> 
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/sessio...
> chrome/test/webdriver/session.h:244: // function sets |value| and the caller
> owns the ownership.
> On 2011/06/07 15:12:04, kkania wrote:
> > owns the -> takes
> 
> Done.

[nodchip, 2011-06-13 03:07:13]

Hi,

Could you review my code? I'm waiting for your reply.

Thanks,

2011年6月9日12:08  <hnoda@google.com>:
> I'm sorry. I fixed a compile error in automation.cc. Could you review the
> code?
>
> On 2011/06/08 02:37:19, nodchip wrote:
>
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/chrome...
>>
>> File chrome/test/webdriver/chromedriver_tests.py (right):
>
>
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/chrome...
>>
>> chrome/test/webdriver/chromedriver_tests.py:621:
>> self.assertTrue(path.endswith(os.path.basename(file.name)))
>> I think we don't need to manually close/delete the temporary files. The
>> files
>> will be automatically closed at the end of the script process and deleted
>> at
>> that time.
>
>> 10.6. tempfile ― Generate temporary files and directories ― Python v2.7.1
>> documentation http://docs.python.org/library/tempfile.html
>
>
>> On 2011/06/07 15:12:04, kkania wrote:
>> > do you need to close/delete the temp file?
>
>
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
>>
>> File chrome/test/webdriver/commands/webelement_commands.cc (right):
>
>
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
>>
>> chrome/test/webdriver/commands/webelement_commands.cc:541:
>> StringPrintf("Attribute '%s' did not have a string value",
>> On 2011/06/07 15:12:04, kkania wrote:
>> > base::StringPrintf is preferred
>
>> Done.
>
>
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
>>
>> chrome/test/webdriver/commands/webelement_commands.cc:560:
>
> StringPrintf("%zd-th
>>
>> element in 'value' is not string",
>> On 2011/06/07 15:12:04, kkania wrote:
>> > The style guide says use %"PRIuS" for size_t
>> >
>
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...),
>>
>> > include base/format_macros.h
>> >
>> > How about base::StringPrintf("'value' list item #%" PRIuS " is not a
>
> string",
>>
>> i
>> > + 1)
>
>> Done.
>
>
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
>>
>> chrome/test/webdriver/commands/webelement_commands.cc:564: if
>> (!file_util::PathExists(FilePath(path))) {
>> There are no spec about file uploading in the official spec. IEDriver
>> unofficially supports the stuff. In IEDriver, if the file does not exits,
>> the
>> file dialog would show an error dialog. (I have not tested, though.) I'm
>> not
>> sure what is the best behavior when the file does not exist.
>
>> On 2011/06/07 15:12:04, kkania wrote:
>> > Does the spec actually disallow non-existent file paths?
>
>
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
>>
>> chrome/test/webdriver/commands/webelement_commands.cc:566:
>> StringPrintf("'%s'
>> does not exist on the file system",
>> On 2011/06/07 15:12:04, kkania wrote:
>> > base::
>
>> Done.
>
>
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/session.h
>>
>> File chrome/test/webdriver/session.h (right):
>
>
>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/sessio...
>>
>> chrome/test/webdriver/session.h:244: // function sets |value| and the
>> caller
>> owns the ownership.
>> On 2011/06/07 15:12:04, kkania wrote:
>> > owns the -> takes
>
>> Done.
>
>
>
> http://codereview.chromium.org/7055004/
>



-- 
Hisayori NODA

[nodchip, 2011-06-15 01:28:09]

HI,

This is a reminder. Could you review my code?

Thanks,

2011年6月13日12:07 Hisayori Noda <hnoda@google.com>:
> Hi,
>
> Could you review my code? I'm waiting for your reply.
>
> Thanks,
>
> 2011年6月9日12:08  <hnoda@google.com>:
>> I'm sorry. I fixed a compile error in automation.cc. Could you review the
>> code?
>>
>> On 2011/06/08 02:37:19, nodchip wrote:
>>
>>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/chrome...
>>>
>>> File chrome/test/webdriver/chromedriver_tests.py (right):
>>
>>
>>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/chrome...
>>>
>>> chrome/test/webdriver/chromedriver_tests.py:621:
>>> self.assertTrue(path.endswith(os.path.basename(file.name)))
>>> I think we don't need to manually close/delete the temporary files. The
>>> files
>>> will be automatically closed at the end of the script process and deleted
>>> at
>>> that time.
>>
>>> 10.6. tempfile ― Generate temporary files and directories ― Python v2.7.1
>>> documentation http://docs.python.org/library/tempfile.html
>>
>>
>>> On 2011/06/07 15:12:04, kkania wrote:
>>> > do you need to close/delete the temp file?
>>
>>
>>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
>>>
>>> File chrome/test/webdriver/commands/webelement_commands.cc (right):
>>
>>
>>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
>>>
>>> chrome/test/webdriver/commands/webelement_commands.cc:541:
>>> StringPrintf("Attribute '%s' did not have a string value",
>>> On 2011/06/07 15:12:04, kkania wrote:
>>> > base::StringPrintf is preferred
>>
>>> Done.
>>
>>
>>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
>>>
>>> chrome/test/webdriver/commands/webelement_commands.cc:560:
>>
>> StringPrintf("%zd-th
>>>
>>> element in 'value' is not string",
>>> On 2011/06/07 15:12:04, kkania wrote:
>>> > The style guide says use %"PRIuS" for size_t
>>> >
>>
>>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...),
>>>
>>> > include base/format_macros.h
>>> >
>>> > How about base::StringPrintf("'value' list item #%" PRIuS " is not a
>>
>> string",
>>>
>>> i
>>> > + 1)
>>
>>> Done.
>>
>>
>>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
>>>
>>> chrome/test/webdriver/commands/webelement_commands.cc:564: if
>>> (!file_util::PathExists(FilePath(path))) {
>>> There are no spec about file uploading in the official spec. IEDriver
>>> unofficially supports the stuff. In IEDriver, if the file does not exits,
>>> the
>>> file dialog would show an error dialog. (I have not tested, though.) I'm
>>> not
>>> sure what is the best behavior when the file does not exist.
>>
>>> On 2011/06/07 15:12:04, kkania wrote:
>>> > Does the spec actually disallow non-existent file paths?
>>
>>
>>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/comman...
>>>
>>> chrome/test/webdriver/commands/webelement_commands.cc:566:
>>> StringPrintf("'%s'
>>> does not exist on the file system",
>>> On 2011/06/07 15:12:04, kkania wrote:
>>> > base::
>>
>>> Done.
>>
>>
>>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/session.h
>>>
>>> File chrome/test/webdriver/session.h (right):
>>
>>
>>
http://codereview.chromium.org/7055004/diff/9003/chrome/test/webdriver/sessio...
>>>
>>> chrome/test/webdriver/session.h:244: // function sets |value| and the
>>> caller
>>> owns the ownership.
>>> On 2011/06/07 15:12:04, kkania wrote:
>>> > owns the -> takes
>>
>>> Done.
>>
>>
>>
>> http://codereview.chromium.org/7055004/
>>
>
>
>
> --
> Hisayori NODA
>



-- 
Hisayori NODA

[kkania, 2011-06-15 02:33:12]

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

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:80: const std::vector<std::string>& paths,
one arg per line please

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

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:640:
self.assertTrue(os.path.basename(path) in filenames)
I think this test should actually check that the multiple files are selected;

you should be able to check the 'files' attribute on the input element to verify

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:558: for (size_t i = 0; i
< path_list->GetSize(); ++i) {
if there are multiple paths and the file input element does not allow multiple
selections, I think we should return an error.
Maybe consider adding a test case for this scenario too

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/uploa...
File chrome/test/webdriver/upload.html (right):

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/uploa...
chrome/test/webdriver/upload.html:5: style="border: 1px black solid;" />
I think you should add another input for doing multiple file uploads too

[nodchip, 2011-06-15 07:58:27]

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

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:80: const std::vector<std::string>& paths,
On 2011/06/15 02:33:12, kkania wrote:
> one arg per line please

Done.

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

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:640:
self.assertTrue(os.path.basename(path) in filenames)
On 2011/06/15 02:33:12, kkania wrote:
> I think this test should actually check that the multiple files are selected;
> 
> you should be able to check the 'files' attribute on the input element to
verify

I changed the test but it does not pass. I tried to get the 'files' attribute of
the upload control element with attribute 'multiple' but NULL was returned. I
checked ElementAttributeCommand and confirmed session_->GetAttribute() returned
NULL. Does the current chromedriver support to retrive 'files' attribute?

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:558: for (size_t i = 0; i
< path_list->GetSize(); ++i) {
On 2011/06/15 02:33:12, kkania wrote:
> if there are multiple paths and the file input element does not allow multiple
> selections, I think we should return an error.
> Maybe consider adding a test case for this scenario too

Done.

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/uploa...
File chrome/test/webdriver/upload.html (right):

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/uploa...
chrome/test/webdriver/upload.html:5: style="border: 1px black solid;" />
On 2011/06/15 02:33:12, kkania wrote:
> I think you should add another input for doing multiple file uploads too

Done.

[kkania, 2011-06-15 16:53:04]

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

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:640:
self.assertTrue(os.path.basename(path) in filenames)
'files' is actually a property, not an attribute. However, get_attribute returns
the property value, unless the value is an object or not defined, in which case
it returns the attribute. See webdriver.element.getAttribute in
http://code.google.com/p/selenium/source/browse/trunk/javascript/webdriver-at...

You probably will have to use execute_script.
You should check that files.length == 4, and then that each file name is
correct.

[nodchip, 2011-06-16 04:49:59]

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

http://codereview.chromium.org/7055004/diff/39001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:640:
self.assertTrue(os.path.basename(path) in filenames)
On 2011/06/15 16:53:04, kkania wrote:
> 'files' is actually a property, not an attribute. However, get_attribute
returns
> the property value, unless the value is an object or not defined, in which
case
> it returns the attribute. See webdriver.element.getAttribute in
>
http://code.google.com/p/selenium/source/browse/trunk/javascript/webdriver-at...
> 
> You probably will have to use execute_script.
> You should check that files.length == 4, and then that each file name is
> correct.

Done.

[kkania, 2011-06-16 15:48:59]

LGTM after changes and changing you description to BUG=none, TEST=none

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

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:655: files = list()
looks like this is never used; remove?

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:656: filepaths = list()
list() -> [], everywhere used

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:668: path = fileupload_single.value
value is supposed to be deprectated in the next release; how about assert that
the 'files' property has length 0 instead? Or I would be perfectly fine with not
doing any checks here

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:675: files = list()
looks like this is never used; remove?

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:690: path =
fileupload_multi.value.replace('\\', '//')
how about remove this and the next line (since it is covered later)

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:693: filesOnElement =
self._driver.execute_script(
files_on_element

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:699: self.assertTrue(name in
filenames)
remove name and use f['name'] directly

[nodchip, 2011-06-27 04:05:53]

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

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:655: files = list()
On 2011/06/16 15:48:59, kkania wrote:
> looks like this is never used; remove?

I think we need to hold file objects because the files will be deleted on GC and
cause error on send_keys().

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:656: filepaths = list()
On 2011/06/16 15:48:59, kkania wrote:
> list() -> [], everywhere used

Done.

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:668: path = fileupload_single.value
On 2011/06/16 15:48:59, kkania wrote:
> value is supposed to be deprectated in the next release; how about assert that
> the 'files' property has length 0 instead? Or I would be perfectly fine with
not
> doing any checks here

Done.

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:675: files = list()
On 2011/06/16 15:48:59, kkania wrote:
> looks like this is never used; remove?

Please see above.

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:690: path =
fileupload_multi.value.replace('\\', '//')
On 2011/06/16 15:48:59, kkania wrote:
> how about remove this and the next line (since it is covered later)

Done.

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:693: filesOnElement =
self._driver.execute_script(
On 2011/06/16 15:48:59, kkania wrote:
> files_on_element

Done.

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
chrome/test/webdriver/chromedriver_tests.py:699: self.assertTrue(name in
filenames)
On 2011/06/16 15:48:59, kkania wrote:
> remove name and use f['name'] directly

Done.

[nodchip, 2011-06-27 04:22:57]

I'm very sorry for late response. I had been forgotten to publish the comments.

2011年6月27日13:05  <hnoda@google.com>:
>
>
http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
> File chrome/test/webdriver/chromedriver_tests.py (right):
>
>
http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
> chrome/test/webdriver/chromedriver_tests.py:655: files = list()
> On 2011/06/16 15:48:59, kkania wrote:
>>
>> looks like this is never used; remove?
>
> I think we need to hold file objects because the files will be deleted
> on GC and cause error on send_keys().
>
>
http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
> chrome/test/webdriver/chromedriver_tests.py:656: filepaths = list()
> On 2011/06/16 15:48:59, kkania wrote:
>>
>> list() -> [], everywhere used
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
> chrome/test/webdriver/chromedriver_tests.py:668: path =
> fileupload_single.value
> On 2011/06/16 15:48:59, kkania wrote:
>>
>> value is supposed to be deprectated in the next release; how about
>
> assert that
>>
>> the 'files' property has length 0 instead? Or I would be perfectly
>
> fine with not
>>
>> doing any checks here
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
> chrome/test/webdriver/chromedriver_tests.py:675: files = list()
> On 2011/06/16 15:48:59, kkania wrote:
>>
>> looks like this is never used; remove?
>
> Please see above.
>
>
http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
> chrome/test/webdriver/chromedriver_tests.py:690: path =
> fileupload_multi.value.replace('\\', '//')
> On 2011/06/16 15:48:59, kkania wrote:
>>
>> how about remove this and the next line (since it is covered later)
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
> chrome/test/webdriver/chromedriver_tests.py:693: filesOnElement =
> self._driver.execute_script(
> On 2011/06/16 15:48:59, kkania wrote:
>>
>> files_on_element
>
> Done.
>
>
http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chrom...
> chrome/test/webdriver/chromedriver_tests.py:699: self.assertTrue(name in
> filenames)
> On 2011/06/16 15:48:59, kkania wrote:
>>
>> remove name and use f['name'] directly
>
> Done.
>
> http://codereview.chromium.org/7055004/
>



-- 
Hisayori NODA

[commit-bot: I haz the power, 2011-06-27 22:18:49]

Presubmit check for 7055004-49005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/common/notification_type.h,content/renderer/render_view.cc,content/browser/renderer_host/render_view_host.cc,content/browser/renderer_host/render_view_host.h,content/common/drag_messages.h

Presubmit checks took 1.4s to calculate.

[kkania, 2011-06-27 23:16:08]

adding jam for content/

[jam, 2011-06-27 23:36:30]

content lgtm

[kkania, 2011-06-28 00:58:38]

http://codereview.chromium.org/7055004/diff/49005/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/49005/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:586:
paths.push_back(path);
whoops, this won't work because path is a FilePath::StringType. I think the
nicest way to fix this would be to change the vector of strings to a vector of
file paths.

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

http://codereview.chromium.org/7055004/diff/49005/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:1288: Error* Session::GetAttribute(const
WebElementId& element,
these should be in the same order as in the .h file

[commit-bot: I haz the power, 2011-06-28 01:25:23]

Try job failure for 7055004-49005 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[nodchip, 2011-06-28 02:30:49]

A member of Tokyo Chrome team (haraken@) confirmed that he can build
successfully.

http://codereview.chromium.org/7055004/diff/49005/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/webelement_commands.cc (right):

http://codereview.chromium.org/7055004/diff/49005/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webelement_commands.cc:586:
paths.push_back(path);
On 2011/06/28 00:58:38, kkania wrote:
> whoops, this won't work because path is a FilePath::StringType. I think the
> nicest way to fix this would be to change the vector of strings to a vector of
> file paths.

Done.

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

http://codereview.chromium.org/7055004/diff/49005/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:1288: Error* Session::GetAttribute(const
WebElementId& element,
On 2011/06/28 00:58:38, kkania wrote:
> these should be in the same order as in the .h file

Done.

[commit-bot: I haz the power, 2011-06-28 17:12:00]

Change committed as 90785