Refactored the PPB_Flash_File_ModuleLocal/FileRef to the new ppapi resource model

Refactored the PPB_Flash_File_ModuleLocal/FileRef to the new ppapi resource model

The refactors PPB_Flash_File_ModuleLocal/FileRef to the new resource model. Calls for both these interfaces are now made directly to the browser. This removes the in-process implementation for these interfaces also (as they are flash-only). Tests are added for PPB_Flash_File_ModuleLocal.

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

[raymes, 2012-11-19 18:45:04]

About 300 lines saved (even with tests added). 

brettw: High level and a few details. In particular, changes to the
browser_ppapi_host.h interface (and impl). Also changes to
pepper_plugin_registry.cc. Note that I gave plugins added on the command line a
default name if it isn't specified because this is used to generate the plugin's
data directory (this was a bug before). This also raises the question of whether
we need to be careful about ensuring plugin names are unique.

yzshen: The rest.

[brettw, 2012-11-20 00:09:34]

browser_ppapi_host and pepper_plugin_registry lgtm.

It sucks adding all of this stuff to browser_ppapi_host, but I don't think
there's a better way.

[raymes, 2012-11-20 00:26:39]

> It sucks adding all of this stuff to browser_ppapi_host, but I don't think
> there's a better way.

I agree. If it gets worse we can consider grouping stuff into structs
that are used for specific purposes.

On Mon, Nov 19, 2012 at 4:09 PM,  <brettw@chromium.org> wrote:
> browser_ppapi_host and pepper_plugin_registry lgtm.
>
> It sucks adding all of this stuff to browser_ppapi_host, but I don't think
> there's a better way.
>
> https://codereview.chromium.org/11359097/

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[raymes, 2012-11-20 00:26:40]

> It sucks adding all of this stuff to browser_ppapi_host, but I don't think
> there's a better way.

I agree. If it gets worse we can consider grouping stuff into structs
that are used for specific purposes.

On Mon, Nov 19, 2012 at 4:09 PM,  <brettw@chromium.org> wrote:
> browser_ppapi_host and pepper_plugin_registry lgtm.
>
> It sucks adding all of this stuff to browser_ppapi_host, but I don't think
> there's a better way.
>
> https://codereview.chromium.org/11359097/

