Add browser_tests for FileAPI with Quota.

Add browser_tests for FileAPI with Quota, and remove unlimited access for file:/// when allow_file_access_from_files.


BUG=95120
TEST=browser_tests:FileSystemBrowserTest*


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

[Dai Mikurube (NOT FULLTIME), 2011-08-24 06:18:48]

It's the first step, and not ready for review.

Just uploaded to share the status.

[Dai Mikurube (NOT FULLTIME), 2011-08-26 05:17:10]

I guess this change includes minimal browser_tests for FileAPI/Quota though it
might require some more tests such as for Copy/Move/Write.

Could you PTAL at this change?

[kinuko, 2011-08-29 08:55:01]

Thanks for working on this.

http://codereview.chromium.org/7715024/diff/7001/chrome/test/data/fileapi/com...
File chrome/test/data/fileapi/common.js (right):

http://codereview.chromium.org/7715024/diff/7001/chrome/test/data/fileapi/com...
chrome/test/data/fileapi/common.js:69: }
These unexpected{Abort,Success,Complete}Callbacks do not seem to be referenced?

http://codereview.chromium.org/7715024/diff/7001/chrome/test/data/fileapi/com...
chrome/test/data/fileapi/common.js:76: function deleteAllObjectStores(db)
Not for fileapi?

http://codereview.chromium.org/7715024/diff/7001/chrome/test/data/fileapi/com...
chrome/test/data/fileapi/common.js:83: // The following functions are based on
I think in the new fileapi tests we're not using these shouldBe methods; am I
correct?  If so let's remove these replicated methods.

http://codereview.chromium.org/7715024/diff/7001/content/browser/file_system/...
File content/browser/file_system/file_system_browsertest.cc (right):

http://codereview.chromium.org/7715024/diff/7001/content/browser/file_system/...
content/browser/file_system/file_system_browsertest.cc:27: virtual void
SetUpCommandLine(CommandLine* command_line) {
nit: OVERRIDE

http://codereview.chromium.org/7715024/diff/7001/content/browser/file_system/...
content/browser/file_system/file_system_browsertest.cc:61: * 1024 *
QuotaManager::kPerHostTemporaryPortion;
super-tiny-nit: usually we don't start lines with operators

http://codereview.chromium.org/7715024/diff/7001/content/browser/file_system/...
content/browser/file_system/file_system_browsertest.cc:91:
ASSERT_TRUE(helper->Run());
Hmm... Maybe we could factor out this quota-related common part somewhere? 
Worth putting a TODO?

[Dai Mikurube (NOT FULLTIME), 2011-08-30 08:07:55]

Thank you for reviewing, Kinuko-san.

http://codereview.chromium.org/7715024/diff/7001/chrome/test/data/fileapi/com...
File chrome/test/data/fileapi/common.js (right):

http://codereview.chromium.org/7715024/diff/7001/chrome/test/data/fileapi/com...
chrome/test/data/fileapi/common.js:69: }
On 2011/08/29 08:55:01, kinuko wrote:
> These unexpected{Abort,Success,Complete}Callbacks do not seem to be
referenced?

Thanks.  I forgot to remove them.  I was using code from IndexedDB's
browser_tests as the first step.

http://codereview.chromium.org/7715024/diff/7001/chrome/test/data/fileapi/com...
chrome/test/data/fileapi/common.js:76: function deleteAllObjectStores(db)
On 2011/08/29 08:55:01, kinuko wrote:
> Not for fileapi?

Done.

http://codereview.chromium.org/7715024/diff/7001/chrome/test/data/fileapi/com...
chrome/test/data/fileapi/common.js:83: // The following functions are based on
On 2011/08/29 08:55:01, kinuko wrote:
> I think in the new fileapi tests we're not using these shouldBe methods; am I
> correct?  If so let's remove these replicated methods.

Done.

http://codereview.chromium.org/7715024/diff/7001/content/browser/file_system/...
File content/browser/file_system/file_system_browsertest.cc (right):

