drive: Stop using FilePathWatcher from FileManagerBrowserTest

drive: Stop using FilePathWatcher from FileManagerBrowserTest

JS test code should wait for the copy/delete completion and check the results.
Remove unused code in file_manager_browsertest.cc

BUG=None
TEST=browser_tests --gtest_filter="*FileManager*"
R=mtomasz@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202515

[mtomasz, 2013-05-28 02:33:41]

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js
(right):

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:129:
* @param {Array.<string>} An entry in a flie list.
nit: flie -> file

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:253:
function isCopyPresent(filename, fileList) {
Inline functions are not recommended in Files.app.
Should be: var isCopyPresent = function(filename, fileList).
I know we have some in test_util.js, but we shouldn't introduce new.

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:255:
for (var i = 0; i < fileList.length; ++i) {
In Files.app i++ is much more common than ++i.
++i: 12 times
i++: 131 times.
Let's be consistent.

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:265:
new RegExp('^' + base_name + '.+' + extension + '$');
in JS camel case: baseName instead of base_name.

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:280:
chrome.test.assertTrue(result);
How about calling waitForFileListChange and then assertTrue(isCopyPresent)? This
would simplify and make if faster in case of failures.

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:300:
setupAndWaitUntilReady(path, function(appId, files) {
@hirono introduced a great pattern for complex multi-step tests. Please take a
look at copyBetweenVolumes. Can we use it here?

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:316:
function waitForDeleteCompletion() {
I think we can use waitForFileListChange here for simpler code.

[hashimoto, 2013-05-28 05:08:11]

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js
(right):

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:129:
* @param {Array.<string>} An entry in a flie list.
On 2013/05/28 02:33:41, mtomasz wrote:
> nit: flie -> file

Done for all three occurrences.

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:253:
function isCopyPresent(filename, fileList) {
On 2013/05/28 02:33:41, mtomasz wrote:
> Inline functions are not recommended in Files.app.
> Should be: var isCopyPresent = function(filename, fileList).
> I know we have some in test_util.js, but we shouldn't introduce new.

Done.

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:255:
for (var i = 0; i < fileList.length; ++i) {
On 2013/05/28 02:33:41, mtomasz wrote:
> In Files.app i++ is much more common than ++i.
> ++i: 12 times
> i++: 131 times.
> Let's be consistent.

Done for all three.

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:265:
new RegExp('^' + base_name + '.+' + extension + '$');
On 2013/05/28 02:33:41, mtomasz wrote:
> in JS camel case: baseName instead of base_name.

Done.

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:280:
chrome.test.assertTrue(result);
On 2013/05/28 02:33:41, mtomasz wrote:
> How about calling waitForFileListChange and then assertTrue(isCopyPresent)?
This
> would simplify and make if faster in case of failures.

Done.

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:300:
setupAndWaitUntilReady(path, function(appId, files) {
On 2013/05/28 02:33:41, mtomasz wrote:
> @hirono introduced a great pattern for complex multi-step tests. Please take a
> look at copyBetweenVolumes. Can we use it here?

Done.

https://codereview.chromium.org/16097003/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:316:
function waitForDeleteCompletion() {
On 2013/05/28 02:33:41, mtomasz wrote:
> I think we can use waitForFileListChange here for simpler code.

Done.

[mtomasz, 2013-05-28 05:10:38]

lgtm with minor nits

https://codereview.chromium.org/16097003/diff/20001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js
(right):

https://codereview.chromium.org/16097003/diff/20001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:332:
// Reply to dialog.
nit: dialog -> the dialog

https://codereview.chromium.org/16097003/diff/20001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:337:
// Wait for file list change.
nit: Wait for -> Wait for a

[hashimoto, 2013-05-28 05:12:44]

https://codereview.chromium.org/16097003/diff/20001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js
(right):

https://codereview.chromium.org/16097003/diff/20001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:332:
// Reply to dialog.
On 2013/05/28 05:10:38, mtomasz wrote:
> nit: dialog -> the dialog

Done.

https://codereview.chromium.org/16097003/diff/20001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:337:
// Wait for file list change.
On 2013/05/28 05:10:38, mtomasz wrote:
> nit: Wait for -> Wait for a

Done.

[commit-bot: I haz the power, 2013-05-28 05:12:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/16097003/26001

[hashimoto, 2013-05-28 07:59:40]

Committed patchset #4 manually as r202515 (presubmit successful).