CRX FileSystem Pepper private API

CRX FileSystem Pepper private API

pp::ExtCrxFileSystemPrivate is introduced in this change to allow plugin to mount (readonly) CRX extension directory as a filesystem.  Files can be access through pp::ExtCrxFileRefPrivate (which is a subclass of pp::FileRef) just like normal file, and the path would look like "/manifest.json".  See ppapi/example/crxfs for example.

Some keypoints in this change:

 * pepper resource/host architecture:
   - please refer to ppapi/proxy/ext_crx_file_system_private_resource.h.

 * webkit/fileapi related:
   - Changes run in browser
   - Isoloated filesystem is the underlying filesystem
   - Grant read permission to corresponding renderer of the plugin
   - See chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc

 * extension related:
   - Changes run in browser
   - This is for getting extension installed directory to mount

TEST=out/Debug/chrome
  --register-pepper-plugins="out/Debug/lib/libppapi_example_crxfs.so#PPAPI Tests##1.2.3;application/x-ppapi-example-crxfs" \
  ppapi/examples/crxfs/crxfs.html

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

[victorhsieh, 2013-04-17 23:09:20]

PTAL

@teravest,
would you please review pepper part as Raymes and Yuzhu (I discussed this API
with them) are OOO next week?

@kinuko,
would you please review fileapi related code in
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:MakeAndGrantCrxDirectoryAccess?

@yoz,
would you please review extension related code in
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:MountCrxDirectory?

@palmer,
would you please check if there's any security issue in the change?  The general
design of pepper plugin/browser/renderer communication is describe in
ppapi/proxy/ext_crx_file_system_private_resource.h.  It may be worth to look
into
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
as well.

[victorhsieh, 2013-04-18 00:04:18]

BTW, here is the instruction to run the demo in ppapi/examples/crxfs:
1. ninja -C out/Debug ppapi_example_crxfs
2. launch chrome by: out/Debug/chrome 
--register-pepper-plugins="out/Debug/lib/libppapi_example_crxfs.so#PPAPI
Tests##1.2.3;application/x-ppapi-example-crxfs"
3. Go chrome://extensions/ , enable developer mode, and load unpacked extension
from ppapi/example/crxfs
4. Open new tab and click the new icon.

[victorhsieh, 2013-04-18 01:09:03]

Please expect a design doc tomorrow, as that would help understanding the
interaction between those subsystems easier.

[kinuko, 2013-04-18 13:45:03]

I'll take another look (maybe after a design doc becomes available), but
webkit/fileapi related part general looks good. I left some cosmetic comments
(it's cool to see this feature being implemented anyway!)

https://codereview.chromium.org/14188019/diff/9001/chrome/browser/renderer_ho...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/9001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:41:
extensions::ExtensionSystem* ext_system =
nit: can we name this 'extension_system' (rather than shorter abbrev'ed name)

https://codereview.chromium.org/14188019/diff/9001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:46:
const ExtensionService* ext_service = ext_system->extension_service();
ditto

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
File chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc (right):

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc:21: return
GURL("filesystem:chrome-extension://" + origin + "/isolated/" + fsid +
I think you'd better use 
extensions::Extension::GetBaseURLFromExtensionId(origin) to get
"chrome-extension://<origin>"

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc:22:
"/crxfs/");
Can we instead use fileapi::GetIsolatedFileSystemRootURIString() defined in
webkit/fileapi/file_system_util.h?

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
File chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.h (right):

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.h:23: // access
(through FileSystemPeeker) on the host side.
Hmm, this Peeker design is interesting..

[teravest, 2013-04-18 16:10:38]

I was nervous about the messages to the browser and the renderer at first, but
it didn't look too bad in the implementation.

https://codereview.chromium.org/14188019/diff/9001/chrome/browser/renderer_ho...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/9001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:68:
const GURL url = browser_ppapi_host->GetDocumentURLForInstance(instance);
Did you mean const GURL& url?

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
File chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.h (right):

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.h:41: virtual
GURL GetRootUrl() const OVERRIDE;
This can probably return const GURL&

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
File ppapi/cpp/private/ext_crx_file_ref_private.h (right):

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
ppapi/cpp/private/ext_crx_file_ref_private.h:26: const char* path) {
nit: whitespace looks funky here.

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
File ppapi/cpp/private/ext_crx_file_system_private.h (right):

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
ppapi/cpp/private/ext_crx_file_system_private.h:19:
ExtCrxFileSystemPrivate(const InstanceHandle& instance);
explicit

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
ppapi/cpp/private/ext_crx_file_system_private.h:20:
ExtCrxFileSystemPrivate(const ExtCrxFileSystemPrivate& other);
explicit

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
ppapi/cpp/private/ext_crx_file_system_private.h:23: int32_t Open(const
CompletionCallback& cc);
It looks like this can probably be const?

https://codereview.chromium.org/14188019/diff/9001/ppapi/proxy/ext_crx_file_s...
File ppapi/proxy/ext_crx_file_system_private_resource.h (right):

https://codereview.chromium.org/14188019/diff/9001/ppapi/proxy/ext_crx_file_s...
ppapi/proxy/ext_crx_file_system_private_resource.h:12: // "isolated filesystem"
points to currernt extension directory in browser.
s/currernt/current/

https://codereview.chromium.org/14188019/diff/9001/ppapi/thunk/ppb_file_ref_t...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/9001/ppapi/thunk/ppb_file_ref_t...
ppapi/thunk/ppb_file_ref_thunk.cc:33: if (enter_file_system.failed() &&
enter_crxfs.failed())
I think you want ||, not && here.

[victorhsieh, 2013-04-18 19:28:38]

PTAL

I've sent the design doc offline.  Let me know if you have any question.

BTW, this CL depends on https://codereview.chromium.org/14079005/ to run safely.

https://codereview.chromium.org/14188019/diff/9001/chrome/browser/renderer_ho...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/9001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:41:
extensions::ExtensionSystem* ext_system =
On 2013/04/18 13:45:03, kinuko wrote:
> nit: can we name this 'extension_system' (rather than shorter abbrev'ed name)

Done.

https://codereview.chromium.org/14188019/diff/9001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:46:
const ExtensionService* ext_service = ext_system->extension_service();
On 2013/04/18 13:45:03, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/14188019/diff/9001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:68:
const GURL url = browser_ppapi_host->GetDocumentURLForInstance(instance);
On 2013/04/18 16:10:38, teravest wrote:
> Did you mean const GURL& url?

Done.

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
File chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc (right):

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc:21: return
GURL("filesystem:chrome-extension://" + origin + "/isolated/" + fsid +
On 2013/04/18 13:45:03, kinuko wrote:
> I think you'd better use 
> extensions::Extension::GetBaseURLFromExtensionId(origin) to get
> "chrome-extension://<origin>"

Done.

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc:22:
"/crxfs/");
On 2013/04/18 13:45:03, kinuko wrote:
> Can we instead use fileapi::GetIsolatedFileSystemRootURIString() defined in
> webkit/fileapi/file_system_util.h?

Done.

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
File chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.h (right):

https://codereview.chromium.org/14188019/diff/9001/chrome/renderer/pepper/pep...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.h:41: virtual
GURL GetRootUrl() const OVERRIDE;
On 2013/04/18 16:10:38, teravest wrote:
> This can probably return const GURL&

Done.

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
File ppapi/cpp/private/ext_crx_file_ref_private.h (right):

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
ppapi/cpp/private/ext_crx_file_ref_private.h:26: const char* path) {
On 2013/04/18 16:10:38, teravest wrote:
> nit: whitespace looks funky here.

Done.

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
File ppapi/cpp/private/ext_crx_file_system_private.h (right):

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
ppapi/cpp/private/ext_crx_file_system_private.h:19:
ExtCrxFileSystemPrivate(const InstanceHandle& instance);
On 2013/04/18 16:10:38, teravest wrote:
> explicit

Done.

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
ppapi/cpp/private/ext_crx_file_system_private.h:20:
ExtCrxFileSystemPrivate(const ExtCrxFileSystemPrivate& other);
On 2013/04/18 16:10:38, teravest wrote:
> explicit

Done.

https://codereview.chromium.org/14188019/diff/9001/ppapi/cpp/private/ext_crx_...
ppapi/cpp/private/ext_crx_file_system_private.h:23: int32_t Open(const
CompletionCallback& cc);
On 2013/04/18 16:10:38, teravest wrote:
> It looks like this can probably be const?

For the current implementation, yes.  But I'd safe not making it a const as
future change may make this "open" non-const.

https://codereview.chromium.org/14188019/diff/9001/ppapi/proxy/ext_crx_file_s...
File ppapi/proxy/ext_crx_file_system_private_resource.h (right):

https://codereview.chromium.org/14188019/diff/9001/ppapi/proxy/ext_crx_file_s...
ppapi/proxy/ext_crx_file_system_private_resource.h:12: // "isolated filesystem"
points to currernt extension directory in browser.
On 2013/04/18 16:10:38, teravest wrote:
> s/currernt/current/

Done.

https://codereview.chromium.org/14188019/diff/9001/ppapi/thunk/ppb_file_ref_t...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/9001/ppapi/thunk/ppb_file_ref_t...
ppapi/thunk/ppb_file_ref_thunk.cc:33: if (enter_file_system.failed() &&
enter_crxfs.failed())
On 2013/04/18 16:10:38, teravest wrote:
> I think you want ||, not && here.

No, actually at least one of them will be failed.  We want to move forward when
either one succeeds.

https://codereview.chromium.org/14188019/diff/21001/ppapi/ppapi_host.gypi
File ppapi/ppapi_host.gypi (right):

https://codereview.chromium.org/14188019/diff/21001/ppapi/ppapi_host.gypi#new...
ppapi/ppapi_host.gypi:16:
'../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',
This is because base/memory/singleton.h depends on this annotation, but it's not
declared in dependent.  I'll send another CL to fix this.

[Yoyo Zhou, 2013-04-19 17:52:56]

MountCrxDirectory LGTM with nits

https://codereview.chromium.org/14188019/diff/21001/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:30:
// Returns filesystem id of isolated filesystem if accesss, or empty string
accesss?

https://codereview.chromium.org/14188019/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:32:
// allows UI thread access.
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))?

[teravest, 2013-04-19 17:57:17]

lgtm

[victorhsieh, 2013-04-19 22:52:56]

BTW, I just learned that the I need to move one line change from 
ppapi/thunk/interfaces_ppb_private.h to
ppapi/thunk/interfaces_ppb_private_no_permissions.h to make it work in NaCl.

https://codereview.chromium.org/14188019/diff/21001/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:30:
// Returns filesystem id of isolated filesystem if accesss, or empty string
On 2013/04/19 17:52:56, Yoyo Zhou wrote:
> accesss?

Done.

https://codereview.chromium.org/14188019/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:32:
// allows UI thread access.
On 2013/04/19 17:52:56, Yoyo Zhou wrote:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))?

