758273 - Device Storage - enumeration option to filter based on last modification date

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Doug Turner (:dougt)]

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)]

Attachment: 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)]

https://tbpl.mozilla.org/?tree=Try&rev=1c7bbc3eca37

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 626872 [details] [diff] [review]
patch v.1

># HG changeset patch
># Parent 8cf563a575fdba335c3a862fd6da19aedb9965c8
># User Doug Turner <[e-mail]>
>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)]

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][[e-mail]]]

Comment on attachment 626872 [details] [diff] [review]
patch v.1

See Bug 752226
Other than that, should be ok.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 629798 [details] [diff] [review]
patch v.2

same patch :(

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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]

Yes, I think we should do this

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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)]

Attachment: patch .4

fixed style

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

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)]

> 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 16

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/401edab3831f

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

dougt backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a24f04189b5

Comment 18

[Doug Turner (:dougt)]

yeah, crash on windows.  i need to check the result of GetDirectoryEntries().   pushing to try.

Comment 19

[Doug Turner (:dougt)]

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]]

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