New issue
Advanced search Search tips

Issue 846374 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 836409



Sign in to add a comment

Audibility tracking is a mess

Project Member Reported by chrisha@chromium.org, May 24 2018

Issue description

Currently, audibility tracking is all over the place:

- Some things on WC.
- Some things on WCO.
- Some things on WCD.
- Some notifications channeled over navigation state changes.
- Duplicate state tracking in a handful of places.

While audio playing and being muted are properties of a WC, the notion of recent audibility is not. This should be factored out of contents entirely. In brief, the plan is to do the following:

1. Move OnAudioStateChange from delegate to observer. This immediately unblocks the stuff Seb is working on that needs the signal in our code, but because of Android can't use TSMO.
2. Create a AudibilityTabHelper that calculates recently/ever audible and has a callback for this state change.
3. Migrate each WC::WasRecentlyAudible/WasEverAudible user to using AudibilityTabHelper.
4. Remove all recently/ever audible functions from WC and WCO APIs.
5. Vastly simplify AudibleContentsTracker to only calculate aggregate audibility of the frame tree.
6. Clean up other misuses of NotifyNavigationStateChanged in order to use WCO directly (indirect consumers of OnAudioStateChange and DidUpdateAudioMutingState).
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb41198f249b0ef19b3498d88ee3ed469a22d8a4

commit eb41198f249b0ef19b3498d88ee3ed469a22d8a4
Author: Chris Hamilton <chrisha@chromium.org>
Date: Thu May 24 18:41:26 2018

Move OnAudioStateChanged from WCD to WCO.

This is the first in a multi-part cleanup, detailed in the bug.

BUG=846374

Change-Id: I76331ceb7469f1262a2c934a956fdd96410c1a19
Reviewed-on: https://chromium-review.googlesource.com/1071884
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561568}
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/chrome/browser/android/tab_web_contents_delegate_android.cc
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/chrome/browser/android/tab_web_contents_delegate_android.h
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/chrome/browser/content_settings/sound_content_setting_observer.cc
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/chrome/browser/content_settings/sound_content_setting_observer.h
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/chrome/browser/ui/browser.cc
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/chrome/browser/ui/browser.h
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/content/browser/renderer_host/render_process_host_browsertest.cc
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/content/public/browser/web_contents_observer.h
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/eb41198f249b0ef19b3498d88ee3ed469a22d8a4/extensions/browser/guest_view/web_view/web_view_guest.h

Project Member

Comment 2 by bugdroid1@chromium.org, May 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/019d2e02bc3d4aefbdf835c49434ca697905acfa

commit 019d2e02bc3d4aefbdf835c49434ca697905acfa
Author: Chris Hamilton <chrisha@chromium.org>
Date: Fri May 25 13:35:16 2018

Create RecentlyAudibleHelper.

A follow-up CL will refactor existing consumers of
WC::WasRecentlyAudible to use this API instead, before removing the
relevant content API. More details in the bug.

BUG=846374

Change-Id: I7a758b7fc4d01ca7e0a98676978d1de333829866
Reviewed-on: https://chromium-review.googlesource.com/1072748
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561848}
[modify] https://crrev.com/019d2e02bc3d4aefbdf835c49434ca697905acfa/chrome/browser/ui/BUILD.gn
[add] https://crrev.com/019d2e02bc3d4aefbdf835c49434ca697905acfa/chrome/browser/ui/recently_audible_helper.cc
[add] https://crrev.com/019d2e02bc3d4aefbdf835c49434ca697905acfa/chrome/browser/ui/recently_audible_helper.h
[add] https://crrev.com/019d2e02bc3d4aefbdf835c49434ca697905acfa/chrome/browser/ui/recently_audible_helper_unittest.cc
[modify] https://crrev.com/019d2e02bc3d4aefbdf835c49434ca697905acfa/chrome/test/BUILD.gn

There is one complication to the plan: BrowserPlugin is a content feature that makes use of WasRecentlyAudible. However, this is only done in order to implement WasRecentlyAudible in WebContents, so this can all just evaporate.
More fallout cleanup: AudibleContentsTracker uses the contorted notification signal path (TabStripModelObserver). It should make use of RecentlyAudibleHelper directly.
More cleanup: TabLifecycleUnitSource also makes use of TSMO in order to track audibility.
More complications:

