Fork form bug1301055 comment20, we want to move all audio channel related code into another new wrapper object, and that can reduce the complexity of media element's code. The media element should only handle spec-related code, and any our custom behaviors codes should be in other wrapper classes.

I'll start to work on this bug after finishing the bug1302350.

Comment on attachment 8809332 [details] Bug 1309162 - part1 : create a separate class to handle audio channel releated stuffs. https://reviewboard.mozilla.org/r/91894/#review92988 r+ except for the CC parts. ::: dom/html/HTMLMediaElement.h:760 (Diff revision 6) > void MarkAsContentSource(CallerAPI aAPI); > > protected: > virtual ~HTMLMediaElement(); > > + class AudioChannelWrapper; Call this AudioChannelAgentCallback. ::: dom/html/HTMLMediaElement.h:1300 (Diff revision 6) > > // The current decoder. Load() has been called on this decoder. > // At most one of mDecoder and mSrcStream can be non-null. > RefPtr<MediaDecoder> mDecoder; > > + uint32_t GetMuted() const { You don't need this function because AudioChannelWrapper can access private members of HTMLMediaElement. ::: dom/html/HTMLMediaElement.cpp:557 (Diff revision 6) > +{ > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_CLASS(AudioChannelWrapper) > + > + explicit AudioChannelWrapper(HTMLMediaElement* aOwner, AudioChannel aChannel) 'explicit' is needed for one-parameter constructor only. ::: dom/html/HTMLMediaElement.cpp:558 (Diff revision 6) > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_CLASS(AudioChannelWrapper) > + > + explicit AudioChannelWrapper(HTMLMediaElement* aOwner, AudioChannel aChannel) > + : mOwner(aOwner), Coding style: MyClass(int aVar, int aVar2) : mVar(aVar) , mVar2(aVar2) { ... } ::: dom/html/HTMLMediaElement.cpp:1164 (Diff revision 6) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSrcStream) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSrcAttrStream) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSourcePointer) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mLoadBlockedDoc) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSourceLoadCandidate) > - NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAudioChannelAgent) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAudioChannelWrapper) Does it really need to participate CC? If not, mAudioChannelWrapper is guaranteed to be non-null for the lifetime of HTMLMediaElement and you can save some null checks.

Comment on attachment 8809333 [details] Bug 1309162 - part2 : remove audio channel code from media element. https://reviewboard.mozilla.org/r/91896/#review92990

Comment on attachment 8810694 [details] Bug 1309162 - part6 : remove useless comment. https://reviewboard.mozilla.org/r/92974/#review92992

Comment on attachment 8809332 [details] Bug 1309162 - part1 : create a separate class to handle audio channel releated stuffs. https://reviewboard.mozilla.org/r/91894/#review95712 ::: dom/html/HTMLMediaElement.cpp:595 (Diff revision 8) > + // but resumed it from page. > + SetSuspended(nsISuspendedTypes::NONE_SUSPENDED); > + UpdateAudioChannelPlayingState(); > + } > + > + NS_IMETHODIMP WindowVolumeChanged(float aVolume, bool aMuted) override use the syntax: returnType MethodName(...) { ...

Comment on attachment 8810328 [details] Bug 1309162 - part3 : modify the place calling UpdateAudioChannelPlayingState(). https://reviewboard.mozilla.org/r/92680/#review95714

Comment on attachment 8810329 [details] Bug 1309162 - part4 : remove checking for mPlayingBeforeSeek. https://reviewboard.mozilla.org/r/92682/#review95716 r+, I hope we have enough tests for this :)

Comment on attachment 8810330 [details] Bug 1309162 - part5 : only set the audible state when stream starts playing. https://reviewboard.mozilla.org/r/92684/#review95718

(In reply to Andrea Marchesini [:baku] from comment #41) > r+, I hope we have enough tests for this :) Yes, we have a test to check that (browserElement_AudioChannelSeeking.js). In addition, now hiding tab audio indicator will be delayed for 3 seconds (bug1232357), so we don't need to worry about the indicator blinking during seeking :)

Comment on attachment 8814765 [details] Bug 1309162 - part7 : wrap custom policy function. https://reviewboard.mozilla.org/r/95858/#review95882

Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6b09c6adcfa5 part1 : create a separate class to handle audio channel releated stuffs. r=baku,jwwang https://hg.mozilla.org/integration/autoland/rev/16ceabbf8eef part2 : remove audio channel code from media element. r=jwwang https://hg.mozilla.org/integration/autoland/rev/b5ab5eae225d part3 : modify the place calling UpdateAudioChannelPlayingState(). r=baku https://hg.mozilla.org/integration/autoland/rev/fe1771c91309 part4 : remove checking for mPlayingBeforeSeek. r=baku https://hg.mozilla.org/integration/autoland/rev/2015de6577af part5 : only set the audible state when stream starts playing. r=baku https://hg.mozilla.org/integration/autoland/rev/17c744162fbe part6 : remove useless comment. r=jwwang https://hg.mozilla.org/integration/autoland/rev/3f0ad2d2c594 part7 : wrap custom policy function. r=jwwang

I had to back this out for valgrind failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7298889&repo=autoland https://hg.mozilla.org/integration/autoland/rev/eb95159069bb4e50d52a004d6df9fcdf1f9eae42

Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/17d4303e4f86 part1 : create a separate class to handle audio channel releated stuffs. r=baku,jwwang https://hg.mozilla.org/integration/autoland/rev/bd25005744ed part2 : remove audio channel code from media element. r=jwwang https://hg.mozilla.org/integration/autoland/rev/37f5c9c13c2f part3 : modify the place calling UpdateAudioChannelPlayingState(). r=baku https://hg.mozilla.org/integration/autoland/rev/67431dae9ec8 part4 : remove checking for mPlayingBeforeSeek. r=baku https://hg.mozilla.org/integration/autoland/rev/583e3edec539 part5 : only set the audible state when stream starts playing. r=baku https://hg.mozilla.org/integration/autoland/rev/eef982c27c7b part6 : remove useless comment. r=jwwang https://hg.mozilla.org/integration/autoland/rev/716be76c9a2f part7 : wrap custom policy function. r=jwwang

https://hg.mozilla.org/mozilla-central/rev/17d4303e4f86 https://hg.mozilla.org/mozilla-central/rev/bd25005744ed https://hg.mozilla.org/mozilla-central/rev/37f5c9c13c2f https://hg.mozilla.org/mozilla-central/rev/67431dae9ec8 https://hg.mozilla.org/mozilla-central/rev/583e3edec539 https://hg.mozilla.org/mozilla-central/rev/eef982c27c7b https://hg.mozilla.org/mozilla-central/rev/716be76c9a2f

Alastor, I'm trying to understand the AutoNoJSAPI and its comments about IsCallerChrome in NotifyAudioChannelAgent. There are no IsCallerChrome checks in audiochannel. What is this AutoNoJSAPI trying to prevent, exactly?

That is previous modification, transfer NI to baku.

> checks in audiochannel. What is this AutoNoJSAPI trying to prevent, exactly? We wanted to be sure that any AudioChannelAgent method was exposed only to chrome code. But at some point, we found out that there was a content operation able to trigger an AudioChannelAgent method (not directly, of course). Instead removing the assertion, I preferred to add AutoNoJSAPI. I don't remember the detail, but in case, I can remove AudioNoJSAPI an push to try to see if this assertion is triggered by some tests.

Please do. I'm not aware of what assertion you're talking about.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8a0f9acd005502b23c50fab333d3ea5bd3caee2&selectedJob=75942019 Green on treeherder. I think we can remove that AutoNoJSAPI.

Great. Bug 1338337.