Media Galleries: Add a dropPermissionForMediaFileSystem() API.

Media Galleries: Add a dropPermissionForMediaFileSystem() API.

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

[Lei Zhang, 2014-04-05 04:38:23]

Do we want to call this forget? release?

[vandebo (ex-Chrome), 2014-04-07 16:01:01]

https://codereview.chromium.org/224963010/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/224963010/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/media_galleries.idl:60: callback
ForgetMediaFileSystemCallback = void (boolean wasForgotten);
Is there a usecase for this?  If we can't think of one, lets leave it out until
we find one.  It'd be easy to add in later.

https://codereview.chromium.org/224963010/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/media_galleries.idl:147: static void
forgetMediaFileSystem(
I think 'cede' or 'drop' would be better than 'forget'

https://codereview.chromium.org/224963010/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/media_galleries.idl:148: [instanceOf=DOMFileSystem]
object mediaFileSystem,
Should this take a FS object, or a gallery id?  You need a FS object to get to a
gallery id, but apps can persist the gallery id's that they have access to and
thus use them as keys for FS's in there UI.  i.e. if there's a long unused
removable device, the app may have the gallery id, but can't get the FS
object...

[Lei Zhang, 2014-04-07 22:32:04]

isherman: metrics OWNERS review please. I'm adding the
NETWORKINGPRIVATE_GETCAPTIVEPORTALSTATUS value because someone forgot to do so
in their CL.

https://codereview.chromium.org/224963010/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/224963010/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/media_galleries.idl:60: callback
ForgetMediaFileSystemCallback = void (boolean wasForgotten);
On 2014/04/07 16:01:01, vandebo wrote:
> Is there a usecase for this?  If we can't think of one, lets leave it out
until
> we find one.  It'd be easy to add in later.

Success is not guaranteed, so there should be someway of letting the caller know
in case of a failure. The alarms API is similar.
https://developer.chrome.com/apps/alarms#method-clearAll

https://codereview.chromium.org/224963010/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/media_galleries.idl:148: [instanceOf=DOMFileSystem]
object mediaFileSystem,
On 2014/04/07 16:01:01, vandebo wrote:
> Should this take a FS object, or a gallery id?  You need a FS object to get to
a
> gallery id, but apps can persist the gallery id's that they have access to and
> thus use them as keys for FS's in there UI.  i.e. if there's a long unused
> removable device, the app may have the gallery id, but can't get the FS
> object...

Done.

[Ilya Sherman, 2014-04-07 22:53:18]

histograms.xml lgtm

[Lei Zhang, 2014-04-08 02:05:11]

tommycli/vandebo: See patch set 3.

https://codereview.chromium.org/224963010/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/224963010/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/media_galleries.idl:60: callback
ForgetMediaFileSystemCallback = void (boolean wasForgotten);
On 2014/04/07 16:01:01, vandebo wrote:
> Is there a usecase for this?  If we can't think of one, lets leave it out
until
> we find one.  It'd be easy to add in later.

Also, since the API is asynchronous, the caller can't count on the call to have
completed until the callback has occurred.

https://codereview.chromium.org/224963010/diff/40001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js
(left):

https://codereview.chromium.org/224963010/diff/40001/chrome/renderer/resource...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:75:
return {};
This doesn't actually work. There is code, presumably generated by the IDL, that
validates the return value is a MediaFileSystemMetadata, with all the required
fields.

[vandebo (ex-Chrome), 2014-04-08 14:58:44]

LGTM

https://codereview.chromium.org/224963010/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/224963010/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:669:
&MediaGalleriesDropPermissionForMediaFileSystemFunction::OnPreferencesInit
Would wrapping at the :: be better than a macro?

https://codereview.chromium.org/224963010/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:672: return
Setup(GetProfile(), &error_, callback);
Consider putting this in the conditional on line 664 so that you can only set
the result if returning false.

https://codereview.chromium.org/224963010/diff/40001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js
(right):

https://codereview.chromium.org/224963010/diff/40001/chrome/renderer/resource...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:74: if
(galleryId) {
if (galleryId && result)

[tommycli, 2014-04-08 16:19:13]

didn't have any comments beyond vandebo's, lgtm

[Lei Zhang, 2014-04-08 17:10:18]

https://codereview.chromium.org/224963010/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/224963010/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:669:
&MediaGalleriesDropPermissionForMediaFileSystemFunction::OnPreferencesInit
On 2014/04/08 14:58:45, vandebo wrote:
> Would wrapping at the :: be better than a macro?

Done.

https://codereview.chromium.org/224963010/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:672: return
Setup(GetProfile(), &error_, callback);
On 2014/04/08 14:58:45, vandebo wrote:
> Consider putting this in the conditional on line 664 so that you can only set
> the result if returning false.

Code shuffled.

https://codereview.chromium.org/224963010/diff/40001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js
(right):

https://codereview.chromium.org/224963010/diff/40001/chrome/renderer/resource...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:74: if
(galleryId) {
On 2014/04/08 14:58:45, vandebo wrote:
> if (galleryId && result)

Done.

[Lei Zhang, 2014-04-08 17:11:41]

kalman: extensions/ OWNERS please.

[not at google - send to devlin, 2014-04-09 15:42:03]

lgtm

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

https://codereview.chromium.org/224963010/diff/100001/chrome/common/extension...
chrome/common/extensions/api/media_galleries.idl:147: static void
dropPermissionForMediaFileSystem(
Consistency bikeshed:

- I'd just call this "dropMediaFileSystem" to parallel "addMediaFileSystems".
- Maybe it should take a list of gallery IDs to further parallel it (... maybe).

[Lei Zhang, 2014-04-09 20:13:28]

On 2014/04/09 15:42:03, kalman wrote:
> - I'd just call this "dropMediaFileSystem" to parallel "addMediaFileSystems".

I discussed this with vandebo. We didn't want to call it "dropMediaFileSystem"
because that can be easily read as "remove the media file system" completely".
All the "addMediaFileSystems" can have a "global" effect in that a new media
gallery is now potentially available for all apps, whereas
dropPermissionForMediaFileSystem is much less powerful and only affects the
caller app.

> - Maybe it should take a list of gallery IDs to further parallel it (...
maybe).

Meh, I don't think this gets called frequent enough to need a more complicated
API.

[Lei Zhang, 2014-04-09 20:13:48]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-04-09 20:14:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/224963010/100001

[not at google - send to devlin, 2014-04-09 20:14:57]

lgtm

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

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

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

Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file extensions/browser/extension_function_histogram_value.h
  Hunk #2 FAILED at 774.
  Hunk #3 succeeded at 785 (offset 3 lines).
  1 out of 3 hunks 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
===================================================================
--- extensions/browser/extension_function_histogram_value.h	(revision 262440)
+++ extensions/browser/extension_function_histogram_value.h	(working copy)
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_
-#define CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_
+#ifndef EXTENSIONS_BROWSER_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_
+#define EXTENSIONS_BROWSER_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_
 
 namespace extensions {
 namespace functions {
@@ -774,6 +774,7 @@
   BLUETOOTH_SET_SOCKET_PAUSED,
   BLUETOOTH_GET_SOCKET,
   BLUETOOTH_GET_SOCKETS,
+  MEDIAGALLERIES_DROPPERMISSIONFORMEDIAFILESYSTEM,
   // Last entry: Add new entries above and ensure to update
   // tools/metrics/histograms/histograms/histograms.xml.
   ENUM_BOUNDARY
@@ -782,4 +783,4 @@
 }  // namespace functions
 }  // namespace extensions
 
-#endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_
+#endif  // EXTENSIONS_BROWSER_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_

[Lei Zhang, 2014-04-09 22:46:57]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-04-09 22:47:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/224963010/120001

[commit-bot: I haz the power, 2014-04-09 23:29:02]

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

[commit-bot: I haz the power, 2014-04-09 23:29:02]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Lei Zhang, 2014-04-09 23:34:45]

The CQ bit was checked by thestig@chromium.org

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/224963010/120001

[commit-bot: I haz the power, 2014-04-10 00:25:27]

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

[commit-bot: I haz the power, 2014-04-10 00:25:28]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Lei Zhang, 2014-04-10 06:10:39]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-04-10 06:10:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/224963010/120001

[commit-bot: I haz the power, 2014-04-10 07:02:30]

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

[commit-bot: I haz the power, 2014-04-10 07:02:30]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Lei Zhang, 2014-04-10 17:28:04]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-04-10 17:28:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/224963010/120001

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

Change committed as 263062