Convert TimeRanges to WebIDL

RESOLVED FIXED in mozilla22

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks: 1 bug)

Trunk
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Comment hidden (empty)

Updated

5 years ago
No longer blocks: 826738
(Assignee)

Comment 1

5 years ago
Created attachment 715099 [details] [diff] [review]
part 1 - renaming
Attachment #715099 - Flags: review?(Ms2ger)
(Assignee)

Comment 2

5 years ago
Created attachment 715100 [details] [diff] [review]
part 2 - webidl
Attachment #715100 - Flags: review?(Ms2ger)
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.
Attachment #715099 - Flags: review?(Ms2ger) → review+
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
Attachment #715100 - Flags: review?(Ms2ger) → review+
And a followup to get rid of the classinfo, if you don't want to do that in this bug.
(Assignee)

Comment 6

5 years ago
> 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.
(Assignee)

Comment 7

5 years ago
Created attachment 719854 [details] [diff] [review]
part 1 - renaming
Attachment #715099 - Attachment is obsolete: true
Attachment #719854 - Flags: review+
(Assignee)

Comment 8

5 years ago
Created attachment 719855 [details] [diff] [review]
part 2 - webidl
Attachment #715100 - Attachment is obsolete: true
Attachment #719855 - Flags: review+
(Assignee)

Comment 9

5 years ago
Created attachment 719856 [details] [diff] [review]
part 2 - webidl

description missing.
Attachment #719856 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #719855 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
And could you please file a followup to get rid of the classinfo?
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.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 14

5 years ago
Created attachment 719919 [details] [diff] [review]
part 2 - webidl
Attachment #719856 - Attachment is obsolete: true
Attachment #719919 - Flags: review?(peterv)
(Assignee)

Comment 15

5 years ago
Created attachment 720290 [details] [diff] [review]
part 2 - webidl
Attachment #719919 - Attachment is obsolete: true
Attachment #719919 - Flags: review?(peterv)
Attachment #720290 - Flags: review?(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.
Attachment #720290 - Flags: review?(peterv) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 720297 [details] [diff] [review]
part 2 - webidl
Attachment #720290 - Attachment is obsolete: true
Attachment #720297 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Depends on: 847088
https://hg.mozilla.org/mozilla-central/rev/bfedd81983e9
https://hg.mozilla.org/mozilla-central/rev/d55e76187db5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Updated

5 years ago
Depends on: 856605
You need to log in before you can comment on or make changes to this bug.