Closed Bug 782352 Opened 7 years ago Closed 7 years ago

Device Storage - track local io and broadcast onchange notifications

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(2 files)

No description provided.
Blocks: 777101
Summary: device Storage - track local io and broadcast onchange notifications → Device Storage - track local io and broadcast onchange notifications
Attached patch patch v.1Splinter Review
Attachment #651560 - Flags: review?(jonas)
One problem I found was if you have different device storage objects.  Suppose pictures and video.  Then a change happens in video.  pictures will send up seeing it.  We need to guard against that.  I'll do that work here.  Should be done tonight/tomorrow.
ignore comment #2.  This isn't a problem now.
Comment on attachment 651560 [details] [diff] [review]
patch v.1

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

I think I'm misunderstanding how the permissions around notifications are working. Would like to understand that better. r=me in the meantime.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +66,5 @@
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +    nsString data;
> +    CopyASCIItoUTF16(mType, data);
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    obs->NotifyObservers(mFile, "file-watcher-update", data.get());    

Nit: trailing whitespace

@@ +185,5 @@
>      return rv;
>    }
>  
> +  nsCOMPtr<IOEventComplete> iocomplete = new IOEventComplete(mFile, "created");
> +  NS_DispatchToMainThread(iocomplete);    

Nit: more trailing whitespace

@@ +265,5 @@
> +DeviceStorageFile::Remove()
> +{
> +  mFile->Remove(true);
> +  nsCOMPtr<IOEventComplete> iocomplete = new IOEventComplete(mFile, "deleted");
> +  NS_DispatchToMainThread(iocomplete);    

Same here

@@ +1332,5 @@
>        }
>  
>        case DEVICE_STORAGE_REQUEST_WATCH:
>        {
> +	// do something?

Don't you need to set some flag here to indicate that sending out notifications is ok? The mIsWatchingFile flag?

@@ +1824,3 @@
>        return NS_OK;
>      }
> +    Notify(creason.get(), file);

You can just do Notify(NS_ConvertUTF16toUTF8(aData)).get(), file)

There's no other advantages than simpler syntax.
Attachment #651560 - Flags: review?(jonas) → review+
the permission stuff for onchange happend in 782391.  It depends on this bug.
https://hg.mozilla.org/mozilla-central/rev/77d2640d9892
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
So it turns out that the words "created" and "modified" mean something different depending on how far down or up the stack you go...

You're using "created" to mean that an empty file has been created and "modified" to mean that bytes have been written into that file.  Those two events always arrive in closely spaced pairs.

To me, "created" means "I have finished writing bytes to a file, and no file by that name existed before I started".  And "modified" means "I have finished writing into a file that already existed".

Given what you've implemented here, I'll ignore the created notifications and just use the "modified" notifications.  But I'll have to check my own database to see if I already know about the file.  If so, I'll treat it as a deletion followed by a creation.  Otherwise, I'll just treat it as a creation.

I'm not going to reopen this bug myself, but I suspect you might want to. It seems to me that exposing the path of a file that is actively being written (by sending the created event before the modified event) just opens the door to race conditions and other ugliness.

I'd love to have an API that makes my kind of distinction between created and modified.  But I don't need to have it.  I'd suggest, however, that the distinction you've made is not useful and could actually be dangerous. So at a minimum, I'd suggest you drop the 'created' notifications.
This patch broadcasts "deleted" change events whenever delete() is called, even if no file by that name existed.  That seems like a bug. (or a general-purpose interapp communication mechanism...)
reopening so that I remember to address these issues.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
followup that prevents Remove() from ever posting an event during failure.
Attachment #654447 - Flags: review?(jonas)
Attachment #654447 - Flags: review?(jonas) → review?(khuey)
Comment on attachment 654447 [details] [diff] [review]
patch v.1 - followup

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

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +263,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
>  DeviceStorageFile::Remove()

Can you add an assertion to this function indicating whether it should run on or off the main thread?

@@ +267,5 @@
>  DeviceStorageFile::Remove()
>  {
> +  bool check;
> +  nsresult rv = mFile->Exists(&check);
> +  

Extra whitespace.

@@ +268,5 @@
>  {
> +  bool check;
> +  nsresult rv = mFile->Exists(&check);
> +  
> +  if (NS_FAILED(rv)) {

if (NS_FAILED(rv) || ! check) {
  return rv;
}

Should be ok, since if the !check fails then rv was NS_OK.

@@ +271,5 @@
> +  
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  

Extra whitespace.

@@ +286,2 @@
>    nsCOMPtr<IOEventComplete> iocomplete = new IOEventComplete(this, "deleted");
>    NS_DispatchToMainThread(iocomplete);    

Extra whitespace at EOL.
Attachment #654447 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/87a187376b3c
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.