Media Galleries: Add chrome.mediaGalleries.addUserSelectedFolder().

Media Galleries: Add chrome.mediaGalleries.addUserSelectedFolder().

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

[vandebo (ex-Chrome), 2013-12-02 18:01:29]

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:84: class
SelectDirectoryDialog : public ui::SelectFileDialog::Listener,
Should this live in c/b/media_galleries?
Should it share code with MediaGalleriesDialogController ?

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:120:
Release();  // Balanced in Show().
Should the callback be called with an empty path in this case?  It shouldn't
happen, but if it does the API would never return, right?

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:164: //
Controller will delete itself.
nit: move to line 166

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:165:
base::Closure cb = base::Bind(
nit: cb -> callback.  Would it be more consistent (172) to inline this?

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:177:
base::ListValue* list = ConstructFileSystemList(filesystems);
Should this be scoped_ptr<base::ListValue> list ?

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:179:
SendResponse(false);
The API didn't necessarily fail if there's an empty list.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:468:
base::ListValue* empty_list = new base::ListValue();
nit: inline in next line

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:474: bool
posted = base::PostTaskAndReplyWithResult(
This appears to be different than MediaGalleriesDialogController::FileSelected,
but the two should be consistent - probably should share code.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:515:
base::ListValue* list = ConstructFileSystemList(found_filesystem);
Ahh, I see - you're returning a set of zero or one file systems. (This doesn't
match the comments in the idl file.) I'm not sure if we can do this.  The
current semantic is that you can add a directory that doesn't exist, but we
don't return file systems for directories that don't exist....

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

https://codereview.chromium.org/93643002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/media_galleries.idl:47: [inline_doc] dictionary
ManageRequestResults {
This is inconsistent with MediaFileSystemsCallback.  What do we gain?

https://codereview.chromium.org/93643002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/media_galleries.idl:52: callback
ManageMediaLocationsCallback = void (ManageRequestResults results);
It doesn't seem like there is a way to know if a 'directory_prompt' request was
successful?

[Lei Zhang, 2013-12-03 03:33:22]

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:84: class
SelectDirectoryDialog : public ui::SelectFileDialog::Listener,
On 2013/12/02 18:01:30, vandebo wrote:
> Should this live in c/b/media_galleries?
> Should it share code with MediaGalleriesDialogController ?

Probably not? The two workflows are different and I don't think there's much
shared code.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:120:
Release();  // Balanced in Show().
On 2013/12/02 18:01:30, vandebo wrote:
> Should the callback be called with an empty path in this case?  It shouldn't
> happen, but if it does the API would never return, right?

No, we should never handle a NOTREACHED() per the Chromium style guide. I'll
remove the Release() as well.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:164: //
Controller will delete itself.
On 2013/12/02 18:01:30, vandebo wrote:
> nit: move to line 166

Done.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:165:
base::Closure cb = base::Bind(
On 2013/12/02 18:01:30, vandebo wrote:
> nit: cb -> callback.  Would it be more consistent (172) to inline this?

I inlined it, but now it's 5 lines. Meh.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:177:
base::ListValue* list = ConstructFileSystemList(filesystems);
On 2013/12/02 18:01:30, vandebo wrote:
> Should this be scoped_ptr<base::ListValue> list ?

Done. We haven't always used scoped_ptrs in all cases.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:179:
SendResponse(false);
On 2013/12/02 18:01:30, vandebo wrote:
> The API didn't necessarily fail if there's an empty list.

This is not the empty list case. |list| is only NULL on failure.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:468:
base::ListValue* empty_list = new base::ListValue();
On 2013/12/02 18:01:30, vandebo wrote:
> nit: inline in next line

Done.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:474: bool
posted = base::PostTaskAndReplyWithResult(
On 2013/12/02 18:01:30, vandebo wrote:
> This appears to be different than
MediaGalleriesDialogController::FileSelected,
> but the two should be consistent - probably should share code.

MediaGalleriesDialogController::FileSelected has to keep track of the added, but
not saved galleries. ( |new_galleries_| ) Whereas this workflow does not have
that issue to deal with. I don't think they share that much code.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:515:
base::ListValue* list = ConstructFileSystemList(found_filesystem);
On 2013/12/02 18:01:30, vandebo wrote:
> Ahh, I see - you're returning a set of zero or one file systems. (This doesn't
> match the comments in the idl file.) I'm not sure if we can do this.  The
> current semantic is that you can add a directory that doesn't exist, but we
> don't return file systems for directories that don't exist....

Leaving this one open. I'll check tomorrow if file dialog pickers let you select
a non-existent path. Even if they do, we can still return a list of 0 galleries
in that case. Even if the file picker lets you, why would a user want to do
that? Even if the user does it, it's a non-existent path, so what can we return
but an empty list?

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

https://codereview.chromium.org/93643002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/media_galleries.idl:47: [inline_doc] dictionary
ManageRequestResults {
On 2013/12/02 18:01:30, vandebo wrote:
> This is inconsistent with MediaFileSystemsCallback.  What do we gain?

Flexibility. You can add new key/value pairs in the results dictionary to
indicate whatever other status you haven't thought of yet.

In your scanning proposal CL, you had the callback as (bool, DOMFileSystem[]),
with the bool indicating success. However, we don't know what the scanning
interface will look like in the end, so I'm leaving this open.

https://codereview.chromium.org/93643002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/media_galleries.idl:52: callback
ManageMediaLocationsCallback = void (ManageRequestResults results);
On 2013/12/02 18:01:30, vandebo wrote:
> It doesn't seem like there is a way to know if a 'directory_prompt' request
was
> successful?

|media_file_systems| has multiple states. As mentioned on line 48, it's NULL on
failure. On success, it's an array of size N, where N can be 0. I'll add more
comments.

[vandebo (ex-Chrome), 2013-12-03 21:08:01]

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:84: class
SelectDirectoryDialog : public ui::SelectFileDialog::Listener,
On 2013/12/03 03:33:23, Lei Zhang wrote:
> On 2013/12/02 18:01:30, vandebo wrote:
> > Should this live in c/b/media_galleries?
> > Should it share code with MediaGalleriesDialogController ?
> 
> Probably not? The two workflows are different and I don't think there's much
> shared code.

They both have a directory picker which results in a added gallery...

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:165:
base::Closure cb = base::Bind(
On 2013/12/03 03:33:23, Lei Zhang wrote:
> On 2013/12/02 18:01:30, vandebo wrote:
> > nit: cb -> callback.  Would it be more consistent (172) to inline this?
> 
> I inlined it, but now it's 5 lines. Meh.

How about:
  new MediaGalleriesDialogController(
      contents, *GetExtension(), base::Bind(
          &MediaGalleriesGetMediaFileSystemsBase::GetAndReturnGalleries, this));

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

https://codereview.chromium.org/93643002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/media_galleries.idl:52: callback
ManageMediaLocationsCallback = void (ManageRequestResults results);
On 2013/12/03 03:33:23, Lei Zhang wrote:
> On 2013/12/02 18:01:30, vandebo wrote:
> > It doesn't seem like there is a way to know if a 'directory_prompt' request
> was
> > successful?
> 
> |media_file_systems| has multiple states. As mentioned on line 48, it's NULL
on
> failure. On success, it's an array of size N, where N can be 0. I'll add more
> comments.

I don't like this interface because it introduces special values and input
dependant results.  I think it would be an easier interface to deal with if it
always returns all the file systems, and also returns a value indicating
directory-add success/failure.

https://codereview.chromium.org/93643002/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/93643002/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:208:
base::ListValue* list = new base::ListValue();
nit: this should probably be a scoped_ptr with a return list.release() at the
end.  There's a lot of space between the allocation and ownership transfer
(return), so a lot of room for someone to later add a return NULL in between.

[Lei Zhang, 2013-12-05 05:52:14]

I'll look into combining the directory pickers later. Let me know if all the
other changes look good.

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/93643002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:165:
base::Closure cb = base::Bind(
On 2013/12/03 21:08:02, vandebo wrote:
> On 2013/12/03 03:33:23, Lei Zhang wrote:
> > On 2013/12/02 18:01:30, vandebo wrote:
> > > nit: cb -> callback.  Would it be more consistent (172) to inline this?
> > 
> > I inlined it, but now it's 5 lines. Meh.
> 
> How about:
>   new MediaGalleriesDialogController(
>       contents, *GetExtension(), base::Bind(
>           &MediaGalleriesGetMediaFileSystemsBase::GetAndReturnGalleries,
this));

Done.

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

https://codereview.chromium.org/93643002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/media_galleries.idl:52: callback
ManageMediaLocationsCallback = void (ManageRequestResults results);
On 2013/12/03 21:08:02, vandebo wrote:
> On 2013/12/03 03:33:23, Lei Zhang wrote:
> > On 2013/12/02 18:01:30, vandebo wrote:
> > > It doesn't seem like there is a way to know if a 'directory_prompt'
request
> > was
> > > successful?
> > 
> > |media_file_systems| has multiple states. As mentioned on line 48, it's NULL
> on
> > failure. On success, it's an array of size N, where N can be 0. I'll add
more
> > comments.
> 
> I don't like this interface because it introduces special values and input
> dependant results.  I think it would be an easier interface to deal with if it
> always returns all the file systems, and also returns a value indicating
> directory-add success/failure.

Done. I did what I said I would do from yesterday's in-person discussion.

https://codereview.chromium.org/93643002/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/93643002/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:208:
base::ListValue* list = new base::ListValue();
On 2013/12/03 21:08:02, vandebo wrote:
> nit: this should probably be a scoped_ptr with a return list.release() at the
> end.  There's a lot of space between the allocation and ownership transfer
> (return), so a lot of room for someone to later add a return NULL in between.

Done.

[vandebo (ex-Chrome), 2013-12-18 20:15:52]

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:103:
base::string16(),  // TODO(thestig) Add a title.
This is a privacy requirement.  Probably should put in an initial one before UI
review changes it.

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:322:
ReadPrefsAndShowDialog();
Shouldn't this just be ShowDialog since the prefs have already been read?

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:507: fsid =
filesystems[i].fsid;
If the just added directory "isn't attached," we won't find it and the
deserialization of this list will fail.  But that will be fixed with a
dictionary.

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.h
(right):

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:29: class
MediaGalleriesGetMediaFileSystemsBase
nit: Maybe name this just GetMediaFileSystemsBase ?

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:36: void
ReadPrefsAndShowDialog();
Is this needed? It looks like all callers may have already read the prefs.

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:39: // Does
not force MediaFileSystemRegistry to read its preferences.
nit: Maybe change the code and comment to assert that preferences are already
read.

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:68: void
AlwaysShowDialog(const std::vector<MediaFileSystemInfo>& filesystems);
Having ReadPrefsAndShowDialog, ShowDialog, and AlwaysShowDialog is a bit
confusing....  I don't think this adaptor needs to be in the class.  Maybe use
something like this, or even upstream is to bind_helpers.h

namespace {
template <typename T>
void IgnoredArgumentRunner(const base::Closure& closure, T ignored_argument) {
  closure.Run();
}
}

template <typename T>
base::Callback<void(T)> IgnoreCallbackArgument(const base::Closure& closure) {
  return base::Bind(IgnoredArgumentRunner, closure);
}

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

https://codereview.chromium.org/93643002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/api/media_galleries.idl:55: DOMString
userSelectedFilesSystemName;
Should this be marked optional? i.e. for non directory_prompt requests it
shouldn't be there.

https://codereview.chromium.org/93643002/diff/70001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js
(right):

https://codereview.chromium.org/93643002/diff/70001/chrome/renderer/resources...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:55: var
fsName = response.pop()
Instead of doing this hack, probably better to just send back a dictionary in
both cases.

[Lei Zhang, 2014-01-08 23:49:53]

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:103:
base::string16(),  // TODO(thestig) Add a title.
On 2013/12/18 20:15:53, vandebo wrote:
> This is a privacy requirement.  Probably should put in an initial one before
UI
> review changes it.

Done. Re-using the one from the media galleries permissions dialog.

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.h
(right):

https://codereview.chromium.org/93643002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:29: class
MediaGalleriesGetMediaFileSystemsBase
On 2013/12/18 20:15:53, vandebo wrote:
> nit: Maybe name this just GetMediaFileSystemsBase ?

With the IDL changes, there's less of a need for a base class.

[vandebo (ex-Chrome), 2014-01-09 00:29:15]

https://codereview.chromium.org/93643002/diff/70001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js
(right):

https://codereview.chromium.org/93643002/diff/70001/chrome/renderer/resources...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:55: var
fsName = response.pop()
On 2013/12/18 20:15:53, vandebo wrote:
> Instead of doing this hack, probably better to just send back a dictionary in
> both cases.

This?

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

https://codereview.chromium.org/93643002/diff/190001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:418:
WebContents* contents = WebContents::FromRenderViewHost(render_view_host());
nit: pull this window parent finding logic into a help function and dedupe.

https://codereview.chromium.org/93643002/diff/190001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:436:
SelectDirectoryDialog::Callback callback = base::Bind(
This create a circular reference, but as we discussed, I think it'll all work
out ok.

https://codereview.chromium.org/93643002/diff/190001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:490:
list->AppendString(fsid);
I still think this should use another level of dictionary.

[Lei Zhang, 2014-01-09 05:28:38]

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

https://codereview.chromium.org/93643002/diff/190001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:418:
WebContents* contents = WebContents::FromRenderViewHost(render_view_host());
On 2014/01/09 00:29:16, vandebo wrote:
> nit: pull this window parent finding logic into a help function and dedupe.

Done.

https://codereview.chromium.org/93643002/diff/190001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:490:
list->AppendString(fsid);
On 2014/01/09 00:29:16, vandebo wrote:
> I still think this should use another level of dictionary.

Done.

[vandebo (ex-Chrome), 2014-01-09 16:17:29]

One question and LGTM

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

https://codereview.chromium.org/93643002/diff/270001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:485:
results->SetStringWithoutPathExpansion("selectedFileSystemName", fsid);
It looks like this value gets to the user unmolested.  Is fsid a useful
identifier?

[Lei Zhang, 2014-01-10 00:16:13]

jar/isherman: histograms OWNERS review from whoever gets to it first

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

https://codereview.chromium.org/93643002/diff/270001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:485:
results->SetStringWithoutPathExpansion("selectedFileSystemName", fsid);
On 2014/01/09 16:17:29, vandebo wrote:
> It looks like this value gets to the user unmolested.  Is fsid a useful
> identifier?

Nope, fixed in patch set 9.

[vandebo (ex-Chrome), 2014-01-10 00:20:11]

LGTM

[Ilya Sherman, 2014-01-10 03:23:13]

histograms.xml lgtm

[commit-bot: I haz the power, 2014-01-10 04:45:04]

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

[commit-bot: I haz the power, 2014-01-10 08:37:56]

Change committed as 244128