[fsp] Add requestUnmount() method together with the request manager.

[fsp] Add requestUnmount() method together with the request manager.

This patch introduces a requestUnmount() method, which is invoked by Files app's private api, and then emits an onUnmountRequested event. When the providing extension receives it, it should handle unmounting, and call the success or failure callback.

Unmounting is the first implemented request in the file system provider implementation. Other requests will be: listing directories, fetching files, etc. These requests are invoked by Files app and are routed to the providing extension, then the response goes back to Files app.

To make the code clean, a RequestManager class has been introduced. It acts as a glue between requests as responses.

TEST=unit_tests: *FileSystemProviderServiceTest.Unmount*, *FileSystemProviderRequestManagerTest*; browser_tests: *FileSystemProviderApiTest.Unmount*
BUG=248427
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262840

[mtomasz, 2014-03-11 10:13:40]

This CL depends on 192573002.

[mtomasz, 2014-03-13 03:59:49]

On 2014/03/11 10:13:40, mtomasz wrote:
> This CL depends on 192573002.

@kinaba, @satorux: PTAL. This depends on 192573002. This CL adds stuff related
to unmounting. It is: fileSystemProvider.unmount(),
fileSystemProvider.onUnmountRequested avent and a request manager. Let me know
if you have any questions.

[mtomasz, 2014-03-24 07:27:36]

On 2014/03/13 03:59:49, mtomasz OOO until 21.03 wrote:
> On 2014/03/11 10:13:40, mtomasz wrote:
> > This CL depends on 192573002.
> 
> @kinaba, @satorux: PTAL. This depends on 192573002. This CL adds stuff related
> to unmounting. It is: fileSystemProvider.unmount(),
> fileSystemProvider.onUnmountRequested avent and a request manager. Let me know
> if you have any questions.

Now it depends on 209663006.

[kinaba, 2014-03-24 08:36:12]

On 2014/03/24 07:27:36, mtomasz OOO until 21.03 wrote:
> On 2014/03/13 03:59:49, mtomasz OOO until 21.03 wrote:
> > On 2014/03/11 10:13:40, mtomasz wrote:
> > > This CL depends on 192573002.
> > 
> > @kinaba, @satorux: PTAL. This depends on 192573002. This CL adds stuff
related
> > to unmounting. It is: fileSystemProvider.unmount(),
> > fileSystemProvider.onUnmountRequested avent and a request manager. Let me
know
> > if you have any questions.
> 
> Now it depends on 209663006.

lg for c/b/**/file_manager. Will take a look on file_system_provider side
tomorrow

[hashimoto, 2014-03-25 10:38:56]

Didn't do serious review, but noticed one nit.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right):

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:16: #include
"content/public/test/test_browser_thread_bundle.h"
nit: This line is duplicated.

[hashimoto, 2014-03-25 10:57:47]

some more nits

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
(right):

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:21:
static const std::string kExtensionId = "mbflcebpggnecokmikipoihdbecnjfoj";
nit: No need to have const. You are already in the unnamed namespace where these
variables have internal linkage.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:168:
ASSERT_FALSE(event->result());
nit: I think this can be EXPECT.
Why don't you allow the test move forward and emit more information to make
debugging easier?

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:249:
ASSERT_EQ(0u, logger.error_events().size());
nit: This can be EXPECT?

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:279:
ASSERT_EQ(0u, logger.error_events().size());
nit: This can be EXPECT?

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:317:
ASSERT_EQ(0u, logger.error_events().size());
nit: This can be EXPECT?

[kinaba, 2014-03-25 11:14:51]

Sorry for my belated response. lgtm with nits

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
(right):

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:21:
static const std::string kExtensionId = "mbflcebpggnecokmikipoihdbecnjfoj";
I think hashimoto meant no need for static.

