Re: File upload API in chromedriver (issue7055004)

[hn...@google.com]

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

http://codereview.chromium.org/7055004/diff/47001/chrome/test/webdriver/chromedriver_tests.py#newcode655
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/chromedriver_tests.py#newcode656
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/chromedriver_tests.py#newcode668
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/chromedriver_tests.py#newcode675
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/chromedriver_tests.py#newcode690
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/chromedriver_tests.py#newcode693
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/chromedriver_tests.py#newcode699
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]

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

2011年6月27日13:05 <hn...@google.com>:

--
Hisayori NODA

[commi...@chromium.org]

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.

http://codereview.chromium.org/7055004/

[kka...@chromium.org]

adding jam for content/

http://codereview.chromium.org/7055004/

[j...@chromium.org]

content lgtm

http://codereview.chromium.org/7055004/

[commi...@chromium.org]

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=39608

http://codereview.chromium.org/7055004/

[hn...@google.com]

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

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

http://codereview.chromium.org/7055004/diff/49005/chrome/test/webdriver/commands/webelement_commands.cc#newcode586
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/session.cc
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/7055004/diff/49005/chrome/test/webdriver/session.cc#newcode1288
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.

http://codereview.chromium.org/7055004/

[commi...@chromium.org]

Change committed as 90785

http://codereview.chromium.org/7055004/