Closed
Bug 1159558
Opened 10 years ago
Closed 10 years ago
Rework state watchers to use a single per-class manager object
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
The current design of state-watching is kind of annoying because it requires consumers to declare intermediate Watcher objects, which are basically just boilerplate obstruction to the use-case of "call this method when this changes".
I'm going to rejigger this stuff and see if I can make it a bit nicer.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
They've proven themselves to be a hassle, and I think we can live without them.
Attachment #8599567 -
Flags: review?(jwwang)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8599568 -
Flags: review?(jwwang)
Updated•10 years ago
|
Attachment #8599567 -
Flags: review?(jwwang) → review+
Comment 4•10 years ago
|
||
Comment on attachment 8599568 [details] [diff] [review]
Part 2 - Redesign watching to use a manager. v1
Review of attachment 8599568 [details] [diff] [review]:
-----------------------------------------------------------------
Overall looks good to me with some thoughts.
::: dom/media/StateWatching.h
@@ +178,5 @@
> +// WatchManager<OwnerType> is intended to be declared as a member of |OwnerType|
> +// objects. Given that, it can't hold a permanent strong ref to the owner, since
> +// that would keep the owner alive indefinitely. Instead, it _only_ holds a
> +// strong ref while waiting for the Direct Task to fire. This ensures that
> +// everything is kept alive just long enough.
Since I don't see HTMLMediaElement deletes mWatchManager explicitly, its life cycle could be extended and owner callbacks could fire unexpectedly.
For our watch mechanics is not intended to be thread-safe, I would prefer to have strict control over the life cycles of watcher/watcherTarget by requiring manual unwatch to prevent callbacks from firing unexpectedly.
@@ +225,5 @@
> +
> + void Notify() override
> + {
> + mArmed = true;
> + mOwner->WatcherNotified();
I would prefer to let PerCallbackWatcher do strong reference promotion on its own so it won't have to go around to mOwner->WatcherNotified() which will call back into PerCallbackWatcher::FireCallback() later.
By doing so, it makes the role of WatchManager clear which is responsible for managing addng/removing PerCallbackWatcher only.
@@ +233,5 @@
> + {
> + return mCallbackMethod == aMethod;
> + }
> +
> + bool IsArmed() const { return mArmed; }
Does |mArmed| exist only for some assertion?
Attachment #8599568 -
Flags: review?(jwwang) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #4)
> > +// WatchManager<OwnerType> is intended to be declared as a member of |OwnerType|
> > +// objects. Given that, it can't hold a permanent strong ref to the owner, since
> > +// that would keep the owner alive indefinitely. Instead, it _only_ holds a
> > +// strong ref while waiting for the Direct Task to fire. This ensures that
> > +// everything is kept alive just long enough.
>
> Since I don't see HTMLMediaElement deletes mWatchManager explicitly, its
> life cycle could be extended and owner callbacks could fire unexpectedly.
>
> For our watch mechanics is not intended to be thread-safe, I would prefer to
> have strict control over the life cycles of watcher/watcherTarget by
> requiring manual unwatch to prevent callbacks from firing unexpectedly.
Some sort of solution to that problem is probably a good idea. Let me see if I can come up with something.
> I would prefer to let PerCallbackWatcher do strong reference promotion on
> its own so it won't have to go around to mOwner->WatcherNotified() which
> will call back into PerCallbackWatcher::FireCallback() later.
I played around with this design, and it turned out to be a nice simplification. I'll attach a new patch.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8599568 -
Attachment is obsolete: true
Attachment #8599700 -
Flags: review?(jwwang)
Assignee | ||
Comment 7•10 years ago
|
||
Small modification.
I think we can improve the teardown situation by giving the watchers an owner
thread. I'll do that in a followup.
Attachment #8599700 -
Attachment is obsolete: true
Attachment #8599700 -
Flags: review?(jwwang)
Attachment #8599707 -
Flags: review?(jwwang)
Updated•10 years ago
|
Attachment #8599707 -
Flags: review?(jwwang) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #8)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d88858911535
Note that this had some android child process weirdness, but it didn't appear in the combined push with this and bug 1159987.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8108fffbe41a
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d27e5cc67dda
https://hg.mozilla.org/mozilla-central/rev/e70d42de8438
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•