Implement the file chooser as a new resource "host"

This implements the PPB_FileChooser resource as a new-style IPC-only resource.

Note that the new file name is file_chooser_resource in the proxy. I decided to drop the ppb_ prefix for the "new-style" files to help differentiate them, and also because it's technically wrong. PPB is an interface, and a resource "object" may support multiple interfaces. I think FooResource is easier to type and read.

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

[brettw, 2012-06-30 18:53:03]

This looks scarier than it is.

It builds on this patch still under review:
https://chromiumcodereview.appspot.com/10578043/

Most of the changed files are trivial plumbing. The nontrivial parts are in the
implementation of the new resource on the plugin and host side. However, most of
this code is just copied from the old implementation, and it now has better
unittests :)

[brettw, 2012-06-30 19:01:58]

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/examples/2d/...
File ppapi/examples/2d/paint_manager_example.cc (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/examples/2d/...
ppapi/examples/2d/paint_manager_example.cc:14: #include
"ppapi/cpp/dev/file_chooser_dev.h"
Ignore the changes in this file; I'll revert before checkin. I'm just using this
as a manual integration ssmoketest.

[bbudge, 2012-07-02 20:06:06]

https://chromiumcodereview.appspot.com/10544089/diff/24001/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host.cc:145:
std::vector<WebKit::WebString> web_accept(accept_mime_types.size());
I like 'mime_types' or 'web_accept_mime_types' better here.

https://chromiumcodereview.appspot.com/10544089/diff/24001/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host.cc:147: web_accept[0] =
WebKit::WebString::fromUTF8(
s/0/i

https://chromiumcodereview.appspot.com/10544089/diff/24001/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host.h (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host.h:25: // Structure to store the
information of chosen files.
s/the information of/information about

https://chromiumcodereview.appspot.com/10544089/diff/24001/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host_browsertest.cc:84: // caught by
the render thread's test message sink).
opening paren missing?

https://chromiumcodereview.appspot.com/10544089/diff/24001/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host_browsertest.cc:92: // Basic
validateion of request.
sp. validation

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
File ppapi/proxy/file_chooser_resource.cc (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource.cc:34: scoped_refptr<TrackedCallback>
callback) {
Shouldn't we check that we have a user gesture here?

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource.cc:49: int32_t
FileChooserResource::Show0_5(scoped_refptr<TrackedCallback> callback) {
User gesture?

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource.cc:57: // Return the next resource in the
queue. These resource have already been
s/resource/resources

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource.cc:77: return;
Check that output is non-NULL?

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
File ppapi/proxy/file_chooser_resource.h (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource.h:46: virtual PP_Resource GetNextChosenFile()
OVERRIDE;
Maybe move this after the Show* methods?

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/plugin...
File ppapi/proxy/plugin_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/plugin...
ppapi/proxy/plugin_dispatcher.cc:200: */
?

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/plugin...
File ppapi/proxy/plugin_resource.h (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/plugin...
ppapi/proxy/plugin_resource.h:49: int32_t CallRenderer(const IPC::Message& msg);
It seems odd that sequence numbers are signed ints.

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/plugin...
ppapi/proxy/plugin_resource.h:54: int next_sequence_number_;
int -> int32_t or uint32_t?

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/shared_impl/...
File ppapi/shared_impl/ppb_file_ref_shared.cc (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/shared_impl/...
ppapi/shared_impl/ppb_file_ref_shared.cc:20: //   
DCHECK(!create_info_.resource.is_null());
?

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/thunk/interf...
File ppapi/thunk/interfaces_ppb_public_dev.h (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/thunk/interf...
ppapi/thunk/interfaces_ppb_public_dev.h:13: //PROXIED_API(PPB_FileChooser)
?

[brettw, 2012-07-07 05:28:04]

New snap up. I think I got everything.

https://chromiumcodereview.appspot.com/10544089/diff/24001/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host.cc:147: web_accept[0] =
WebKit::WebString::fromUTF8(
WHOOPS! Thanks for catching this!

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
File ppapi/proxy/file_chooser_resource.cc (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource.cc:34: scoped_refptr<TrackedCallback>
callback) {
This was addressed on the host side (this code is untrusted).

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource.cc:77: return;
For simple out params I don't usually encourage that since if somebody forgets
to pass it it's an obvious crash that's just as easy to see in a debugger as a
check.

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
File ppapi/proxy/file_chooser_resource.h (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource.h:46: virtual PP_Resource GetNextChosenFile()
OVERRIDE;
This matches the order in the original class, which is what the rule is (there
they're grouped by interface version so it makes more sense).

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/plugin...
File ppapi/proxy/plugin_resource.h (right):

https://chromiumcodereview.appspot.com/10544089/diff/24001/ppapi/proxy/plugin...
ppapi/proxy/plugin_resource.h:49: int32_t CallRenderer(const IPC::Message& msg);
Well, it's going to theoretically wrap anyway (so it doesn't matter) and the
style guide says to use signed values except for bitfields, so I went with
signed.

[bbudge, 2012-07-09 20:41:36]

A few questions, otherwise LGTM.

http://codereview.chromium.org/10544089/diff/32001/content/renderer/pepper/co...
File content/renderer/pepper/content_instance_glue.h (right):

http://codereview.chromium.org/10544089/diff/32001/content/renderer/pepper/co...
content/renderer/pepper/content_instance_glue.h:16: class ContentInstanceGlue {
Can you come up with a better name for this? Also, what is the rule for when
something is named content/ContentXxx vs. content/PepperXxx?

http://codereview.chromium.org/10544089/diff/32001/ppapi/proxy/host_dispatche...
File ppapi/proxy/host_dispatcher.cc (right):

http://codereview.chromium.org/10544089/diff/32001/ppapi/proxy/host_dispatche...
ppapi/proxy/host_dispatcher.cc:189: }
It's not clear to me from the rest of the patch why this is needed here.

[brettw, 2012-07-13 04:57:41]

http://codereview.chromium.org/10544089/diff/32001/ppapi/proxy/host_dispatche...
File ppapi/proxy/host_dispatcher.cc (right):

http://codereview.chromium.org/10544089/diff/32001/ppapi/proxy/host_dispatche...
ppapi/proxy/host_dispatcher.cc:189: }
That;s actually what allows the host to receive messages. It's hooked up through
this filter interface which I added earlier. But when I added the filters I
forgot to actually forward messages through them. This adds that missing code so
it actually works.

[jam, 2012-07-18 00:52:19]

http://codereview.chromium.org/10544089/diff/43001/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/10544089/diff/43001/content/content_tests.gypi...
content/content_tests.gypi:545:
'renderer/pepper/pepper_file_chooser_host_browsertest.cc',
did you mean to add this to content_unittests target instead? this is the
content_browsertest target, which does nothing now since it's still a TODO on my
plate to get it working :)

[brettw, 2012-07-18 19:15:33]

http://codereview.chromium.org/10544089/diff/43001/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/10544089/diff/43001/content/content_tests.gypi...
content/content_tests.gypi:545:
'renderer/pepper/pepper_file_chooser_host_browsertest.cc',
Do I lose anything by moving it? I thought I needed a browsertest. I actually
did run this test, although I had to --gtest_filter to run only this one test.

[jam, 2012-07-18 21:45:40]

http://codereview.chromium.org/10544089/diff/43001/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/10544089/diff/43001/content/content_tests.gypi...
content/content_tests.gypi:545:
'renderer/pepper/pepper_file_chooser_host_browsertest.cc',
On 2012/07/18 19:15:33, brettw wrote:
> Do I lose anything by moving it? I thought I needed a browsertest. I actually
> did run this test, although I had to --gtest_filter to run only this one test.

I'm not sure what you mean by needing a browsertest. That binary, for now, is
basically just like content_unittests since there's no content browser test
infrastructure yet.

content_browsertests doesn't run any buildbot, so this test isn't running
anywhere now.