Closed
Bug 766324
Opened 12 years ago
Closed 12 years ago
Add a volume IDL to make volumes scriptable
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: dhylands, Assigned: dhylands)
References
Details
Attachments
(1 file, 2 obsolete files)
47.24 KB,
patch
|
dhylands
:
review+
|
Details | Diff | Splinter Review |
This should also allow nsIObserver and nsIObserverService to be used.
Assignee | ||
Comment 1•12 years ago
|
||
- 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.
Assignee | ||
Updated•12 years ago
|
Attachment #639199 -
Flags: review?(kyle)
Comment 2•12 years ago
|
||
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. :)
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Assignee | ||
Comment 4•12 years ago
|
||
- 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
Attachment #639199 -
Attachment is obsolete: true
Attachment #639199 -
Flags: review?(kyle)
Attachment #639864 -
Flags: review?(kyle)
Comment 5•12 years ago
|
||
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.
Attachment #639864 -
Flags: review?(kyle) → review+
Assignee | ||
Comment 6•12 years ago
|
||
- 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.
Attachment #639864 -
Attachment is obsolete: true
Attachment #641361 -
Flags: review+
Assignee | ||
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=b95bad2cdbd4
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/413699472929
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•