841014 - Convert TimeRanges to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 3

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

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

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

::: content/html/content/src/Makefile.in
@@ +67,5 @@
>  		HTMLTitleElement.h \
>  		HTMLUnknownElement.h \
>  		UndoManager.h \
>  		ValidityState.h \
> +		TimeRanges.h \

T goes before U

::: content/html/content/src/nsTimeRanges.cpp
@@ +16,5 @@
> +NS_IMPL_ADDREF(TimeRanges)
> +NS_IMPL_RELEASE(TimeRanges)
> +
> +
> +NS_INTERFACE_MAP_BEGIN(TimeRanges)

Just one newline above

::: content/html/content/src/nsTimeRanges.h
@@ +4,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/. */
>  
> +#ifndef mozilla_dom__TimeRanges_h__
> +#define mozilla_dom__TimeRanges_h__

Drop a couple of underscores: mozilla_dom_TimeRanges_h

::: content/media/MediaDecoder.cpp
@@ +533,5 @@
>   * If aValue is not inside a range, false is returned, and aIntervalIndex, if
>   * not null, is set to the index of the range which ends immediately before aValue
>   * (and can be -1 if aValue is before aRanges.Start(0)).
>   */
> +static bool IsInRanges(TimeRanges& aRanges, double aValue, int32_t& aIntervalIndex) {

Move the { to the next line while you're here.

Comment 4

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

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

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

::: content/html/content/src/TimeRanges.cpp
@@ +25,2 @@
>    NS_INTERFACE_MAP_ENTRY(nsIDOMTimeRanges)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMTimeRanges)

I don't understand why this would suddenly be ambiguous. nsWrapperCache doesn't inherit from nsISupports.

@@ +54,5 @@
> +TimeRanges::Start(uint32_t aIndex, ErrorResult& aRv)
> +{
> +  double ret;
> +  aRv = Start(aIndex, &ret);
> +  return ret;

I would prefer switching them around; i.e., XPCOM Start calls WebIDL Start.

@@ +134,5 @@
>  }
>  
> +JSObject*
> +TimeRanges::WrapObject(JSContext* aCx, JSObject* aScope,
> +                          bool* aTriedToWrap)

Indentation

::: content/html/content/src/TimeRanges.h
@@ +30,3 @@
>    NS_DECL_NSIDOMTIMERANGES
>  
>    TimeRanges();

You don't want people to call this constructor, do you? Make it private and MOZ_DELETE, please.

@@ +49,5 @@
> +  {
> +    return mRanges.Length();
> +  }
> +
> +  virtual double Start(uint32_t aIndex, ErrorResult& aRv);

Not virtual

::: dom/bindings/Bindings.conf
@@ +955,5 @@
>  }],
>  
> +'TimeRanges': {
> +    'hasInstanceInterface': 'nsIDOMTimeRanges',
> +},

Drop this

Comment 5

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

And a followup to get rid of the classinfo, if you don't want to do that in this bug.

Comment 6

[Andrea Marchesini [:baku]]

> You don't want people to call this constructor, do you? Make it private and
> MOZ_DELETE, please.

Actually yes. TimeRanges is used in many place of the code just for calculations.
I don't think we should change all of those code.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

description missing.

Comment 10

[Andrea Marchesini [:baku]]

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

Comment 11

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

And could you please file a followup to get rid of the classinfo?

Comment 12

[Peter Van der Beken [:peterv]]

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

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

I think that we can make this |'wrapperCache': False| and not implement nsWrapperCache, we only ever return this by creating a new TimeRanges object so the cache is useless. This would also mean we can remove the parent stuff.

::: content/html/content/src/TimeRanges.cpp
@@ +15,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(TimeRanges)

You'd need to add mParent here if we keep storing that or we'll leak.

@@ +25,2 @@
>    NS_INTERFACE_MAP_ENTRY(nsIDOMTimeRanges)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMTimeRanges)

This shouldn't be needed.

Comment 13

[Andrea Marchesini [:baku]]

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

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 16

[Peter Van der Beken [:peterv]]

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

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

Since we're not traversing anything anymore there's no need for the cycle collector stuff.

::: content/html/content/src/TimeRanges.cpp
@@ +16,5 @@
>  namespace mozilla {
>  namespace dom {
>  
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(TimeRanges)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(TimeRanges)

Undo this change.

@@ +21,2 @@
>  
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(TimeRanges)

And this.

@@ +32,5 @@
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(TimeRanges)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END

And this.

::: content/html/content/src/TimeRanges.h
@@ +22,5 @@
>  class TimeRanges MOZ_FINAL : public nsIDOMTimeRanges
>  {
>  public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(TimeRanges)

And this.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfedd81983e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d55e76187db5

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bfedd81983e9
https://hg.mozilla.org/mozilla-central/rev/d55e76187db5