Common code from linux and windows implementations of SystemTrayDelegate

Common code from linux and windows implementations of SystemTrayDelegate
is extracted into an individual base class.

BUG=421056
TEST=none

Committed: https://crrev.com/1915361c70dea2712ff92a5fbb57fc82bd0e0c8e
Cr-Commit-Position: refs/heads/master@{#299459}

[ygorshenin1, 2014-10-07 13:22:22]

ygorshenin@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2014-10-07 16:01:13]

https://codereview.chromium.org/636853002/diff/1/chrome/browser/ui/ash/system...
File chrome/browser/ui/ash/system_tray_delegate_common.h (right):

https://codereview.chromium.org/636853002/diff/1/chrome/browser/ui/ash/system...
chrome/browser/ui/ash/system_tray_delegate_common.h:32: 
style guide says no newlines between these. And use override

https://codereview.chromium.org/636853002/diff/1/chrome/browser/ui/ash/system...
File chrome/browser/ui/ash/system_tray_delegate_linux.cc (right):

https://codereview.chromium.org/636853002/diff/1/chrome/browser/ui/ash/system...
chrome/browser/ui/ash/system_tray_delegate_linux.cc:19:
chrome::ScopedTabbedBrowserDisplayer displayer(
Is this really linux specific? If this is the only different between windows and
linux I'm ok with an ifdef for now.

https://codereview.chromium.org/636853002/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/636853002/diff/1/chrome/chrome_browser_ui.gyp...
chrome/chrome_browser_ui.gypi:1293:
'browser/ui/ash/system_tray_delegate_win.cc',
You should be able to list this in chrome_browser_ui_non_ios_sources and have
the right thing happen.

https://codereview.chromium.org/636853002/diff/1/chrome/chrome_browser_ui.gyp...
chrome/chrome_browser_ui.gypi:2478:
'browser/ui/ash/system_tray_delegate_linux.cc',
I thought we only want this on linux, no-chromeos?

https://codereview.chromium.org/636853002/diff/1/chrome/chrome_browser_ui.gyp...
chrome/chrome_browser_ui.gypi:2920: ['OS!="win" and OS!="linux" and
OS!="chromeos"', {
chromeos is not an OS. Is this ever needed? You'll compile this on mac/android,
which we don't want. So, I'm having a hard time seeing when we would want to
compile these files.

[ygorshenin1, 2014-10-07 16:18:09]

PTAL

https://codereview.chromium.org/636853002/diff/1/chrome/browser/ui/ash/system...
File chrome/browser/ui/ash/system_tray_delegate_common.h (right):

https://codereview.chromium.org/636853002/diff/1/chrome/browser/ui/ash/system...
chrome/browser/ui/ash/system_tray_delegate_common.h:32: 
On 2014/10/07 16:01:13, sky wrote:
> style guide says no newlines between these. And use override

Done.

https://codereview.chromium.org/636853002/diff/1/chrome/browser/ui/ash/system...
File chrome/browser/ui/ash/system_tray_delegate_linux.cc (right):

https://codereview.chromium.org/636853002/diff/1/chrome/browser/ui/ash/system...
chrome/browser/ui/ash/system_tray_delegate_linux.cc:19:
chrome::ScopedTabbedBrowserDisplayer displayer(
On 2014/10/07 16:01:13, sky wrote:
> Is this really linux specific? If this is the only different between windows
and
> linux I'm ok with an ifdef for now.

Done.

[sky, 2014-10-07 16:24:50]

https://codereview.chromium.org/636853002/diff/20001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_common.cc (right):

https://codereview.chromium.org/636853002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_common.cc:315: #if defined(OS_LINUX)
|| defined(OS_WIN)
I don't think you should need this ifdef.

[ygorshenin1, 2014-10-08 15:15:14]

PTAL

https://codereview.chromium.org/636853002/diff/20001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_common.cc (right):

https://codereview.chromium.org/636853002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_common.cc:315: #if defined(OS_LINUX)
|| defined(OS_WIN)
On 2014/10/07 16:24:50, sky wrote:
> I don't think you should need this ifdef.

Done.

[sky, 2014-10-08 15:39:54]

https://codereview.chromium.org/636853002/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate_views.cc (right):

https://codereview.chromium.org/636853002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate_views.cc:188: return new
DefaultSystemTrayDelegate();
AFAICT the else here is never hit. Am I wrong? Assuming that is the case you can
nuke CreateSystemTrayDelegate() and have this return a
system_tray_delegate_common and nuke default_system_tray_delegate.

[ygorshenin1, 2014-10-09 09:14:44]

https://codereview.chromium.org/636853002/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate_views.cc (right):

https://codereview.chromium.org/636853002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate_views.cc:188: return new
DefaultSystemTrayDelegate();
On 2014/10/08 15:39:53, sky wrote:
> AFAICT the else here is never hit. Am I wrong? Assuming that is the case you
can
> nuke CreateSystemTrayDelegate() and have this return a
> system_tray_delegate_common and nuke default_system_tray_delegate.

ChromeShellDelegate is instantiated and used in all places where ash is enabled,
take a look at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....
So probably it would be better to keep old behavior as is, what do you think?

[sky, 2014-10-09 17:22:39]

https://codereview.chromium.org/636853002/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate_views.cc (right):

https://codereview.chromium.org/636853002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate_views.cc:188: return new
DefaultSystemTrayDelegate();
On 2014/10/09 09:14:44, ygorshenin1 wrote:
> On 2014/10/08 15:39:53, sky wrote:
> > AFAICT the else here is never hit. Am I wrong? Assuming that is the case you
> can
> > nuke CreateSystemTrayDelegate() and have this return a
> > system_tray_delegate_common and nuke default_system_tray_delegate.
> 
> ChromeShellDelegate is instantiated and used in all places where ash is
enabled,
> take a look at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....
> So probably it would be better to keep old behavior as is, what do you think?

You made me look. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_brow...
. This file is not built on chromeos.

[ygorshenin1, 2014-10-10 12:02:54]

PTAL

https://codereview.chromium.org/636853002/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate_views.cc (right):

https://codereview.chromium.org/636853002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate_views.cc:188: return new
DefaultSystemTrayDelegate();
On 2014/10/09 17:22:39, sky wrote:
> On 2014/10/09 09:14:44, ygorshenin1 wrote:
> > On 2014/10/08 15:39:53, sky wrote:
> > > AFAICT the else here is never hit. Am I wrong? Assuming that is the case
you
> > can
> > > nuke CreateSystemTrayDelegate() and have this return a
> > > system_tray_delegate_common and nuke default_system_tray_delegate.
> > 
> > ChromeShellDelegate is instantiated and used in all places where ash is
> enabled,
> > take a look at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....
> > So probably it would be better to keep old behavior as is, what do you
think?
> 
> You made me look. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_brow...
> . This file is not built on chromeos.

Done.

[sky, 2014-10-10 17:40:31]

https://codereview.chromium.org/636853002/diff/70001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate_views.cc (right):

https://codereview.chromium.org/636853002/diff/70001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate_views.cc:184: return
::CreateSystemTrayDelegate();
Sorry, one more. There is no point in the function now that there is no ifdef
and one only implementation. Have this directly create SystemTrayDelegateCommon.

[ygorshenin1, 2014-10-13 12:06:17]

PTAL

https://codereview.chromium.org/636853002/diff/70001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate_views.cc (right):

https://codereview.chromium.org/636853002/diff/70001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate_views.cc:184: return
::CreateSystemTrayDelegate();
On 2014/10/10 17:40:31, sky wrote:
> Sorry, one more. There is no point in the function now that there is no ifdef
> and one only implementation. Have this directly create
SystemTrayDelegateCommon.

Done.

[sky, 2014-10-13 17:14:01]

LGTM - thanks for the cleanup (and patience)!

[ygorshenin1, 2014-10-14 08:47:49]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-10-14 08:48:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636853002/180001

[commit-bot: I haz the power, 2014-10-14 09:03:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-14 09:03:22]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ygorshenin1, 2014-10-14 10:24:13]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-10-14 10:24:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636853002/290001

[commit-bot: I haz the power, 2014-10-14 11:25:06]

Committed patchset #6 (id:290001)

[commit-bot: I haz the power, 2014-10-14 11:25:50]

Patchset 6 (id:??) landed as
https://crrev.com/1915361c70dea2712ff92a5fbb57fc82bd0e0c8e
Cr-Commit-Position: refs/heads/master@{#299459}

[ygorshenin1, 2014-10-14 11:31:25]

Many thanks, Scott!