On 15/06/2005, at 1:32 AM, Kevin DeKorte wrote:
> Well I took a look at this patch and you removed a few lines of
> code that are
I thought I probably would, but it's really hard to tell what the
purpose of some parts of it is.
> Looks like you removed the ability to have a media that is
> autostart=false and
> then be started when the play button is clicked.
No, that's what mine is, and it works.
> The reason controlwindow is in there is for the case where the user
> is in a X
> widget mode and does not have a button to click.
Ah, right. Like I said, I want to make sure I'm not making things
worse before I go on trying to make them better...
> Looks like you also moved some code around.. lets see if we can get
> a cleaner
> patch and get this solved.
Yes. I think it makes more sense if it can be moved into a more
logical order; the tests around each block were really ugly and error-
prone. If it can be re-ordered so the tests become more intuitive and
less complex, I reckon that would be a really good thing.
Essentially it seems like it makes sense to go through the blocks
(and tests) in that method in the order that they would be performed
to start the thing up in the first place - thread setup first, if
needed, followed by launching the player, making sure the playlist is
set up, signaling the thread, whatever.
I would also suggest making the functions that are called from there
consistent in either requiring or not requiring that the control
mutex be held on entry -- signalPlayerThread and launchPlayerThread
being the most glaring examples here. Since I wasn't familiar enough
with the code to be confident I understood exactly what the mutex was
protecting against and where those functions would be called from, I
didn't touch any of that.
The other thing that struck me as worth considering is whether or not
the player thread really ought to exit when it finishes the playlist;
I guess so long as the thread handling code in the main thread is
simple enough and can reliably restart it, that's not a problem.
Letting it exit is probably better on balance, as it protects against
any problems caused by mozilla not cleaning up properly when the page/
tab is closed, or a new page loaded. We have had problems in this
area too, but apparently only when the new page is loaded by
submitting a form (haven't got to the bottom of this yet).
Happy to help as much as I can, anyway.