Closed Bug 1440555 Opened 2 years ago Closed 2 years ago

Improve event registration from video suspend tests.

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: kamidphish, Assigned: kamidphish)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

This test has intermittent failures on Windows. Since the feature is dormant, remove this test.
Priority: -- → P3
:cpearce wasn't keen to see the tests disabled.  Instead, I've rewritten the tests failing with intermittent orange to better hook into the `ended` event.
Summary: Disable test_background_video_suspend_ends.html on Windows → Improve event registration from video suspend tests.
Comment on attachment 8953341 [details]
Bug 1440555 - P1: Allow setVisible to take effect before decoder creation.

https://reviewboard.mozilla.org/r/222628/#review228900

I think we can use OwnerDoc()->IsVisible() here instead of maintaining a copy of that state.

::: dom/html/HTMLMediaElement.cpp:7337
(Diff revision 2)
>      ShutdownDecoder();
>    }
>    mDecoder = aDecoder;
>    DDLINKCHILD("decoder", mDecoder.get());
> +  if (mDecoder && mForcedHidden) {
> +    mDecoder->SetForcedHidden(mForcedHidden);

Can you just pass the value of  OwnerDoc()->IsVisible() here instead of maintaining what looks like duplicate of that state?
Attachment #8953341 - Flags: review?(cpearce) → review-
Comment on attachment 8954263 [details]
Bug 1440555 - P2: Re-write tests that check for end after suspend.

https://reviewboard.mozilla.org/r/223408/#review229680
Attachment #8954263 - Flags: review?(cpearce) → review+
Comment on attachment 8953341 [details]
Bug 1440555 - P1: Allow setVisible to take effect before decoder creation.

https://reviewboard.mozilla.org/r/222628/#review229778

::: dom/html/HTMLMediaElement.cpp:7337
(Diff revision 2)
>      ShutdownDecoder();
>    }
>    mDecoder = aDecoder;
>    DDLINKCHILD("decoder", mDecoder.get());
> +  if (mDecoder && mForcedHidden) {
> +    mDecoder->SetForcedHidden(mForcedHidden);

SetForcedHidden is a special function for use by mochitests. So that value doesn't match OwnerDoc()->IsVisible, ie, I can hide a video on a visible page to witness the suspend, restart seeks, etc behaviour.
Comment on attachment 8953341 [details]
Bug 1440555 - P1: Allow setVisible to take effect before decoder creation.

https://reviewboard.mozilla.org/r/222628/#review230074

OK, thanks for explaining.
Attachment #8953341 - Flags: review- → review+
Pushed by dglastonbury@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4357de13e0f
P1: Allow setVisible to take effect before decoder creation. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/4048365eb9c5
P2: Re-write tests that check for end after suspend. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/a4357de13e0f
https://hg.mozilla.org/mozilla-central/rev/4048365eb9c5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.