std::string => char[] (as far as I understand objects in global scope is not
allowed in Chromium)

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:33:
~SuccessEvent() {};
semicolon not necessary.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:46:
~ErrorEvent() {};
Drop semicolon

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:306:
base::FilePath(
base::FilePath::FromUTF8Unsafe (or FILE_PATH_LITERAL)

[mtomasz, 2014-03-26 00:52:34]

Thanks for comments. PTAL.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
(right):

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:21:
static const std::string kExtensionId = "mbflcebpggnecokmikipoihdbecnjfoj";
On 2014/03/25 11:14:51, kinaba wrote:
> I think hashimoto meant no need for static.
> 
> std::string => char[] (as far as I understand objects in global scope is not
> allowed in Chromium)

Done.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:21:
static const std::string kExtensionId = "mbflcebpggnecokmikipoihdbecnjfoj";
On 2014/03/25 10:57:48, hashimoto wrote:
> nit: No need to have const. You are already in the unnamed namespace where
these
> variables have internal linkage.

Done.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:33:
~SuccessEvent() {};
On 2014/03/25 11:14:51, kinaba wrote:
> semicolon not necessary.

Done.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:46:
~ErrorEvent() {};
On 2014/03/25 11:14:51, kinaba wrote:
> Drop semicolon

Done.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:168:
ASSERT_FALSE(event->result());
On 2014/03/25 10:57:48, hashimoto wrote:
> nit: I think this can be EXPECT.
> Why don't you allow the test move forward and emit more information to make
> debugging easier?

Done.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:249:
ASSERT_EQ(0u, logger.error_events().size());
On 2014/03/25 10:57:48, hashimoto wrote:
> nit: This can be EXPECT?

Done.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:279:
ASSERT_EQ(0u, logger.error_events().size());
On 2014/03/25 10:57:48, hashimoto wrote:
> nit: This can be EXPECT?

Done.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:306:
base::FilePath(
On 2014/03/25 11:14:51, kinaba wrote:
> base::FilePath::FromUTF8Unsafe (or FILE_PATH_LITERAL)

Done.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:317:
ASSERT_EQ(0u, logger.error_events().size());
On 2014/03/25 10:57:48, hashimoto wrote:
> nit: This can be EXPECT?

Done.

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right):

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:16: #include
"content/public/test/test_browser_thread_bundle.h"
On 2014/03/25 10:38:57, hashimoto wrote:
> nit: This line is duplicated.

Done.

[hashimoto, 2014-03-26 01:07:56]

lgtm

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
(right):

https://codereview.chromium.org/194693002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:21:
static const std::string kExtensionId = "mbflcebpggnecokmikipoihdbecnjfoj";
On 2014/03/25 11:14:51, kinaba wrote:
> I think hashimoto meant no need for static.
oops, right, indeed.
> 
> std::string => char[] (as far as I understand objects in global scope is not
> allowed in Chromium)

[satorux1, 2014-03-26 08:15:02]

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:220:
VolumeInfo::VolumeInfo() {
I think we should initialize file_system_id and other members of primitive types

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
(right):

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:27:
class EventLogger {
class comment is missing.

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:32: // Utility function,
which constructs a safe and unique mount point path for
Just start with "Constructs"?

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:47: // Utility function,
creates values to be passed to request events. These values
ditto.

https://codereview.chromium.org/194693002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/194693002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:12: // in JavaScript.
did you talk to kinuko@ on this?

https://codereview.chromium.org/194693002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js
(right):

https://codereview.chromium.org/194693002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:28:
function unmount() {
This is testing the most common case, right? Could you add a brief comment for
each test, explaining what they intend to test?

https://codereview.chromium.org/194693002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:75:
chrome.fileBrowserPrivate.removeMount(secondVolumeId);
In this test, unmounting is initiated by fileBrowserPrivate.removeMount() rather
than hrome.fileSystemProvider.unmount(), right? As mentioned above, it'd be nice
to add some comment about intention of the test.

[mtomasz, 2014-03-26 10:27:41]

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:220:
VolumeInfo::VolumeInfo() {
On 2014/03/26 08:15:03, satorux1 wrote:
> I think we should initialize file_system_id and other members of primitive
types

Done.

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
(right):

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:27:
class EventLogger {
On 2014/03/26 08:15:03, satorux1 wrote:
> class comment is missing.

Done.

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:32: // Utility function,
which constructs a safe and unique mount point path for
On 2014/03/26 08:15:03, satorux1 wrote:
> Just start with "Constructs"?

Done.

https://codereview.chromium.org/194693002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:47: // Utility function,
creates values to be passed to request events. These values
On 2014/03/26 08:15:03, satorux1 wrote:
> ditto.

Done.

https://codereview.chromium.org/194693002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/194693002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:12: // in JavaScript.
On 2014/03/26 08:15:03, satorux1 wrote:
> did you talk to kinuko@ on this?

Nope. I'll talk tomorrow. But I'm sceptical if it is doable.

https://codereview.chromium.org/194693002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js
(right):

https://codereview.chromium.org/194693002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:28:
function unmount() {
On 2014/03/26 08:15:03, satorux1 wrote:
> This is testing the most common case, right? Could you add a brief comment for
> each test, explaining what they intend to test?

I added comments for each test case. Done.

https://codereview.chromium.org/194693002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:75:
chrome.fileBrowserPrivate.removeMount(secondVolumeId);
On 2014/03/26 08:15:03, satorux1 wrote:
> In this test, unmounting is initiated by fileBrowserPrivate.removeMount()
rather
> than hrome.fileSystemProvider.unmount(), right? As mentioned above, it'd be
nice
> to add some comment about intention of the test.

Exactly. The previous one tests fileSystemProvider.unmount(), and this one
fileBrowserPrivate.removeMount() with onSuccess(). Added comments. Done.

[satorux1, 2014-03-27 04:43:24]

LGTM with comments:

https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js
(right):

https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:28: //
Tests the fileBrowserPrivate.unmount(). Verifies if the unmount event
Did you mean: fileSystemProvider.unmount

https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:52: //
Tests the fileBrowserPrivate.unmount() with a wrong id. Verifies that
ditto

https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:87: //
results in an unmount event fired from the VolumeManager instance.
I found these comments very useful. Could you add these to mount/test.js as
well?

[mtomasz, 2014-03-27 04:55:29]

@benwells: PTAL at *.idl.
@kinuko: PTAL at fileapi.

Thanks.

https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js
(right):

https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:28: //
Tests the fileBrowserPrivate.unmount(). Verifies if the unmount event
On 2014/03/27 04:43:24, satorux1 wrote:
> Did you mean: fileSystemProvider.unmount

Done.

https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:52: //
Tests the fileBrowserPrivate.unmount() with a wrong id. Verifies that
On 2014/03/27 04:43:24, satorux1 wrote:
> ditto

Done.

https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:87: //
results in an unmount event fired from the VolumeManager instance.
On 2014/03/27 04:43:24, satorux1 wrote:
> I found these comments very useful. Could you add these to mount/test.js as
> well?

Done.

[mtomasz, 2014-03-27 04:57:00]

On 2014/03/27 04:55:29, mtomasz OOO until 21.03 wrote:
> @benwells: PTAL at *.idl.
> @kinuko: PTAL at fileapi.
> 
> Thanks.
> 
>
https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
> File chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js
> (right):
> 
>
https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:28:
//
> Tests the fileBrowserPrivate.unmount(). Verifies if the unmount event
> On 2014/03/27 04:43:24, satorux1 wrote:
> > Did you mean: fileSystemProvider.unmount
> 
> Done.
> 
>
https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:52:
//
> Tests the fileBrowserPrivate.unmount() with a wrong id. Verifies that
> On 2014/03/27 04:43:24, satorux1 wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/194693002/diff/300001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js:87:
//
> results in an unmount event fired from the VolumeManager instance.
> On 2014/03/27 04:43:24, satorux1 wrote:
> > I found these comments very useful. Could you add these to mount/test.js as
> > well?
> 
> Done.

@kinuko: Rollback. No fileapi changes in here.

[benwells, 2014-03-30 22:37:07]

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:10: // Error codes, same
as in base::Files::Error. Note, that DOMError would be
This note doesn't seem right for public documentation.

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:44: [nodoc,
instanceOf=DOMError] object error);
Why is the error nodoc?

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:86: ProviderErrorCallback
errorCallback);
What happens if a provider is uncooperative and doesn't play ball? E.g. they
don't unmount, or they don't call either successCallback ro errorCallback?

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider_internal.idl (right):

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider_internal.idl:1: // Copyright
2013 The Chromium Authors. All rights reserved.
Nit: 2014

[mtomasz, 2014-03-31 01:01:47]

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:10: // Error codes, same
as in base::Files::Error. Note, that DOMError would be
On 2014/03/30 22:37:07, benwells wrote:
> This note doesn't seem right for public documentation.

Done.

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:44: [nodoc,
instanceOf=DOMError] object error);
On 2014/03/30 22:37:07, benwells wrote:
> Why is the error nodoc?

Done.

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:44: [nodoc,
instanceOf=DOMError] object error);
On 2014/03/30 22:37:07, benwells wrote:
> Why is the error nodoc?

Copy pasted. Removed. Done.

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:86: ProviderErrorCallback
errorCallback);
On 2014/03/30 22:37:07, benwells wrote:
> What happens if a provider is uncooperative and doesn't play ball? E.g. they
> don't unmount, or they don't call either successCallback ro errorCallback?

Then the request will never be completed. I'm aware of it, and there is a plan
to implement a timeout for requests soon. It is mentioned in the API design doc.

[mtomasz, 2014-03-31 01:08:52]

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/194693002/diff/340001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:44: [nodoc,
instanceOf=DOMError] object error);
On 2014/03/31 01:01:47, mtomasz wrote:
> On 2014/03/30 22:37:07, benwells wrote:
> > Why is the error nodoc?
> 
> Copy pasted. Removed. Done.

Actually, there is a reason. Error is passed via the error callback. See #63.
The |error| argument is forwarded to ErroCallback (declared in #53).

This trick is used to be able to have two callbacks (success and error), and it
has been discussed in https://codereview.chromium.org/50703013.

[benwells, 2014-03-31 02:24:10]

One more question ...

https://codereview.chromium.org/194693002/diff/380001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/194693002/diff/380001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:369: "contexts":
["blessed_extension", "unblessed_extension"]
Why is this available to more contexts than fileSystemProvider?

[mtomasz, 2014-03-31 02:27:14]

https://codereview.chromium.org/194693002/diff/380001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/194693002/diff/380001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:369: "contexts":
["blessed_extension", "unblessed_extension"]
On 2014/03/31 02:24:10, benwells wrote:
> Why is this available to more contexts than fileSystemProvider?

That wasn't my intention. It referred to fileBrowserHandlerInternal, which is
declared the same way. Can we restrict it to fileSystemProvider only?

[benwells, 2014-03-31 02:38:02]

https://codereview.chromium.org/194693002/diff/380001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/194693002/diff/380001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:369: "contexts":
["blessed_extension", "unblessed_extension"]
On 2014/03/31 02:27:15, mtomasz wrote:
> On 2014/03/31 02:24:10, benwells wrote:
> > Why is this available to more contexts than fileSystemProvider?
> 
> That wasn't my intention. It referred to fileBrowserHandlerInternal, which is
> declared the same way. Can we restrict it to fileSystemProvider only?

Can you just remove the unblessed_extension?

[mtomasz, 2014-03-31 03:16:46]

https://codereview.chromium.org/194693002/diff/380001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/194693002/diff/380001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:369: "contexts":
["blessed_extension", "unblessed_extension"]
On 2014/03/31 02:38:03, benwells wrote:
> On 2014/03/31 02:27:15, mtomasz wrote:
> > On 2014/03/31 02:24:10, benwells wrote:
> > > Why is this available to more contexts than fileSystemProvider?
> > 
> > That wasn't my intention. It referred to fileBrowserHandlerInternal, which
is
> > declared the same way. Can we restrict it to fileSystemProvider only?
> 
> Can you just remove the unblessed_extension?

Done.

[mtomasz, 2014-04-01 17:39:31]

On 2014/03/31 03:16:46, mtomasz wrote:
>
https://codereview.chromium.org/194693002/diff/380001/chrome/common/extension...
> File chrome/common/extensions/api/_api_features.json (right):
> 
>
https://codereview.chromium.org/194693002/diff/380001/chrome/common/extension...
> chrome/common/extensions/api/_api_features.json:369: "contexts":
> ["blessed_extension", "unblessed_extension"]
> On 2014/03/31 02:38:03, benwells wrote:
> > On 2014/03/31 02:27:15, mtomasz wrote:
> > > On 2014/03/31 02:24:10, benwells wrote:
> > > > Why is this available to more contexts than fileSystemProvider?
> > > 
> > > That wasn't my intention. It referred to fileBrowserHandlerInternal, which
> is
> > > declared the same way. Can we restrict it to fileSystemProvider only?
> > 
> > Can you just remove the unblessed_extension?
> 
> Done.

@benwells: Ping.

[benwells, 2014-04-01 20:40:34]

Lgtm

setn from phonr
On Mar 31, 2014 2:16 PM, <mtomasz@chromium.org> wrote:

>
> https://codereview.chromium.org/194693002/diff/380001/
> chrome/common/extensions/api/_api_features.json
> File chrome/common/extensions/api/_api_features.json (right):
>
> https://codereview.chromium.org/194693002/diff/380001/
> chrome/common/extensions/api/_api_features.json#newcode369
> chrome/common/extensions/api/_api_features.json:369: "contexts":
> ["blessed_extension", "unblessed_extension"]
> On 2014/03/31 02:38:03, benwells wrote:
>
>> On 2014/03/31 02:27:15, mtomasz wrote:
>> > On 2014/03/31 02:24:10, benwells wrote:
>> > > Why is this available to more contexts than fileSystemProvider?
>> >
>> > That wasn't my intention. It referred to fileBrowserHandlerInternal,
>>
> which is
>
>> > declared the same way. Can we restrict it to fileSystemProvider
>>
> only?
>
>  Can you just remove the unblessed_extension?
>>
>
> Done.
>
> https://codereview.chromium.org/194693002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[benwells, 2014-04-01 21:04:52]

lgtm

[mtomasz, 2014-04-04 20:41:38]

@isherman: PTAL at histograms.xml.

[Ilya Sherman, 2014-04-04 21:49:22]

histograms.xml lgtm

[mtomasz, 2014-04-07 18:32:16]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-07 18:32:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/440001

[commit-bot: I haz the power, 2014-04-07 19:21:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-07 19:21:14]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[mtomasz, 2014-04-07 19:42:57]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-07 19:43:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/460001

[commit-bot: I haz the power, 2014-04-07 19:43:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-07 19:43:47]

Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file extensions/browser/extension_function_histogram_value.h
  Hunk #1 FAILED at 770.
  1 out of 1 hunk FAILED -- saving rejects to file
extensions/browser/extension_function_histogram_value.h.rej

Patch:       extensions/browser/extension_function_histogram_value.h
Index: extensions/browser/extension_function_histogram_value.h
diff --git a/extensions/browser/extension_function_histogram_value.h
b/extensions/browser/extension_function_histogram_value.h
index
17eee77878f9a35962936f359d6c6c2db4c7b0fe..8c83c182b0ec9dac719f572769fa236cd55c0504
100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -770,6 +770,9 @@ enum HistogramValue {
   BLUETOOTHPRIVATE_SETPAIRINGRESPONSE,
   NETWORKINGPRIVATE_GETCAPTIVEPORTALSTATUS,
   AUTOMATIONINTERNAL_PERFORMACTION,
+  FILESYSTEMPROVIDER_UNMOUNT,
+  FILESYSTEMPROVIDERINTERNAL_UNMOUNTREQUESTEDSUCCESS,
+  FILESYSTEMPROVIDERINTERNAL_UNMOUNTREQUESTEDERROR,
   // Last entry: Add new entries above and ensure to update
   // tools/metrics/histograms/histograms/histograms.xml.
   ENUM_BOUNDARY

[mtomasz, 2014-04-07 19:51:24]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-07 19:52:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/480001

[commit-bot: I haz the power, 2014-04-07 20:06:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-07 20:06:19]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel
  tryserver.chromium on linux_chromium_clang_dbg

[mtomasz, 2014-04-07 20:33:49]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-07 20:34:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/500001

[commit-bot: I haz the power, 2014-04-07 22:09:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-07 22:09:17]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[mtomasz, 2014-04-07 22:26:07]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-07 22:26:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/500001

[commit-bot: I haz the power, 2014-04-07 23:46:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-07 23:47:00]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[mtomasz, 2014-04-08 02:14:28]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-08 02:14:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/500001

[commit-bot: I haz the power, 2014-04-08 02:15:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-08 02:15:08]

Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file extensions/browser/extension_function_histogram_value.h
  Hunk #1 FAILED at 770.
  1 out of 1 hunk FAILED -- saving rejects to file
extensions/browser/extension_function_histogram_value.h.rej

Patch:       extensions/browser/extension_function_histogram_value.h
Index: extensions/browser/extension_function_histogram_value.h
diff --git a/extensions/browser/extension_function_histogram_value.h
b/extensions/browser/extension_function_histogram_value.h
index
17eee77878f9a35962936f359d6c6c2db4c7b0fe..63197b223a56de60df69f9030fc37b8cf6bc81e0
100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -770,6 +770,13 @@ enum HistogramValue {
   BLUETOOTHPRIVATE_SETPAIRINGRESPONSE,
   NETWORKINGPRIVATE_GETCAPTIVEPORTALSTATUS,
   AUTOMATIONINTERNAL_PERFORMACTION,
+  BLUETOOTH_UPDATE_SOCKET,
+  BLUETOOTH_SET_SOCKET_PAUSED,
+  BLUETOOTH_GET_SOCKET,
+  BLUETOOTH_GET_SOCKETS,
+  FILESYSTEMPROVIDER_UNMOUNT,
+  FILESYSTEMPROVIDERINTERNAL_UNMOUNTREQUESTEDSUCCESS,
+  FILESYSTEMPROVIDERINTERNAL_UNMOUNTREQUESTEDERROR,
   // Last entry: Add new entries above and ensure to update
   // tools/metrics/histograms/histograms/histograms.xml.
   ENUM_BOUNDARY

[mtomasz, 2014-04-08 21:21:13]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-08 21:21:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/540001

[commit-bot: I haz the power, 2014-04-08 22:40:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-08 22:40:09]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[mtomasz, 2014-04-09 01:34:56]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-09 01:35:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/540001

[commit-bot: I haz the power, 2014-04-09 01:46:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/540001

[commit-bot: I haz the power, 2014-04-09 01:57:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/540001

[commit-bot: I haz the power, 2014-04-09 02:16:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/540001

[mtomasz, 2014-04-09 03:15:24]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-09 04:01:28]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[commit-bot: I haz the power, 2014-04-09 04:01:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/560001

[commit-bot: I haz the power, 2014-04-09 04:43:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/560001

[commit-bot: I haz the power, 2014-04-09 04:48:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/560001

[commit-bot: I haz the power, 2014-04-09 04:53:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/560001

[commit-bot: I haz the power, 2014-04-09 04:58:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/560001

[commit-bot: I haz the power, 2014-04-09 06:17:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-09 06:17:43]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[mtomasz, 2014-04-09 19:25:47]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-09 19:26:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/194693002/580001

[commit-bot: I haz the power, 2014-04-09 22:25:33]

Change committed as 262840