840559 - Convert MediaError to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

What do we do for enums in webIDL? I left the nsIDOMHTMLMediaError ones.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 3

[Andrea Marchesini [:baku]]

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

Comment 4

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

Comment on attachment 712964 [details] [diff] [review]
part 1 - renaming

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

::: content/html/content/src/nsMediaError.cpp
@@ +21,5 @@
>    NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(MediaError)
>  NS_INTERFACE_MAP_END
>  
> +MediaError::MediaError(uint16_t aCode)
> +:  mCode(aCode)

Nit:

  : mCode(aCode)

(two spaces, :, one space, mCode)

Comment 5

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

Comment on attachment 712965 [details] [diff] [review]
part 2 - webidl

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

You need more CC magic, at least for the wrapper and mParent.

::: content/html/content/src/MediaError.cpp
@@ +23,5 @@
>  NS_INTERFACE_MAP_END
>  
> +MediaError::MediaError(nsHTMLMediaElement* aParent, uint16_t aCode)
> +: mParent(aParent)
> +, mCode(aCode)

Same

@@ +37,5 @@
>  }
>  
> +JSObject*
> +MediaError::WrapObject(JSContext* cx, JSObject* scope,
> +                       bool* triedToWrap)

aFoo for arguments

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl (b)

Comment 8

[Boris Zbarsky [:bzbarsky]]

> What do we do for enums in webIDL?

WebIDL uses string-valued enums, because numeric ones don't really make sense in JavaScript...

If we need the numeric constants on the C++ side, we can just leave it in the XPIDL as you have or put them in the C++ impl.

Comment 9

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

Comment on attachment 712978 [details] [diff] [review]
part 2 - webidl (b)


> namespace mozilla {
> namespace dom {
> 
>-NS_IMPL_ADDREF(MediaError)
>-NS_IMPL_RELEASE(MediaError)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(MediaError)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mParent)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> 
>-NS_INTERFACE_MAP_BEGIN(MediaError)
>-  NS_INTERFACE_MAP_ENTRY(nsISupports)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(MediaError)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(MediaError)
>+  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
>+NS_IMPL_CYCLE_COLLECTION_TRACE_END


NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(MediaError, mParent)
should work.

Comment 10

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

Comment on attachment 712978 [details] [diff] [review]
part 2 - webidl (b)

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

r=me with those comments addressed and using NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1.

::: content/html/content/src/MediaError.cpp
@@ -20,2 @@
>    NS_INTERFACE_MAP_ENTRY(nsIDOMMediaError)
> -  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(MediaError)

I don't think you should remove this.

@@ +36,5 @@
> +
> +
> +MediaError::MediaError(nsHTMLMediaElement* aParent, uint16_t aCode)
> +: mParent(aParent)
> +, mCode(aCode)

Should indent this by two spaces, as I mentioned.

@@ +43,5 @@
>  
>  NS_IMETHODIMP MediaError::GetCode(uint16_t* aCode)
>  {
>    if (aCode)
>      *aCode = mCode;

Make this Code()

::: content/html/content/src/MediaError.h
@@ +47,2 @@
>    // Error code
>    uint16_t mCode;

This could be const

::: dom/webidl/MediaError.webidl
@@ +3,5 @@
> + * 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/.
> + *
> + * The origin of this IDL file is
> + * http://dev.w3.org/html5/spec-author-view/video.html#mediaerror

You don't want to be looking at w3.org for HTML, and definitely not at spec-author-view. Please link to <http://www.whatwg.org/html/#mediaerror>.

@@ +15,5 @@
> +  const unsigned short MEDIA_ERR_ABORTED = 1;
> +  const unsigned short MEDIA_ERR_NETWORK = 2;
> +  const unsigned short MEDIA_ERR_DECODE = 3;
> +  const unsigned short MEDIA_ERR_SRC_NOT_SUPPORTED = 4;
> +  readonly attribute unsigned short code;

Add [Constant]

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl (d)

Comment 12

[Andrea Marchesini [:baku]]

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

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 14

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

Comment on attachment 713883 [details] [diff] [review]
part 2 - webidl

mParent needs to be strong and it needs to be traversed/unlinked

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a61cbf1da700
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0ff1ebd9885

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a61cbf1da700
https://hg.mozilla.org/mozilla-central/rev/d0ff1ebd9885