Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
178883004:
Enable the volume slider in Ash for windows. (Closed)
|
Created:
6 years, 10 months ago by zturner Modified:
6 years, 9 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable the volume slider in Ash for windows.
BUG=227247
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255215
Patch Set 1 #
Total comments: 14
Patch Set 2 : Fix a logic error in the delegate, and a few nits. #
Total comments: 5
Patch Set 3 : Add OWNERS file for ash/system/win #Patch Set 4 : Change roundf() to round() since x64 still uses vs2012 (no c99) #Patch Set 5 : Don't call round() since it doesn't exist before VS2013. #Patch Set 6 : Initialize COM in the test runner during DesktopNotificationsTest #Patch Set 7 : Uninitialize on TearDown(), don't wait for destructor. #Patch Set 8 : Try again, got a 500 last time. #
Total comments: 5
Patch Set 9 : Also check for USE_ASH. #Patch Set 10 : Check for null device in case there is no audio hardware. #Patch Set 11 : Fix COM initialization errors (the right way). #
Messages
Total messages: 60 (0 generated)
henrika, tommi: media/*, [non-owners] ash/system/win/audio/* jennyz: ash/system/tray/system_tray.cc ash/system/audio/tray_audio_delegate.h ash/system/win/audio/* [after henrika, tommi give lgtm] jennyz@: If you prefer I can add myself to OWNERS of ash/system/win/audio, or ash/system/win, so I don't have to go through you for windows lgtms in the future. Up to you.
Are there any unit tests or other test clients I can try to understand better what this CL does? Even if we have had off-line discussions, I find it difficult to review when I don't know exactly what you are trying to do here.
https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... File ash/system/win/audio/tray_audio_delegate_win.cc (right): https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... ash/system/win/audio/tray_audio_delegate_win.cc:19: const int kMuteThresholdPercent = 1; How do you define muted? When the volume is low or when the user has clicked the mute button? https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... ash/system/win/audio/tray_audio_delegate_win.cc:87: ScopedComPtr<IAudioSessionManager> session_manager; Why are you using the SessionManager here? What is the difference in functionality compared with using IMMDevice->IAudioClient-> instead?
On 2014/02/25 07:56:20, henrika wrote: > https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... > File ash/system/win/audio/tray_audio_delegate_win.cc (right): > > https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... > ash/system/win/audio/tray_audio_delegate_win.cc:19: const int > kMuteThresholdPercent = 1; > How do you define muted? When the volume is low or when the user has clicked the > mute button? If the volume goes below a certain threshold, we set the session state to muted and display the mute icon. So it's both of what you said. When the volume is low (but greater than 0), we display the mute icon, but also if the user clicks the mute icon. This is basically just to imitate the ChromeOS functionality, but upon looking at the change again I didn't set the mute threshold low enough, so I will try to fix this. See chromium/src/chromeos/audio/cras_audio_handler.cc and src/ash/system/chromeos/audio/tray_audio_delegate_chromeos.cc for the "equivalent" ChromeOS implementation. > > https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... > ash/system/win/audio/tray_audio_delegate_win.cc:87: > ScopedComPtr<IAudioSessionManager> session_manager; > Why are you using the SessionManager here? What is the difference in > functionality compared with using IMMDevice->IAudioClient-> instead? There's no difference, but this was a line or two less of code and a bit more readable due to calling GetSimpleAudioVolume() instead of GetService() with some ugly uuids etc. If you prefer IAudioClient though, let me know.
On 2014/02/25 07:46:37, henrika wrote: > Are there any unit tests or other test clients I can try to understand better > what this CL does? Even if we have had off-line discussions, I find it difficult > to review when I don't know exactly what you are trying to do here. Unit tests would be at a lower level, such as system_tray.cc, or tray_audio.cc, as this class exists solely to map the interface used by TrayAudio (tray_audio.cc) to the platform specific interfaces used to get at the audio hardware on CrOS / Win. To summarize the CL: The changes in system_tray.cc cause the view to be displayed as part of the system tray in Win Ash. If you scroll up a bit you'll see that TrayAudioChromeOs is created on that platform. So all the UI code that this code will "turn on" is UI that is already in place and working on CrOS. That UI code uses the TrayAudioDelegate to talk to the hardware. This change implements TrayAudioDelegate for windows by acting as a very thin layer over CoreAudio and mostly just forwarding calls. Let me know if this still isnt' clear or if you need more info.
I have checked the ChromeOS implementation and understand better now. If it works with IAudioSessionManager, I am fine.
On 2014/02/25 09:31:44, henrika wrote: > I have checked the ChromeOS implementation and understand better now. > > If it works with IAudioSessionManager, I am fine. Thanks. BTW I did make a cosmetic change in media/audio/win/core_audio_util_win.h, so I think I still need an actual LGTM from you, if you don't mind.
https://codereview.chromium.org/178883004/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/178883004/diff/1/ash/system/tray/system_tray.... ash/system/tray/system_tray.cc:210: if (media::CoreAudioUtil::IsSupported()) are you on the UI thread here? There's a race in IsSupported() that we will eventually fix but for now you need to be aware of it: https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/win/co... https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... File ash/system/win/audio/tray_audio_delegate_win.cc (right): https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... ash/system/win/audio/tray_audio_delegate_win.cc:35: return kMuteThresholdPercent; nit: do you actually need the constant? might as well move the comment here and return 1; Since there's a method specifically to return this value, I'd guess that having a constant for it might not be desirable since users shouldn't depend on it. https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... ash/system/win/audio/tray_audio_delegate_win.cc:52: one empty line https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... ash/system/win/audio/tray_audio_delegate_win.cc:58: bool TrayAudioDelegateWin::IsOutputAudioMuted() { Would be good to have thread checks for all of these methods since it looks like they're assuming single threaded usage. https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... File ash/system/win/audio/tray_audio_delegate_win.h (right): https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... ash/system/win/audio/tray_audio_delegate_win.h:34: base::win::ScopedComPtr<ISimpleAudioVolume> CreateDefaultVolumeControl(); fix indent https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... File ash/system/win/audio/tray_audio_win.h (right): https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... ash/system/win/audio/tray_audio_win.h:21: DISALLOW_COPY_AND_ASSIGN(TrayAudioWin); This should be behind |private:|. As is I think you might actually be allowing copy and assign :) (although it would cause a linker error)
https://codereview.chromium.org/178883004/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/178883004/diff/1/ash/system/tray/system_tray.... ash/system/tray/system_tray.cc:210: if (media::CoreAudioUtil::IsSupported()) On 2014/02/25 16:47:38, tommi wrote: > are you on the UI thread here? > > There's a race in IsSupported() that we will eventually fix but for now you need > to be aware of it: > https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/win/co... Yes, it's from the UI thread. I will double check that we're calling it from the main thread first. https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... File ash/system/win/audio/tray_audio_delegate_win.cc (right): https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... ash/system/win/audio/tray_audio_delegate_win.cc:35: return kMuteThresholdPercent; On 2014/02/25 16:47:38, tommi wrote: > nit: do you actually need the constant? might as well move the comment here and > return 1; > > Since there's a method specifically to return this value, I'd guess that having > a constant for it might not be desirable since users shouldn't depend on it. I also used the constant in AdjustOutputVolumeToAudibleLevel (not in this CL, but that's because I had an oversight, but will be fixed in next upload). So it's used both internally and externally https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... ash/system/win/audio/tray_audio_delegate_win.cc:58: bool TrayAudioDelegateWin::IsOutputAudioMuted() { On 2014/02/25 16:47:38, tommi wrote: > Would be good to have thread checks for all of these methods since it looks like > they're assuming single threaded usage. Is CoreAudio inherently single-threaded? CreateDefaultVolumeControl, for example, creates a new instance every time and doesn't re-use any shared variables, so I think it should be ok right?
https://codereview.chromium.org/178883004/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/178883004/diff/1/ash/system/tray/system_tray.... ash/system/tray/system_tray.cc:210: if (media::CoreAudioUtil::IsSupported()) On 2014/02/25 17:19:56, zturner wrote: > On 2014/02/25 16:47:38, tommi wrote: > > are you on the UI thread here? > > > > There's a race in IsSupported() that we will eventually fix but for now you > need > > to be aware of it: > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/win/co... > > Yes, it's from the UI thread. I will double check that we're calling it from > the main thread first. This is indeed called from the main thread before this code (on the UI thread) executes, so it looks like we have nothing to worry about? https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...
https://codereview.chromium.org/178883004/diff/20001/ash/system/tray/system_t... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/178883004/diff/20001/ash/system/tray/system_t... ash/system/tray/system_tray.cc:210: if (media::CoreAudioUtil::IsSupported()) Just checking. What happens on Windows XP here? https://codereview.chromium.org/178883004/diff/20001/ash/system/win/audio/tra... File ash/system/win/audio/tray_audio_delegate_win.cc (right): https://codereview.chromium.org/178883004/diff/20001/ash/system/win/audio/tra... ash/system/win/audio/tray_audio_delegate_win.cc:83: ScopedComPtr<ISimpleAudioVolume> It is not clear to me why you call this method in every method which needs the volume control. Would it not be possible to create a volume control once (during construction) and use that after. Perhaps I am missing something regarding the use case. Perhaps tommi@ knows the answer as well.
https://codereview.chromium.org/178883004/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/178883004/diff/1/ash/system/tray/system_tray.... ash/system/tray/system_tray.cc:210: if (media::CoreAudioUtil::IsSupported()) On 2014/02/25 17:52:00, zturner wrote: > On 2014/02/25 17:19:56, zturner wrote: > > On 2014/02/25 16:47:38, tommi wrote: > > > are you on the UI thread here? > > > > > > There's a race in IsSupported() that we will eventually fix but for now you > > need > > > to be aware of it: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/win/co... > > > > Yes, it's from the UI thread. I will double check that we're calling it from > > the main thread first. > > This is indeed called from the main thread before this code (on the UI thread) > executes, so it looks like we have nothing to worry about? > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br... Great. thanks for checking. Yes, fine as is then. https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... File ash/system/win/audio/tray_audio_delegate_win.cc (right): https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... ash/system/win/audio/tray_audio_delegate_win.cc:58: bool TrayAudioDelegateWin::IsOutputAudioMuted() { On 2014/02/25 17:19:56, zturner wrote: > On 2014/02/25 16:47:38, tommi wrote: > > Would be good to have thread checks for all of these methods since it looks > like > > they're assuming single threaded usage. > > Is CoreAudio inherently single-threaded? CreateDefaultVolumeControl, for > example, creates a new instance every time and doesn't re-use any shared > variables, so I think it should be ok right? CoreAudio isn't single threaded but not every thread is COM initialized. There are some quirks in the implementation on Windows 8 that require the first thread that accesses an IAudioClient, to be an STA thread. From there on MTA access is fine (I'm assuming since the threading model is 'Both'). As is, our threads are only MTA initialized so we need to fix that. Given this and that we're already in a position to isolate usage of CoreAudio routines to a specific thread (although you're not using that particular thread here), I think we're going to make that change soon in the media code. The thread to use will be the AudioManager's thread: https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... So I think it might be a good idea here to avoid having synchronous methods that use the API such as IsOutputAudioMuted and GetOutputVolumeLevel. If they could use a callback to deliver the results and do the actual work on the audio thread, then that would be best. (PostTaskAndReply type of thing) We will then change the AudioManager's thread to be an STA thread and also initialize our CoreAudio routines in a way that avoids the race I mentioned earlier. Furthermore we'll start to add DCHECKs to all CoreAudio related routines that assert we're on the correct thread. https://codereview.chromium.org/178883004/diff/20001/ash/system/win/audio/tra... File ash/system/win/audio/tray_audio_delegate_win.cc (right): https://codereview.chromium.org/178883004/diff/20001/ash/system/win/audio/tra... ash/system/win/audio/tray_audio_delegate_win.cc:83: ScopedComPtr<ISimpleAudioVolume> On 2014/02/26 09:10:05, henrika wrote: > It is not clear to me why you call this method in every method which needs the > volume control. Would it not be possible to create a volume control once (during > construction) and use that after. Perhaps I am missing something regarding the > use case. > Perhaps tommi@ knows the answer as well. If these methods are infrequently called, then I think it's fine as is. Caching the default device could be problematic if it changes and right now the code avoids problems because of that.
LGTM from my side.
lgtm. Yes, please add yourself as the owners of ash/system/win.
> https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... > File ash/system/win/audio/tray_audio_delegate_win.cc (right): > > https://codereview.chromium.org/178883004/diff/1/ash/system/win/audio/tray_au... > ash/system/win/audio/tray_audio_delegate_win.cc:58: bool > TrayAudioDelegateWin::IsOutputAudioMuted() { > On 2014/02/25 17:19:56, zturner wrote: > > On 2014/02/25 16:47:38, tommi wrote: > > > Would be good to have thread checks for all of these methods since it looks > > like > > > they're assuming single threaded usage. > > > > Is CoreAudio inherently single-threaded? CreateDefaultVolumeControl, for > > example, creates a new instance every time and doesn't re-use any shared > > variables, so I think it should be ok right? > > CoreAudio isn't single threaded but not every thread is COM initialized. There > are some quirks in the implementation on Windows 8 that require the first thread > that accesses an IAudioClient, to be an STA thread. From there on MTA access is > fine (I'm assuming since the threading model is 'Both'). As is, our threads are > only MTA initialized so we need to fix that. > > Given this and that we're already in a position to isolate usage of CoreAudio > routines to a specific thread (although you're not using that particular thread > here), I think we're going to make that change soon in the media code. The > thread to use will be the AudioManager's thread: > https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... > > So I think it might be a good idea here to avoid having synchronous methods that > use the API such as IsOutputAudioMuted and GetOutputVolumeLevel. If they could > use a callback to deliver the results and do the actual work on the audio > thread, then that would be best. (PostTaskAndReply type of thing) > > We will then change the AudioManager's thread to be an STA thread and also > initialize our CoreAudio routines in a way that avoids the race I mentioned > earlier. Furthermore we'll start to add DCHECKs to all CoreAudio related > routines that assert we're on the correct thread. So assuming your change goes through, I assume it will cause the current code as-written to break? Making the methods asynchronous is probably going to be somewhat involved, so if you think I need to do that before submitting this change I may have to go back to the drawing board a little before submitting this. https://codereview.chromium.org/178883004/diff/20001/ash/system/tray/system_t... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/178883004/diff/20001/ash/system/tray/system_t... ash/system/tray/system_tray.cc:210: if (media::CoreAudioUtil::IsSupported()) On 2014/02/26 09:10:05, henrika wrote: > Just checking. What happens on Windows XP here? "Windows Ash" is mostly synonymous with "Chrome Windows 8 mode". So as the name implies, you cannot run Ash on any version of Windows prior to 8 (actually you can make it work on Win7, but it's not a supported configuration, and may be broken). https://codereview.chromium.org/178883004/diff/20001/ash/system/win/audio/tra... File ash/system/win/audio/tray_audio_delegate_win.cc (right): https://codereview.chromium.org/178883004/diff/20001/ash/system/win/audio/tra... ash/system/win/audio/tray_audio_delegate_win.cc:83: ScopedComPtr<ISimpleAudioVolume> On 2014/02/26 11:10:44, tommi wrote: > On 2014/02/26 09:10:05, henrika wrote: > > It is not clear to me why you call this method in every method which needs the > > volume control. Would it not be possible to create a volume control once > (during > > construction) and use that after. Perhaps I am missing something regarding the > > use case. > > Perhaps tommi@ knows the answer as well. > > If these methods are infrequently called, then I think it's fine as is. Caching > the default device could be problematic if it changes and right now the code > avoids problems because of that. Yes, the default device changing is exactly the reason I did it this way.
Nah, I don't think it's worth it to complicate this change right now. If/when we get to it, we'll pick up on this code since it uses IsSupported() and then we might ping you if we need help. lgtm.
The CQ bit was checked by zturner@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/178883004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_dbg_simulator for step(s) base_unittests, compile, components_unittests, content_unittests, crypto_unittests, net_unittests, sql_unittests, ui_unittests, url_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
The CQ bit was checked by zturner@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/178883004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_x64_rel for step(s) base_unittests, chrome_elf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
The CQ bit was checked by tommi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/178883004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
The CQ bit was checked by zturner@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/178883004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by zturner@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/178883004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_x64_rel for step(s) base_unittests, chrome_elf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
On 2014/02/27 21:40:52, I haz the power (commit-bot) wrote: > Retried try job too often on win_x64_rel for step(s) base_unittests, > chrome_elf_unittests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Still compilation errors. Looks like you're running into this: http://social.msdn.microsoft.com/Forums/vstudio/en-US/260e04fc-dd05-4a96-8953...
On 2014/02/27 21:52:02, tommi wrote: > On 2014/02/27 21:40:52, I haz the power (commit-bot) wrote: > > Retried try job too often on win_x64_rel for step(s) base_unittests, > > chrome_elf_unittests > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... > > Still compilation errors. Looks like you're running into this: > http://social.msdn.microsoft.com/Forums/vstudio/en-US/260e04fc-dd05-4a96-8953... Yea, my mind is blown. Visual Studio prior to 2013 doesn't have *any round() function at all*.
The CQ bit was checked by zturner@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/178883004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
+dewittj for the COM initialization in the DesktopNotificationsTest runner. I added some calls to COM to interface with audio hardware on Windows. For chrome this is fine, COM is already initialized in the UI thread on startup. For this particular test runner, that had been no reason yet to introduce COM until this change.
I don't understand why we need the audio subsystem to be initialized for the desktop notification service tests to work. Why is this service the only one that needs a ScopedComInitializer?
On 2014/03/03 22:11:28, dewittj wrote: > I don't understand why we need the audio subsystem to be initialized for the > desktop notification service tests to work. Why is this service the only one > that needs a ScopedComInitializer? The desktop notification service makes use of the ash system tray, and this change adds an audio slider to that system tray. When the tray is created, it adds all the panels that it's going to use to itself. Previously in Windows this was almost nothing, but now it's the volume slider panel.
lgtm with a few nits: https://codereview.chromium.org/178883004/diff/140001/chrome/browser/notifica... File chrome/browser/notifications/desktop_notifications_unittest.cc (right): https://codereview.chromium.org/178883004/diff/140001/chrome/browser/notifica... chrome/browser/notifications/desktop_notifications_unittest.cc:110: #if defined(OS_WIN) same here https://codereview.chromium.org/178883004/diff/140001/chrome/browser/notifica... chrome/browser/notifications/desktop_notifications_unittest.cc:111: com_initializer_.reset(new base::win::ScopedCOMInitializer()); Please add a comment referencing the code that requires this initializer; Probably the DesktopNotificationService needs a refactoring so its unit tests don't have all these outside dependencies. https://codereview.chromium.org/178883004/diff/140001/chrome/browser/notifica... chrome/browser/notifications/desktop_notifications_unittest.cc:153: #if defined(OS_WIN) Same #define comment here. https://codereview.chromium.org/178883004/diff/140001/chrome/browser/notifica... File chrome/browser/notifications/desktop_notifications_unittest.h (right): https://codereview.chromium.org/178883004/diff/140001/chrome/browser/notifica... chrome/browser/notifications/desktop_notifications_unittest.h:24: #if defined(OS_WIN) Probably should read: defined(OS_WIN) && defined(USE_ASH) https://codereview.chromium.org/178883004/diff/140001/chrome/browser/notifica... chrome/browser/notifications/desktop_notifications_unittest.h:141: #if defined(OS_WIN) same here
The CQ bit was checked by zturner@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/178883004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by zturner@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/178883004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, unit_tests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by zturner@chromium.org
On 2014/03/05 22:18:00, I haz the power (commit-bot) wrote: > Retried try job too often on win_rel for step(s) app_list_unittests, > ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, > check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, > components_unittests, compositor_unittests, content_browsertests, > content_unittests, crypto_unittests, events_unittests, gfx_unittests, > google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, > jingle_unittests, media_unittests, mini_installer_test, nacl_integration, > ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, > sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, > telemetry_unittests, unit_tests, views_unittests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Apparently the win bots are really flaky due to disk space related to the VS2013 migration, so I guess I just have to keep on trying this until it works.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/178883004/180001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zturner@chromium.org/178883004/180001
Message was sent while issue was closed.
Change committed as 255215
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/188693004/ by pkotwicz@chromium.org. The reason for reverting is: Experimentally reverting this CL because it may have broken AshTestHelperTest.AshTestHelper on Windows Vista . Will revert the revert if this CL is innocent See http://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%282%29/bui.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
