Add native file picker impl using SelectFileDialog.

Add native file picker impl using SelectFileDialog.
Currently there's no good way to send the file data back through to the client. We'll work on that issue in a followup CL.

BUG=129769
TBR=jhawkins@chromium.org


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

[Steve McKay, 2012-10-06 21:09:25]

In case Greg gets bored this weekend :)

[Greg Billock, 2012-10-08 20:36:12]

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/native_file_picker_service.cc (right):

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:55: // pick+*/* intents.
"pick + */*"

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:63: // Weak pointer to
the web contents on which we'll be displayed.
we'll --> the selector

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:69: class
FileSelectedListener : public ui::SelectFileDialog::Listener {
It looks like this should be an inner class on NativeFilePickerService, or (even
better) that that class should be a ui::SelectFileDialog::Listener.

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:125:
ui::SelectFileDialog* dialog = ui::SelectFileDialog::Create(listener, NULL);
Where does this get deleted? Do we need to hang onto it?

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:144: delete this;
Let's not have service hosts be self-deleting. They should get managed by
whoever calls the code to create them.

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:153: return new
NativeFilePickerService(web_contents);
This leaves us without a good hook for testing. Putting the impl in a .h file
enables us to write a unit test.

[groby-ooo-7-16, 2012-10-08 20:58:21]

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/native_file_picker_service.cc (right):

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:153: return new
NativeFilePickerService(web_contents);
If we need to inject here for testing, I suggest looking at
URLFetcher/URLFetcherFactory and how they implement the injection of a for-test
factory.

On 2012/10/08 20:36:12, Greg Billock wrote:
> This leaves us without a good hook for testing. Putting the impl in a .h file
> enables us to write a unit test.

[Steve McKay, 2012-10-08 22:39:53]

PTAL. Am looking into writing browser test.

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/native_file_picker_service.cc (right):

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:125:
ui::SelectFileDialog* dialog = ui::SelectFileDialog::Create(listener, NULL);
On 2012/10/08 20:36:12, Greg Billock wrote:
> Where does this get deleted? Do we need to hang onto it?

Now ref counted.

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:144: delete this;
On 2012/10/08 20:36:12, Greg Billock wrote:
> Let's not have service hosts be self-deleting. They should get managed by
> whoever calls the code to create them.

For the time being, let's keep this self-deleting. We can change this when the
refactoring happens.

http://codereview.chromium.org/11071005/diff/10002/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:153: return new
NativeFilePickerService(web_contents);
On 2012/10/08 20:58:21, groby wrote:
> If we need to inject here for testing, I suggest looking at
> URLFetcher/URLFetcherFactory and how they implement the injection of a
for-test
> factory.
> 
> On 2012/10/08 20:36:12, Greg Billock wrote:
> > This leaves us without a good hook for testing. Putting the impl in a .h
file
> > enables us to write a unit test.
> 

Talked this over, can use test factory for the dialog to write browser test.

[Steve McKay, 2012-10-09 23:19:01]

PTAL.

[Greg Billock, 2012-10-10 17:12:51]

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/intents/nat...
File chrome/browser/intents/native_services.h (right):

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/intents/nat...
chrome/browser/intents/native_services.h:61: // Returns a NativeService instance
suitable to handle
Add what the |url| and |web_contents| are.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/native_file_picker_service.cc (right):

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:29: // static
I don't think you need this for an anonymous method.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:47: if (description_id)
braces around multi-line statement

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:51:
net::GetExtensionsForMimeType(mime_type, extensions);
Can this be moved above the description stuff? It would also be nice to just
write ...(mime_type, &info->extensions.back()) and avoid the temporary.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:55: // pick + */*
intents.
It'll be the creator's responsibility to make sure the lifecycle extends past
the dispatcher. Do you want to add that in the comment of IntentServiceHost?

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:63: virtual void
FileSelected(
Eliminate whitespace between all these overrides. -- Makes it clear they're all
for the listener.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:70: private:
add blank line above "private:"

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:71: virtual
~NativeFilePickerService();
Destructor should now be public, no?

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:108: const string16
title = FilePickerFactory::GetServiceTitle();
Will this be the same as the title in the picker? Seems fine at first thought.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:124: // Return the
readable path until we can return a real blob.
Add a TODO that we want to return the FilePath. (Caller can turn it into a blob
if needed, but we'll need the real filepath for some situations.)

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:129: void
NativeFilePickerService::MultiFilesSelected(
This is an empty impl in the superclass. Just leave out?

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:140:
NativeFilePickerService::~NativeFilePickerService() {}
Move under constructor

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/web_intent_picker_controller.cc:139: // and a service
once a response is received.
I think we can just do this with a closure using RegisterReplyNotification on
the original dispatcher, right?

dispatcher.RegisterReplyNotification(&DeleteService, service);

void DeleteService(IntentServiceHost* service) {
  delete service;
}

[Steve McKay, 2012-10-10 23:12:57]

Addressed your comments. PTAL.

Still trying to work around android issues. I'll post another patch for that
tomorrow.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/intents/nat...
File chrome/browser/intents/native_services.h (right):

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/intents/nat...
chrome/browser/intents/native_services.h:61: // Returns a NativeService instance
suitable to handle
On 2012/10/10 17:12:51, Greg Billock wrote:
> Add what the |url| and |web_contents| are.

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/native_file_picker_service.cc (right):

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:29: // static
On 2012/10/10 17:12:51, Greg Billock wrote:
> I don't think you need this for an anonymous method.

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:47: if (description_id)
On 2012/10/10 17:12:51, Greg Billock wrote:
> braces around multi-line statement

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:51:
net::GetExtensionsForMimeType(mime_type, extensions);
On 2012/10/10 17:12:51, Greg Billock wrote:
> Can this be moved above the description stuff? It would also be nice to just
> write ...(mime_type, &info->extensions.back()) and avoid the temporary.

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:55: // pick + */*
intents.
On 2012/10/10 17:12:51, Greg Billock wrote:
> It'll be the creator's responsibility to make sure the lifecycle extends past
> the dispatcher. Do you want to add that in the comment of IntentServiceHost?

Done. Just simply stated that the creator should be responsible for any
instances.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:63: virtual void
FileSelected(
On 2012/10/10 17:12:51, Greg Billock wrote:
> Eliminate whitespace between all these overrides. -- Makes it clear they're
all
> for the listener.

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:63: virtual void
FileSelected(
On 2012/10/10 17:12:51, Greg Billock wrote:
> Eliminate whitespace between all these overrides. -- Makes it clear they're
all
> for the listener.

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:70: private:
On 2012/10/10 17:12:51, Greg Billock wrote:
> add blank line above "private:"

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:71: virtual
~NativeFilePickerService();
On 2012/10/10 17:12:51, Greg Billock wrote:
> Destructor should now be public, no?

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:108: const string16
title = FilePickerFactory::GetServiceTitle();
On 2012/10/10 17:12:51, Greg Billock wrote:
> Will this be the same as the title in the picker? Seems fine at first thought.

Up to the implementation, but that is how it works out.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:124: // Return the
readable path until we can return a real blob.
On 2012/10/10 17:12:51, Greg Billock wrote:
> Add a TODO that we want to return the FilePath. (Caller can turn it into a
blob
> if needed, but we'll need the real filepath for some situations.)

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:129: void
NativeFilePickerService::MultiFilesSelected(
On 2012/10/10 17:12:51, Greg Billock wrote:
> This is an empty impl in the superclass. Just leave out?

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/native_file_picker_service.cc:140:
NativeFilePickerService::~NativeFilePickerService() {}
On 2012/10/10 17:12:51, Greg Billock wrote:
> Move under constructor

Done.

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/11071005/diff/17001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/web_intent_picker_controller.cc:139: // and a service
once a response is received.
On 2012/10/10 17:12:51, Greg Billock wrote:
> I think we can just do this with a closure using RegisterReplyNotification on
> the original dispatcher, right?
> 
> dispatcher.RegisterReplyNotification(&DeleteService, service);
> 
> void DeleteService(IntentServiceHost* service) {
>   delete service;
> }

Done.

[Steve McKay, 2012-10-11 21:11:30]

Android should be good now. PTAL.

[sky, 2012-10-11 21:50:47]

What parts of this do you need me to review?

[Steve McKay, 2012-10-11 22:09:51]

On 2012/10/11 21:50:47, sky wrote:
> What parts of this do you need me to review?

sky@, just the delete of chrome/browser/ui/views/web_intents_native_services.cc

[sky, 2012-10-11 23:29:08]

The file you deleted? LGTM

[Greg Billock, 2012-10-11 23:50:04]

On 2012/10/11 23:29:08, sky wrote:
> The file you deleted? LGTM

lgtm

[commit-bot: I haz the power, 2012-10-12 01:42:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/11071005/14011

[commit-bot: I haz the power, 2012-10-12 02:39:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-12 04:30:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/11071005/26002

[commit-bot: I haz the power, 2012-10-12 06:49:48]

Change committed as 161517