Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(288)

Issue 268573003: [fsp] Cleanup browser tests. (Closed)

Created:
6 years, 7 months ago by mtomasz
Modified:
6 years, 7 months ago
Reviewers:
hirono
CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org
Visibility:
Public.

Description

[fsp] Cleanup browser tests. This patch adds jsdocs, and missing chrome.test.callbackPass invocations. Moreover, it makes the api methods return true in case of error. It was wrong to cause an immediate error together with returning DOMError. The immediate error should be only returned in case of wrong argument, which is checked by the EXTENSION_FUNCTION_VALIDATE macro. TEST=browser_tests: *FileSystemProvider* BUG=248427 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267508

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fixed. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -18 lines) Patch
M chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc View 7 chunks +7 lines, -7 lines 0 comments Download
M chrome/test/data/extensions/api_test/file_system_provider/mount/test.js View 1 7 chunks +14 lines, -5 lines 0 comments Download
M chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js View 8 chunks +22 lines, -6 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
mtomasz
@hirono: PTAL. Thanks!
6 years, 7 months ago (2014-05-01 05:05:54 UTC) #1
hirono
lgtm with a nit. https://codereview.chromium.org/268573003/diff/1/chrome/test/data/extensions/api_test/file_system_provider/mount/test.js File chrome/test/data/extensions/api_test/file_system_provider/mount/test.js (right): https://codereview.chromium.org/268573003/diff/1/chrome/test/data/extensions/api_test/file_system_provider/mount/test.js#newcode18 chrome/test/data/extensions/api_test/file_system_provider/mount/test.js:18: chrome.test.assertTrue(fileSystemId == 1); Please use ...
6 years, 7 months ago (2014-05-01 05:16:18 UTC) #2
mtomasz
https://codereview.chromium.org/268573003/diff/1/chrome/test/data/extensions/api_test/file_system_provider/mount/test.js File chrome/test/data/extensions/api_test/file_system_provider/mount/test.js (right): https://codereview.chromium.org/268573003/diff/1/chrome/test/data/extensions/api_test/file_system_provider/mount/test.js#newcode18 chrome/test/data/extensions/api_test/file_system_provider/mount/test.js:18: chrome.test.assertTrue(fileSystemId == 1); On 2014/05/01 05:16:18, hirono wrote: > ...
6 years, 7 months ago (2014-05-01 05:19:24 UTC) #3
mtomasz
The CQ bit was checked by mtomasz@chromium.org
6 years, 7 months ago (2014-05-01 05:19:26 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/268573003/20001
6 years, 7 months ago (2014-05-01 05:21:08 UTC) #5
commit-bot: I haz the power
6 years, 7 months ago (2014-05-01 10:41:28 UTC) #6
Message was sent while issue was closed.
Change committed as 267508

Powered by Google App Engine
This is Rietveld 408576698