Done.

[kinuko, 2013-04-22 15:12:35]

https://codereview.chromium.org/14188019/diff/30001/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/30001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:75:
browser_ppapi_host->GetPpapiHost()->SendReply(
Just to make sure, don't we need to go back to IO thread before calling this?

https://codereview.chromium.org/14188019/diff/30001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:93:
browser_ppapi_host->GetPpapiHost()->SendReply(
ditto

https://codereview.chromium.org/14188019/diff/30001/ppapi/examples/crxfs/crxf...
File ppapi/examples/crxfs/crxfs.cc (right):

https://codereview.chromium.org/14188019/diff/30001/ppapi/examples/crxfs/crxf...
ppapi/examples/crxfs/crxfs.cc:102: }
Probably it doesn't necessarily be in this example, but have you made sure
writing to any files in the crx fs fails?

[victorhsieh, 2013-04-22 17:51:38]

https://codereview.chromium.org/14188019/diff/30001/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/30001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:75:
browser_ppapi_host->GetPpapiHost()->SendReply(
On 2013/04/22 15:12:36, kinuko wrote:
> Just to make sure, don't we need to go back to IO thread before calling this?

Since IPC design doc suggests to send message in IO thread, I've followed the
advice now.

https://codereview.chromium.org/14188019/diff/30001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:93:
browser_ppapi_host->GetPpapiHost()->SendReply(
On 2013/04/22 15:12:36, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/14188019/diff/30001/ppapi/examples/crxfs/crxf...
File ppapi/examples/crxfs/crxfs.cc (right):

https://codereview.chromium.org/14188019/diff/30001/ppapi/examples/crxfs/crxf...
ppapi/examples/crxfs/crxfs.cc:102: }
On 2013/04/22 15:12:36, kinuko wrote:
> Probably it doesn't necessarily be in this example, but have you made sure
> writing to any files in the crx fs fails?

Yes, it fails.

[victorhsieh, 2013-04-22 19:31:49]

I also made a small type change in pepper api (patch set 5).  Sorry for making
the change during code review, but I feel it's better than starting another
review for this.

[kinuko, 2013-04-23 08:44:19]

One more question reg: threading and lifetime issue. (Once it becomes clear it
looks good to me)

https://codereview.chromium.org/14188019/diff/40001/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/40001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:69:
browser_ppapi_host->GetPpapiHost()->SendReply(
How can we make sure browser_ppapi_host (and PpapiHost()) is still valid?

https://codereview.chromium.org/14188019/diff/40001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:75:
void ReplyInUIThread(content::BrowserPpapiHost* browser_ppapi_host,
nit: PostReplyToIOThread may make more sense to me.. (no strong opinion though)

Or maybe just call this Reply or ReplyOnIOThread and call PostTask only if
CurrentlyOn(IO) is false

https://codereview.chromium.org/14188019/diff/40001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_system_host.cc (right):

https://codereview.chromium.org/14188019/diff/40001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_system_host.cc:68: }
nit: indent is off

[victorhsieh, 2013-04-23 22:52:52]

https://codereview.chromium.org/14188019/diff/40001/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/40001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:69:
browser_ppapi_host->GetPpapiHost()->SendReply(
On 2013/04/23 08:44:20, kinuko wrote:
> How can we make sure browser_ppapi_host (and PpapiHost()) is still valid?

Thanks for catching.  I rewrite the PostTask flow.  Now this class supports
WeakPtr and Bind will check it.  browser_ppapi_host shouldn't die before this
class.

https://codereview.chromium.org/14188019/diff/40001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_system_host.cc (right):

https://codereview.chromium.org/14188019/diff/40001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_system_host.cc:68: }
On 2013/04/23 08:44:20, kinuko wrote:
> nit: indent is off

Done.

[kinuko, 2013-04-24 04:52:47]

lgtm

[palmer, 2013-04-25 00:01:23]

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

https://codereview.chromium.org/14188019/diff/45001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1352: std::string /* fsid */)
I don't feel great about this arbitrary string |fsid|. Can we make it come from
a limited set of static strings, or get rid of it? It is ultimately passed down
to the isolated filesystem API, where it is an optional parameter anyway.

https://codereview.chromium.org/14188019/diff/45001/ppapi/thunk/ppb_file_ref_...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/45001/ppapi/thunk/ppb_file_ref_...
ppapi/thunk/ppb_file_ref_thunk.cc:29: // Make sure the file_system is one of
expected types.
NIT: "...one of the..."

[victorhsieh, 2013-04-25 02:21:51]

kinuko@, please advice on the issue, thanks!

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

https://codereview.chromium.org/14188019/diff/45001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1352: std::string /* fsid */)
On 2013/04/25 00:01:23, Chromium Palmer wrote:
> I don't feel great about this arbitrary string |fsid|. Can we make it come
from
> a limited set of static strings, or get rid of it? It is ultimately passed
down
> to the isolated filesystem API, where it is an optional parameter anyway.

I guess you're talking about the case that a malicious plugin sends a fake fsid
to renderer host (instead of forwarding what the browser host gets from isolated
filesystem).

kinuko@, the filesystem:chrome-extension:// passed to fileapi can be manipulated
in current implementatoin.  Do you have any suggestion on making this secure
from fileapi's perspective?

https://codereview.chromium.org/14188019/diff/45001/ppapi/thunk/ppb_file_ref_...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/45001/ppapi/thunk/ppb_file_ref_...
ppapi/thunk/ppb_file_ref_thunk.cc:29: // Make sure the file_system is one of
expected types.
On 2013/04/25 00:01:23, Chromium Palmer wrote:
> NIT: "...one of the..."

Done.

https://codereview.chromium.org/14188019/diff/26002/ppapi/proxy/ppapi_param_t...
File ppapi/proxy/ppapi_param_traits.cc (right):

https://codereview.chromium.org/14188019/diff/26002/ppapi/proxy/ppapi_param_t...
ppapi/proxy/ppapi_param_traits.cc:133: system_type !=
PP_FILESYSTEMTYPE_ISOLATED)
Adding this missing part to make FileIO::Query work.

[kinuko, 2013-04-25 09:27:15]

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

https://codereview.chromium.org/14188019/diff/45001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1352: std::string /* fsid */)
On 2013/04/25 02:21:51, Victor Hsieh wrote:
> On 2013/04/25 00:01:23, Chromium Palmer wrote:
> > I don't feel great about this arbitrary string |fsid|. Can we make it come
> from
> > a limited set of static strings, or get rid of it? It is ultimately passed
> down
> > to the isolated filesystem API, where it is an optional parameter anyway.
> 
> I guess you're talking about the case that a malicious plugin sends a fake
fsid
> to renderer host (instead of forwarding what the browser host gets from
isolated
> filesystem).
> 
> kinuko@, the filesystem:chrome-extension:// passed to fileapi can be
manipulated
> in current implementatoin.  Do you have any suggestion on making this secure
> from fileapi's perspective?

Sending an arbitrary fsid doesn't work unless the fsid is registered for the
renderer process ID in the browser process upfront. Also fsid is 128-bit string
(it's like GUID) and I don't think picking an arbitrary fsid could make any
meaningful attack. I'm not an expert in security area though, so please
enlighten me if we seem to need reconsider. Thanks!

[victorhsieh, 2013-04-25 15:41:10]

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

https://codereview.chromium.org/14188019/diff/45001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1352: std::string /* fsid */)
On 2013/04/25 09:27:16, kinuko wrote:
> On 2013/04/25 02:21:51, Victor Hsieh wrote:
> > On 2013/04/25 00:01:23, Chromium Palmer wrote:
> > > I don't feel great about this arbitrary string |fsid|. Can we make it come
> > from
> > > a limited set of static strings, or get rid of it? It is ultimately passed
> > down
> > > to the isolated filesystem API, where it is an optional parameter anyway.
> > 
> > I guess you're talking about the case that a malicious plugin sends a fake
> fsid
> > to renderer host (instead of forwarding what the browser host gets from
> isolated
> > filesystem).
> > 
> > kinuko@, the filesystem:chrome-extension:// passed to fileapi can be
> manipulated
> > in current implementatoin.  Do you have any suggestion on making this secure
> > from fileapi's perspective?
> 
> Sending an arbitrary fsid doesn't work unless the fsid is registered for the
> renderer process ID in the browser process upfront. Also fsid is 128-bit
string
> (it's like GUID) and I don't think picking an arbitrary fsid could make any
> meaningful attack. I'm not an expert in security area though, so please
> enlighten me if we seem to need reconsider. Thanks!

Here are two cases that I can think of:

1. attacker can brute-forcely scanning other isolated fsid.  In this case, I
think it's not a big problem because when the filesystem is created, it should
always be granted the limited permission in the same renderer.  I'd say the risk
is very low.

2. attacker can give fsid like "../../../../persistent" or something.  But I
guess this will be caught by "cracking"?

[palmer, 2013-04-25 23:35:51]

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

https://codereview.chromium.org/14188019/diff/45001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1352: std::string /* fsid */)
First, a question: Isn't the |extension_id| the true separator of extensions
from each other?

What purpose does the |fsid| serve?

> 1. attacker can brute-forcely scanning other isolated fsid.  In this case, I
> think it's not a big problem because when the filesystem is created, it should
> always be granted the limited permission in the same renderer.  I'd say the
risk
> is very low.

kinuko said that |fsid| is a 128-bit GUID. If it is generated cryptographically
randomly, brute-forcing it is infeasible. But I don't see where |fsid| is
generated. For that matter, I don't see code enforcing that the std::string is
GUID-shaped — from what I can see here, it could be anything, and that's why I
don't like it. :) strings are weak, blob types, and for IPC security we prefer
strong, specific types.

> 2. attacker can give fsid like "../../../../persistent" or something.  But I
> guess this will be caught by "cracking"? 

This was my fear, too. And kinuko assured me that such parent-references should
be caught by URL parsing. Even so, I have a CL in flight to double-check that
references to the parent directory not work inside the Filesystem API
implementation.

But those two things are less important; first I want to understand why |fsid|
is necessary, and is it expected to perform any security function (such as
isolating two extensions' filesystems from each other).

[kinuko, 2013-04-26 05:58:56]

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

https://codereview.chromium.org/14188019/diff/45001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1352: std::string /* fsid */)
On 2013/04/25 23:35:51, Chromium Palmer wrote:
> First, a question: Isn't the |extension_id| the true separator of extensions
> from each other?
> 
> What purpose does the |fsid| serve?

It's for reusing an existing architecture for CRX directory access and |fsid|
serves as a necessary indirection layer for content/webkit layer FileAPI code
to access the extension specific CRX directory (about which FileAPI layer has
no knowledge about that, because extension or extension ID is chrome-specific
concept).

Instead we can implement a special FS module for CRX directory access in the
browser
process and plug it in webkit layer, but that needs a lot more code and we'll
need yet another security review for the new code, and I didn't like to add new
code
just for this purpose (with an assumption that adding more code usually weakens
the security too).

> > 1. attacker can brute-forcely scanning other isolated fsid.  In this case, I
> > think it's not a big problem because when the filesystem is created, it
should
> > always be granted the limited permission in the same renderer.  I'd say the
> risk
> > is very low.
> 
> kinuko said that |fsid| is a 128-bit GUID. If it is generated
cryptographically
> randomly, brute-forcing it is infeasible.

What's more, just guessing existing |fsid| doesn't work, the renderer
needs to be granted permission for the |fsid| prior. (That means
there's no point to brute-force fsids)

> But I don't see where |fsid| is
> generated. For that matter, I don't see code enforcing that the std::string is
> GUID-shaped — from what I can see here, it could be anything, and that's why I
> don't like it. :) strings are weak, blob types, and for IPC security we prefer
> strong, specific types.

It's GUID-like 128-bit random string, but we're not using GUID.
We can easily change the code to use GenerateGUID() and IsValidGUID() though.

The code generating a new fsid is here, fyi:
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/fileapi/iso...

> > 2. attacker can give fsid like "../../../../persistent" or something.  But I
> > guess this will be caught by "cracking"? 
> 
> This was my fear, too. And kinuko assured me that such parent-references
should
> be caught by URL parsing. Even so, I have a CL in flight to double-check that
> references to the parent directory not work inside the Filesystem API
> implementation.
> 
> But those two things are less important; first I want to understand why |fsid|
> is necessary, and is it expected to perform any security function (such as
> isolating two extensions' filesystems from each other).

[palmer, 2013-04-29 19:26:15]

This almost looks G to M. I think with an fsid sanity check we'll be ready to
go.

https://codereview.chromium.org/14188019/diff/26002/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/26002/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:110:
if (fsid.empty()) {
Since we expect this to be GUID-shaped, it'd be nice to run this through a

    bool LooksLikeAGuid(const std::string& fsid) { ... }

function. We should all reading and writing on such a check. Since the string is
a fixed-length number of hex digits, it should be fast and simple.

Then we can be sure callers aren't sending us tricky or malformed fsids, which
makes reasoning about this code easier.

[victorhsieh, 2013-04-29 21:05:41]

PTAL

https://codereview.chromium.org/14188019/diff/26002/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/26002/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:110:
if (fsid.empty()) {
On 2013/04/29 19:26:15, Chromium Palmer wrote:
> Since we expect this to be GUID-shaped, it'd be nice to run this through a
> 
>     bool LooksLikeAGuid(const std::string& fsid) { ... }
> 
> function. We should all reading and writing on such a check. Since the string
is
> a fixed-length number of hex digits, it should be fast and simple.
> 
> Then we can be sure callers aren't sending us tricky or malformed fsids, which
> makes reasoning about this code easier.

Done.

[victorhsieh, 2013-04-29 21:06:20]

[+yzshen for ppapi owner]

[yzshen1, 2013-04-30 19:31:18]

Thanks!

I haven't reviewed the example.

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:23:
#include "ppapi/proxy/resource_message_params.h"
It is already included in the .h file.

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:110:
// by a plugin (which could be malicious), we run a sanity check here before
fsid is not sent by the plugin, right?

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:112:
if (LooksLikeAGuid(fsid)) {
Is the condition inverted?

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:145:
const int kExpectedFsIdSize = 16;
I don't understand this very well. If it is a GUID, then you could use
base::IsValidGUID(). (But I noticed that this is different from a valid GUID.)

Is there a common place where to put the logic of generating and validating
FSID? It seems this method doesn't belong here.

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h
(right):

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:28:
class PepperExtCrxFileSystemBrowserHost
Because this performs its task on the UI thread only, you should use
ppapi::host::ResourceMessageFilter to override the thread. Please see the
comments of ResourceMessageFilter.

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:30:
public base::SupportsWeakPtr<PepperExtCrxFileSystemBrowserHost> {
You already have weak_ptr_factory_. You probably don't need this one.

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:55:
// Performs a sanity check that fsid received from plugin looks not harmful.
This fsid is *not* received from plugin, right? The document URL (based on which
you generate this fsid, I think) is actually from the renderer.

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/ch...
File chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/ch...
chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc:57: // Private
interfaces.
Please see line 111. We already have a private section.

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/pe...
File chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc (right):

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc:9: #include
"googleurl/src/gurl.h"
you already include this in .h.

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc:14: #include
"ppapi/proxy/resource_message_params.h"
ditto.

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc:81: return
PP_OK_COMPLETIONPENDING;
You could set the context->reply_msg and directly return PP_OK, instead of doing
line 77-80 and returning PP_OK_COMPLETIONPENDING.

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:69: return (new
ppapi::proxy::ExtCrxFileSystemPrivateResource(
Do we need to support in-process mode?

https://codereview.chromium.org/14188019/diff/61050/ppapi/api/pp_file_info.idl
File ppapi/api/pp_file_info.idl (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/api/pp_file_info.id...
ppapi/api/pp_file_info.idl:37: PP_FILESYSTEMTYPE_ISOLATED = 4
Because this changes a stable interface, we should send a mail to pepper-team to
notify people of this change and see if there is any concern.

https://codereview.chromium.org/14188019/diff/61050/ppapi/api/private/ppb_ext...
File ppapi/api/private/ppb_ext_crx_file_system_private.idl (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/api/private/ppb_ext...
ppapi/api/private/ppb_ext_crx_file_system_private.idl:16: interface
PPB_Ext_CrxFileSystem_Private {
IMO, you could remove Ext from the name because Crx already indicates that this
is for extensions.
Besides, my original plan is to reserve PPB_Ext_ and pp::ext:: for only those
Pepper equivalents of extension JS APIs.

But I am fine if you would like to leave it as it is and revisit it later if
necessary.

https://codereview.chromium.org/14188019/diff/61050/ppapi/api/private/ppb_ext...
ppapi/api/private/ppb_ext_crx_file_system_private.idl:18: * Create() creates a
CRX filesystem resource.  It may fail when called from
Is "may" precise here?

https://codereview.chromium.org/14188019/diff/61050/ppapi/cpp/private/ext_crx...
File ppapi/cpp/private/ext_crx_file_system_private.h (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/cpp/private/ext_crx...
ppapi/cpp/private/ext_crx_file_system_private.h:16: class
ExtCrxFileSystemPrivate : public FileSystem {
It doesn't make sense to inherit from FileSystem. A crx filesystem PP_Resource
is not a filesystem PP_Resource.

https://codereview.chromium.org/14188019/diff/61050/ppapi/host/file_system_re...
File ppapi/host/file_system_registry.h (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/host/file_system_re...
ppapi/host/file_system_registry.h:27: class PPAPI_HOST_EXPORT FileSystemRegistry
{
I think we don't need to have yet another map.
We could have a common base class FileSystemHostBase for PepperFileSystemHost
and PepperExtCrxFileSystemRendererHost. And then we can cast a ResourceHost to
FileSystemHostBase, right?

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
File ppapi/proxy/ext_crx_file_system_private_resource.cc (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.cc:45: const std::string& fsid)
{
Test whether callback is still pending before doing other things.

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.cc:59:
callback->Run(params.result());
ditto.

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
File ppapi/proxy/ext_crx_file_system_private_resource.h (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.h:5: // CRX filesystem is a
filesystem that allows an extension to read its own
nit: I think it is better to put this comment right above the Class declaration.
At least it should below #ifndef/#define and #include.

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.h:12: // "isolated filesystem"
points to current extension directory in browser.
points -> pointing

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.h:15: // The only purpose of
renderer is make it accessible by FileRef, which runs in
renderer -> renderer CRX filesystem resource host?

is make -> is to make

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.h:20: // host.  Once the
browser host successfully create the underlying "isolated
create -> creates

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

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1349:
IPC_MESSAGE_CONTROL0(PpapiBrowserHostMsg_Ext_CrxFileSystem_Open)
It is better to keep the first part of the msg name PpapiHostMsg to be
consistent, you could move Browser/Renderer to the last part:
PpapiHostMsg_Ext_CrxFileSystem_Browser/RendererOpen.

especially you already have Browser/RendererOpenReply

https://codereview.chromium.org/14188019/diff/61050/ppapi/thunk/ppb_file_ref_...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/thunk/ppb_file_ref_...
ppapi/thunk/ppb_file_ref_thunk.cc:30: EnterResourceNoLock<PPB_FileSystem_API>
enter_file_system(file_system, true);
you should use 'false' (and also line 32).

[palmer, 2013-04-30 20:54:00]

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:145:
const int kExpectedFsIdSize = 16;
> I don't understand this very well. If it is a GUID, then you could use
> base::IsValidGUID(). (But I noticed that this is different from a valid GUID.)
> 
> Is there a common place where to put the logic of generating and validating
> FSID? It seems this method doesn't belong here.

It's not exactly a GUID, it's just a pseudo-random hexadecimal string of a
128-bit value.

Thus, kExpectedFsIdSize should be 32, right?

[victorhsieh, 2013-04-30 22:04:32]

PTAL

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:23:
#include "ppapi/proxy/resource_message_params.h"
On 2013/04/30 19:31:19, yzshen1 wrote:
> It is already included in the .h file.

Done.

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:110:
// by a plugin (which could be malicious), we run a sanity check here before
On 2013/04/30 19:31:19, yzshen1 wrote:
> fsid is not sent by the plugin, right?

It's forwarded by the plugin.  Please see my design doc (CRX FileSystem Pepper
(private) API).

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:112:
if (LooksLikeAGuid(fsid)) {
On 2013/04/30 19:31:19, yzshen1 wrote:
> Is the condition inverted?

Shame on me to not have a careful test.

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:145:
const int kExpectedFsIdSize = 16;
On 2013/04/30 20:54:00, Chromium Palmer wrote:
> > I don't understand this very well. If it is a GUID, then you could use
> > base::IsValidGUID(). (But I noticed that this is different from a valid
GUID.)
> > 
> > Is there a common place where to put the logic of generating and validating
> > FSID? It seems this method doesn't belong here.
> 
> It's not exactly a GUID, it's just a pseudo-random hexadecimal string of a
> 128-bit value.
> 
> Thus, kExpectedFsIdSize should be 32, right?

And shame on me again.

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h
(right):

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:28:
class PepperExtCrxFileSystemBrowserHost
On 2013/04/30 19:31:19, yzshen1 wrote:
> Because this performs its task on the UI thread only, you should use
> ppapi::host::ResourceMessageFilter to override the thread. Please see the
> comments of ResourceMessageFilter.

But since we need to reply message in IO thread, is it still beneficial to
override task runner?

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:30:
public base::SupportsWeakPtr<PepperExtCrxFileSystemBrowserHost> {
On 2013/04/30 19:31:19, yzshen1 wrote:
> You already have weak_ptr_factory_. You probably don't need this one.

Done.

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:55:
// Performs a sanity check that fsid received from plugin looks not harmful.
On 2013/04/30 19:31:19, yzshen1 wrote:
> This fsid is *not* received from plugin, right? The document URL (based on
which
> you generate this fsid, I think) is actually from the renderer.

Please see my design doc (CRX FileSystem Pepper (private) API).

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/ch...
File chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/ch...
chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc:57: // Private
interfaces.
On 2013/04/30 19:31:19, yzshen1 wrote:
> Please see line 111. We already have a private section.

Done.

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/pe...
File chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc (right):

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc:9: #include
"googleurl/src/gurl.h"
On 2013/04/30 19:31:19, yzshen1 wrote:
> you already include this in .h.

Done.

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc:14: #include
"ppapi/proxy/resource_message_params.h"
On 2013/04/30 19:31:19, yzshen1 wrote:
> ditto.

Done.

https://codereview.chromium.org/14188019/diff/61050/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_ext_crx_file_system_renderer_host.cc:81: return
PP_OK_COMPLETIONPENDING;
On 2013/04/30 19:31:19, yzshen1 wrote:
> You could set the context->reply_msg and directly return PP_OK, instead of
doing
> line 77-80 and returning PP_OK_COMPLETIONPENDING.

Done.

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:69: return (new
ppapi::proxy::ExtCrxFileSystemPrivateResource(
On 2013/04/30 19:31:19, yzshen1 wrote:
> Do we need to support in-process mode?

Probably not, but why not?

https://codereview.chromium.org/14188019/diff/61050/ppapi/api/pp_file_info.idl
File ppapi/api/pp_file_info.idl (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/api/pp_file_info.id...
ppapi/api/pp_file_info.idl:37: PP_FILESYSTEMTYPE_ISOLATED = 4
On 2013/04/30 19:31:19, yzshen1 wrote:
> Because this changes a stable interface, we should send a mail to pepper-team
to
> notify people of this change and see if there is any concern.

Done.

https://codereview.chromium.org/14188019/diff/61050/ppapi/api/private/ppb_ext...
File ppapi/api/private/ppb_ext_crx_file_system_private.idl (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/api/private/ppb_ext...
ppapi/api/private/ppb_ext_crx_file_system_private.idl:16: interface
PPB_Ext_CrxFileSystem_Private {
On 2013/04/30 19:31:19, yzshen1 wrote:
> IMO, you could remove Ext from the name because Crx already indicates that
this
> is for extensions.
> Besides, my original plan is to reserve PPB_Ext_ and pp::ext:: for only those
> Pepper equivalents of extension JS APIs.
> 
> But I am fine if you would like to leave it as it is and revisit it later if
> necessary.

Yeah, I saw you have Ext there so I just name it that way.  I remember you said
you will fit this into your ext work later, so let's rename it together at that
time.

https://codereview.chromium.org/14188019/diff/61050/ppapi/api/private/ppb_ext...
ppapi/api/private/ppb_ext_crx_file_system_private.idl:18: * Create() creates a
CRX filesystem resource.  It may fail when called from
On 2013/04/30 19:31:19, yzshen1 wrote:
> Is "may" precise here?

It's referenced from pp::FileSystem.  I think it's "may" because the operation
can fail on later Open.

https://codereview.chromium.org/14188019/diff/61050/ppapi/cpp/private/ext_crx...
File ppapi/cpp/private/ext_crx_file_system_private.h (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/cpp/private/ext_crx...
ppapi/cpp/private/ext_crx_file_system_private.h:16: class
ExtCrxFileSystemPrivate : public FileSystem {
On 2013/04/30 19:31:19, yzshen1 wrote:
> It doesn't make sense to inherit from FileSystem. A crx filesystem PP_Resource
> is not a filesystem PP_Resource.

That's true, but if we don't do this, we'll have to create a new FileRef and
FileIO for this filesystem.  And when we want to add another filesystem (e.g.
gallery), it will have more.  We probably should keep FileRef and FileIO
independent to FileSystem from the API perspective.

I know it's tricky since this inheritance depends on the fact that pp_resource
is used now is simply an id.  But I didn't come up with a better idea.  Thought?

https://codereview.chromium.org/14188019/diff/61050/ppapi/host/file_system_re...
File ppapi/host/file_system_registry.h (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/host/file_system_re...
ppapi/host/file_system_registry.h:27: class PPAPI_HOST_EXPORT FileSystemRegistry
{
On 2013/04/30 19:31:19, yzshen1 wrote:
> I think we don't need to have yet another map.
> We could have a common base class FileSystemHostBase for PepperFileSystemHost
> and PepperExtCrxFileSystemRendererHost. And then we can cast a ResourceHost to
> FileSystemHostBase, right?

Do you mean to add a ...
FileSystemHostBase* ResourceHost::GetFileSystemHostBase(PP_Resource); ?  Simply
cast a ResourceHost to FileSystemHostBase sounds error prune to me.

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
File ppapi/proxy/ext_crx_file_system_private_resource.cc (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.cc:45: const std::string& fsid)
{
On 2013/04/30 19:31:19, yzshen1 wrote:
> Test whether callback is still pending before doing other things.

Done.

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.cc:59:
callback->Run(params.result());
On 2013/04/30 19:31:19, yzshen1 wrote:
> ditto.

Done.

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
File ppapi/proxy/ext_crx_file_system_private_resource.h (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.h:12: // "isolated filesystem"
points to current extension directory in browser.
On 2013/04/30 19:31:19, yzshen1 wrote:
> points -> pointing

Done.

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.h:15: // The only purpose of
renderer is make it accessible by FileRef, which runs in
On 2013/04/30 19:31:19, yzshen1 wrote:
> renderer -> renderer CRX filesystem resource host?
> 
> is make -> is to make

Done.

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ext_crx_file_...
ppapi/proxy/ext_crx_file_system_private_resource.h:20: // host.  Once the
browser host successfully create the underlying "isolated
On 2013/04/30 19:31:19, yzshen1 wrote:
> create -> creates

Done.

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

https://codereview.chromium.org/14188019/diff/61050/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1349:
IPC_MESSAGE_CONTROL0(PpapiBrowserHostMsg_Ext_CrxFileSystem_Open)
On 2013/04/30 19:31:19, yzshen1 wrote:
> It is better to keep the first part of the msg name PpapiHostMsg to be
> consistent, you could move Browser/Renderer to the last part:
> PpapiHostMsg_Ext_CrxFileSystem_Browser/RendererOpen.
> 
> especially you already have Browser/RendererOpenReply

Done.

https://codereview.chromium.org/14188019/diff/61050/ppapi/thunk/ppb_file_ref_...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/thunk/ppb_file_ref_...
ppapi/thunk/ppb_file_ref_thunk.cc:30: EnterResourceNoLock<PPB_FileSystem_API>
enter_file_system(file_system, true);
On 2013/04/30 19:31:19, yzshen1 wrote:
> you should use 'false' (and also line 32).

I remember you said only Is* should have false for enter?

[yzshen1, 2013-05-01 17:45:10]

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:110:
// by a plugin (which could be malicious), we run a sanity check here before
Please see my comment in the .h file.
On 2013/04/30 22:04:32, victorhsieh wrote:
> On 2013/04/30 19:31:19, yzshen1 wrote:
> > fsid is not sent by the plugin, right?
> 
> It's forwarded by the plugin.  Please see my design doc (CRX FileSystem Pepper
> (private) API).

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h
(right):

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:28:
class PepperExtCrxFileSystemBrowserHost
The code looks cleaner. There are other places where we use
ResourceMessageFilter, you could take a look.
The SendReply() method can be called from any thread, I know it will jump to the
IO thread, but the code looks cleaner.
On 2013/04/30 22:04:32, victorhsieh wrote:
> On 2013/04/30 19:31:19, yzshen1 wrote:
> > Because this performs its task on the UI thread only, you should use
> > ppapi::host::ResourceMessageFilter to override the thread. Please see the
> > comments of ResourceMessageFilter.
> 
> But since we need to reply message in IO thread, is it still beneficial to
> override task runner?

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:55:
// Performs a sanity check that fsid received from plugin looks not harmful.
I read it. The fsid is passed to the plugin process and then passed to the
renderer process. But this is in the browser process. 

On 2013/04/30 22:04:32, victorhsieh wrote:
> On 2013/04/30 19:31:19, yzshen1 wrote:
> > This fsid is *not* received from plugin, right? The document URL (based on
> which
> > you generate this fsid, I think) is actually from the renderer.
> 
> Please see my design doc (CRX FileSystem Pepper (private) API).

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:69: return (new
ppapi::proxy::ExtCrxFileSystemPrivateResource(
The goal is to get rid of in-process mode eventually.
So if not necessary, it is discouraged to add in-process support
On 2013/04/30 22:04:32, victorhsieh wrote:
> On 2013/04/30 19:31:19, yzshen1 wrote:
> > Do we need to support in-process mode?
> 
> Probably not, but why not?

https://codereview.chromium.org/14188019/diff/61050/ppapi/cpp/private/ext_crx...
File ppapi/cpp/private/ext_crx_file_system_private.h (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/cpp/private/ext_crx...
ppapi/cpp/private/ext_crx_file_system_private.h:16: class
ExtCrxFileSystemPrivate : public FileSystem {
On 2013/04/30 22:04:32, victorhsieh wrote:
> On 2013/04/30 19:31:19, yzshen1 wrote:
> > It doesn't make sense to inherit from FileSystem. A crx filesystem
PP_Resource
> > is not a filesystem PP_Resource.
> 
> That's true, but if we don't do this, we'll have to create a new FileRef and
> FileIO for this filesystem. 
You could create a subclass of FileRef, say, ExtCrxFileRefPrivate, which adds a
new constructor to take a ExtCrxFileSystemPrivate instead of a FileSystem. You
don't have to do anything to FileIO, because ExtCrxFileRefPrivate is still a
FileRef.

> And when we want to add another filesystem (e.g.
> gallery), it will have more. 
I don't think we want to add different kinds of file system *PP_Resource*. I
wish we could do it by extending FileSystem itself. I suggested before that we
make a private API to create a PPB_FileSystem PP_Resource which is of isolated
type, but not creating a different file system PP_Resource, right?

The problem we encounter here is one reason we should go that way, IMO.

> We probably should keep FileRef and FileIO
> independent to FileSystem from the API perspective.
> 
> I know it's tricky since this inheritance depends on the fact that pp_resource
> is used now is simply an id.  But I didn't come up with a better idea. 
Thought?

https://codereview.chromium.org/14188019/diff/61050/ppapi/host/file_system_re...
File ppapi/host/file_system_registry.h (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/host/file_system_re...
ppapi/host/file_system_registry.h:27: class PPAPI_HOST_EXPORT FileSystemRegistry
{
> Do you mean to add a ...
> FileSystemHostBase* 
> ResourceHost::GetFileSystemHostBase(PP_Resource); ? 
> Simply cast a ResourceHost to FileSystemHostBase sounds 
> error prune to me.
I agree. :/ This is another reason I think we should not add a different kind of
file system PP_Resource.

https://codereview.chromium.org/14188019/diff/61050/ppapi/thunk/ppb_file_ref_...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/thunk/ppb_file_ref_...
ppapi/thunk/ppb_file_ref_thunk.cc:30: EnterResourceNoLock<PPB_FileSystem_API>
enter_file_system(file_system, true);
Sorry if I didn't explain clearly. :) The boolean here means "whether to report
error of mismatched resource type".

Is*() is used to query whether a PP_Resource is of a specific type, it makes
sense to not report any error.

Here, |file_system| is either PPB_FileSystem or PPB_Ext_CrxFileSystem, if you
use 'true', you will for sure report error for one of them (or both). That
doesn't make sense.

On 2013/04/30 22:04:32, victorhsieh wrote:
> On 2013/04/30 19:31:19, yzshen1 wrote:
> > you should use 'false' (and also line 32).
> 
> I remember you said only Is* should have false for enter?

https://codereview.chromium.org/14188019/diff/80001/chrome/renderer/pepper/ch...
File chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/14188019/diff/80001/chrome/renderer/pepper/ch...
chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc:108: case
PpapiHostMsg_Ext_CrxFileSystem_Create::ID:
Please use {}

[victorhsieh, 2013-05-02 18:59:06]

PTAL

Chris, sorry about the late architecture change.  The renderer host is now gone
and the logic of isolated file system is merged into
content/renderer/pepper/pepper_file_system_host.cc, with a new message sending
fsid from plugin (and no longer sending extension id which the renderer host can
get).  Please let me know if there's any problem.  Thanks.

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h
(right):

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:28:
class PepperExtCrxFileSystemBrowserHost
On 2013/05/01 17:45:10, yzshen1 wrote:
> The code looks cleaner. There are other places where we use
> ResourceMessageFilter, you could take a look.
> The SendReply() method can be called from any thread, I know it will jump to
the
> IO thread, but the code looks cleaner.
> On 2013/04/30 22:04:32, victorhsieh wrote:
> > On 2013/04/30 19:31:19, yzshen1 wrote:
> > > Because this performs its task on the UI thread only, you should use
> > > ppapi::host::ResourceMessageFilter to override the thread. Please see the
> > > comments of ResourceMessageFilter.
> > 
> > But since we need to reply message in IO thread, is it still beneficial to
> > override task runner?
> 

Done.  Thanks for your previous hint of using context->reply_msg, we don't need
to actually post task back.  That saves more code :)

https://codereview.chromium.org/14188019/diff/61050/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:55:
// Performs a sanity check that fsid received from plugin looks not harmful.
On 2013/05/01 17:45:10, yzshen1 wrote:
> I read it. The fsid is passed to the plugin process and then passed to the
> renderer process. But this is in the browser process. 
> 
> On 2013/04/30 22:04:32, victorhsieh wrote:
> > On 2013/04/30 19:31:19, yzshen1 wrote:
> > > This fsid is *not* received from plugin, right? The document URL (based on
> > which
> > > you generate this fsid, I think) is actually from the renderer.
> > 
> > Please see my design doc (CRX FileSystem Pepper (private) API).
> 

Done.

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:69: return (new
ppapi::proxy::ExtCrxFileSystemPrivateResource(
On 2013/05/01 17:45:10, yzshen1 wrote:
> The goal is to get rid of in-process mode eventually.
> So if not necessary, it is discouraged to add in-process support
> On 2013/04/30 22:04:32, victorhsieh wrote:
> > On 2013/04/30 19:31:19, yzshen1 wrote:
> > > Do we need to support in-process mode?
> > 
> > Probably not, but why not?
> 

Done.

https://codereview.chromium.org/14188019/diff/61050/ppapi/cpp/private/ext_crx...
File ppapi/cpp/private/ext_crx_file_system_private.h (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/cpp/private/ext_crx...
ppapi/cpp/private/ext_crx_file_system_private.h:16: class
ExtCrxFileSystemPrivate : public FileSystem {
On 2013/05/01 17:45:10, yzshen1 wrote:
> On 2013/04/30 22:04:32, victorhsieh wrote:
> > On 2013/04/30 19:31:19, yzshen1 wrote:
> > > It doesn't make sense to inherit from FileSystem. A crx filesystem
> PP_Resource
> > > is not a filesystem PP_Resource.
> > 
> > That's true, but if we don't do this, we'll have to create a new FileRef and
> > FileIO for this filesystem. 
> You could create a subclass of FileRef, say, ExtCrxFileRefPrivate, which adds
a
> new constructor to take a ExtCrxFileSystemPrivate instead of a FileSystem. You
> don't have to do anything to FileIO, because ExtCrxFileRefPrivate is still a
> FileRef.
> 
> > And when we want to add another filesystem (e.g.
> > gallery), it will have more. 
> I don't think we want to add different kinds of file system *PP_Resource*. I
> wish we could do it by extending FileSystem itself. I suggested before that we
> make a private API to create a PPB_FileSystem PP_Resource which is of isolated
> type, but not creating a different file system PP_Resource, right?
> 
> The problem we encounter here is one reason we should go that way, IMO.
> 
> > We probably should keep FileRef and FileIO
> > independent to FileSystem from the API perspective.
> > 
> > I know it's tricky since this inheritance depends on the fact that
pp_resource
> > is used now is simply an id.  But I didn't come up with a better idea. 
> Thought?
> 

Done.

https://codereview.chromium.org/14188019/diff/61050/ppapi/thunk/ppb_file_ref_...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/61050/ppapi/thunk/ppb_file_ref_...
ppapi/thunk/ppb_file_ref_thunk.cc:30: EnterResourceNoLock<PPB_FileSystem_API>
enter_file_system(file_system, true);
On 2013/05/01 17:45:10, yzshen1 wrote:
> Sorry if I didn't explain clearly. :) The boolean here means "whether to
report
> error of mismatched resource type".
> 
> Is*() is used to query whether a PP_Resource is of a specific type, it makes
> sense to not report any error.
> 
> Here, |file_system| is either PPB_FileSystem or PPB_Ext_CrxFileSystem, if you
> use 'true', you will for sure report error for one of them (or both). That
> doesn't make sense.
> 
> On 2013/04/30 22:04:32, victorhsieh wrote:
> > On 2013/04/30 19:31:19, yzshen1 wrote:
> > > you should use 'false' (and also line 32).
> > 
> > I remember you said only Is* should have false for enter?
> 

Done.

https://codereview.chromium.org/14188019/diff/80001/chrome/renderer/pepper/ch...
File chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/14188019/diff/80001/chrome/renderer/pepper/ch...
chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc:108: case
PpapiHostMsg_Ext_CrxFileSystem_Create::ID:
On 2013/05/01 17:45:10, yzshen1 wrote:
> Please use {}

Done.

[palmer, 2013-05-02 21:30:09]

LGTM after you fix integer types, and range checking.

Also some nits, et c.

https://codereview.chromium.org/14188019/diff/107001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/107001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:47:
// otherwise.  This is expected to run in UI thread as ProfileManager only
Nit: "This must run on the UI thread because ProfileManager only allows access
on that thread."

https://codereview.chromium.org/14188019/diff/107001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:117:
browser_ppapi_host_->GetProfileDataDirectory(),
I think this is the only call-site for the function
|CrxFileSystemFilter::CreateIsolateFileSystem|, correct? It would seem that you
could therefore remove the |profile_directory| argument , and have
|CrxFileSystemFilter::CreateIsolateFileSystem| call |GetProfileDataDirectory|.

That would make it easier to understand that it is not possible to visit
arbitrary file pathnames with that function.

https://codereview.chromium.org/14188019/diff/107001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_system_host.cc (right):

https://codereview.chromium.org/14188019/diff/107001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.cc:55: const int
kExpectedFsIdSize = 32;
std::string::size returns std::string::size_type, which is size_t. So declare
this as a size_t.

I doubt it matters, but you could make it static too.

https://codereview.chromium.org/14188019/diff/107001/ppapi/api/private/ppb_ex...
File ppapi/api/private/ppb_ext_crx_file_system_private.idl (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/api/private/ppb_ex...
ppapi/api/private/ppb_ext_crx_file_system_private.idl:21: PP_Resource
Create([in] PP_Instance insance);
Typo: |instance|.

https://codereview.chromium.org/14188019/diff/107001/ppapi/c/pp_file_info.h
File ppapi/c/pp_file_info.h (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/c/pp_file_info.h#n...
ppapi/c/pp_file_info.h:51: PP_FILESYSTEMTYPE_ISOLATED = 4
Do these have to be kept in sync with the IDL file? If so, should we have a
comment stating that?

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
File ppapi/examples/crxfs/crxfs.cc (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:20: const int kBufSize = 1024;
size_t

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:54: char read_buf_[8192];
Not kBufSize?

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:69:
pp::CompletionCallbackWithOutput<pp::FileSystem>
Nit:

  pp::CompletionCallbackWithOutput<pp::FileSystem> callback = 
      factory_.NewCallbackWithOutput(&MyInstance::CrxFileSystemCallback);

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:116: content.append(read_buf_, pp_error);
Check that pp_error is not > sizeof(read_buf_), and then also
static_cast<size_t>.

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:122: snprintf(buf, sizeof(buf), "%s failed,
pp_error: %d", name, rv);
Why not StringPrintf?

Alternately, check the return value of snprintf to verify that the output string
did not have to be truncated to fit in sizeof(buf) chars.

https://codereview.chromium.org/14188019/diff/107001/ppapi/host/file_system_r...
File ppapi/host/file_system_registry.cc (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/host/file_system_r...
ppapi/host/file_system_registry.cc:48: PP_Resource resource) {
spaces

https://codereview.chromium.org/14188019/diff/107001/ppapi/host/file_system_r...
ppapi/host/file_system_registry.cc:55: PP_Resource resource) const {
This all fits on one line.

https://codereview.chromium.org/14188019/diff/107001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:2646: PP_Resource
Pnacl_M28_PPB_Ext_CrxFileSystem_Private_Create(PP_Instance insance) {
Typo: instance

https://codereview.chromium.org/14188019/diff/107001/ppapi/thunk/ppb_ext_crx_...
File ppapi/thunk/ppb_ext_crx_file_system_private_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/thunk/ppb_ext_crx_...
ppapi/thunk/ppb_ext_crx_file_system_private_thunk.cc:23: PP_Resource
Create(PP_Instance insance) {
Typo

https://codereview.chromium.org/14188019/diff/107001/ppapi/thunk/ppb_file_ref...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/thunk/ppb_file_ref...
ppapi/thunk/ppb_file_ref_thunk.cc:36: // Since file_system is either type of
filesystem above, we don't need to
Style: |file_system|

[yzshen1, 2013-05-02 23:22:01]

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:69: return (new
ppapi::proxy::ExtCrxFileSystemPrivateResource(
Please move this to resource_creation_impl. All the NOTIMPLEMENTED methods are
there.
(And also line 100-104)
On 2013/05/02 18:59:06, victorhsieh wrote:
> On 2013/05/01 17:45:10, yzshen1 wrote:
> > The goal is to get rid of in-process mode eventually.
> > So if not necessary, it is discouraged to add in-process support
> > On 2013/04/30 22:04:32, victorhsieh wrote:
> > > On 2013/04/30 19:31:19, yzshen1 wrote:
> > > > Do we need to support in-process mode?
> > > 
> > > Probably not, but why not?
> > 
> 
> Done.

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:34:
: browser_ppapi_host_(browser_ppapi_host), instance_(instance) { }
nit: zero space between {}

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:44:
~CrxFileSystemFilter() {}
use virtual, please (I know it makes no difference. But better to have it. :))

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:58:
content::BrowserPpapiHost* browser_ppapi_host_;
This can only be used on IO thread. You should retrieve what you need in the
constructor (because that is run on IO), and then forget about host and
instance.

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:136:
return PP_OK_COMPLETIONPENDING;
You could set the context->reply_msg and return PP_OK (and get rid of the whole
Reply() below).

Reply and then return PP_OK_COMPLETIONPENDING is a little bit weird.

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:146:
browser_ppapi_host_->GetPpapiHost()->SendReply(
This should not be used on UI thread. You should use the way I suggested above,
or use ResourceMessageFilter::SendReply().

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h
(right):

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:26:
class PepperExtCrxFileSystemBrowserHost : public ppapi::host::ResourceHost {
With a message filter, you don't need this empty host anymore. We have a generic
container for this scenario. Please take a look at MessageFilterHost. An example
of how to use it:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_system_host.cc (right):

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.cc:130: return
PP_ERROR_INPROGRESS;
This is a little bit wired. It returns INPROGRESS even after the open operation
completed.

(But we could take care of it in a separate CL.)

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.cc:170: // sanity check
Please use a sentence. (Capital initial and '.' at the end.)

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.cc:176: url.GetWithEmptyPath(),
fsid, "crxfs"));
Shall we use url.GetOrigin?

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_system_host.h (right):

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.h:50: const std::string&
root_url);
fsid please.

https://codereview.chromium.org/14188019/diff/109001/ppapi/api/pp_file_info.idl
File ppapi/api/pp_file_info.idl (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/api/pp_file_info.i...
ppapi/api/pp_file_info.idl:36: /** For isolated file system types (only for
CrxFileSystem) */
I think we should avoid adding comments for private APIs to this public API
document.

Maybe we add a comment to PPB_FileSystem.Open saying that currently it cannot be
used to open isolated file system?

https://codereview.chromium.org/14188019/diff/109001/ppapi/api/private/ppb_ex...
File ppapi/api/private/ppb_ext_crx_file_system_private.idl (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/api/private/ppb_ex...
ppapi/api/private/ppb_ext_crx_file_system_private.idl:21: PP_Resource
Create([in] PP_Instance insance);
You don't need to Create this PP_Resource and instead use the PP_Instance in
Open(). (I am fine if you would like to keep it, but you have also added
singleton resource-related code.)

https://codereview.chromium.org/14188019/diff/109001/ppapi/api/private/ppb_ex...
ppapi/api/private/ppb_ext_crx_file_system_private.idl:30: * to a FileSystem.
Please be more specific saying that it is a PPB_FileSystem.

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/file_system.h
File ppapi/cpp/file_system.h (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/file_system.h#...
ppapi/cpp/file_system.h:31: FileSystem(PassRef, PP_Resource resource);
Add comments such as:

/// A constructor used when you have received a PP_Resource as a return
/// value that has already been reference counted.
///
/// @param[in] resource A PP_Resource corresponding to a file system.

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/private/ext_cr...
File ppapi/cpp/private/ext_crx_file_system_private.cc (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/private/ext_cr...
ppapi/cpp/private/ext_crx_file_system_private.cc:8: #include
"ppapi/cpp/completion_callback.h"
You have included it in the .h.

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/private/ext_cr...
File ppapi/cpp/private/ext_crx_file_system_private.h (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/private/ext_cr...
ppapi/cpp/private/ext_crx_file_system_private.h:17: class
ExtCrxFileSystemPrivate : public FileSystem {
Remove the base class.

https://codereview.chromium.org/14188019/diff/109001/ppapi/host/file_system_r...
File ppapi/host/file_system_registry.cc (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/host/file_system_r...
ppapi/host/file_system_registry.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
You can delete this class entirely.

https://codereview.chromium.org/14188019/diff/109001/ppapi/host/file_system_r...
File ppapi/host/file_system_registry.h (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/host/file_system_r...
ppapi/host/file_system_registry.h:27: class PPAPI_HOST_EXPORT FileSystemRegistry
{
Please delete it entirely.

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ext_crx_file...
File ppapi/proxy/ext_crx_file_system_private_resource.cc (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:35:
DCHECK(file_system_resource);
Please return failure if necessary instead of DCHECK. (Because this is used by a
public API.)

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:37: if
(!TrackedCallback::IsPending(callback))
If you want to check whether it is in progress, you should check the callback
you received in a previous call, or have a boolean flag.

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:52: const std::string& fsid)
{
Test whether callback is still pending before doing other things. It may have
been aborted and in that case |file_system_resource| may have been released by
the plugin.

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:61: return;
[I don't know the answer but would like to make sure you have considered that]
do we need to somehow notify the browser if we never pass this fsid to the
renderer. Will the file system stay open forever?

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ppb_instance...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:370: case CRX_FILESYSTEM_SINGLETON_ID:
If you make it a singleton, you don't need the Create method.
You can take a look at the other singleton resources above how that is done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/shared_impl/single...
File ppapi/shared_impl/singleton_resource_id.h (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/shared_impl/single...
ppapi/shared_impl/singleton_resource_id.h:26: CRX_FILESYSTEM_SINGLETON_ID,
Alphabetically.

https://codereview.chromium.org/14188019/diff/109001/ppapi/thunk/ppb_file_ref...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/thunk/ppb_file_ref...
ppapi/thunk/ppb_file_ref_thunk.cc:31:
EnterResourceNoLock<PPB_Ext_CrxFileSystem_Private_API> enter_crxfs(
You don't need this anymore.

https://codereview.chromium.org/14188019/diff/109001/webkit/plugins/ppapi/ppa...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://codereview.chromium.org/14188019/diff/109001/webkit/plugins/ppapi/ppa...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2152: case
::ppapi::CRX_FILESYSTEM_SINGLETON_ID:
alphabetically, please.

https://codereview.chromium.org/14188019/diff/119001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_system_host.h (right):

https://codereview.chromium.org/14188019/diff/119001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.h:22: public
ppapi::host::FileSystemRegistry::FileSystemPeeker,
no need to have this peeker interface.

https://codereview.chromium.org/14188019/diff/119001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://codereview.chromium.org/14188019/diff/119001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1026: return
ppapi::host::FileSystemRegistry::GetInstance()->
You should remove all changes in this file.

[victorhsieh, 2013-05-03 17:26:58]

PTAL

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/14188019/diff/61050/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:69: return (new
ppapi::proxy::ExtCrxFileSystemPrivateResource(
On 2013/05/02 23:22:01, yzshen1 wrote:
> Please move this to resource_creation_impl. All the NOTIMPLEMENTED methods are
> there.
> (And also line 100-104)
> On 2013/05/02 18:59:06, victorhsieh wrote:
> > On 2013/05/01 17:45:10, yzshen1 wrote:
> > > The goal is to get rid of in-process mode eventually.
> > > So if not necessary, it is discouraged to add in-process support
> > > On 2013/04/30 22:04:32, victorhsieh wrote:
> > > > On 2013/04/30 19:31:19, yzshen1 wrote:
> > > > > Do we need to support in-process mode?
> > > > 
> > > > Probably not, but why not?
> > > 
> > 
> > Done.
> 

Done.

https://codereview.chromium.org/14188019/diff/107001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/107001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:47:
// otherwise.  This is expected to run in UI thread as ProfileManager only
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> Nit: "This must run on the UI thread because ProfileManager only allows access
> on that thread."

Done.

https://codereview.chromium.org/14188019/diff/107001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:117:
browser_ppapi_host_->GetProfileDataDirectory(),
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> I think this is the only call-site for the function
> |CrxFileSystemFilter::CreateIsolateFileSystem|, correct? It would seem that
you
> could therefore remove the |profile_directory| argument , and have
> |CrxFileSystemFilter::CreateIsolateFileSystem| call |GetProfileDataDirectory|.
> 
> That would make it easier to understand that it is not possible to visit
> arbitrary file pathnames with that function.

Done.

https://codereview.chromium.org/14188019/diff/107001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_system_host.cc (right):

https://codereview.chromium.org/14188019/diff/107001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.cc:55: const int
kExpectedFsIdSize = 32;
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> std::string::size returns std::string::size_type, which is size_t. So declare
> this as a size_t.

Done.

> I doubt it matters, but you could make it static too.

I prefer not to, as my first doubt of static here is.. the this value will be
kept/shared for the next call.  But of course it doesn't matter, or is likely be
optimized out as it's a const.

https://codereview.chromium.org/14188019/diff/107001/ppapi/api/private/ppb_ex...
File ppapi/api/private/ppb_ext_crx_file_system_private.idl (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/api/private/ppb_ex...
ppapi/api/private/ppb_ext_crx_file_system_private.idl:21: PP_Resource
Create([in] PP_Instance insance);
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> Typo: |instance|.

Done.

https://codereview.chromium.org/14188019/diff/107001/ppapi/c/pp_file_info.h
File ppapi/c/pp_file_info.h (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/c/pp_file_info.h#n...
ppapi/c/pp_file_info.h:51: PP_FILESYSTEMTYPE_ISOLATED = 4
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> Do these have to be kept in sync with the IDL file? If so, should we have a
> comment stating that?

Oops, forgot to run generator.

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
File ppapi/examples/crxfs/crxfs.cc (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:20: const int kBufSize = 1024;
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> size_t

Done.

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:54: char read_buf_[8192];
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> Not kBufSize?

Done.

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:69:
pp::CompletionCallbackWithOutput<pp::FileSystem>
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> Nit:
> 
>   pp::CompletionCallbackWithOutput<pp::FileSystem> callback = 
>       factory_.NewCallbackWithOutput(&MyInstance::CrxFileSystemCallback);

Done.

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:116: content.append(read_buf_, pp_error);
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> Check that pp_error is not > sizeof(read_buf_), and then also
> static_cast<size_t>.

I think this is a serious API bug if it's true.  FileIO::Read guarantee to read
at most sizeof(read_buf_) bytes.  And I'd keep this simple to understand as this
is an example.

https://codereview.chromium.org/14188019/diff/107001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:122: snprintf(buf, sizeof(buf), "%s failed,
pp_error: %d", name, rv);
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> Why not StringPrintf?
> 
> Alternately, check the return value of snprintf to verify that the output
string
> did not have to be truncated to fit in sizeof(buf) chars.

I think most developer don't have StringPrintf, so we better not use it in this
example.

https://codereview.chromium.org/14188019/diff/107001/ppapi/host/file_system_r...
File ppapi/host/file_system_registry.cc (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/host/file_system_r...
ppapi/host/file_system_registry.cc:48: PP_Resource resource) {
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> spaces

Done.

https://codereview.chromium.org/14188019/diff/107001/ppapi/host/file_system_r...
ppapi/host/file_system_registry.cc:55: PP_Resource resource) const {
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> This all fits on one line.

Done.

https://codereview.chromium.org/14188019/diff/107001/ppapi/thunk/ppb_file_ref...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/107001/ppapi/thunk/ppb_file_ref...
ppapi/thunk/ppb_file_ref_thunk.cc:36: // Since file_system is either type of
filesystem above, we don't need to
On 2013/05/02 21:30:09, Chromium Palmer wrote:
> Style: |file_system|

Reverted these change as it's no longer needed.

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc
(right):

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:34:
: browser_ppapi_host_(browser_ppapi_host), instance_(instance) { }
On 2013/05/02 23:22:01, yzshen1 wrote:
> nit: zero space between {}

Done.

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:44:
~CrxFileSystemFilter() {}
On 2013/05/02 23:22:01, yzshen1 wrote:
> use virtual, please (I know it makes no difference. But better to have it. :))

Done.

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:58:
content::BrowserPpapiHost* browser_ppapi_host_;
On 2013/05/02 23:22:01, yzshen1 wrote:
> This can only be used on IO thread. You should retrieve what you need in the
> constructor (because that is run on IO), and then forget about host and
> instance.

Done.

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:136:
return PP_OK_COMPLETIONPENDING;
On 2013/05/02 23:22:01, yzshen1 wrote:
> You could set the context->reply_msg and return PP_OK (and get rid of the
whole
> Reply() below).
> 
> Reply and then return PP_OK_COMPLETIONPENDING is a little bit weird.

Done.

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.cc:146:
browser_ppapi_host_->GetPpapiHost()->SendReply(
On 2013/05/02 23:22:01, yzshen1 wrote:
> This should not be used on UI thread. You should use the way I suggested
above,
> or use ResourceMessageFilter::SendReply().

Done.

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h
(right):

https://codereview.chromium.org/14188019/diff/109001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_ext_crx_file_system_browser_host.h:26:
class PepperExtCrxFileSystemBrowserHost : public ppapi::host::ResourceHost {
On 2013/05/02 23:22:01, yzshen1 wrote:
> With a message filter, you don't need this empty host anymore. We have a
generic
> container for this scenario. Please take a look at MessageFilterHost. An
example
> of how to use it:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Done.

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_system_host.cc (right):

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.cc:170: // sanity check
On 2013/05/02 23:22:01, yzshen1 wrote:
> Please use a sentence. (Capital initial and '.' at the end.)

Done.

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.cc:176: url.GetWithEmptyPath(),
fsid, "crxfs"));
On 2013/05/02 23:22:01, yzshen1 wrote:
> Shall we use url.GetOrigin?

Yes, that seems to be a better solution as it eliminate username and password,
which we don't expect.

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_system_host.h (right):

https://codereview.chromium.org/14188019/diff/109001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.h:50: const std::string&
root_url);
On 2013/05/02 23:22:01, yzshen1 wrote:
> fsid please.

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/api/pp_file_info.idl
File ppapi/api/pp_file_info.idl (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/api/pp_file_info.i...
ppapi/api/pp_file_info.idl:36: /** For isolated file system types (only for
CrxFileSystem) */
On 2013/05/02 23:22:01, yzshen1 wrote:
> I think we should avoid adding comments for private APIs to this public API
> document.
> 
> Maybe we add a comment to PPB_FileSystem.Open saying that currently it cannot
be
> used to open isolated file system?

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/api/private/ppb_ex...
File ppapi/api/private/ppb_ext_crx_file_system_private.idl (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/api/private/ppb_ex...
ppapi/api/private/ppb_ext_crx_file_system_private.idl:21: PP_Resource
Create([in] PP_Instance insance);
On 2013/05/02 23:22:01, yzshen1 wrote:
> You don't need to Create this PP_Resource and instead use the PP_Instance in
> Open(). (I am fine if you would like to keep it, but you have also added
> singleton resource-related code.)

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/api/private/ppb_ex...
ppapi/api/private/ppb_ext_crx_file_system_private.idl:30: * to a FileSystem.
On 2013/05/02 23:22:01, yzshen1 wrote:
> Please be more specific saying that it is a PPB_FileSystem.

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/file_system.h
File ppapi/cpp/file_system.h (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/file_system.h#...
ppapi/cpp/file_system.h:31: FileSystem(PassRef, PP_Resource resource);
On 2013/05/02 23:22:01, yzshen1 wrote:
> Add comments such as:
> 
> /// A constructor used when you have received a PP_Resource as a return
> /// value that has already been reference counted.
> ///
> /// @param[in] resource A PP_Resource corresponding to a file system.

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/private/ext_cr...
File ppapi/cpp/private/ext_crx_file_system_private.cc (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/private/ext_cr...
ppapi/cpp/private/ext_crx_file_system_private.cc:8: #include
"ppapi/cpp/completion_callback.h"
On 2013/05/02 23:22:01, yzshen1 wrote:
> You have included it in the .h.

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/private/ext_cr...
File ppapi/cpp/private/ext_crx_file_system_private.h (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/cpp/private/ext_cr...
ppapi/cpp/private/ext_crx_file_system_private.h:17: class
ExtCrxFileSystemPrivate : public FileSystem {
On 2013/05/02 23:22:01, yzshen1 wrote:
> Remove the base class.

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/host/file_system_r...
File ppapi/host/file_system_registry.cc (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/host/file_system_r...
ppapi/host/file_system_registry.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/05/02 23:22:01, yzshen1 wrote:
> You can delete this class entirely.

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/host/file_system_r...
File ppapi/host/file_system_registry.h (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/host/file_system_r...
ppapi/host/file_system_registry.h:27: class PPAPI_HOST_EXPORT FileSystemRegistry
{
On 2013/05/02 23:22:01, yzshen1 wrote:
> Please delete it entirely.

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ext_crx_file...
File ppapi/proxy/ext_crx_file_system_private_resource.cc (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:35:
DCHECK(file_system_resource);
On 2013/05/02 23:22:01, yzshen1 wrote:
> Please return failure if necessary instead of DCHECK. (Because this is used by
a
> public API.)

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:37: if
(!TrackedCallback::IsPending(callback))
On 2013/05/02 23:22:01, yzshen1 wrote:
> If you want to check whether it is in progress, you should check the callback
> you received in a previous call, or have a boolean flag.

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:52: const std::string& fsid)
{
On 2013/05/02 23:22:01, yzshen1 wrote:
> Test whether callback is still pending before doing other things. It may have
> been aborted and in that case |file_system_resource| may have been released by
> the plugin.

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:61: return;
On 2013/05/02 23:22:01, yzshen1 wrote:
> [I don't know the answer but would like to make sure you have considered that]
> do we need to somehow notify the browser if we never pass this fsid to the
> renderer. Will the file system stay open forever?

Isolated filesystem will live until the end of renderer process (see
crbug.com/142533).

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ppb_instance...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:370: case CRX_FILESYSTEM_SINGLETON_ID:
On 2013/05/02 23:22:01, yzshen1 wrote:
> If you make it a singleton, you don't need the Create method.
> You can take a look at the other singleton resources above how that is done.

Ack.

https://codereview.chromium.org/14188019/diff/109001/ppapi/shared_impl/single...
File ppapi/shared_impl/singleton_resource_id.h (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/shared_impl/single...
ppapi/shared_impl/singleton_resource_id.h:26: CRX_FILESYSTEM_SINGLETON_ID,
On 2013/05/02 23:22:01, yzshen1 wrote:
> Alphabetically.

Done.

https://codereview.chromium.org/14188019/diff/109001/ppapi/thunk/ppb_file_ref...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/109001/ppapi/thunk/ppb_file_ref...
ppapi/thunk/ppb_file_ref_thunk.cc:31:
EnterResourceNoLock<PPB_Ext_CrxFileSystem_Private_API> enter_crxfs(
On 2013/05/02 23:22:01, yzshen1 wrote:
> You don't need this anymore.

Done.

https://codereview.chromium.org/14188019/diff/109001/webkit/plugins/ppapi/ppa...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://codereview.chromium.org/14188019/diff/109001/webkit/plugins/ppapi/ppa...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2152: case
::ppapi::CRX_FILESYSTEM_SINGLETON_ID:
On 2013/05/02 23:22:01, yzshen1 wrote:
> alphabetically, please.

Done.

https://codereview.chromium.org/14188019/diff/119001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_system_host.h (right):

https://codereview.chromium.org/14188019/diff/119001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.h:22: public
ppapi::host::FileSystemRegistry::FileSystemPeeker,
On 2013/05/02 23:22:01, yzshen1 wrote:
> no need to have this peeker interface.

Done.

https://codereview.chromium.org/14188019/diff/119001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://codereview.chromium.org/14188019/diff/119001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1026: return
ppapi::host::FileSystemRegistry::GetInstance()->
On 2013/05/02 23:22:01, yzshen1 wrote:
> You should remove all changes in this file.

Done.

[yzshen1, 2013-05-06 17:34:57]

Only a few minor issues left.

Thanks!

https://codereview.chromium.org/14188019/diff/138001/chrome/browser/renderer_...
File chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/14188019/diff/138001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc:59:
host_->GetRenderViewIDsForInstance(instance,
- Please check whether it fails.
- [optional] it seems better to have a static Create() for the message filter,
which takes host as parameter and do
GetRenderViewIDsForInstance/GetProfileDataDirectory/GetDocumentURLForInstance
there. So that we don't have to bloat this file.

https://codereview.chromium.org/14188019/diff/138001/ppapi/cpp/private/ext_cr...
File ppapi/cpp/private/ext_crx_file_system_private.h (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/cpp/private/ext_cr...
ppapi/cpp/private/ext_crx_file_system_private.h:12: #include
"ppapi/cpp/resource.h"
It sees this is not needed?

https://codereview.chromium.org/14188019/diff/138001/ppapi/examples/crxfs/crx...
File ppapi/examples/crxfs/crxfs.cc (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:5: #include <stdio.h>
nit: an empty line below line 5.

https://codereview.chromium.org/14188019/diff/138001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:19: // Buffer size for reading network data.
nit: network data?

https://codereview.chromium.org/14188019/diff/138001/ppapi/examples/crxfs/man...
File ppapi/examples/crxfs/manifest.json (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/examples/crxfs/man...
ppapi/examples/crxfs/manifest.json:2: "name": "My Native Client Test",
nit: currently it is not compiled as a native client module?

https://codereview.chromium.org/14188019/diff/138001/ppapi/proxy/ext_crx_file...
File ppapi/proxy/ext_crx_file_system_private_resource.cc (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:41: 
What if a second call to Open() happens before |opened_| is set to true? Does
that work as expected?

https://codereview.chromium.org/14188019/diff/138001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:71: if
(*file_system_resource > 0) {
nit: Please use != 0. (for consistency)

https://codereview.chromium.org/14188019/diff/138001/ppapi/proxy/resource_cre...
File ppapi/proxy/resource_creation_proxy.h (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/proxy/resource_cre...
ppapi/proxy/resource_creation_proxy.h:42: virtual PP_Resource
CreateExtCrxFileSystemPrivate(PP_Instance instance)
As I said, I don't think you still need this. (And please also change
ResourceCreationAPI and the impl at the renderer side.)

https://codereview.chromium.org/14188019/diff/138001/ppapi/thunk/ppb_file_ref...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/thunk/ppb_file_ref...
ppapi/thunk/ppb_file_ref_thunk.cc:29: EnterResourceNoLock<PPB_FileSystem_API>
enter_file_system(file_system, false);
Now you could use 'true' again. :P (reverting all changes in this file.)

https://codereview.chromium.org/14188019/diff/138001/webkit/plugins/ppapi/res...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

https://codereview.chromium.org/14188019/diff/138001/webkit/plugins/ppapi/res...
webkit/plugins/ppapi/resource_creation_impl.cc:84: PP_Resource
ResourceCreationImpl::CreateExtCrxFileSystemPrivate(
You don't need this method.

[victorhsieh, 2013-05-06 18:32:01]

Thanks for review!

https://codereview.chromium.org/14188019/diff/138001/chrome/browser/renderer_...
File chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/14188019/diff/138001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc:59:
host_->GetRenderViewIDsForInstance(instance,
On 2013/05/06 17:34:57, yzshen1 wrote:
> - Please check whether it fails.
> - [optional] it seems better to have a static Create() for the message filter,
> which takes host as parameter and do
> GetRenderViewIDsForInstance/GetProfileDataDirectory/GetDocumentURLForInstance
> there. So that we don't have to bloat this file.

Done.

https://codereview.chromium.org/14188019/diff/138001/ppapi/cpp/private/ext_cr...
File ppapi/cpp/private/ext_crx_file_system_private.h (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/cpp/private/ext_cr...
ppapi/cpp/private/ext_crx_file_system_private.h:12: #include
"ppapi/cpp/resource.h"
On 2013/05/06 17:34:57, yzshen1 wrote:
> It sees this is not needed?

Done.

https://codereview.chromium.org/14188019/diff/138001/ppapi/examples/crxfs/crx...
File ppapi/examples/crxfs/crxfs.cc (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:5: #include <stdio.h>
On 2013/05/06 17:34:57, yzshen1 wrote:
> nit: an empty line below line 5.

Done.

https://codereview.chromium.org/14188019/diff/138001/ppapi/examples/crxfs/crx...
ppapi/examples/crxfs/crxfs.cc:19: // Buffer size for reading network data.
On 2013/05/06 17:34:57, yzshen1 wrote:
> nit: network data?

Done.

https://codereview.chromium.org/14188019/diff/138001/ppapi/examples/crxfs/man...
File ppapi/examples/crxfs/manifest.json (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/examples/crxfs/man...
ppapi/examples/crxfs/manifest.json:2: "name": "My Native Client Test",
On 2013/05/06 17:34:57, yzshen1 wrote:
> nit: currently it is not compiled as a native client module?

Done.

https://codereview.chromium.org/14188019/diff/138001/ppapi/proxy/ext_crx_file...
File ppapi/proxy/ext_crx_file_system_private_resource.cc (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:41: 
On 2013/05/06 17:34:57, yzshen1 wrote:
> What if a second call to Open() happens before |opened_| is set to true? Does
> that work as expected?

Now it works like FileSystem: Open can be called only once (though there can be
a better implementation).

https://codereview.chromium.org/14188019/diff/138001/ppapi/proxy/ext_crx_file...
ppapi/proxy/ext_crx_file_system_private_resource.cc:71: if
(*file_system_resource > 0) {
On 2013/05/06 17:34:57, yzshen1 wrote:
> nit: Please use != 0. (for consistency)

Done.

https://codereview.chromium.org/14188019/diff/138001/ppapi/proxy/resource_cre...
File ppapi/proxy/resource_creation_proxy.h (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/proxy/resource_cre...
ppapi/proxy/resource_creation_proxy.h:42: virtual PP_Resource
CreateExtCrxFileSystemPrivate(PP_Instance instance)
On 2013/05/06 17:34:57, yzshen1 wrote:
> As I said, I don't think you still need this. (And please also change
> ResourceCreationAPI and the impl at the renderer side.)

Done.

https://codereview.chromium.org/14188019/diff/138001/ppapi/thunk/ppb_file_ref...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://codereview.chromium.org/14188019/diff/138001/ppapi/thunk/ppb_file_ref...
ppapi/thunk/ppb_file_ref_thunk.cc:29: EnterResourceNoLock<PPB_FileSystem_API>
enter_file_system(file_system, false);
On 2013/05/06 17:34:57, yzshen1 wrote:
> Now you could use 'true' again. :P (reverting all changes in this file.)

Done.

https://codereview.chromium.org/14188019/diff/138001/webkit/plugins/ppapi/res...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

https://codereview.chromium.org/14188019/diff/138001/webkit/plugins/ppapi/res...
webkit/plugins/ppapi/resource_creation_impl.cc:84: PP_Resource
ResourceCreationImpl::CreateExtCrxFileSystemPrivate(
On 2013/05/06 17:34:57, yzshen1 wrote:
> You don't need this method.

Done.

[yzshen1, 2013-05-06 20:50:28]

LGTM

Thanks!

[victorhsieh, 2013-05-06 21:16:29]

@joi, could you please review the change for
content/public/renderer/renderer_ppapi_host.h, thanks.

[Jói, 2013-05-06 22:30:34]

LGTM. Adding jam@ as FYI since this is an interface change.

[commit-bot: I haz the power, 2013-05-06 22:36:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/14188019/167003

[commit-bot: I haz the power, 2013-05-06 22:58:37]

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

[commit-bot: I haz the power, 2013-05-06 22:59:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/14188019/76002

[commit-bot: I haz the power, 2013-05-06 23:37:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/14188019/201027

[commit-bot: I haz the power, 2013-05-07 00:38:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
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, 2013-05-07 15:36:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/14188019/211003

[commit-bot: I haz the power, 2013-05-07 15:49:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_rel_device.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
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.

[jam, 2013-05-07 16:27:20]

https://codereview.chromium.org/14188019/diff/211003/content/public/renderer/...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/14188019/diff/211003/content/public/renderer/...
content/public/renderer/renderer_ppapi_host.h:123: virtual GURL
GetDocumentURLForInstance(PP_Instance instance) const = 0;
this doesn't seem necessary?
i.e. the embedder can call

GetRenderViewForInstance()->GetWebView()->mainFrame()->document().url()

[victorhsieh, 2013-05-07 17:51:30]

https://codereview.chromium.org/14188019/diff/211003/content/public/renderer/...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/14188019/diff/211003/content/public/renderer/...
content/public/renderer/renderer_ppapi_host.h:123: virtual GURL
GetDocumentURLForInstance(PP_Instance instance) const = 0;
On 2013/05/07 16:27:20, jam wrote:
> this doesn't seem necessary?
> i.e. the embedder can call
> 
> GetRenderViewForInstance()->GetWebView()->mainFrame()->document().url()

Done.

[commit-bot: I haz the power, 2013-05-07 18:28:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/14188019/234001

[commit-bot: I haz the power, 2013-05-07 18:56:51]

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

[commit-bot: I haz the power, 2013-05-07 21:14:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/14188019/234001

[commit-bot: I haz the power, 2013-05-08 18:05:00]

Change committed as 198938