It turns out that audio propagates from inner webcontents to outer webcontents via NvigationStateChanges that are propagated along this channel. Calling "IsCurrentlyAudible" or "WasEverAudible" on the outer webcontents won't take into account the inner webcontents state. However, WasRecentlyAudible *does* take into account inner contents, and is the only path by which audio state changes propagate: when a TAB invalidation is received, WasRecentlyAudible is directly queries.

This is all kinds of gross. I plan to address this by adding direct tracking in WebContentsImpl of audio state changes observed in any child webcontents.
Project Member

Comment 7 by bugdroid1@chromium.org, May 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df0d72cd331cf8a71b5833dd3ea2ecaf622e6314

commit df0d72cd331cf8a71b5833dd3ea2ecaf622e6314
Author: Chris Hamilton <chrisha@chromium.org>
Date: Tue May 29 16:23:53 2018

Fix audibility tracking for contents with guests.

This fixes audibility tracking for contents with guests by having
inner contents notify outer contents of their audio state changes,
and having the outer contents track the aggregate state of all
guests and itself.

This is in preparation of removing WasRecentlyAudible from WebContents
and moving that tracking to a UI tab helper.

BUG=846374

Change-Id: Ia4eea8994d09825748495de3df10eb4e77e03b79
Reviewed-on: https://chromium-review.googlesource.com/1075553
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562454}
[modify] https://crrev.com/df0d72cd331cf8a71b5833dd3ea2ecaf622e6314/content/browser/browser_plugin/browser_plugin_embedder.cc
[modify] https://crrev.com/df0d72cd331cf8a71b5833dd3ea2ecaf622e6314/content/browser/browser_plugin/browser_plugin_embedder.h
[modify] https://crrev.com/df0d72cd331cf8a71b5833dd3ea2ecaf622e6314/content/browser/media/audio_stream_monitor.cc
[modify] https://crrev.com/df0d72cd331cf8a71b5833dd3ea2ecaf622e6314/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/df0d72cd331cf8a71b5833dd3ea2ecaf622e6314/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/df0d72cd331cf8a71b5833dd3ea2ecaf622e6314/content/public/browser/web_contents.h
[modify] https://crrev.com/df0d72cd331cf8a71b5833dd3ea2ecaf622e6314/content/test/test_web_contents.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2013965fad65c0bdeb6bc9e53018bed529448d69

commit 2013965fad65c0bdeb6bc9e53018bed529448d69
Author: Chris Hamilton <chrisha@chromium.org>
Date: Fri Jun 01 15:55:01 2018

Remove WebContents::WasRecentlyAudible.

Follow-up CLs will simplify AudioStreamMonitor.

BUG=846374

Change-Id: I35fd7bf3f255195fa3d0a6f68ec24e3eccfdd114
Reviewed-on: https://chromium-review.googlesource.com/1073747
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563652}
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/content_settings/sound_content_setting_observer_unittest.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/extensions/api/tabs/tabs_event_router.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/extensions/extension_tab_util.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/media/media_engagement_browsertest.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/media/media_engagement_contents_observer.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/media/media_engagement_contents_observer_unittest.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/metrics/desktop_session_duration/audible_contents_tracker.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/metrics/desktop_session_duration/audible_contents_tracker.h
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/metrics/tab_stats_data_store_unittest.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/resource_coordinator/tab_activity_watcher_unittest.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/resource_coordinator/tab_metrics_logger.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/ui/recently_audible_helper.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/ui/recently_audible_helper.h
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/ui/recently_audible_helper_unittest.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/ui/tab_helpers.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chrome/browser/ui/tabs/tab_utils.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/chromecast/browser/extensions/api/automation_internal/automation_internal_api.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/content/browser/browser_plugin/browser_plugin_embedder.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/content/browser/browser_plugin/browser_plugin_embedder.h
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/content/public/browser/web_contents.h
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/content/public/test/web_contents_tester.h
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/content/test/test_web_contents.cc
[modify] https://crrev.com/2013965fad65c0bdeb6bc9e53018bed529448d69/content/test/test_web_contents.h

Sign in to add a comment