[PPAPI] Added VarResource_Dev class.

[PPAPI] Added pp::VarResource_Dev class.

This is a C++ wrapper for the C API PPB_VarResource_Dev.

Also added methods to pp::FileSystem for converting a pp::Resource to a
pp::FileSystem, which are necessary for making use of a resource
extracted using pp::VarResource_Dev.

(Committed by yzshen@chromium.org on behalf of mgiuca@chromium.org)

BUG=177017
R=dmichael@chromium.org, noelallen@chromium.org, noelallen@google.com, yzshen@chromium.org

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

[Matt Giuca, 2013-10-30 11:35:27]

Hi guys,

This CL is not quite ready yet. But as I want to get it in for M32, I am mailing
it out for an early review. Please take a quick look and let me know if you see
any issues. I will have it ready for final review tomorrow.

I just realised that I forgot to check our original design document before
implementing this. The design doc called for adding a new Var constructor (const
Resource&) and a new method AsResource. Here, I have implemented it as a
separate class. I'm happy to change it back to adding methods to Var (much
simpler). One downside of this is that we won't be able to make it Dev-only. But
it is probably a cleaner API for users, so I am inclined to do that.

Still to do:
- Change the existing tests to use this API.
- Making sure ref counting is good.

I will also be adding pp::FileSystem::ResourceIsFileSystem and a way to safely
downcast from pp::Resource to pp::FileSystem (in another CL).

noelallen: For OWNERS on library.dsc.

[yzshen1, 2013-10-30 16:50:01]

> I just realised that I forgot to check our original design document before
> implementing this. The design doc called for adding a new Var constructor
(const
> Resource&) and a new method AsResource. Here, I have implemented it as a
> separate class. I'm happy to change it back to adding methods to Var (much
> simpler). One downside of this is that we won't be able to make it Dev-only.
But
> it is probably a cleaner API for users, so I am inclined to do that.
The current way is consistent with other "complex" var types. But since we only
need to provide conversion methods from and back to resource, it sounds good to
me to directly add methods to Var.

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
File ppapi/cpp/dev/var_resource_dev.cc (right):

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:60: if (other.is_array()) {
resource :)

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:69: pp::Resource VarResource_Dev::ToResource()
{
Maybe we could also consider something like:

template <class ResourceSubclassType>
bool ToResource(ResourceSubclassType* resource) const

or
template <class ResourceSubclassType>
ResourceSubclassType ToResource() const?

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:74: return pp::Resource(
I think you need to use Resource(PassRef, PP_Resource).

[dmichael (off chromium), 2013-10-30 18:03:09]

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
File ppapi/cpp/dev/var_resource_dev.cc (right):

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:29: resource.pp_resource());
This should be fine, but you also have to set "is_managed_" to true. :'-(

It might be worthwhile adding something like:
void TakeOwnership(PP_Var var)
as a protected member of Var, so you can use it here and in VarArrayBuffer,
VarArray, and VarDictionary. These latter two actually look buggy to me, because
they don't set is_managed_.

(Note...  I have an old bug sitting around that we should get rid of is_managed_
entirely https://code.google.com/p/chromium/issues/detail?id=52106 )

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:41: VarResource_Dev::VarResource_Dev(const
PP_Var& var) : Var(var) {
Are you using this for anything? If you can avoid it, I'd rather leave it out.
We should keep the PP_Var stuff to a minimum in the C++ API, because it opens
the door to ref-counting mistakes.

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:74: return pp::Resource(
On 2013/10/30 16:50:01, yzshen1 wrote:
> I think you need to use Resource(PassRef, PP_Resource).
+1

[yzshen1, 2013-10-30 18:10:34]

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
File ppapi/cpp/dev/var_resource_dev.cc (right):

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:29: resource.pp_resource());
On 2013/10/30 18:03:10, dmichael wrote:
> This should be fine, but you also have to set "is_managed_" to true. :'-(
> 
> It might be worthwhile adding something like:
> void TakeOwnership(PP_Var var)
> as a protected member of Var, so you can use it here and in VarArrayBuffer,
> VarArray, and VarDictionary. These latter two actually look buggy to me,
because
> they don't set is_managed_.

Var(Null) constructor sets |is_managed_| to true, right?

> 
> (Note...  I have an old bug sitting around that we should get rid of
is_managed_
> entirely https://code.google.com/p/chromium/issues/detail?id=52106 )

[dmichael (off chromium), 2013-10-30 18:13:27]

On Wed, Oct 30, 2013 at 12:10 PM, <yzshen@chromium.org> wrote:

>
> https://codereview.chromium.**org/52233002/diff/1/ppapi/cpp/**
>
dev/var_resource_dev.cc<https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_dev.cc>
> File ppapi/cpp/dev/var_resource_**dev.cc (right):
>
> https://codereview.chromium.**org/52233002/diff/1/ppapi/cpp/**
>
dev/var_resource_dev.cc#**newcode29<https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_dev.cc#newcode29>
> ppapi/cpp/dev/var_resource_**dev.cc:29: resource.pp_resource());
> On 2013/10/30 18:03:10, dmichael wrote:
>
>> This should be fine, but you also have to set "is_managed_" to true.
>>
> :'-(
>
>  It might be worthwhile adding something like:
>> void TakeOwnership(PP_Var var)
>> as a protected member of Var, so you can use it here and in
>>
> VarArrayBuffer,
>
>> VarArray, and VarDictionary. These latter two actually look buggy to
>>
> me, because
>
>> they don't set is_managed_.
>>
>
> Var(Null) constructor sets |is_managed_| to true, right?

Oh, right, thanks. I guess you can ignore those comments. It's confusing,
because "is_managed_" really only matters for ref-counted Vars. I still
think we ought to kill it with fire :-)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2013-10-30 18:19:42]

> Oh, right, thanks. I guess you can ignore those comments. It's confusing,
> because "is_managed_" really only matters for ref-counted Vars. I still
> think we ought to kill it with fire :-)

