Last Comment Bug 758273 - Device Storage - enumeration option to filter based on last modification date
: Device Storage - enumeration option to filter based on last modification date
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Doug Turner (:dougt)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-24 10:06 PDT by Doug Turner (:dougt)
Modified: 2012-06-12 03:06 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v.1 (20.46 KB, patch)
2012-05-24 10:38 PDT, Doug Turner (:dougt)
bugs: review-
bugs: feedback-
Details | Diff | Splinter Review
patch v.2 (19.93 KB, patch)
2012-06-04 09:14 PDT, Doug Turner (:dougt)
doug.turner: review-
Details | Diff | Splinter Review
patch v.3 (19.91 KB, patch)
2012-06-05 10:20 PDT, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
patch .4 (20.23 KB, patch)
2012-06-07 11:07 PDT, Doug Turner (:dougt)
bugs: review+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2012-05-24 10:06:55 PDT
In order to speed up enumeration, it would be nice to be able to pass an option into enumerate so that we can filter out any older modification dates.


For example:

  var storage = navigator.getDeviceStorage("pictures");
  ok(navigator.getDeviceStorage, "Should have getDeviceStorage");

// 836031600 is a long time ago
  var cursor = storage[0].enumerate(path, {"since": new Date(836031600)});
Comment 1 Doug Turner (:dougt) 2012-05-24 10:38:34 PDT
Created attachment 626872 [details] [diff] [review]
patch v.1

mostly concerned about the jsval parameter parsing that I wrote.  It seems to work, but I am not confident that it is correct/safe.
Comment 2 Doug Turner (:dougt) 2012-05-24 20:34:20 PDT
https://tbpl.mozilla.org/?tree=Try&rev=1c7bbc3eca37
Comment 3 Olli Pettay [:smaug] 2012-05-28 08:34:23 PDT
Comment on attachment 626872 [details] [diff] [review]
patch v.1

