New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 661247 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

merge TrayAudio and TrayAudioChromeOs

Project Member Reported by est...@chromium.org, Nov 1 2016

Issue description

TrayAudioChromeOs overrides TrayAudio. TrayAudio is never used on its own. These two classes should be merged.
 
Cc: jen...@chromium.org
Components: Internals>MUS UI>Shell
Labels: -Pri-3 Proj-Mustash-Mash Pri-1
Owner: jamescook@chromium.org
Status: Started (was: Available)
I'm doing this as part of the mustash refactor so that I can make TrayAudio observe CrasAudioHandler directly.

Cc: bruthig@chromium.org
Ben, FYI.

James, any idea on when you will start this work? I'm in the process of modifying the audio detailed view for MD (issue 632115) targeted for M-56 branch next Thursday.
FYI I have this change in progress too.  Not sure if it will affect your efforts James.

https://codereview.chromium.org/2482043002/
I just sent https://codereview.chromium.org/2483393002/ out for review.

Changes to VolumeView should be trivial, I just moved the file. I didn't touch AudioDetailedView.

TrayAudio is significantly changed, but it has very little UI layout code in it, so I doubt it will collide very much.

FYI I also sent out a dependent CL which makes small changes to TrayAudio and deletes one level of observers. Again, I don't think this will affect UI work.

https://codereview.chromium.org/2489723005

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 11 2016

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

commit 96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39
Author: jamescook <jamescook@chromium.org>
Date: Fri Nov 11 03:10:19 2016

chromeos: Combine TrayAudio and TrayAudioChromeOs classes

The two classes were split when we supported ash on Windows. We don't support
ash on Windows anymore, so the two can be combined. This is a pure refactor
with no functional changes.

After doing this we can make TrayAudio directly observe CrasAudioHandler, which
will eliminate the audio observer code in SystemTrayNotifier and allow some
ash-specific code to be removed from chrome's SystemTrayDelegateChromeos.

This also moves all tray audio code into ash/common/system/chromeos/audio.

TODO: Eliminate TrayAudioDelegate and inline the code into TrayAudio.

BUG= 661247 
TEST=manual testing on device of input switching and volume changes

Review-Url: https://codereview.chromium.org/2483393002
Cr-Commit-Position: refs/heads/master@{#431479}

[modify] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/BUILD.gn
[rename] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/common/system/chromeos/audio/audio_observer.h
[rename] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/common/system/chromeos/audio/tray_audio.cc
[rename] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/common/system/chromeos/audio/tray_audio.h
[delete] https://crrev.com/eaec7ad493abe9ec36d7a0097738dcdd6f114441/ash/common/system/chromeos/audio/tray_audio_chromeos.cc
[delete] https://crrev.com/eaec7ad493abe9ec36d7a0097738dcdd6f114441/ash/common/system/chromeos/audio/tray_audio_chromeos.h
[rename] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/common/system/chromeos/audio/tray_audio_delegate.h
[modify] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/common/system/chromeos/audio/tray_audio_delegate_chromeos.h
[rename] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/common/system/chromeos/audio/volume_view.cc
[rename] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/common/system/chromeos/audio/volume_view.h
[modify] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/common/system/tray/system_tray.cc
[modify] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/common/system/tray/system_tray_notifier.cc
[modify] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/common/system/tray/system_tray_notifier.h
[modify] https://crrev.com/96ea4cf812df1ac6c0a0dbd5eed579a2fb1c7e39/ash/wm/power_button_controller.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 11 2016

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

commit 8fb49c827b29e822a0d45dd6aa9c77ae71aef16e
Author: jamescook <jamescook@chromium.org>
Date: Fri Nov 11 19:04:36 2016

chromeos: Make system tray audio item observe CrasAudioHandler directly

For mustash the chrome browser process will not be able to directly access
code in ash (e.g. WmShell). This CL reduces chrome's dependency on ash
in SystemTrayDelegateChromeos by having the ash TrayAudio class observe
CrasAudioHandler directly.

This also allows one extra layer of observers (ash::AudioObserver) to be
deleted.

WmRootWindowController now exposes SystemTray because ARC++ needs to show
the volume popup sometimes. That has to happen on all monitors, so TrayAudio
needs to be able to iterate through all SystemTrays.

BUG= 661247 , 647412 
TEST=added to ash_unittests, also modified the code to trigger the volume
popup manually and tested at the login screen with an external display, also
tested on device with ARC++ and Netflix app

Review-Url: https://codereview.chromium.org/2489723005
Cr-Commit-Position: refs/heads/master@{#431608}

[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/ash/BUILD.gn
[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/ash/common/BUILD.gn
[delete] https://crrev.com/62fa72cbd02ff4e279efe12145dd4b1a3d221821/ash/common/system/chromeos/audio/audio_observer.h
[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/ash/common/system/chromeos/audio/tray_audio.cc
[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/ash/common/system/chromeos/audio/tray_audio.h
[add] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/ash/common/system/chromeos/audio/tray_audio_unittest.cc
[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/ash/common/system/tray/system_tray_notifier.cc
[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/ash/common/system/tray/system_tray_notifier.h
[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/ash/common/wm_root_window_controller.cc
[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/ash/common/wm_root_window_controller.h
[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/chrome/browser/ui/ash/system_tray_delegate_chromeos.h
[modify] https://crrev.com/8fb49c827b29e822a0d45dd6aa9c77ae71aef16e/components/arc/audio/arc_audio_bridge.cc

Status: Fixed (was: Started)
This is almost done. I filed  issue 664642  for folding AudioDelegateChromeOS into VolumeView. That will require some changes to VolumeView so it should wait until after MD ships.

Comment 9 by dchan@google.com, Jan 21 2017

Labels: VerifyIn-57

Comment 10 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58
Labels: mash

Comment 12 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59
Status: Verified (was: Fixed)
Components: -Internals>MUS Internals>Services>WindowService

Sign in to add a comment