Closed Bug 1159558 Opened 5 years ago Closed 5 years ago

Rework state watchers to use a single per-class manager object

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 1158448
They've proven themselves to be a hassle, and I think we can live without them.
Attachment #8599567 - Flags: review?(jwwang)
Attachment #8599568 - Flags: review?(jwwang)
Blocks: 1159974
Blocks: 1159987
Attachment #8599567 - Flags: review?(jwwang) → review+
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+
(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.
Attachment #8599568 - Attachment is obsolete: true
Attachment #8599700 - Flags: review?(jwwang)
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)
Attachment #8599707 - Flags: review?(jwwang) → review+
Blocks: 1160064
(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
https://hg.mozilla.org/mozilla-central/rev/d27e5cc67dda
https://hg.mozilla.org/mozilla-central/rev/e70d42de8438
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.