># HG changeset patch
># Parent 8cf563a575fdba335c3a862fd6da19aedb9965c8
># User Doug Turner <dougt@dougt.org>
>Bug 758273 - Device Storage - enumeration option to filter based on last modification date. r=
>
>diff --git a/dom/devicestorage/nsDeviceStorage.cpp b/dom/devicestorage/nsDeviceStorage.cpp
>--- a/dom/devicestorage/nsDeviceStorage.cpp
>+++ b/dom/devicestorage/nsDeviceStorage.cpp
>@@ -11,16 +11,17 @@
> #include "nsIDOMFile.h"
> #include "nsDOMBlobBuilder.h"
> #include "nsNetUtil.h"
> #include "nsCycleCollectionParticipant.h"
> #include "nsIContentPermissionPrompt.h"
> #include "nsIPrincipal.h"
> #include "mozilla/Preferences.h"
> #include "nsJSUtils.h"
>+#include "DictionaryHelpers.h"
> 
> using namespace mozilla::dom;
> 
> #include "nsDirectoryServiceDefs.h"
> 
> class DeviceStorageFile : public nsISupports {
> public:
>   DeviceStorageFile(nsIFile* aFile, const nsAString& aPath)
>@@ -42,17 +43,17 @@ public:
> 
> private:
>   void NormalizeFilePath() {
> #if defined(XP_WIN)
>     PRUnichar* cur = mPath.BeginWriting();
>     PRUnichar* end = mPath.EndWriting();
>     for (; cur < end; ++cur) {
>       if (PRUnichar('\\') == *cur)
>-	*cur = PRUnichar('/');
>+        *cur = PRUnichar('/');
>     }
Well, if you fix coding style, fix it properly ;)
if (expr) {
  stmt;
}
>+nsDOMDeviceStorage::EnumerateInternal(const JS::Value & aName, const JS::Value & aOptions, JSContext* aCx, PRUint8 _argc, bool aEditable, nsIDOMDeviceStorageCursor * *_retval NS_OUTPARAM)
> {
>   nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mOwner);
>   if (!win)
>     return NS_ERROR_UNEXPECTED;
> 
>-  nsRefPtr<DeviceStorageFile> dsf = new DeviceStorageFile(mFile, aPath);
>+  PRTime since = 0;
>+  nsString path;
>+  path.SetIsVoid(true);
> 
>-  nsDOMDeviceStorageCursor* cursor = new nsDOMDeviceStorageCursor(win, mURI, dsf, aEditable);
>+  if (_argc > 0) {
>+    // inspect the first value to see if it is a string
>+    if (JSVAL_IS_STRING(aName)) {
>+      JSString* jsstr = JS_ValueToString(aCx, aName);
>+      nsDependentJSString jspath;
>+      jspath.init(aCx, jsstr);
>+      path.Assign(jspath);
>+    }
>+    else if (!JSVAL_IS_PRIMITIVE(aName)) {
} else if (expr) {

 
>   nsresult GetInternal(const JS::Value & aName, JSContext* aCx, nsIDOMDOMRequest * *_retval NS_OUTPARAM, bool aEditable);
>-  nsresult EnumerateInternal(const nsAString & aName, nsIDOMDeviceStorageCursor * *_retval NS_OUTPARAM, bool aEditable);
>+
>+  nsresult EnumerateInternal(const JS::Value & aName, const JS::Value & aOptions, JSContext* aCx, PRUint8 _argc, bool aEditable, nsIDOMDeviceStorageCursor * *_retval NS_OUTPARAM);
Very much overlong line.
And please use Mozilla coding style.
Parameters should be
aParameterName

r- because of rather major coding style issues.

But I really don't know this API. sicking should review this, I think.
Comment 4 Doug Turner (:dougt) 2012-05-29 07:43:34 PDT
Comment on attachment 626872 [details] [diff] [review]
patch v.1

lets ignore the code style issues - is the parsing of the jsval correct?  That is what I was mostly concerned with.
Comment 5 Olli Pettay [:smaug] 2012-06-04 01:51:58 PDT
Comment on attachment 626872 [details] [diff] [review]
patch v.1

See Bug 752226
Other than that, should be ok.
Comment 6 Doug Turner (:dougt) 2012-06-04 09:14:49 PDT
Created attachment 629798 [details] [diff] [review]
patch v.2
Comment 7 Doug Turner (:dougt) 2012-06-05 10:09:34 PDT
Comment on attachment 629798 [details] [diff] [review]
patch v.2

same patch :(
Comment 8 Doug Turner (:dougt) 2012-06-05 10:20:10 PDT
Created attachment 630217 [details] [diff] [review]
patch v.3
Comment 9 Olli Pettay [:smaug] 2012-06-06 12:34:03 PDT
I still think sicking should give feedback about the API.
I haven't followed all the DeviceStorage stuff.
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-06-06 15:15:26 PDT
Yes, I think we should do this
Comment 11 Olli Pettay [:smaug] 2012-06-07 05:14:04 PDT
Comment on attachment 630217 [details] [diff] [review]
patch v.3


>     while (NS_SUCCEEDED(files->GetNextFile(getter_AddRefs(f))) && f) {
>+        nsDOMDeviceStorageCursor* cursor = static_cast<nsDOMDeviceStorageCursor*>(mRequest.get());
extra space before nsDOMDeviceStorageCursor


> nsDOMDeviceStorageCursor::nsDOMDeviceStorageCursor(nsIDOMWindow* aWindow,
>                                                    nsIURI* aURI,
>                                                    DeviceStorageFile* aFile,
>-                                                   bool aEditable)
>+                                                   bool aEditable,
>+						   PRUint64 aSince)
tabs

>+nsDOMDeviceStorage::Enumerate(const JS::Value & aName,
>+			      const JS::Value & aOptions,
>+			      JSContext* aCx,
>+			      PRUint8 aArgc,
>+			      nsIDOMDeviceStorageCursor * *aRetval)
tabs
const JS::Value& aOptions
nsIDOMDeviceStorageCursor** aRetval

Same things also elsewhere
>+static PRTime ExtractDateFromOptions(JSContext* aCx, const JS::Value& aOptions)
static PRTime should be in its own line

>+{
>+  PRTime result =0;
space after =

>+      path.Assign(jspath);
>+    }
>+    else if (!JSVAL_IS_PRIMITIVE(aName)) {
} else if (expr) {



>+      // it also might be an options object
>+      since = ExtractDateFromOptions(aCx, aName);
>+    }
>+    else {
} else {


>+
>+  nsresult EnumerateInternal(const JS::Value & aName, const JS::Value & aOptions, JSContext* aCx, PRUint8 _argc, bool aEditable, nsIDOMDeviceStorageCursor * *_retval NS_OUTPARAM);
Fix the coding style, please
Comment 12 Doug Turner (:dougt) 2012-06-07 11:07:27 PDT
Created attachment 631048 [details] [diff] [review]
patch .4

fixed style
Comment 13 Olli Pettay [:smaug] 2012-06-08 09:13:31 PDT
Comment on attachment 631048 [details] [diff] [review]
patch .4

>@@ -503,16 +506,25 @@ public:
> 
>-nsDOMDeviceStorage::Enumerate(const nsAString & aPath,
>-                              nsIDOMDeviceStorageCursor * *_retval NS_OUTPARAM)
>+nsDOMDeviceStorage::Enumerate(const JS::Value & aName,
>+                             const JS::Value & aOptions,
>+                             JSContext* aCx,
>+                             PRUint8 aArgc,
>+                             nsIDOMDeviceStorageCursor** aRetval)
Please align parameters.
And I'd prefer const JS::Value&, not const JS::Value &


> {
>-  return EnumerateInternal(aPath, _retval, false);
>+  return EnumerateInternal(aName, aOptions, aCx, aArgc, false, aRetval);
> }
> 
> NS_IMETHODIMP
>-nsDOMDeviceStorage::EnumerateEditable(const nsAString & aPath,
>-                                      nsIDOMDeviceStorageCursor * *_retval NS_OUTPARAM)
>+nsDOMDeviceStorage::EnumerateEditable(const JS::Value & aName,
>+                                     const JS::Value & aOptions,
>+                                     JSContext* aCx,
>+                                     PRUint8 aArgc,
>+                                     nsIDOMDeviceStorageCursor** aRetval)
ditto

>+nsDOMDeviceStorage::EnumerateInternal(const JS::Value & aName,
>+                                     const JS::Value & aOptions,
>+                                     JSContext* aCx,
>+                                     PRUint8 aArgc,
>+                                     bool aEditable,
>+                                     nsIDOMDeviceStorageCursor** aRetval)
ditto

>   nsresult GetInternal(const JS::Value & aName, JSContext* aCx, nsIDOMDOMRequest * *_retval NS_OUTPARAM, bool aEditable);
>-  nsresult EnumerateInternal(const nsAString & aName, nsIDOMDeviceStorageCursor * *_retval NS_OUTPARAM, bool aEditable);
>+
>+  nsresult EnumerateInternal(const JS::Value & aName, const JS::Value & aOptions, JSContext* aCx, PRUint8 aArgc, bool aEditable, nsIDOMDeviceStorageCursor** aRetval);
over long line

> [scriptable, uuid(05C0D0C8-D698-4CCD-899C-7198A33BD7EC)]
> interface nsIDOMDeviceStorage : nsISupports
update uuid
Comment 14 Olli Pettay [:smaug] 2012-06-08 09:15:06 PDT
Comment on attachment 631048 [details] [diff] [review]
patch .4

> 
>+dictionary DeviceStorageEnumerationParameters
>+{
>+  jsval since;
>+};

Shouldn't we allow both since and path here?
Could be

dictionary DeviceStorageEnumerationParameters
{
  jsval since;
  DOMString path;
};
Comment 15 Doug Turner (:dougt) 2012-06-08 19:36:23 PDT
> Shouldn't we allow both since and path here?

I thought about it.  The current APIs on Device Storage looks like:

DOMDOMRequest function(optional DOMString path)

All of the other APIs on device storage take an optional string.  I didn't want enumerate to take this optional string only when it was an attribute inside an options object.  This is true for add(), get(), delete(), and enumerate().
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2012-06-09 07:30:37 PDT
dougt backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a24f04189b5
Comment 18 Doug Turner (:dougt) 2012-06-10 22:51:53 PDT
yeah, crash on windows.  i need to check the result of GetDirectoryEntries().   pushing to try.
Comment 19 Doug Turner (:dougt) 2012-06-11 08:22:47 PDT
I added a check for a null return from GetDirectoryEntries(), and pushed:

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

inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/c21a2f60c068
Comment 20 Graeme McCutcheon [:graememcc] 2012-06-12 03:06:35 PDT
https://hg.mozilla.org/mozilla-central/rev/c21a2f60c068

Note You need to log in before you can comment on or make changes to this bug.