763976 - Add onchange notifications to DeviceStorage

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Doug Turner (:dougt)]

Design doc:
  https://wiki.mozilla.org/WebAPI/ProposedDeviceStorageAPIWithNotifications#API

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

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

::: dom/base/Navigator.cpp
@@ +172,5 @@
>  #endif
> +
> +  PRUint32 len = mDeviceStorageStores.Length();
> +  for (PRUint32 i = 0; i < len; ++i) {
> +    nsRefPtr<nsDOMDeviceStorage> store = mDeviceStorageStores[i];

Do you need this extra addref/release pair? (I hope not). If not just do:

  mDeviceStorageStores[i]->Shutdown()

@@ +872,5 @@
> +  nsDOMDeviceStorage::CreateDeviceStoragesFor(win, aType, stores);
> +
> +  nsCOMPtr<nsIWritableVariant> result = do_CreateInstance("@mozilla.org/variant;1");
> +  if (!result) {
> +    return NS_ERROR_FAILURE;

Nit: NS_ENSURE_TRUE would get you a warning too.

@@ +875,5 @@
> +  if (!result) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  result->SetAsArray(nsIDataType::VTYPE_INTERFACE,

Nit: This can fail, apparently if Length() is 0, among other things. Either we should check or assert, depending on whether that's possible.

@@ +879,5 @@
> +  result->SetAsArray(nsIDataType::VTYPE_INTERFACE,
> +                     &NS_GET_IID(nsIDOMDeviceStorage),
> +                     stores.Length(),
> +                     const_cast<void*>(static_cast<const void*>(stores.Elements())));
> +  NS_ADDREF(*_retval = result);

Nit: I can't remember if 'result.forget(result)' can handle this conversion. If it can let's use it.

::: dom/base/nsDOMClassInfo.cpp
@@ +1423,5 @@
>    NS_DEFINE_CLASSINFO_DATA(MessageEvent, nsDOMGenericSH,
>                             DOM_DEFAULT_SCRIPTABLE_FLAGS)
>  
>    NS_DEFINE_CLASSINFO_DATA(DeviceStorage, nsDOMGenericSH,
>                             DOM_DEFAULT_SCRIPTABLE_FLAGS)

Now that you're making DeviceStorage an event target you should use nsEventTargetSH and EVENTTARGET_SCRIPTABLE_FLAGS.

@@ +4116,5 @@
>       DOM_CLASSINFO_MAP_ENTRY(nsIDOMDeviceStorage)
>    DOM_CLASSINFO_MAP_END
>  
> +  DOM_CLASSINFO_MAP_BEGIN(DeviceStorageChangeEvent, nsIDOMDeviceStorageChangeEvent)
> +     DOM_CLASSINFO_MAP_ENTRY(nsIDOMDeviceStorageChangeEvent)

You need nsIDOMEvent here too

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1193,5 @@
>      nsresult rv;
>      nsRefPtr<nsDOMDeviceStorage> storage = new nsDOMDeviceStorage();
>      rv = storage->Init(aWin, aType, index++);
>      if (NS_FAILED(rv))
>        break;

Hm, don't you want to warn if that fails?

@@ +1501,5 @@
> +    return NS_OK;
> +  }
> +  
> +  nsAString::size_type len = rootpath.Length() + 1; // +1 for the trailing /
> +  nsDependentSubstring newPath = Substring(fullpath, len);

I think you can pass fullpath/len as constructor args to nsDependentSubstring.

Also, might be smart to assert that fullpath is longer than rootpath.

@@ +1506,5 @@
> +
> +  nsString change;
> +  change.AssignASCII(aTopic);
> +
> +

Nit: extra newline

@@ +1515,5 @@
> +  nsCOMPtr<nsIDOMDeviceStorageChangeEvent> e = event.get();
> +
> +  len = mChangeListeners.Length();
> +  for (PRUint32 i = 0; i < len; ++i) {
> +    nsCOMPtr<nsIDOMEventListener> l = mChangeListeners[i];

This isn't mutation safe... But you'll get that for free with nsDOMEventTargetHelper.

::: dom/devicestorage/nsDeviceStorage.h
@@ +24,2 @@
>  
> +class nsDOMDeviceStorage : public nsIDOMDeviceStorage, public nsIObserver

This should now inherit nsDOMEventTargetHelper which, among other things, makes this a cycle-collected class. You'll need to extend the traverse method to go over your listeners.

::: dom/interfaces/devicestorage/nsIDOMDeviceStorage.idl
@@ +14,5 @@
>    jsval since;
>  };
>  
>  [scriptable, uuid(05C0D0C8-D698-4CCD-899C-7198A33BD7EC)]
>  interface nsIDOMDeviceStorage : nsISupports

Hm... Shouldn't this inherit nsIDOMEventTarget now? Then you wouldn't need the add/remove event listener stuff (which, btw, have different signatures than nsIDOMEventTarget...)

::: dom/interfaces/devicestorage/nsIDOMDeviceStorageChangeEvent.idl
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "domstubs.idl"

Nit: Shouldn't need this, it's included through nsIDOMEvent.idl

Comment 3

[Paul Dagnelie]

Attachment: patch v.2

Comment 4

[Paul Dagnelie]

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

(In reply to ben turner [:bent] from comment #2)
> Comment on attachment 632853 [details] [diff] [review]
> patch v.1
> 
> Review of attachment 632853 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +1193,5 @@
> >      nsresult rv;
> >      nsRefPtr<nsDOMDeviceStorage> storage = new nsDOMDeviceStorage();
> >      rv = storage->Init(aWin, aType, index++);
> >      if (NS_FAILED(rv))
> >        break;
> 
> Hm, don't you want to warn if that fails?

No, not really.  If Init fails, it happens for one of basically two reasons: First, there are no more devices to get storage for, and Second, if some utterly improbable events occur.  In the first case, we want to stop and simply return the array in its current form, so there's no reason to warn.  The second case is if either of the following occurs:

>  mOwner = do_GetWeakReference(aWindow);
>  if (!mOwner) {
>    return NS_ERROR_FAILURE;
>  }
>
>  // Grab the uri of the document                                                       
>  nsCOMPtr<nsIDOMDocument> domdoc;
>  aWindow->GetDocument(getter_AddRefs(domdoc));
>  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc);
>  if (!doc) {
>    return NS_ERROR_FAILURE;
>  }

Now, aWindow is the global window, so it should always support a weak reference, and "doc" would be null only the window has no document.  We could assert on those, or we could just not check them, since in practice, they'll basically never happen.  What do you think?

Relatedly, this is also why we don't want to warn on SetAsArray failing: because stores could have length 0 in a perfectly legal fashion.

> 
> ::: dom/devicestorage/nsDeviceStorage.h
> @@ +24,2 @@
> >  
> > +class nsDOMDeviceStorage : public nsIDOMDeviceStorage, public nsIObserver
> 
> This should now inherit nsDOMEventTargetHelper which, among other things,
> makes this a cycle-collected class. You'll need to extend the traverse
> method to go over your listeners.

See below.

> 
> ::: dom/interfaces/devicestorage/nsIDOMDeviceStorage.idl
> @@ +14,5 @@
> >    jsval since;
> >  };
> >  
> >  [scriptable, uuid(05C0D0C8-D698-4CCD-899C-7198A33BD7EC)]
> >  interface nsIDOMDeviceStorage : nsISupports
> 
> Hm... Shouldn't this inherit nsIDOMEventTarget now? Then you wouldn't need
> the add/remove event listener stuff (which, btw, have different signatures
> than nsIDOMEventTarget...)
>

This class isn't strictly an nsDOMEventTarget.  We don't want to have bubbling, and we don't want to have dispatchEvent, or really any of the other infrastructure surrounding nsDOMEventTarget.  We also weren't sure if this is exactly the interface we want, but it's the one we're using for now.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Paul Dagnelie from comment #4)
> This class isn't strictly an nsDOMEventTarget.

Yeah, talked this over with jst, we're both in agreement that DOM objects that can be targeted with events should be event targets :)

> We don't want to have bubbling, and we don't want to have dispatchEvent, or really
> any of the other infrastructure surrounding nsDOMEventTarget.

Bubbling only happens if you define a propagation chain (e.g. XHR doesn't really have bubbling events either), so you won't get bubbling events unless you ask for them. And DispatchEvent always takes into account whether or not the event was generated by the browser (a "trusted" event) or by a web page ("untrusted"), so it shouldn't matter whether it exists or not.

> We also weren't sure if this is exactly the interface we want, but it's the one
> we're using for now.

Understood, but we've got lots and lots of new APIs that use EventTarget, and this seems to be another good fit.

Comment 6

[Paul Dagnelie]

Attachment: patch v.3

Comment 7

[Paul Dagnelie]

Comment on attachment 636419 [details] [diff] [review]
patch v.3

Here's the new version, updated to use the EventListener interface and the cycle collector.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 636419 [details] [diff] [review]
patch v.3

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

Hm, there's a bunch of code here that has been commented out but left in the patch. Can you clean this up a little before I review?

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 636419 [details] [diff] [review]
patch v.3

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

looking better!

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +304,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +  NS_FORWARD_NSIDOMEVENT(nsDOMEvent::)

no new line is between the NS_* Macros

@@ +349,5 @@
> +NS_IMETHODIMP
> +nsDOMDeviceStorageChangeEvent::Init(const nsAString & aEventTypeArg,
> +                                    bool aCanBubbleArg,
> +                                    bool aCancelableArg,
> +                                    nsAString& aPath)

Remove all tabs from this diff.

@@ +991,5 @@
> +    mFile->mFile->GetNativePath(path);
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +
> +    PR_AtomicIncrement(&mDeviceStorage->mNumListeners);
> +    printf("%d\n",mDeviceStorage->mNumListeners);

remove all printfs()

@@ +996,5 @@
> +    if (mDeviceStorage->mNumListeners == 1) {
> +        mFile->mFile->Watch(mDeviceStorage);
> +    }
> +
> +    mDeviceStorage->nsDOMEventTargetHelper::AddEventListener(NS_LITERAL_STRING("change"), mListener, false, false, 2);

line is long.  bring args down on to the next line.

@@ +1047,1 @@
>                           bool aEditable,

Sorry, but please rebase this patch on top of what I just landed on inbound (it should be on central by tomorrow).  See:
   https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c707e4f15fd4

@@ +1201,3 @@
>    NS_INTERFACE_MAP_ENTRY(nsIDOMDeviceStorage)
> +  NS_INTERFACE_MAP_ENTRY(nsIFileUpdateListener)
> +//  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMDeviceStorage)

remove commented out line.

@@ +1415,5 @@
>    NS_DispatchToMainThread(r);
>    return NS_OK;
>  }
>  
> +/*NS_IMETHODIMP

remove.

@@ +1594,5 @@
> +  bool ret;
> +  DispatchEvent(e, &ret);
> +  if(ret)
> +      return NS_OK;
> +  else

non-sequitur. no else after if(){ return; }

@@ +1598,5 @@
> +  else
> +      return NS_ERROR_UNEXPECTED;
> +}
> +
> +/* [optional_argc] void addEventListener (in DOMString type, in nsIDOMEventListener listener, [optional] in boolean useCapture, [optional] in boolean wantsUntrusted); */

Kill all of these generated comments.

@@ +1619,5 @@
> +  nsRefPtr<DeviceStorageFile> dsf = new DeviceStorageFile(mFile);
> +
> +  nsDOMDeviceStorage *ptr = this;
> +  
> +  r = new DeviceStorageRequest(DeviceStorageRequest::DEVICE_STORAGE_REQUEST_WATCH,

You don't need spaces between each of these lines.

@@ +1709,5 @@
> +
> +/* [nostdcall,notxpcom] JSContextPtr GetJSContextForEventHandlers (); */
> +JSContext * nsDOMDeviceStorage::GetJSContextForEventHandlers()
> +{
> +    return nsDOMEventTargetHelper::GetJSContextForEventHandlers();

I hope there is a better way than this.

::: dom/tests/mochitest/devicestorage/test_watch.html
@@ +35,5 @@
> +}
> +
> +var onChange = {
> +  HandleEvent: function(e) {
> +    dump("**** saw: " + e.path + " " + e.path.length +  "\n");

remove dump()

@@ +62,5 @@
> +var storage = navigator.getDeviceStorage("profile");
> +ok(navigator.getDeviceStorage, "Should have getDeviceStorage");
> +storage[0].addEventListener("change", onChange);
> +
> +setTimeout(makeFile,1000);

can you make the timeout smaller?  Zero even?

Comment 10

[Paul Dagnelie]

Attachment: patch v.4

(In reply to Doug Turner (:dougt) from comment #9) 
> @@ +62,5 @@
> > +var storage = navigator.getDeviceStorage("profile");
> > +ok(navigator.getDeviceStorage, "Should have getDeviceStorage");
> > +storage[0].addEventListener("change", onChange);
> > +
> > +setTimeout(makeFile,1000);
> 
> can you make the timeout smaller?  Zero even?

If it's too small, the file gets added before the watch is applied, which rather ruins the test.  100 seems to work, but 0 seems to fail sometimes.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 636867 [details] [diff] [review]
patch v.4

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

okay.  lets see another patch.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 636867 [details] [diff] [review]
patch v.4

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

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +109,5 @@
>      PRUnichar* cur = mPath.BeginWriting();
>      PRUnichar* end = mPath.EndWriting();
>      for (; cur < end; ++cur) {
>        if (PRUnichar('\\') == *cur)
> +  *cur = PRUnichar('/');

white space is wrong here.

@@ +625,5 @@
>        mFile->mFile->GetPath(rootPath);
>  
>        if (!StringBeginsWith(fullpath, rootPath)) {
> +  NS_WARNING("collectFiles returned a path that does not belong!");
> +  continue;

white space is wrong.

@@ +973,5 @@
> +                   nsCOMPtr<nsIDOMEventListener> aListener,
> +                   nsDOMDeviceStorage *aDeviceStorage)
> +  : mFile(aFile)
> +  , mDeviceStorage(aDeviceStorage)
> +  , mListener(aListener)

another white space nit: statements starting with : and , should be 2 spaces indented from where the construct line starts.

@@ +984,5 @@
> +  NS_IMETHOD Run() 
> +  {
> +    nsCString path;
> +    mFile->mFile->GetNativePath(path);
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

this assertion should be the first thing you do in run.

@@ +991,5 @@
> +    if (mDeviceStorage->mNumListeners == 1) {
> +        mFile->mFile->Watch(mDeviceStorage);
> +    }
> +
> +    mDeviceStorage->nsDOMEventTargetHelper::AddEventListener(NS_LITERAL_STRING("change"),

can't:
 mDeviceStorage->nsDOMEventTargetHelper 

just be:
   mDeviceStorage->AddEventListener

@@ +992,5 @@
> +        mFile->mFile->Watch(mDeviceStorage);
> +    }
> +
> +    mDeviceStorage->nsDOMEventTargetHelper::AddEventListener(NS_LITERAL_STRING("change"),
> +                                                             mListener, false, false, 2);

Magic number - what is 2?

@@ +995,5 @@
> +    mDeviceStorage->nsDOMEventTargetHelper::AddEventListener(NS_LITERAL_STRING("change"),
> +                                                             mListener, false, false, 2);
> +    nsRefPtr<nsRunnable> r;
> +
> +    r = new PostResultEvent(mRequest, mFile->mPath);

nsRefPtr<nsRunnable> r = new PostResultEvent(mRequest, mFile->mPath);

@@ +996,5 @@
> +                                                             mListener, false, false, 2);
> +    nsRefPtr<nsRunnable> r;
> +
> +    r = new PostResultEvent(mRequest, mFile->mPath);
> +    NS_DispatchToMainThread(r);

or
NS_DispatchToMainThread(new PostResultEvent(mRequest, mFile->mPath));

@@ +1003,5 @@
> +
> +private:
> +  nsRefPtr<DeviceStorageFile> mFile;
> +  nsRefPtr<DOMRequest> mRequest;
> +  nsDOMDeviceStorage *mDeviceStorage;

nsRefPtr<nsDOMDeviceStorage>

@@ +1004,5 @@
> +private:
> +  nsRefPtr<DeviceStorageFile> mFile;
> +  nsRefPtr<DOMRequest> mRequest;
> +  nsDOMDeviceStorage *mDeviceStorage;
> +  nsCOMPtr<nsIDOMEventListener> mListener;

You want to order these the same way that the constructor does so that you avoid compiler warnings.

@@ +1024,5 @@
>                           DeviceStorageFile *aFile,
>                           DOMRequest* aRequest,
> +                         nsDOMDeviceStorage *aDeviceStorage,
> +                         nsCOMPtr<nsIDOMEventListener> aListener)
> +        : mRequestType(aRequestType)

I think this is intended to much.

@@ +1128,5 @@
>        }
> +      case DEVICE_STORAGE_REQUEST_WATCH:
> +      {
> +        r = new WatchFileEvent(mFile, mRequest, mListener, mDeviceStorage);
> +        NS_DispatchToMainThread(r); //this request must be called from the main thread, and is not suitable for the streamtransportservice

move the comment up so that it doesnt wrap on smaller terminals.

@@ +1149,5 @@
>    nsRefPtr<DeviceStorageFile> mFile;
>  
>    nsRefPtr<DOMRequest> mRequest;
>    nsCOMPtr<nsIDOMBlob> mBlob;
> +  nsDOMDeviceStorage *mDeviceStorage;

nsRefPtr<>

@@ +1510,5 @@
> +
> +  nsString fullpath;
> +  aFile->GetPath(fullpath);
> +
> +  NS_ASSERTION(fullpath.Length()>rootpath.Length(), "Root path longer than full path!");

spaces around the operator '>'.

@@ +1524,5 @@
> +  nsRefPtr<nsDOMDeviceStorageChangeEvent> event = new nsDOMDeviceStorageChangeEvent();
> +  nsresult rv = event->Init(NS_LITERAL_STRING("change"), true, false, newPath);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIDOMDeviceStorageChangeEvent> e = event.get();

You shouldn't need this - the concrete class can be dispatched.

@@ +1529,5 @@
> +
> +  bool ret;
> +  DispatchEvent(e, &ret);
> +  if (ret)
> +      return NS_OK;

nit: two spaces.

@@ +1533,5 @@
> +      return NS_OK;
> +  return NS_ERROR_UNEXPECTED;
> +}
> +
> +NS_IMETHODIMP nsDOMDeviceStorage::AddEventListener(const nsAString & aType, nsIDOMEventListener *aListener, bool useCapture, bool wantsUntrusted, PRUint8 _argc)

each param on its own line.  All parameters starting with lower case 'a'.

@@ +1545,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  nsRefPtr<DOMRequest> request = new DOMRequest(win);
> +  nsCOMPtr<nsIRunnable> r;

move r to where it is used.

@@ +1546,5 @@
> +  }
> +
> +  nsRefPtr<DOMRequest> request = new DOMRequest(win);
> +  nsCOMPtr<nsIRunnable> r;
> +  nsRefPtr<DeviceStorageFile> dsf = new DeviceStorageFile(mFile);\

what is the \ about?

@@ +1547,5 @@
> +
> +  nsRefPtr<DOMRequest> request = new DOMRequest(win);
> +  nsCOMPtr<nsIRunnable> r;
> +  nsRefPtr<DeviceStorageFile> dsf = new DeviceStorageFile(mFile);\
> +  nsDOMDeviceStorage *ptr = this;

why is this needed?  You should be able to use 'this' as a param.

@@ +1555,5 @@
> +  NS_DispatchToMainThread(r);
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP nsDOMDeviceStorage::AddSystemEventListener(const nsAString & aType, nsIDOMEventListener *aListener, bool aUseCapture, bool aWantsUntrusted, PRUint8 _argc)

same nits as above about how these should be written.

@@ +1576,5 @@
> +}
> +
> +NS_IMETHODIMP nsDOMDeviceStorage::RemoveSystemEventListener(const nsAString & aType, nsIDOMEventListener *aListener, bool aUseCapture)
> +{
> +    return nsDOMDeviceStorage::RemoveEventListener(aType, aListener, aUseCapture);

nit: two spaces.

::: dom/devicestorage/nsDeviceStorage.h
@@ +29,3 @@
>  
> +class nsDOMDeviceStorage MOZ_FINAL: public nsIDOMDeviceStorage,
> +                           public nsIFileUpdateListener,

class nsDOMDeviceStorage MOZ_FINAL
  : whatever()
  , whoever()

@@ +46,5 @@
>    nsresult Init(nsPIDOMWindow* aWindow, const nsAString &aType, const PRInt32 aIndex);
>  
>    PRInt32 SetRootFileForType(const nsAString& aType, const PRInt32 aIndex);
>  
> +  static void CreateDeviceStoragesFor(nsPIDOMWindow* aWin, const nsAString &aType, nsTArray<nsRefPtr<nsDOMDeviceStorage> > &aStores);

each param on its own line.

::: dom/tests/mochitest/devicestorage/test_watch.html
@@ +33,5 @@
> +  ok(false, "addError was called : " + e.target.error.name);
> +  devicestorage_cleanup();
> +}
> +
> +var onChange = {

this should just be a function:

function onchange(e) {}

@@ +61,5 @@
> +var storage = navigator.getDeviceStorage("profile");
> +ok(navigator.getDeviceStorage, "Should have getDeviceStorage");
> +storage[0].addEventListener("change", onChange);
> +
> +setTimeout(makeFile,100);

hmm.

i don't like setTimeout.  Please add a comment why this needs to be.

Comment 13

[Paul Dagnelie]

Attachment: patch v.5

(In reply to Doug Turner (:dougt) from comment #12)
> Comment on attachment 636867 [details] [diff] [review]
> patch v.4
> 
> Review of attachment 636867 [details] [diff] [review]:
> @@ +991,5 @@
> > +    if (mDeviceStorage->mNumListeners == 1) {
> > +        mFile->mFile->Watch(mDeviceStorage);
> > +    }
> > +
> > +    mDeviceStorage->nsDOMEventTargetHelper::AddEventListener(NS_LITERAL_STRING("change"),
> 
> can't:
>  mDeviceStorage->nsDOMEventTargetHelper 
> 
> just be:
>    mDeviceStorage->AddEventListener

No, because mDeviceStorage->AddEventListener is what causes the WatchFileEvent to be run in the first place.  nsDOMEventTargetHelper::AddFileListener is what registers the listener in the data structures so it can be dispatched to and removed later.
  
> @@ +1524,5 @@
> > +  nsRefPtr<nsDOMDeviceStorageChangeEvent> event = new nsDOMDeviceStorageChangeEvent();
> > +  nsresult rv = event->Init(NS_LITERAL_STRING("change"), true, false, newPath);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  nsCOMPtr<nsIDOMDeviceStorageChangeEvent> e = event.get();
> 
> You shouldn't need this - the concrete class can be dispatched.

"error: 'nsIDOMEvent' is an ambiguous base of 'nsDOMDEviceStorageChangeEvent'" when i try to dispatch "event" directly.

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 639420 [details] [diff] [review]
patch v.5

ben and i are on vacation.  Jonas, can you take a look at this?

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.6

clean up and rebased on top of http://hg.mozilla.org/users/dougt_mozilla.com/device_storage_ipc

Comment 16

[Doug Turner (:dougt)]

Attachment: patch v.7

Comment 17

[Doug Turner (:dougt)]

Attachment: ipc support v.1

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 642770 [details] [diff] [review]
patch v.7

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

::: dom/base/Navigator.cpp
@@ +182,5 @@
>  #endif
> +  PRUint32 len = mDeviceStorageStores.Length();
> +  for (PRUint32 i = 0; i < len; ++i) {
> +    mDeviceStorageStores[i]->Shutdown();
> +  }

You also should clear mDeviceStorageStores.  Otherwise you'll get ownership cycles.

::: dom/devicestorage/DeviceStorage.h
@@ +56,5 @@
> +  nsCOMPtr<nsIURI> mURI;
> +
> +  friend class WatchFileEvent;
> +
> +  PRInt32 mNumListeners;

You don't really care how many listeners you have, just if you've registered as a file watcher.  So use a boolean for that and test mListenerManager->HasListeners to find out if you need to register or not.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +469,5 @@
> +DOMCI_DATA(DeviceStorageChangeEvent, nsDOMDeviceStorageChangeEvent)
> +
> +NS_INTERFACE_MAP_BEGIN(nsDOMDeviceStorageChangeEvent)
> +NS_INTERFACE_MAP_ENTRY(nsIDOMDeviceStorageChangeEvent)
> +NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(DeviceStorageChangeEvent)

Indent the entries.

@@ +1004,5 @@
> +      , mListener(aListener)
> +      , mDeviceStorage(aDeviceStorage)
> +    {
> +      mRequest.swap(aRequest);
> +    }

Indent this two spaces instead of 4.  Also, add an assertion for this being created on or off the main thread as appropriate.

@@ +1012,5 @@
> +  NS_IMETHOD Run() 
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +    nsCString path;
> +    mFile->mFile->GetNativePath(path);

Why are you grabbing the path?  You don't use it.

@@ +1017,5 @@
> +
> +    PR_AtomicIncrement(&mDeviceStorage->mNumListeners);
> +    if (mDeviceStorage->mNumListeners == 1) {
> +      mFile->mFile->Watch(mDeviceStorage);
> +    }

Why is this an atomic operation?  Isn't mNumListeners only touched from the main thread?

If this really is supposed to be an atomic operation, you're doing it wrong.  PR_AtomicIncrement returns the value of the variable after incrementing.  Because you don't use that, but grab the value from memory again, you could race against a thread and from zero to two without ever seeing mNumListeners == 1.

@@ +1023,5 @@
> +    mDeviceStorage->nsDOMEventTargetHelper::AddEventListener(NS_LITERAL_STRING("change"),
> +                                                             mListener, 
> +                                                             false,
> +                                                             false,
> +                                                             2); //argc

Do you really need to fully qualify AddEventListener?

@@ +1031,5 @@
> +private:
> +  nsRefPtr<DeviceStorageFile> mFile;
> +  nsRefPtr<DOMRequest> mRequest;
> +  nsCOMPtr<nsIDOMEventListener> mListener;
> +  nsRefPtr<nsDOMDeviceStorage> mDeviceStorage;

These need to be released on the main thread.  If this runnable is created off the main thread you either need to null them out in Run (and hope that nobody adds an early return) or NS_ProxyRelease them from the dtor.

@@ +1050,3 @@
>      };
> +
> +    DeviceStorageRequest(const PRInt32 aRequestType,

Why don't you give the enum a name and use it as a type here?

@@ +1061,5 @@
> +      , mURI(aURI)
> +      , mFile(aFile)
> +      , mRequest(aRequest)
> +      , mDeviceStorage(aDeviceStorage)
> +      , mListener(aListener) {}    

House style in DOM is

: mFoo(aFoo),
  mBar(aBar)

@@ +1092,5 @@
>        // because owner implements nsITabChild, we can assume that it is
>        // the one and only TabChild.
>        TabChild* child = GetTabChildFrom(mWindow->GetDocShell());
>        if (!child)
> +        return false;

if () {
  single line;
}

@@ +1180,2 @@
>  
> +          stream->Read((char*)buffer, bufSize, &numRead);

Uh .... isn't this doing main thread I/O?

@@ +1212,5 @@
>        }
> +      case DEVICE_STORAGE_REQUEST_WATCH:
> +      {
> +        r = new WatchFileEvent(mFile, mRequest, mListener, mDeviceStorage);
> +        NS_DispatchToMainThread(r); //this request must be called from the main thread,

If you're on the main thread right here, use NS_DispatchToCurrentThread.

@@ +1251,5 @@
>  
>    nsRefPtr<DOMRequest> mRequest;
>    nsCOMPtr<nsIDOMBlob> mBlob;
> +  nsRefPtr<nsDOMDeviceStorage> mDeviceStorage;
> +  nsCOMPtr<nsIDOMEventListener> mListener;

These need to be added to cycle collection.

@@ +1333,5 @@
>  {
>  }
>  
>  void
> +nsDOMDeviceStorage::Shutdown()

Add an appropriate threading assertion.

@@ +1664,5 @@
> +{
> +  // totally ignore anything other than what we know about
> +  if (!aType.Equals(NS_LITERAL_STRING("change"))) {
> +    return NS_OK;
> +  }

This is just wrong, and not how AddEventListener works.  Why do you want this?

@@ +1665,5 @@
> +  // totally ignore anything other than what we know about
> +  if (!aType.Equals(NS_LITERAL_STRING("change"))) {
> +    return NS_OK;
> +  }
> +  nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mOwner);

nsDOMEventTargetHelper has an mOwner.  nsDOMDeviceStorage should just use that.

@@ +1696,5 @@
> +{
> +  // totally ignore anything other than what we know about
> +  if (!aType.Equals(NS_LITERAL_STRING("change"))) {
> +    return NS_OK;
> +  }

Again, this is not how RemoveEventListener works.

@@ +1701,5 @@
> +
> +  PR_AtomicDecrement(&mNumListeners);
> +  if (mNumListeners == 0) {
> +    mFile->Unwatch(this);
> +  }

I still don't think these should be atomic ops, but if they should be you're doing it wrong again.

::: dom/interfaces/devicestorage/nsIDOMDeviceStorage.idl
@@ +15,5 @@
>    jsval since;
>  };
>  
> +[scriptable, uuid(05C0D0C8-D698-4CCD-899C-7198A33BD7EC), builtinclass]
> +interface nsIDOMDeviceStorage : nsIDOMEventTarget

You need to update the IID.

Comment 19

[Doug Turner (:dougt)]

> +  PRInt32 mNumListeners;

I need to keep track of the number of listeners on this device storage since I don't want to create multiple os file watches.  

for example, 

 var storage = navigator.getDeviceStorage("pictures");
 storage[0].addEventListener("change", onChange1);
 storage[0].addEventListener("change", onChange2);

 setTimeout(function() { storage[0].removeEventListener("change", onChange2);}, 1000);
 
If I only used a bool, when this code ran, after a second we'd call removeEventListener.  This would need to determine if we could call file->Unwatch().  Clearly, we don't know with only a bool.

> About the atomic inc/dec.

Paul, do you know why this was added?  I'll revert it and test.

> Do you really need to fully qualify AddEventListener?

Yeah, not sure of a better way.

> These need to be released on the main thread. 

i think... we can just get rid of the this event.

> Uh .... isn't this doing main thread I/O?

yes.  terrible.  waiting for PBlob support - bug 773798.  We could fix it now, but I am really hoping to get pblob support this week.

Comment 20

[Doug Turner (:dougt)]

Attachment: patch v.8

Comment 21

[Doug Turner (:dougt)]

Attachment: ipc support v.1 (rebased against v.8)

Comment 22

[Doug Turner (:dougt)]

Attachment: patch v.9

fixes that delayed addEventListener.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 643140 [details] [diff] [review]
patch v.9

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

::: dom/devicestorage/DeviceStorage.h
@@ +51,5 @@
> +
> +  PRInt32 mStorageType;
> +  nsCOMPtr<nsIFile> mFile;
> +
> +  nsWeakPtr mOwner;

You still didn't address my comment to use nsDOMEventTargetHelper's mOwner instead of your own version.

@@ +57,5 @@
> +
> +  friend class WatchFileEvent;
> +
> + public:
> +  PRInt32 mNumListeners;

You also don't need this.  You just need a boolean to indicate whether you're watching the file or not.

Also this should not be public.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1176,5 @@
> +      {
> +	mDeviceStorage->mNumListeners++;
> +	if (mDeviceStorage->mNumListeners == 1) {
> +	  mFile->mFile->Watch(mDeviceStorage);
> +	}

You don't need a number of listeners, just a boolean to keep track of whether or not you've registered.

@@ +1237,1 @@
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_END

Indent the UNLINK_NSCOMPTR macros.

@@ +1245,1 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

And the TRAVERSE_NSCOMPTR macros.

@@ +1600,5 @@
> +  nsString rootpath;
> +  mFile->GetPath(rootpath);
> +
> +  nsString fullpath;
> +  aFile->GetPath(fullpath);

Shouldn't you check the rv for GetPath?

@@ +1616,5 @@
> +  nsRefPtr<nsDOMDeviceStorageChangeEvent> event = new nsDOMDeviceStorageChangeEvent();
> +  nsresult rv = event->Init(NS_LITERAL_STRING("change"), true, false, newPath);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIDOMDeviceStorageChangeEvent> e = event.get();

Do you really need this?  DispatchEvent(event, &ignore) doesn't work?

@@ +1661,5 @@
> +{
> +  mNumListeners--;
> +  if (mNumListeners == 0) {
> +    mFile->Unwatch(this);
> +  }

You don't need this count.  Just call nsDOMEventTargetHelper::RemoveEventListener first, and then test elm->HasEventListeners() && mWatchingFile.

Comment 24

[Doug Turner (:dougt)]

> +  nsCOMPtr<nsIDOMDeviceStorageChangeEvent> e = event.get();


mozilla-central/dom/devicestorage/nsDeviceStorage.cpp:1636:31: error: ‘nsIDOMEvent’ is an ambiguous base of ‘nsDOMDeviceStorageChangeEvent’

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 643046 [details] [diff] [review]
ipc support v.1 (rebased against v.8)

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

::: dom/ipc/ContentParent.cpp
@@ +1384,5 @@
> +#ifdef DEBUG
> +  nsCString cpath;
> +  aFile->GetNativePath(cpath);
> +  printf("ContentParent::WatchedFile::Update: %s\n", cpath.get());
> +#endif

Please remove this before landing.

::: dom/ipc/ContentParent.h
@@ +261,5 @@
> +        }
> +
> +        PRInt32 mUsageCount;
> +        nsCOMPtr<nsIFile> mFile;
> +        nsRefPtr<ContentParent> mParent;

If these are held in a hashtable hanging off the ContentParent, do they really need to hold a strong ref to the ContentParent?

Comment 26

[Doug Turner (:dougt)]

> If these are held in a hashtable hanging off the ContentParent, do they really need to hold a strong ref to the ContentParent?

No, not at all.  I just really don't like having weak pointers hanging around.  I don't think it matters either way.

Comment 27

[Doug Turner (:dougt)]

Attachment: patch v.10 - rebase - including type of change

Comment 28

[Doug Turner (:dougt)]

Attachment: ipc support v.1 (rebased against v.10)

Comment 29

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1a6847f6df3
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a279333ade

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/42a279333ade
https://hg.mozilla.org/mozilla-central/rev/d1a6847f6df3

Comment 31

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

Why doesn't this use the event implementation codegen?

Comment 32

[Doug Turner (:dougt)]

Not a great answer, but I finished this patch before smaug was done w/ the code generator.  I knew I also needed to do something similar for bug 777088.  I figured I could do both at the same time.  Should get to both shortly.

Comment 33

[Landry Breuil (:gaston)]

Attachment: Remove extra comma at end of enum

Micro followup fix.. my gcc 4.2.1/openbsd builds broke with:

../../dist/include/DeviceStorage.h:69: error: comma at end of enumerator list

(see http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/472/steps/build/logs/stdio)

Patch probably fixes it, building here.

Comment 34

[Landry Breuil (:gaston)]

Confirmed, removing the extra comma fixes my builds.

Comment 35

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

Comment on attachment 648641 [details] [diff] [review]
Remove extra comma at end of enum

rs=me

Comment 36

[Landry Breuil (:gaston)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d8010258ceea

Comment 37

[Doug Turner (:dougt)]

thanks :)

Comment 38

[Ed Morley [:emorley]]

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