871287 - Move DeviceStorage to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

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

Attachment: Patch v1

Comment 2

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

Comment on attachment 748716 [details] [diff] [review]
Patch v1


> class nsDOMDeviceStorage MOZ_FINAL
>   : public nsDOMEventTargetHelper
>   , public nsIDOMDeviceStorage
>   , public nsIObserver
> {
>+  typedef mozilla::ErrorResult ErrorResult;
>+  typedef mozilla::dom::DeviceStorageEnumerationParameters
>+    EnumerationParameters;
>+  typedef mozilla::dom::DOMCursor DOMCursor;
>+  typedef mozilla::dom::DOMRequest DOMRequest;
Not sure I like these typedefs, but ok.

>+  already_AddRefed<DOMRequest>
>+  FreeSpace(ErrorResult& aRv);
>+  already_AddRefed<DOMRequest>
>+  UsedSpace(ErrorResult& aRv);
>+  already_AddRefed<DOMRequest>
>+  Available(ErrorResult& aRv);
This is hard to read. Please add some newlines, or just put return value to the same line with method name, if there is enough space.


> NS_IMETHODIMP
> nsDOMDeviceStorage::Add(nsIDOMBlob *aBlob, nsIDOMDOMRequest * *_retval)
Could you rename _retval

> nsDOMDeviceStorage::AddNamed(nsIDOMBlob *aBlob,
>                              const nsAString & aPath,
>                              nsIDOMDOMRequest * *_retval)
ditto
> nsDOMDeviceStorage::FreeSpace(nsIDOMDOMRequest** aRetval)
> {
>+  ErrorResult rv;
>+  nsRefPtr<DOMRequest> request = FreeSpace(rv);
>+  request.forget(aRetval);
I think I'd write this and other similar cases like
*aRetVal = FreeSpace(rv).get();
But if you prefer using forget, that is ok.


>+++ b/dom/interfaces/devicestorage/nsIDOMDeviceStorage.idl
update uuid

but apparently new patch coming.

Comment 3

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

Attachment: Part a: Random cleanup

Comment 4

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

Attachment: Part c: Move DeviceStorage to Paris bindings (v2)

Second try

Comment 5

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

Comment on attachment 753826 [details] [diff] [review]
Part c: Move DeviceStorage to Paris bindings (v2)

Still not right... Apparently

nsRefPtr<DOMRequest> request = new DOMRequest(win);
nsCOMPtr<nsIRunnable> r = new PostErrorEvent(request, POST_ERROR_EVENT_UNKNOWN);

will leave |request| null.

Comment 6

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

Attachment: Part b: Don't silently clear nsRefPtrs

Comment 7

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

Comment on attachment 753826 [details] [diff] [review]
Part c: Move DeviceStorage to Paris bindings (v2)

I hope we have some tests for enumerate*

Comment 8

[Dave Hylands [:dhylands]]

Comment on attachment 753821 [details] [diff] [review]
Part a: Random cleanup

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

r=me with prototype cleanup

::: dom/devicestorage/DeviceStorage.h
@@ +194,5 @@
>                         nsIDOMDOMRequest** aRetval,
>                         bool aEditable);
>  
> +  void
> +  GetInternal(nsPIDOMWindow* aWin, const nsAString& aPath, DOMRequest* aRequest,

nit: The other prototypes here have the return type on the same line as the function name. This should follow suit.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/9f0b6d606468
https://hg.mozilla.org/mozilla-central/rev/468594dc72a1
https://hg.mozilla.org/mozilla-central/rev/49c962578899