http://codereview.chromium.org/7715024/diff/7001/content/browser/file_system/...
content/browser/file_system/file_system_browsertest.cc:27: virtual void
SetUpCommandLine(CommandLine* command_line) {
On 2011/08/29 08:55:01, kinuko wrote:
> nit: OVERRIDE

Done.

http://codereview.chromium.org/7715024/diff/7001/content/browser/file_system/...
content/browser/file_system/file_system_browsertest.cc:61: * 1024 *
QuotaManager::kPerHostTemporaryPortion;
On 2011/08/29 08:55:01, kinuko wrote:
> super-tiny-nit: usually we don't start lines with operators

Done.

http://codereview.chromium.org/7715024/diff/7001/content/browser/file_system/...
content/browser/file_system/file_system_browsertest.cc:91:
ASSERT_TRUE(helper->Run());
On 2011/08/29 08:55:01, kinuko wrote:
> Hmm... Maybe we could factor out this quota-related common part somewhere? 
> Worth putting a TODO?

Do you mean factoring out for other quota-enabled storage APIs like IndexedDB,
or other tests in this file?  I don't understand "this quota-related common
part" and "somewhere" here.

[kinuko, 2011-08-30 12:12:04]

Thanks.  Some more comments...

http://codereview.chromium.org/7715024/diff/7001/content/browser/file_system/...
File content/browser/file_system/file_system_browsertest.cc (right):

http://codereview.chromium.org/7715024/diff/7001/content/browser/file_system/...
content/browser/file_system/file_system_browsertest.cc:91:
ASSERT_TRUE(helper->Run());
On 2011/08/30 08:07:55, Dai Mikurube wrote:
> On 2011/08/29 08:55:01, kinuko wrote:
> > Hmm... Maybe we could factor out this quota-related common part somewhere? 
> > Worth putting a TODO?
> 
> Do you mean factoring out for other quota-enabled storage APIs like IndexedDB,

Yes this was what I thought.  Since this file and IndexedDB's browser test seem
to have some code in common.

> or other tests in this file?  I don't understand "this quota-related common
> part" and "somewhere" here.

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/co...
File chrome/test/data/fileapi/common.js (right):

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/co...
chrome/test/data/fileapi/common.js:29: return "" +
document.getElementById('status').innerHTML;
"" -> '' for consistency?

is this '"" +' placed to make sure we get a string?

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/co...
chrome/test/data/fileapi/common.js:35: msg = 'QUOTA_EXCEEDED_ERR';
looks like we can simply do this?

return 'QUOTA_EXCEEDED_ERR';

otherwise I think we should declare 'var msg' at the top of this method.

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/co...
chrome/test/data/fileapi/common.js:52: };
nit: semicolon is not needed here?

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/cr...
File chrome/test/data/fileapi/create_test.js (right):

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/cr...
chrome/test/data/fileapi/create_test.js:8: done();
I guess we can simply pass 'done' here.

fs.root.getFile('foo', { ... }, done, function(e) { ... });

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/cr...
chrome/test/data/fileapi/create_test.js:9: }, function(e) { fail("Open"); } );
nit: " -> '

should we also show the result of fileErrorToString(e)?

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
File chrome/test/data/fileapi/quota_test.js (right):

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:21: }, function(e) { fail("Open"); } );
is it intentional not to use unexpectedErrorCallback here?

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:24: function requestSuccess(fs) {
nit: requestSuccess -> requestFileSystemSuccess for the sake of clarity?

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:36: fail("No error by quota");
'Unexpectedly succeeded' ?

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:46: origReturnedQuota = returnedQuota =
quota;
returned{Usage,Quota} variables are not used?

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:61: window.jsTestIsAsync = true;
Not used/referred?
(This variable is usually referred by a helper script in LayoutTests.)

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:67: debug("This test requires
window.webkitStorageInfo.");
nit: " -> '

[Dai Mikurube (NOT FULLTIME), 2011-08-30 12:58:30]

Thanks for reviewing.

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/cr...
File chrome/test/data/fileapi/create_test.js (right):

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/cr...
chrome/test/data/fileapi/create_test.js:8: done();
On 2011/08/30 12:12:04, kinuko wrote:
> I guess we can simply pass 'done' here.
> 
> fs.root.getFile('foo', { ... }, done, function(e) { ... });

Done.

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/cr...
chrome/test/data/fileapi/create_test.js:9: }, function(e) { fail("Open"); } );
On 2011/08/30 12:12:04, kinuko wrote:
> nit: " -> '
> 
> should we also show the result of fileErrorToString(e)?

