Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
98583004:
Added CrOS startup sound. (Closed)
|
Created:
7 years ago by ygorshenin1 Modified:
7 years ago Reviewers:
dzhioev (left Google) CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded CrOS startup sound.
BUG=315108
TEST=manual tests on lumpy and pixel
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238416
Patch Set 1 #
Total comments: 20
Patch Set 2 : Fix. #
Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:89: const int kMaxDelayAfterLoginPromptVisibleMs = 2000; Rename these constants to names containing 'Sound'. https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:361: std::vector<base::StringPiece> sound_resources( I think it's safer to use std::map instead of std::vector (in case if somebody change SoundsManager::Sound enum). https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:369: sound_resources[media::SoundsManager::SOUND_SHUTDOWN] = Do we need load all the sounds? We play only SOUND_STARTUP as I can see? Can't we load them lazily? https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:375: if (!media::SoundsManager::Get()->Initialize(sound_resources)) Why SoundsManager can't initialize by itself? I mean load all needed resources. https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:684: void LoginDisplayHostImpl::StartAppLaunch(const std::string& app_id) { What's about this method? https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:1056: void LoginDisplayHostImpl::PlayStartupSound() { Why it's called PlayStartupSound if it doesn't play sound every call? How did you choose which conditions to place in TryPlayStartupSound and which to place here? https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:1063: const base::TimeDelta delta = Please rename delay and delta to something more meaningful. https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:1077: if (delta > delay) { >= https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:1078: PlayStartupSoundHelper(startup_sound_honors_spoken_feedback_); It's a bad name for function. Please rename.
PTAL https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:89: const int kMaxDelayAfterLoginPromptVisibleMs = 2000; On 2013/12/03 08:28:32, dzhioev wrote: > Rename these constants to names containing 'Sound'. Done. https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:361: std::vector<base::StringPiece> sound_resources( We have a separate issue for this: crbug.com/321335. On 2013/12/03 08:28:32, dzhioev wrote: > I think it's safer to use std::map instead of std::vector (in case if somebody > change SoundsManager::Sound enum). https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:369: sound_resources[media::SoundsManager::SOUND_SHUTDOWN] = We have a separate issue for this: crbug.com/321335. On 2013/12/03 08:28:32, dzhioev wrote: > Do we need load all the sounds? We play only SOUND_STARTUP as I can see? Can't > we load them lazily? https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:375: if (!media::SoundsManager::Get()->Initialize(sound_resources)) Because it lives in media/ and has no idea about resource bundle. On 2013/12/03 08:28:32, dzhioev wrote: > Why SoundsManager can't initialize by itself? I mean load all needed resources. https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:684: void LoginDisplayHostImpl::StartAppLaunch(const std::string& app_id) { Could you please paraphrase your question? On 2013/12/03 08:28:32, dzhioev wrote: > What's about this method? https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:1056: void LoginDisplayHostImpl::PlayStartupSound() { On 2013/12/03 08:28:32, dzhioev wrote: > Why it's called PlayStartupSound if it doesn't play sound every call? How did > you choose which conditions to place in TryPlayStartupSound and which to place > here? Good point, done. https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:1063: const base::TimeDelta delta = On 2013/12/03 08:28:32, dzhioev wrote: > Please rename delay and delta to something more meaningful. Done. https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:1077: if (delta > delay) { I thought these variables will be equal with probability 0. On 2013/12/03 08:28:32, dzhioev wrote: > >= https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:1078: PlayStartupSoundHelper(startup_sound_honors_spoken_feedback_); On 2013/12/03 08:28:32, dzhioev wrote: > It's a bad name for function. Please rename. Done.
https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:684: void LoginDisplayHostImpl::StartAppLaunch(const std::string& app_id) { On 2013/12/03 09:03:28, ygorshenin1 wrote: > Could you please paraphrase your question? > > On 2013/12/03 08:28:32, dzhioev wrote: > > What's about this method? > Don't we want to play startup sound in this mode? I suspect that it's Kiosk mode but don't sure.
https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:684: void LoginDisplayHostImpl::StartAppLaunch(const std::string& app_id) { As you can see from the bug description (crbug.com/315108) this sound should be played at OOBE and signin screen, moreover, at signin screen it's played iff spoken feedback is enabled. So I'm not sure that it should be played in the kiosk mode. On 2013/12/03 09:14:29, dzhioev wrote: > On 2013/12/03 09:03:28, ygorshenin1 wrote: > > Could you please paraphrase your question? > > > > On 2013/12/03 08:28:32, dzhioev wrote: > > > What's about this method? > > > > Don't we want to play startup sound in this mode? I suspect that it's Kiosk mode > but don't sure.
On 2013/12/03 09:24:51, ygorshenin1 wrote: > https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... > File chrome/browser/chromeos/login/login_display_host_impl.cc (right): > > https://codereview.chromium.org/98583004/diff/1/chrome/browser/chromeos/login... > chrome/browser/chromeos/login/login_display_host_impl.cc:684: void > LoginDisplayHostImpl::StartAppLaunch(const std::string& app_id) { > As you can see from the bug description (crbug.com/315108) this sound should be > played at OOBE and signin screen, moreover, at signin screen it's played iff > spoken feedback is enabled. So I'm not sure that it should be played in the > kiosk mode. > > On 2013/12/03 09:14:29, dzhioev wrote: > > On 2013/12/03 09:03:28, ygorshenin1 wrote: > > > Could you please paraphrase your question? > > > > > > On 2013/12/03 08:28:32, dzhioev wrote: > > > > What's about this method? > > > > > > > Don't we want to play startup sound in this mode? I suspect that it's Kiosk > mode > > but don't sure. LGTM
Many thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/98583004/20001
Message was sent while issue was closed.
Change committed as 238416 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
