766324 - Add a volume IDL to make volumes scriptable

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Dave Hylands [:dhylands]]

This should also allow nsIObserver and nsIObserverService to be used.

Comment 1

[Dave Hylands [:dhylands]]

Attachment: Bug 766324 - Add a volume IDL to make volumes scriptable

- Added nsIVolume.idl, nsIVolumeStat.idl, and nsIVolumeService.idl along with the associate C++ code.
- Updated Volume class to use STATE constants from nsIVolume.idl
- Updated VolumeManager to use nsTArray rather than std::vector
- Added test code for the nsIVolume*.idl files.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 639199 [details] [diff] [review]
Bug 766324 - Add a volume IDL to make volumes scriptable

Review of attachment 639199 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, mostly issues with nsCOMPtr/nsRefPtr consistency in VolumeService.

::: dom/system/gonk/nsVolume.cpp
@@ +29,5 @@
> +  *aState = mState;
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP nsVolume::GetStats(nsIVolumeStat * *aResult NS_OUTPARAM)

Nit: No space between *'s.

::: dom/system/gonk/nsVolume.h
@@ +20,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIVOLUME
> +
> +  nsVolume(const Volume *aVolume)
> +    : mName(NS_ConvertUTF8toUTF16(aVolume->Name())),

Just curious, how much are these getting passed around internally? I've been trying to make a habit of storing into nsString at the lowest level possible (which isn't something you've had to worry about thus far since this hasn't been XPCOM exposed, I know), but I'm not sure where this goes down the vold.

::: dom/system/gonk/nsVolumeService.cpp
@@ +86,5 @@
> +
> +  nsVolume::Array::size_type  numVolumes = mVolumeArray.Length();
> +  nsVolume::Array::index_type volIndex;
> +  for (volIndex = 0; volIndex < numVolumes; volIndex++) {
> +    nsCOMPtr<nsVolume> vol = mVolumeArray[volIndex];

This should be an nsRefPtr if you're going to use the concrete nsVolume type, otherwise switch it to nsCOMPtr<nsIVolume>

@@ +98,5 @@
> +  }
> +  return NS_ERROR_NOT_AVAILABLE;
> +}
> +
> +already_AddRefed<nsVolume> nsVolumeService::FindVolumeByName(const nsAString &aName)

Same nsRefPtr/nsCOMPtr issue as above, or since this isn't exposed, you could return TemporaryRef for the RefPtr.

@@ +113,5 @@
> +  }
> +  return NULL;
> +}
> +
> +already_AddRefed<nsVolume> nsVolumeService::FindAddVolumeByName(const nsAString &aName)

And one more time. :)

There's a couple more below too.

::: dom/system/gonk/nsVolumeServiceIOThread.h
@@ +15,5 @@
> +/***************************************************************************
> +* The nsVolumeServiceIOThread is a companion class to the nsVolumeService
> +* class, but whose methods are called from IOThread.
> +*/
> +class nsVolumeServiceIOThread : public VolumeManager::StateObserver,

If this isn't exposed to XPCOM (which I don't think it is?), does it need the ns prefix?

::: dom/system/gonk/nsVolumeServiceTest.cpp
@@ +31,5 @@
> +
> +/***************************************************************************
> +* A test class to verify that the Observer stuff is working properly.
> +*/
> +class NSVolumeTestObserver : public nsIObserver

Lowercase ns if you're gonna use it, otherwise we look like obj-c. :)

Comment 3

[Dave Hylands [:dhylands]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #2)
> Comment on attachment 639199 [details] [diff] [review]
> Bug 766324 - Add a volume IDL to make volumes scriptable
> 
> Review of attachment 639199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, mostly issues with nsCOMPtr/nsRefPtr consistency in
> VolumeService.

I wasn't aware of the nsCOMPtr vs nsRefPtr semantics, so I'll fix those up.

> ::: dom/system/gonk/nsVolume.cpp
> @@ +29,5 @@
> > +  *aState = mState;
> > +  return NS_OK;
> > +}
> > +
> > +NS_IMETHODIMP nsVolume::GetStats(nsIVolumeStat * *aResult NS_OUTPARAM)
> 
> Nit: No space between *'s.