Agreed. Thanks, David!

[Matt Giuca, 2013-10-30 21:54:41]

Hi, thanks very much for your initial comments. I will take them into account.

But I don't think either of you addressed the question: should we actually have
VarResource_Dev at all, or should we just add a ToResource method, and a
from-Resource constructor in Var? Now that I have thought about it, I think that
the simpler approach (in Var) is better for users. But the one concern is that I
don't know how to do this while the API is in Dev preview.

Is it possible we could do VarResource_Dev, and then when we move it out of Dev,
change the API so it's just new methods on Var? Or does that defeat the whole
point of Dev?

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
File ppapi/cpp/dev/var_resource_dev.cc (right):

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:60: if (other.is_array()) {
Oops, as you can see, I made this in a hurry and didn't have time last night to
write any test code. I just wanted to get a first opinion from you guys as
quickly as possible. Sorry!

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:69: pp::Resource VarResource_Dev::ToResource()
{
I'll have to think about this. What would the usage look like?

VarResource_Dev var_resource = ...;
FileSystem filesystem;
if (!var_resource.ToResource(&filesystem))
  // ERROR: Not a file system.
// Use filesystem.

?

Seems like an OK user interface, but I have some questions:
- Is the second version (nicer) actually possible? I thought you couldn't
control a template based on the return type of the function.
- What would happen if you just passed a pp::Resource base class? Would it still
be expected to work?
- How would this be implemented? Would we need a template specialization for
each class? I thought we were trying to avoid having code in VarResource_Dev for
each subclass. The original design was for each of the resource types such as
FileSystem to have a converter from Resource to itself.
- I'm typically wary of template magic, especially exposing it to the user. I
think this method is going to behave quite differently to any other method, as
it will automatically downcast depending on the type you give it. I feel like a
simpler non template-based one is going to be much easier to document and easier
for users to think about, even if they have to do a bit more work.

Thoughts?

[dmichael (off chromium), 2013-10-30 22:03:11]

On Wed, Oct 30, 2013 at 3:54 PM, <mgiuca@chromium.org> wrote:

> Hi, thanks very much for your initial comments. I will take them into
> account.
>
> But I don't think either of you addressed the question: should we actually
> have
> VarResource_Dev at all

Yes, IMHO. This is consistent with what we've done for other new Var types,
and keeps us from bloating pp::Var.


> , or should we just add a ToResource method, and a
> from-Resource constructor in Var? Now that I have thought about it, I
> think that
> the simpler approach (in Var) is better for users. But the one concern is
> that I
> don't know how to do this while the API is in Dev preview.
>
> Is it possible we could do VarResource_Dev, and then when we move it out
> of Dev,
> change the API so it's just new methods on Var? Or does that defeat the
> whole
> point of Dev?

Yes, we could do it that way. But I prefer we keep VarResource separate
anyway.


>
>
>
> https://codereview.chromium.**org/52233002/diff/1/ppapi/cpp/**
>
dev/var_resource_dev.cc<https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_dev.cc>
> File ppapi/cpp/dev/var_resource_**dev.cc (right):
>
> https://codereview.chromium.**org/52233002/diff/1/ppapi/cpp/**
>
dev/var_resource_dev.cc#**newcode60<https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_dev.cc#newcode60>
> ppapi/cpp/dev/var_resource_**dev.cc:60: if (other.is_array()) {
> Oops, as you can see, I made this in a hurry and didn't have time last
> night to write any test code. I just wanted to get a first opinion from
> you guys as quickly as possible. Sorry!
>
>
> https://codereview.chromium.**org/52233002/diff/1/ppapi/cpp/**
>
dev/var_resource_dev.cc#**newcode69<https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_dev.cc#newcode69>
> ppapi/cpp/dev/var_resource_**dev.cc:69: pp::Resource
> VarResource_Dev::ToResource() {
> I'll have to think about this. What would the usage look like?
>
> VarResource_Dev var_resource = ...;
> FileSystem filesystem;
> if (!var_resource.ToResource(&**filesystem))
>   // ERROR: Not a file system.
> // Use filesystem.
>
> ?
>
> Seems like an OK user interface, but I have some questions:
> - Is the second version (nicer) actually possible? I thought you
> couldn't control a template based on the return type of the function.
>
What you're showing above has the template parameter type as an argument,
so the compiler can deduce it. If you use Yuzhu's second version, it would
look like this:
FileSystem filesystem = var_resource.ToResource<FileSystem>();

I think I like the first one better, where you don't have to provide the
template parameter, and it's clear that you have to check for an error
result.


> - What would happen if you just passed a pp::Resource base class? Would
> it still be expected to work?
> - How would this be implemented? Would we need a template specialization
> for each class? I thought we were trying to avoid having code in
> VarResource_Dev for each subclass.

I think the template specialization could live in file_system.h.


> The original design was for each of
> the resource types such as FileSystem to have a converter from Resource
> to itself.
> - I'm typically wary of template magic, especially exposing it to the
> user. I think this method is going to behave quite differently to any
> other method, as it will automatically downcast depending on the type
> you give it. I feel like a simpler non template-based one is going to be
> much easier to document and easier for users to think about, even if
> they have to do a bit more work.
>
> Thoughts?
>
I agree, we should go with the simplest way for now. We can consider adding
Yuzhu's suggestion later if it seems compelling.


>
>
https://codereview.chromium.**org/52233002/<https://codereview.chromium.org/5...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Matt Giuca, 2013-10-30 23:54:14]

> Yes, IMHO. This is consistent with what we've done for other new Var types,
> and keeps us from bloating pp::Var.

Fair enough.

> What you're showing above has the template parameter type as an argument,
> so the compiler can deduce it. If you use Yuzhu's second version, it would
> look like this:
> FileSystem filesystem = var_resource.ToResource<FileSystem>();

Right. That's kind of a burden on the user.

> I think I like the first one better, where you don't have to provide the
> template parameter, and it's clear that you have to check for an error
> result.

Agreed.

> I agree, we should go with the simplest way for now. We can consider adding
> Yuzhu's suggestion later if it seems compelling.

Yup, OK, for this CL we'll just do the simple approach. (While I would love to
move this to stable before M32 branch point, at this point I don't think it's
feasible, so we will have a bit of time to play around with the API.)

[yzshen1, 2013-10-31 00:48:31]

On Wed, Oct 30, 2013 at 2:54 PM, <mgiuca@chromium.org> wrote:

> Hi, thanks very much for your initial comments. I will take them into
> account.
>
> But I don't think either of you addressed the question: should we actually
> have
> VarResource_Dev at all, or should we just add a ToResource method, and a
> from-Resource constructor in Var? Now that I have thought about it, I
> think that
> the simpler approach (in Var) is better for users. But the one concern is
> that I
> don't know how to do this while the API is in Dev preview.


(I did say something about this in the previous mail. :))

IMO, it also makes some sense to add conversion methods in pp::Var.
VarResource_Dev doesn't provide many operations like VarArray or
VarDictionary.

It feels more like string var, for which we directly provide a constructor
and a AsString() method in Var.

What do you think?

Is it possible we could do VarResource_Dev, and then when we move it out of
> Dev,
> change the API so it's just new methods on Var? Or does that defeat the
> whole
> point of Dev?

If we would like to add conversion methods in pp::Var, maybe we could
create a subclass of pp::Var, say, pp::Var_Dev, and put the new methods in
it.



>
> https://codereview.chromium.**org/52233002/diff/1/ppapi/cpp/**
>
dev/var_resource_dev.cc<https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_dev.cc>
> File ppapi/cpp/dev/var_resource_**dev.cc (right):
>
> https://codereview.chromium.**org/52233002/diff/1/ppapi/cpp/**
>
dev/var_resource_dev.cc#**newcode60<https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_dev.cc#newcode60>
> ppapi/cpp/dev/var_resource_**dev.cc:60: if (other.is_array()) {
> Oops, as you can see, I made this in a hurry and didn't have time last
> night to write any test code. I just wanted to get a first opinion from
> you guys as quickly as possible. Sorry!


No worries! (I did similar things many times. :))


>
>
> https://codereview.chromium.**org/52233002/diff/1/ppapi/cpp/**
>
dev/var_resource_dev.cc#**newcode69<https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_dev.cc#newcode69>
> ppapi/cpp/dev/var_resource_**dev.cc:69: pp::Resource
> VarResource_Dev::ToResource() {
> I'll have to think about this. What would the usage look like?
>
> VarResource_Dev var_resource = ...;
> FileSystem filesystem;
> if (!var_resource.ToResource(&**filesystem))
>   // ERROR: Not a file system.
> // Use filesystem.
>
> ?
>
> Seems like an OK user interface, but I have some questions:
> - Is the second version (nicer) actually possible? I thought you
> couldn't control a template based on the return type of the function.
> - What would happen if you just passed a pp::Resource base class? Would
> it still be expected to work?
> - How would this be implemented? Would we need a template specialization
> for each class? I thought we were trying to avoid having code in
> VarResource_Dev for each subclass. The original design was for each of
> the resource types such as FileSystem to have a converter from Resource
> to itself.
> - I'm typically wary of template magic, especially exposing it to the
> user. I think this method is going to behave quite differently to any
> other method, as it will automatically downcast depending on the type
> you give it. I feel like a simpler non template-based one is going to be
> much easier to document and easier for users to think about, even if
> they have to do a bit more work.
>
> Thoughts?
>

I am fine that we consider template helpers later.

I just feel that it is not recommended to directly use pp::Resource. (Maybe
I am wrong.) Although it has public constructors, it doesn't have a way to
set the internal PP_Resource unless we copy/assign another pp::Resource. It
doesn't look like some kind of smart pointer for generic PP_Resources to
me. What do you think, David?



>
>
https://codereview.chromium.**org/52233002/<https://codereview.chromium.org/5...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Matt Giuca, 2013-10-31 06:33:46]

I have responded to all the comments. Also I decided to add the FileSystem
methods here, since I think the number of CLs I will be able to submit before
the branch point is limited.

Now I think I agree with Yuzhu about the methods being in Var instead of
VarResource. I will try to do the subclass.

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
File ppapi/cpp/dev/var_resource_dev.cc (right):

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:29: resource.pp_resource());
Correct. So this code should be fine as it is.

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:41: VarResource_Dev::VarResource_Dev(const
PP_Var& var) : Var(var) {
On 2013/10/30 18:03:10, dmichael wrote:
> Are you using this for anything? If you can avoid it, I'd rather leave it out.
> We should keep the PP_Var stuff to a minimum in the C++ API, because it opens
> the door to ref-counting mistakes.

Done.

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:60: if (other.is_array()) {
On 2013/10/30 21:54:41, Matt Giuca wrote:
> Oops, as you can see, I made this in a hurry and didn't have time last night
to
> write any test code. I just wanted to get a first opinion from you guys as
> quickly as possible. Sorry!

Done.

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:69: pp::Resource VarResource_Dev::ToResource()
{
As discussed, saving for another CL, if at all. Let's see how we go with the API
discussed in the design doc.

https://codereview.chromium.org/52233002/diff/1/ppapi/cpp/dev/var_resource_de...
ppapi/cpp/dev/var_resource_dev.cc:74: return pp::Resource(
On 2013/10/30 18:03:10, dmichael wrote:
> On 2013/10/30 16:50:01, yzshen1 wrote:
> > I think you need to use Resource(PassRef, PP_Resource).
> +1

Done.

[Matt Giuca, 2013-10-31 06:42:11]

On 2013/10/31 00:48:31, yzshen1 wrote:
> IMO, it also makes some sense to add conversion methods in pp::Var.
> VarResource_Dev doesn't provide many operations like VarArray or
> VarDictionary.
> 
> It feels more like string var, for which we directly provide a constructor
> and a AsString() method in Var.

Yes, my feeling is that it is more like AsString (which is why I wrote it this
way in the design doc). Having played with the current API (VarResource_Dev) a
bit, it does seem a bit annoying that to get a resource out of a Var, you have
to do:

pp::Var var;
pp::VarResource_Dev var_resource(var);
pp::Resource resource = var_resource.AsResource();

However, it does seem pretty annoying to have to provide the Var_Dev subclass.
The user would have to copy the Var to a Var_Dev before they can access the new
methods, so it's effectively equivalent to VarResource_Dev anyway. Since we are
short on time, my opinion is that we should just go with VarResource_Dev for
now, and if we decide to move the methods into Var, we can just do that at the
time when we move this API into stable (directly moving the methods from
VarResource_Dev to Var). Does this sound like a good plan?

Other than that, this CL is ready for full review, so PTAL! Thanks.

[dmichael (off chromium), 2013-10-31 16:48:55]

lgtm

I agree, Var_Dev would look the same as VarResource_Dev in practice, so might as
well use VarResource_Dev for now. Since it's only one method, I'm fine with us
sticking AsResource in to Var when the time is right.

https://codereview.chromium.org/52233002/diff/160001/ppapi/cpp/file_system.cc
File ppapi/cpp/file_system.cc (right):

https://codereview.chromium.org/52233002/diff/160001/ppapi/cpp/file_system.cc...
ppapi/cpp/file_system.cc:32: PP_DCHECK(ResourceIsFileSystem(resource));
Let's make the Resource a "Null" one in this case (for release builds). It might
make sense to add something like a "Clear" or "Release" in pp::Resource so you
can just use that here.

[noelallen_use_chromium, 2013-10-31 21:49:01]

The change to the .dsc LGTM

However, is this really needed?  Can't we just add this to Var?  Do we really
need to add another class and interface?

[Matt Giuca, 2013-10-31 22:10:40]

On 2013/10/31 21:49:01, noelallen wrote:
> However, is this really needed?  Can't we just add this to Var?  Do we really
> need to add another class and interface?

There was a discussion about this above (see comments #7 through to #13). The
rough consensus was that it would be cleaner to just add it to Var, but due to
complications about making it Dev-only, we will keep VarResource_Dev for now.
Most likely when we move the API to stable, we will just move those methods into
Var itself, and not create a VarResource class.

[Matt Giuca, 2013-10-31 23:24:43]

https://codereview.chromium.org/52233002/diff/160001/ppapi/cpp/file_system.cc
File ppapi/cpp/file_system.cc (right):

https://codereview.chromium.org/52233002/diff/160001/ppapi/cpp/file_system.cc...
ppapi/cpp/file_system.cc:32: PP_DCHECK(ResourceIsFileSystem(resource));
On 2013/10/31 16:48:56, dmichael wrote:
> Let's make the Resource a "Null" one in this case (for release builds). It
might
> make sense to add something like a "Clear" or "Release" in pp::Resource so you
> can just use that here.

Done.

Note: I added Clear() as a protected method on Resource. It wouldn't hurt to
make it public, but I didn't want to change the public interface at this stage.
We can upgrade it to public later if you think that's desirable.

[yzshen1, 2013-11-01 00:14:41]

LGTM with two nits.

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

https://codereview.chromium.org/52233002/diff/60002/ppapi/cpp/file_system.h#n...
ppapi/cpp/file_system.h:39: explicit FileSystem(Resource resource);
const & ?

https://codereview.chromium.org/52233002/diff/60002/ppapi/cpp/file_system.h#n...
ppapi/cpp/file_system.h:78: static bool ResourceIsFileSystem(const Resource&
resource);
Maybe 'Resource' is not necessary in the method name. That way it is more
consistent with the C interface (which has an IsFileSystem)

[commit-bot: I haz the power, 2013-11-01 01:44:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/52233002/450001

[Matt Giuca, 2013-11-01 02:06:22]

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

https://codereview.chromium.org/52233002/diff/60002/ppapi/cpp/file_system.h#n...
ppapi/cpp/file_system.h:39: explicit FileSystem(Resource resource);
On 2013/11/01 00:14:41, yzshen1 wrote:
> const & ?

Done.

https://codereview.chromium.org/52233002/diff/60002/ppapi/cpp/file_system.h#n...
ppapi/cpp/file_system.h:78: static bool ResourceIsFileSystem(const Resource&
resource);
On 2013/11/01 00:14:41, yzshen1 wrote:
> Maybe 'Resource' is not necessary in the method name. That way it is more
> consistent with the C interface (which has an IsFileSystem)

Done.

[Matt Giuca, 2013-11-01 02:44:32]

TBR=noelallen@chromium.org
(He used his google.com email to LGTM this which doesn't give OWNERS.)

[commit-bot: I haz the power, 2013-11-01 02:51:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/52233002/450001

[commit-bot: I haz the power, 2013-11-01 03:14:48]

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

[noelallen1, 2013-11-01 16:26:02]

lgtm

[yzshen1, 2013-11-01 22:17:25]

Committed patchset #5 manually as r232482.