[yzshen1, 2012-11-21 00:38:59]

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:26: //
PPB_Flash_File interfaces and so it doesn't matter.
nit: please remove 'and'.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:47:
if (host_->GetPpapiHost()->permissions().HasPermission(
Use GetPermissions(), please.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_flash_file_host.cc (right):

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:67: :
plugin_process_id_(plugin_process_id_),
Please don't use a suffix of '_' for the local variable.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:69: if
(profile_data_directory.empty() || plugin_name.empty()) {
good catch. :)

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:143: return
ppapi::PlatformFileErrorToPepperError(
is |file_handle| leaked here?

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:209:
kWritePermissions);
This should be kReadPermissions.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_flash_file_host.h (right):

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.h:42: // All file
messages are handled on the file thread by this message filter.
These messages are not handled on the FILE thread. Please update the comment.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.h:43: class
FileThreadMessageFilter : public ppapi::host::ResourceMessageFilter {
- Please move this to the .cc file.
- nit, optional: FileThread sounds like these messages are run on the FILE
thread. Maybe FileMessageFilter is better.

https://codereview.chromium.org/11359097/diff/10032/content/public/browser/br...
File content/public/browser/browser_ppapi_host.h (right):

https://codereview.chromium.org/11359097/diff/10032/content/public/browser/br...
content/public/browser/browser_ppapi_host.h:79: // Returns the users profile
data directory.
users -> user's

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
File ppapi/proxy/flash_file_resource.cc (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:43: FilePath::FromUTF8Unsafe(path_from));
nit: explicitly include file_path.h

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:59: int32_t error = SyncCall<IPC::Message>(
wrong indent.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:150:
StringVar::FromPPVar(enter.object()->GetAbsolutePath());
the PP_Var is leaked.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:168:
StringVar::FromPPVar(enter.object()->GetAbsolutePath());
- the PP_Var is leaked.
- Is it possible the reuse the code in OpenFileRef/QueryFileRef?

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:176: int32_t
FlashFileResource::OpenFileHelper(PP_Instance instance,
instance is not used.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:187: PepperFilePath pepper_path(domain_type,
FilePath::FromUTF8Unsafe(path));
[I don't know the answer. Just to make sure you have thought about it.]
Is FromUTF8Unsafe() suitable here (and the method below)?

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:205: int32_t
FlashFileResource::QueryFileHelper(PP_Instance instance,
instance is not used.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
File ppapi/proxy/flash_file_resource.h (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.h:15: struct PP_ArrayOutput;
Why do we need this?

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.h:22: public
NON_EXPORTED_BASE(thunk::PPB_Flash_File_API) {
You don't need NON_EXPORTED_BASE here.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:161: IPC_STRUCT_TRAITS_BEGIN(ppapi::DirEntry)
nit: move it the below ppapi::DeviceRefData.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1651:
IPC_MESSAGE_CONTROL1(PpapiPluginMsg_FlashFile_OpenFileReply,
This one is not needed.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppb_flash_pro...
File ppapi/proxy/ppb_flash_proxy.cc (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppb_flash_pro...
ppapi/proxy/ppb_flash_proxy.cc:29: #include
"ppapi/shared_impl/file_type_conversion.h"
is this needed?

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppb_flash_pro...
File ppapi/proxy/ppb_flash_proxy.h (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppb_flash_pro...
ppapi/proxy/ppb_flash_proxy.h:9: #include <vector>
not needed.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppb_flash_pro...
ppapi/proxy/ppb_flash_proxy.h:12: #include "ipc/ipc_platform_file.h"
probably this is not needed.

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
File ppapi/shared_impl/file_path.cc (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
ppapi/shared_impl/file_path.cc:7: #include <string>
this is not needed.

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
ppapi/shared_impl/file_path.cc:10: #include "base/utf_string_conversions.h"
this is not needed.

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
ppapi/shared_impl/file_path.cc:17: FilePath GetFilePathFromUTF8(const
std::string& utf8_path) {
this is not needed.

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
File ppapi/shared_impl/file_path.h (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
ppapi/shared_impl/file_path.h:8: #include <string>
no need to have this.

https://codereview.chromium.org/11359097/diff/10032/webkit/plugins/ppapi/plug...
File webkit/plugins/ppapi/plugin_delegate.h (left):

https://codereview.chromium.org/11359097/diff/10032/webkit/plugins/ppapi/plug...
webkit/plugins/ppapi/plugin_delegate.h:476: virtual base::PlatformFileError
OpenFile(
Please also change the mock.

https://codereview.chromium.org/11359097/diff/10032/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_flash_impl.cc (right):

https://codereview.chromium.org/11359097/diff/10032/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_flash_impl.cc:16: #include
"ppapi/shared_impl/file_path.h"
Quite a few includes and using's are not needed. please remove them.

[raymes, 2012-11-21 22:44:52]

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:26: //
PPB_Flash_File interfaces and so it doesn't matter.
On 2012/11/21 00:38:59, yzshen1 wrote:
> nit: please remove 'and'.

Done.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:47:
if (host_->GetPpapiHost()->permissions().HasPermission(
On 2012/11/21 00:38:59, yzshen1 wrote:
> Use GetPermissions(), please.

Done.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_flash_file_host.cc (right):

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:67: :
plugin_process_id_(plugin_process_id_),
oops thanks.
Done.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:69: if
(profile_data_directory.empty() || plugin_name.empty()) {
On 2012/11/21 00:38:59, yzshen1 wrote:
> good catch. :)

Done.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:143: return
ppapi::PlatformFileErrorToPepperError(
Yes good catch. I think it potentially is. This bug existed before.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:209:
kWritePermissions);
Good catch. Done.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_flash_file_host.h (right):

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.h:42: // All file
messages are handled on the file thread by this message filter.
On 2012/11/21 00:38:59, yzshen1 wrote:
> These messages are not handled on the FILE thread. Please update the comment.

Done.

https://codereview.chromium.org/11359097/diff/10032/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.h:43: class
FileThreadMessageFilter : public ppapi::host::ResourceMessageFilter {
On 2012/11/21 00:38:59, yzshen1 wrote:
> - Please move this to the .cc file.
> - nit, optional: FileThread sounds like these messages are run on the FILE
> thread. Maybe FileMessageFilter is better.

Done.

https://codereview.chromium.org/11359097/diff/10032/content/public/browser/br...
File content/public/browser/browser_ppapi_host.h (right):

https://codereview.chromium.org/11359097/diff/10032/content/public/browser/br...
content/public/browser/browser_ppapi_host.h:79: // Returns the users profile
data directory.
On 2012/11/21 00:38:59, yzshen1 wrote:
> users -> user's

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
File ppapi/proxy/flash_file_resource.cc (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:43: FilePath::FromUTF8Unsafe(path_from));
On 2012/11/21 00:38:59, yzshen1 wrote:
> nit: explicitly include file_path.h

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:59: int32_t error = SyncCall<IPC::Message>(
On 2012/11/21 00:38:59, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:150:
StringVar::FromPPVar(enter.object()->GetAbsolutePath());
On 2012/11/21 00:38:59, yzshen1 wrote:
> the PP_Var is leaked.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:168:
StringVar::FromPPVar(enter.object()->GetAbsolutePath());
On 2012/11/21 00:38:59, yzshen1 wrote:
> - the PP_Var is leaked.
> - Is it possible the reuse the code in OpenFileRef/QueryFileRef?

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:176: int32_t
FlashFileResource::OpenFileHelper(PP_Instance instance,
On 2012/11/21 00:38:59, yzshen1 wrote:
> instance is not used.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:187: PepperFilePath pepper_path(domain_type,
FilePath::FromUTF8Unsafe(path));
If the plugin provides a non-UTF8 string, a best-effort conversion will happen.
The browser process will still correctly check the permissions of the path so
nothing bad will happen.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.cc:205: int32_t
FlashFileResource::QueryFileHelper(PP_Instance instance,
On 2012/11/21 00:38:59, yzshen1 wrote:
> instance is not used.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
File ppapi/proxy/flash_file_resource.h (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.h:15: struct PP_ArrayOutput;
On 2012/11/21 00:38:59, yzshen1 wrote:
> Why do we need this?

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/flash_file_re...
ppapi/proxy/flash_file_resource.h:22: public
NON_EXPORTED_BASE(thunk::PPB_Flash_File_API) {
On 2012/11/21 00:38:59, yzshen1 wrote:
> You don't need NON_EXPORTED_BASE here.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:161: IPC_STRUCT_TRAITS_BEGIN(ppapi::DirEntry)
On 2012/11/21 00:38:59, yzshen1 wrote:
> nit: move it the below ppapi::DeviceRefData.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1651:
IPC_MESSAGE_CONTROL1(PpapiPluginMsg_FlashFile_OpenFileReply,
good catch. Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppb_flash_pro...
File ppapi/proxy/ppb_flash_proxy.cc (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppb_flash_pro...
ppapi/proxy/ppb_flash_proxy.cc:29: #include
"ppapi/shared_impl/file_type_conversion.h"
On 2012/11/21 00:38:59, yzshen1 wrote:
> is this needed?

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppb_flash_pro...
File ppapi/proxy/ppb_flash_proxy.h (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppb_flash_pro...
ppapi/proxy/ppb_flash_proxy.h:9: #include <vector>
On 2012/11/21 00:38:59, yzshen1 wrote:
> not needed.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/proxy/ppb_flash_pro...
ppapi/proxy/ppb_flash_proxy.h:12: #include "ipc/ipc_platform_file.h"
On 2012/11/21 00:38:59, yzshen1 wrote:
> probably this is not needed.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
File ppapi/shared_impl/file_path.cc (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
ppapi/shared_impl/file_path.cc:7: #include <string>
On 2012/11/21 00:38:59, yzshen1 wrote:
> this is not needed.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
ppapi/shared_impl/file_path.cc:10: #include "base/utf_string_conversions.h"
On 2012/11/21 00:38:59, yzshen1 wrote:
> this is not needed.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
ppapi/shared_impl/file_path.cc:17: FilePath GetFilePathFromUTF8(const
std::string& utf8_path) {
On 2012/11/21 00:38:59, yzshen1 wrote:
> this is not needed.

Done.

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
File ppapi/shared_impl/file_path.h (right):

https://codereview.chromium.org/11359097/diff/10032/ppapi/shared_impl/file_pa...
ppapi/shared_impl/file_path.h:8: #include <string>
On 2012/11/21 00:38:59, yzshen1 wrote:
> no need to have this.

Done.

https://codereview.chromium.org/11359097/diff/10032/webkit/plugins/ppapi/plug...
File webkit/plugins/ppapi/plugin_delegate.h (left):

https://codereview.chromium.org/11359097/diff/10032/webkit/plugins/ppapi/plug...
webkit/plugins/ppapi/plugin_delegate.h:476: virtual base::PlatformFileError
OpenFile(
On 2012/11/21 00:38:59, yzshen1 wrote:
> Please also change the mock.

Done.

https://codereview.chromium.org/11359097/diff/10032/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_flash_impl.cc (right):

https://codereview.chromium.org/11359097/diff/10032/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_flash_impl.cc:16: #include
"ppapi/shared_impl/file_path.h"
On 2012/11/21 00:38:59, yzshen1 wrote:
> Quite a few includes and using's are not needed. please remove them.

Done.

[yzshen1, 2012-12-03 18:12:23]

LGTM with two nits

Thanks!

https://codereview.chromium.org/11359097/diff/14078/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_flash_file_host.cc (right):

https://codereview.chromium.org/11359097/diff/14078/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:45: const
FilePath& profile_data_directory,
wrong indent.

https://codereview.chromium.org/11359097/diff/14078/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:107:
FileMessageFilter::FileMessageFilter(
It looks weird to have the implementation outside of the anonymous namespace.

[raymes, 2012-12-04 00:37:18]

+tsepez for ppapi_messages.h. Note that the attack surface is unchanged.

[Tom Sepez, 2012-12-04 00:46:00]

LGTM on _messages.h

[raymes, 2012-12-04 20:06:14]

https://chromiumcodereview.appspot.com/11359097/diff/14078/content/browser/re...
File content/browser/renderer_host/pepper/pepper_flash_file_host.cc (right):

https://chromiumcodereview.appspot.com/11359097/diff/14078/content/browser/re...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:45: const
FilePath& profile_data_directory,
On 2012/12/03 18:12:23, yzshen1 wrote:
> wrong indent.

Done.

https://chromiumcodereview.appspot.com/11359097/diff/14078/content/browser/re...
content/browser/renderer_host/pepper/pepper_flash_file_host.cc:107:
FileMessageFilter::FileMessageFilter(
On 2012/12/03 18:12:23, yzshen1 wrote:
> It looks weird to have the implementation outside of the anonymous namespace.

Done.

[commit-bot: I haz the power, 2012-12-04 20:19:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11359097/11108

[commit-bot: I haz the power, 2012-12-04 20:19:48]

Presubmit check for 11359097-11108 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
Missing PPAPI IDL for private interface, please generate IDL:

***************
  ppapi/c/private/ppb_flash_file.h
***************

Presubmit checks took 6.0s to calculate.

[raymes, 2012-12-07 19:51:53]

yzshen: PTAL. The only change is that I fixed the issue causing
http://code.google.com/p/chromium/issues/detail?id=164450 where the
plugin_child_id was being used to check file permissions instead of the
render_child_id. This is also the issue cpu was seeing.

[yzshen1, 2012-12-07 20:10:23]

I have difficulty to find out what is the real change.
Could you please tell me which patchset to diff against, and which files to look
at?
Thanks!

On 2012/12/07 19:51:53, raymes wrote:
> yzshen: PTAL. The only change is that I fixed the issue causing
> http://code.google.com/p/chromium/issues/detail?id=164450 where the
> plugin_child_id was being used to check file permissions instead of the
> render_child_id. This is also the issue cpu was seeing.

[raymes, 2012-12-07 20:45:08]

You can diff patch set 14. Thanks.

On Fri, Dec 7, 2012 at 12:10 PM,  <yzshen@chromium.org> wrote:
> I have difficulty to find out what is the real change.
> Could you please tell me which patchset to diff against, and which files to
> look
> at?
> Thanks!
>
>
> On 2012/12/07 19:51:53, raymes wrote:
>>
>> yzshen: PTAL. The only change is that I fixed the issue causing
>> http://code.google.com/p/chromium/issues/detail?id=164450 where the
>> plugin_child_id was being used to check file permissions instead of the
>> render_child_id. This is also the issue cpu was seeing.
>
>
>
>
> https://codereview.chromium.org/11359097/

[raymes, 2012-12-07 20:45:10]

You can diff patch set 14. Thanks.

On Fri, Dec 7, 2012 at 12:10 PM,  <yzshen@chromium.org> wrote:
> I have difficulty to find out what is the real change.
> Could you please tell me which patchset to diff against, and which files to
> look
> at?
> Thanks!
>
>
> On 2012/12/07 19:51:53, raymes wrote:
>>
>> yzshen: PTAL. The only change is that I fixed the issue causing
>> http://code.google.com/p/chromium/issues/detail?id=164450 where the
>> plugin_child_id was being used to check file permissions instead of the
>> render_child_id. This is also the issue cpu was seeing.
>
>
>
>
> https://codereview.chromium.org/11359097/

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[yzshen1, 2012-12-07 21:06:17]

lgtm

[commit-bot: I haz the power, 2012-12-07 21:07:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11359097/47005

[commit-bot: I haz the power, 2012-12-07 21:08:12]

Presubmit check for 11359097-47005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
Missing PPAPI IDL for private interface, please generate IDL:

***************
  ppapi/c/private/ppb_flash_file.h
***************

Presubmit checks took 5.9s to calculate.