So I normally wouldn't have put the space there. The header generated from the .idl put it there, and I figured it must know better. It also used _result rather than aResult.

> ::: dom/system/gonk/nsVolume.h
> @@ +20,5 @@
> > +  NS_DECL_ISUPPORTS
> > +  NS_DECL_NSIVOLUME
> > +
> > +  nsVolume(const Volume *aVolume)
> > +    : mName(NS_ConvertUTF8toUTF16(aVolume->Name())),
> 
> Just curious, how much are these getting passed around internally? I've been
> trying to make a habit of storing into nsString at the lowest level possible
> (which isn't something you've had to worry about thus far since this hasn't
> been XPCOM exposed, I know), but I'm not sure where this goes down the vold.

volume names and mount points are utf8. So on the IOThread side of things (i.e. Volume class), I store everything using nsCString. The JS side does everything using wide strings, so I use nsStrings there (i.e. nsVolume class)

> ::: dom/system/gonk/nsVolumeServiceIOThread.h
> @@ +15,5 @@
> > +/***************************************************************************
> > +* The nsVolumeServiceIOThread is a companion class to the nsVolumeService
> > +* class, but whose methods are called from IOThread.
> > +*/
> > +class nsVolumeServiceIOThread : public VolumeManager::StateObserver,
> 
> If this isn't exposed to XPCOM (which I don't think it is?), does it need
> the ns prefix?

I used the ns prefix just because it was an IOThread helper class for the class of the same name. I don't mind dropping the ns prefix if that really means XPCOM.

> ::: dom/system/gonk/nsVolumeServiceTest.cpp
> @@ +31,5 @@
> > +
> > +/***************************************************************************
> > +* A test class to verify that the Observer stuff is working properly.
> > +*/
> > +class NSVolumeTestObserver : public nsIObserver
> 
> Lowercase ns if you're gonna use it, otherwise we look like obj-c. :)

I'll drop the prefix altogether. It's just test code.

Comment 4

[Dave Hylands [:dhylands]]

Attachment: Bug 766324 - Add a volume IDL to make volumes scriptable - v2

- Added nsIVolume.idl, nsIVolumeStat.idl, and nsIVolumeService.idl along with the associate C++ code.
- Updated Volume class to use STATE constants from nsIVolume.idl
- Updated VolumeManager to use nsTArray rather than std::vector
- Added test code for the nsIVolume*.idl files.


Updated after Kyle's review

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 639864 [details] [diff] [review]
Bug 766324 - Add a volume IDL to make volumes scriptable - v2

Review of attachment 639864 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits picked

::: dom/system/gonk/VolumeServiceIOThread.cpp
@@ +70,5 @@
> +void
> +ShutdownVolumeServiceIOThread()
> +{
> +  MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> +  sVolumeServiceIOThread = NULL;

nit: nsnull

::: dom/system/gonk/VolumeServiceIOThread.h
@@ +16,5 @@
> +* The nsVolumeServiceIOThread is a companion class to the nsVolumeService
> +* class, but whose methods are called from IOThread.
> +*/
> +class VolumeServiceIOThread : public VolumeManager::StateObserver,
> +                                public Volume::EventObserver,

Nit: indentation is off, should line up.

Comment 6

[Dave Hylands [:dhylands]]

Attachment: Bug 766324 - Add a volume IDL to make volumes scriptable - v3

- Added nsIVolume.idl, nsIVolumeStat.idl, and nsIVolumeService.idl along with the associate C++ code.
- Updated Volume class to use STATE constants from nsIVolume.idl
- Updated VolumeManager to use nsTArray rather than std::vector
- Added test code for the nsIVolume*.idl files.

Comment 7

[Dave Hylands [:dhylands]]

I decided to leave the NULL as is. Actually I tried to change it to nullptr (see bug 626472), but that requires gcc 4.6.0 and we're still using 4.4.3.

Based on this discussion: https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/CDDsJdBRL6k I've decided that I should leave the NULL and not use nsnull.

Comment 8

[Dave Hylands [:dhylands]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=b95bad2cdbd4

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/413699472929