Tetsuharu OHZEKI [:tetsuharu] (UTC+9) Assignee
	Description • 12 years ago

      No description provided.

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9) Assignee
	Comment 2 • 12 years ago

ECMA-357(E4X) is deprecated.
We should use for-of statement instead of for-each statement.

	Justin Dolske [:Dolske]
	Comment 3 • 12 years ago

Comment on attachment 661459 [details] [diff] [review]
patch rev2

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

r+ with the one change, all the callers are indeed passing in arrays.

::: toolkit/content/widgets/videocontrols.xml
@@ +658,3 @@
>                          element.item.removeEventListener(element.event, element.func, false);
>  
> +                    this.controlListeners = [];

Why this change? Seems unrelated, so please revert to |delete this.controlListeners|.

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9) Assignee
	Comment 4 • 12 years ago

(In reply to Justin Dolske [:Dolske] from comment #3)
> Comment on attachment 661459 [details] [diff] [review]
> patch rev2
> 
> Review of attachment 661459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the one change, all the callers are indeed passing in arrays.
> 
> ::: toolkit/content/widgets/videocontrols.xml
> @@ +658,3 @@
> >                          element.item.removeEventListener(element.event, element.func, false);
> >  
> > +                    this.controlListeners = [];
> 
> Why this change? Seems unrelated, so please revert to |delete
> this.controlListeners|.

This is a safety guard.
for-of statement is different from for-each, so it occur the error if the iteration target has no property; like "TypeError: (this.controlListeners) is not iterable".

I know that we will avoid it with using the syntax like:

  for (let [,i] of Iterator(this.controlListeners)) {
  ...
  }

But this is not clearly. I don't like this.

Or should I write following?:

  if (this.controlListeners) {
    for (let element of this.controlListeners) {
    ...
    }
  }

	Justin Dolske [:Dolske]
	Comment 5 • 12 years ago

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #4)
 
> > > +                    this.controlListeners = [];
> > 
> > Why this change? Seems unrelated, so please revert to |delete
> > this.controlListeners|.
> 
> This is a safety guard.

Ah. It shouldn't be needed, though... terminateEventListeners() is only called when the video controls are being torn down, after which no further code form that context should be running. I'd consider it a bug if anything was to reference controlListeners after this point.

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9) Assignee
	Comment 6 • 12 years ago

(In reply to Justin Dolske [:Dolske] from comment #5)
> Ah. It shouldn't be needed, though... terminateEventListeners() is only
> called when the video controls are being torn down, after which no further
> code form that context should be running. I'd consider it a bug if anything
> was to reference controlListeners after this point.

Previously, I had tested this patch, this parts caused the error. So I insert its guard. Even so should we fix to reference controlListeners after this point? Or make the patch to fix it at now?

	Justin Dolske [:Dolske]
	Comment 7 • 12 years ago

Can you reproduce the error? How?

It shouldn't be happening, so if it is there's a (separate) bug going on. I'd like to understand what's happening... Then I can either fix it or let you land with the workaround.

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9) Assignee
	Comment 8 • 12 years ago

This is the result which I tried on try-server:
https://tbpl.mozilla.org/?tree=Try&rev=3b2e59478b7d

From the result, it looks videocontrols.xml is failed on relating in mochitest-1.