Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

unspecified
mozilla16
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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)});
(Assignee)

Comment 1

5 years ago
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.
Attachment #626872 - Flags: review?(bugs)
(Assignee)

Comment 2

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

Comment 3

5 years ago
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.
Attachment #626872 - Flags: review?(bugs) → review-
(Assignee)

Comment 4

5 years ago
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.
Attachment #626872 - Flags: feedback?(bugs)

Comment 5

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

See Bug 752226
Other than that, should be ok.
Attachment #626872 - Flags: feedback?(bugs) → feedback-
(Assignee)

Comment 6

5 years ago
Created attachment 629798 [details] [diff] [review]
patch v.2
Attachment #629798 - Flags: review?(bugs)
(Assignee)

Comment 7

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

same patch :(
Attachment #629798 - Flags: review?(bugs) → review-
(Assignee)

Comment 8

5 years ago
Created attachment 630217 [details] [diff] [review]
patch v.3
Attachment #626872 - Attachment is obsolete: true
Attachment #629798 - Attachment is obsolete: true
Attachment #630217 - Flags: review?(bugs)

Comment 9

5 years ago
I still think sicking should give feedback about the API.
I haven't followed all the DeviceStorage stuff.
Yes, I think we should do this
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
Attachment #630217 - Flags: review?(bugs) → review-
(Assignee)

Comment 12

5 years ago
Created attachment 631048 [details] [diff] [review]
patch .4

fixed style
Attachment #630217 - Attachment is obsolete: true
Attachment #631048 - Flags: review?(bugs)
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
Attachment #631048 - Flags: review?(bugs) → review+
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;
};
(Assignee)

Comment 15

5 years ago
> 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().
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/401edab3831f
dougt backed out:

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

Comment 18

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

Comment 19

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/c21a2f60c068
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.