Done.

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
File chrome/test/data/fileapi/quota_test.js (right):

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:21: }, function(e) { fail("Open"); } );
On 2011/08/30 12:12:04, kinuko wrote:
> is it intentional not to use unexpectedErrorCallback here?

It will be useful to show where the test fails.  (createWriter uses
unexectedErrorCallback since I guess createWriter hardly fails.)

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:24: function requestSuccess(fs) {
On 2011/08/30 12:12:04, kinuko wrote:
> nit: requestSuccess -> requestFileSystemSuccess for the sake of clarity?

Done.

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:36: fail("No error by quota");
On 2011/08/30 12:12:04, kinuko wrote:
> 'Unexpectedly succeeded' ?

Done.

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:46: origReturnedQuota = returnedQuota =
quota;
On 2011/08/30 12:12:04, kinuko wrote:
> returned{Usage,Quota} variables are not used?

Done.

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:61: window.jsTestIsAsync = true;
On 2011/08/30 12:12:04, kinuko wrote:
> Not used/referred?
> (This variable is usually referred by a helper script in LayoutTests.)

Done.

http://codereview.chromium.org/7715024/diff/12001/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:67: debug("This test requires
window.webkitStorageInfo.");
On 2011/08/30 12:12:04, kinuko wrote:
> nit: " -> '

Done.

[kinuko, 2011-09-01 05:11:53]

lgtm

http://codereview.chromium.org/7715024/diff/18002/chrome/test/data/fileapi/qu...
File chrome/test/data/fileapi/quota_test.js (right):

http://codereview.chromium.org/7715024/diff/18002/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:38: }
nit: semicolon after '}'

[kinuko, 2011-09-01 05:53:30]

http://codereview.chromium.org/7715024/diff/18002/content/browser/file_system...
File content/browser/file_system/file_system_browsertest.cc (right):

http://codereview.chromium.org/7715024/diff/18002/content/browser/file_system...
content/browser/file_system/file_system_browsertest.cc:73: quota::StorageType,
int64>& params) {
nit: weird indentation

[Dai Mikurube (NOT FULLTIME), 2011-09-01 06:54:09]

Thanks.  Fixed it, rebased and submitted to try bots.

http://codereview.chromium.org/7715024/diff/18002/chrome/test/data/fileapi/qu...
File chrome/test/data/fileapi/quota_test.js (right):

http://codereview.chromium.org/7715024/diff/18002/chrome/test/data/fileapi/qu...
chrome/test/data/fileapi/quota_test.js:38: }
On 2011/09/01 05:11:53, kinuko wrote:
> nit: semicolon after '}'

Done.

http://codereview.chromium.org/7715024/diff/18002/content/browser/file_system...
File content/browser/file_system/file_system_browsertest.cc (right):

http://codereview.chromium.org/7715024/diff/18002/content/browser/file_system...
content/browser/file_system/file_system_browsertest.cc:73: quota::StorageType,
int64>& params) {
On 2011/09/01 05:53:30, kinuko wrote:
> nit: weird indentation

Done.

[Dai Mikurube (NOT FULLTIME), 2011-09-06 11:22:12]

On 2011/09/01 06:54:09, Dai Mikurube wrote:
> Thanks.  Fixed it, rebased and submitted to try bots.
> 
>
http://codereview.chromium.org/7715024/diff/18002/chrome/test/data/fileapi/qu...
> File chrome/test/data/fileapi/quota_test.js (right):
> 
>
http://codereview.chromium.org/7715024/diff/18002/chrome/test/data/fileapi/qu...
> chrome/test/data/fileapi/quota_test.js:38: }
> On 2011/09/01 05:11:53, kinuko wrote:
> > nit: semicolon after '}'
> 
> Done.
> 
>
http://codereview.chromium.org/7715024/diff/18002/content/browser/file_system...
> File content/browser/file_system/file_system_browsertest.cc (right):
> 
>
http://codereview.chromium.org/7715024/diff/18002/content/browser/file_system...
> content/browser/file_system/file_system_browsertest.cc:73: quota::StorageType,
> int64>& params) {
> On 2011/09/01 05:53:30, kinuko wrote:
> > nit: weird indentation
> 
> Done.

Errors of trybots look not related to this change.  Checked the "Commit" box.

[commit-bot: I haz the power, 2011-09-06 12:47:56]

Change committed as 99726