Added Aura notification that relaunch and powerwash is required in case of downgrade.

Added Aura notification that relaunch and powerwash is required in case of downgrade.

BUG=260649
TEST=manual tests on a falco device

Committed: https://crrev.com/9903cf4807ac6604ce3d94e82700115e6f7d8d7c
Cr-Commit-Position: refs/heads/master@{#298408}

[ygorshenin1, 2014-10-01 13:03:42]

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

[ygorshenin1, 2014-10-01 13:03:43]

Steven, could you please take a look at the CL?

[stevenjb, 2014-10-01 22:26:44]

https://codereview.chromium.org/614363002/diff/80001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/614363002/diff/80001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:285: <message name="IDS_ASH_STATUS_TRAY_RESTART_TO_UPDATE"
desc="The label used in the tray popup to notify that the user should restart to
get system updates.">
Note: If the string hasn't changed, it's probably better to leave it as-is,
otherwise this will trigger a re-translation of the string (not really a big
deal either way).

https://codereview.chromium.org/614363002/diff/80001/ash/system/tray/system_t...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/614363002/diff/80001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:141: virtual bool
SystemShouldUpgrade(UpdateInfo* info) const = 0;
Looking at the chrome implementation, it looks like this should actually be
'GetSystemUpdateInfoAndNotify()' with a better comment. 

We should also make 'update_required' part of UpdateInfo instead of returning a
bool.

https://codereview.chromium.org/614363002/diff/80001/ash/system/tray_update.h
File ash/system/tray_update.h (right):

https://codereview.chromium.org/614363002/diff/80001/ash/system/tray_update.h...
ash/system/tray_update.h:43: UpdateInfo::UpdateSeverity severity_;
It looks like we don't actually need this anymore?

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/upgrade_detector_chromeos.cc (right):

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:20: const std::string&
target_channel) {
This should either be moved to UpdateEngineClient (where hopefully
:kReleaseChannelsList is reasonably well documented), or should be documented
here, it's not obvious why this works.

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:36: void(const
std::string&, const std::string&)>& callback) {
This callback should be typedfed somewhere

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:49: void
SetCurrentChannel(const std::string& current_channel) {
DCHECK(!current_channel.empty()), otherwise an empty result will cause
TriggerCallbackIfReady to never fire.

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:54: void
SetTargetChannel(const std::string& target_channel) {
Ditto.

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:140:
IsTargetChannelMoreStable(current_channel, target_channel));
Could you add a comment explaining why IsTargetChannelMoreStable corresponds to
factory_reset_required?

https://codereview.chromium.org/614363002/diff/80001/chromeos/dbus/update_eng...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/614363002/diff/80001/chromeos/dbus/update_eng...
chromeos/dbus/update_engine_client.h:26: static const std::vector<std::string>
kReleaseChannelsList;
No complex static members:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl...

I don't think we should expose this publicly anyway, we should create helper
methods as part of this class instead.

[stevenjb, 2014-10-01 22:27:20]

Note: Someone familiar with UpgradeDetector should probably also review this.

[ygorshenin1, 2014-10-02 14:21:36]

https://codereview.chromium.org/614363002/diff/80001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/614363002/diff/80001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:285: <message name="IDS_ASH_STATUS_TRAY_RESTART_TO_UPDATE"
desc="The label used in the tray popup to notify that the user should restart to
get system updates.">
On 2014/10/01 22:26:44, stevenjb wrote:
> Note: If the string hasn't changed, it's probably better to leave it as-is,
> otherwise this will trigger a re-translation of the string (not really a big
> deal either way).

Done.

https://codereview.chromium.org/614363002/diff/80001/ash/system/tray/system_t...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/614363002/diff/80001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:141: virtual bool
SystemShouldUpgrade(UpdateInfo* info) const = 0;
I renamed the method to GetSystemUpdateInfo(), but why it should have the suffix
"AndNotify"? It's a job of SystemTrayNotifier to notify observers about events.

On 2014/10/01 22:26:44, stevenjb wrote:
> Looking at the chrome implementation, it looks like this should actually be
> 'GetSystemUpdateInfoAndNotify()' with a better comment. 
> 
> We should also make 'update_required' part of UpdateInfo instead of returning
a
> bool.
>

https://codereview.chromium.org/614363002/diff/80001/ash/system/tray_update.h
File ash/system/tray_update.h (right):

https://codereview.chromium.org/614363002/diff/80001/ash/system/tray_update.h...
ash/system/tray_update.h:43: UpdateInfo::UpdateSeverity severity_;
On 2014/10/01 22:26:44, stevenjb wrote:
> It looks like we don't actually need this anymore?

Done.

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/upgrade_detector_chromeos.cc (right):

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:20: const std::string&
target_channel) {
On 2014/10/01 22:26:44, stevenjb wrote:
> This should either be moved to UpdateEngineClient (where hopefully
> :kReleaseChannelsList is reasonably well documented), or should be documented
> here, it's not obvious why this works.
>  

Done.

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:36: void(const
std::string&, const std::string&)>& callback) {
On 2014/10/01 22:26:44, stevenjb wrote:
> This callback should be typedfed somewhere

Done.

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:49: void
SetCurrentChannel(const std::string& current_channel) {
On 2014/10/01 22:26:44, stevenjb wrote:
> DCHECK(!current_channel.empty()), otherwise an empty result will cause
> TriggerCallbackIfReady to never fire.

Done.

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:54: void
SetTargetChannel(const std::string& target_channel) {
On 2014/10/01 22:26:44, stevenjb wrote:
> Ditto.

Done.

https://codereview.chromium.org/614363002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/upgrade_detector_chromeos.cc:140:
IsTargetChannelMoreStable(current_channel, target_channel));
On 2014/10/01 22:26:44, stevenjb wrote:
> Could you add a comment explaining why IsTargetChannelMoreStable corresponds
to
> factory_reset_required?

Done.

https://codereview.chromium.org/614363002/diff/80001/chromeos/dbus/update_eng...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/614363002/diff/80001/chromeos/dbus/update_eng...
chromeos/dbus/update_engine_client.h:26: static const std::vector<std::string>
kReleaseChannelsList;
On 2014/10/01 22:26:44, stevenjb wrote:
> No complex static members:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl...
> 
> I don't think we should expose this publicly anyway, we should create helper
> methods as part of this class instead.

Done.

[ygorshenin1, 2014-10-02 14:22:12]

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

[ygorshenin1, 2014-10-02 14:22:13]

+ Finnur for chrome/browser/upgrade_detector.{cc|h}

[Finnur, 2014-10-02 15:00:51]

LGTM

[stevenjb, 2014-10-02 15:33:56]

lgtm

https://codereview.chromium.org/614363002/diff/80001/ash/system/tray/system_t...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/614363002/diff/80001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:141: virtual bool
SystemShouldUpgrade(UpdateInfo* info) const = 0;
On 2014/10/02 14:21:35, ygorshenin1 wrote:
> I renamed the method to GetSystemUpdateInfo(), but why it should have the
suffix
> "AndNotify"? It's a job of SystemTrayNotifier to notify observers about
events.
> 
> On 2014/10/01 22:26:44, stevenjb wrote:
> > Looking at the chrome implementation, it looks like this should actually be
> > 'GetSystemUpdateInfoAndNotify()' with a better comment. 
> > 
> > We should also make 'update_required' part of UpdateInfo instead of
returning
> a
> > bool.
> > 
> 
Sorry, that was some intermediate confusion on my part. I was looking at the
chromeos implementation and got confused by the
UpgradeDetector::notify_upgrade() method (which should really be named something
like should_notify, but probably not here).
Anyway, please ignore that, sorry for the confusion.

https://codereview.chromium.org/614363002/diff/100001/ash/system/tray/default...
File ash/system/tray/default_system_tray_delegate.cc (right):

https://codereview.chromium.org/614363002/diff/100001/ash/system/tray/default...
ash/system/tray/default_system_tray_delegate.cc:95: return;
This should just be DCHECK(info) - there is no reason to call this with a NULL
info.

[ygorshenin1, 2014-10-02 15:40:21]

Many thanks, Steven and Finnur!

https://codereview.chromium.org/614363002/diff/100001/ash/system/tray/default...
File ash/system/tray/default_system_tray_delegate.cc (right):

https://codereview.chromium.org/614363002/diff/100001/ash/system/tray/default...
ash/system/tray/default_system_tray_delegate.cc:95: return;
On 2014/10/02 15:33:55, stevenjb wrote:
> This should just be DCHECK(info) - there is no reason to call this with a NULL
> info.

Done.

[ygorshenin1, 2014-10-02 15:42:17]

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

[ygorshenin1, 2014-10-02 15:42:18]

+ Scott for chrome/browser/*

[sky, 2014-10-02 17:23:06]

https://codereview.chromium.org/614363002/diff/120001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/614363002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:185: void
GetUpdateInfo(const UpgradeDetector* detector, ash::UpdateInfo* info) {
Isn't this code the same as the _win and _linux code?

https://codereview.chromium.org/614363002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:186: if (!detector ||
!info)
Do we really allow either of these to be NULL? Should this be a DCHECK?

https://codereview.chromium.org/614363002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:188:
ash::UpdateInfo::UpdateSeverity severity = ash::UpdateInfo::UPDATE_NORMAL;
Why bother with the local and not set info->severity directly?

https://codereview.chromium.org/614363002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:204: severity =
ash::UpdateInfo::UPDATE_NORMAL;
Isn't this covered by 188? I recommend nuking 188 and 207 entirely. Also, no
default.

[ygorshenin1, 2014-10-03 14:55:37]

PTAL

https://codereview.chromium.org/614363002/diff/120001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/614363002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:185: void
GetUpdateInfo(const UpgradeDetector* detector, ash::UpdateInfo* info) {
Yes, this code is the same for all platforms. I've moved it to
system_tray_delegate_utils.{cc|h}, thank you for pointing on this!

On 2014/10/02 17:23:06, sky wrote:
> Isn't this code the same as the _win and _linux code?

https://codereview.chromium.org/614363002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:186: if (!detector ||
!info)
On 2014/10/02 17:23:06, sky wrote:
> Do we really allow either of these to be NULL? Should this be a DCHECK?

Done.

https://codereview.chromium.org/614363002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:188:
ash::UpdateInfo::UpdateSeverity severity = ash::UpdateInfo::UPDATE_NORMAL;
On 2014/10/02 17:23:06, sky wrote:
> Why bother with the local and not set info->severity directly?

Done.

https://codereview.chromium.org/614363002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:204: severity =
ash::UpdateInfo::UPDATE_NORMAL;
On 2014/10/02 17:23:06, sky wrote:
> Isn't this covered by 188? I recommend nuking 188 and 207 entirely. Also, no
> default.

Done.

[sky, 2014-10-03 16:28:46]

I'm LGTMing this, but I think the win and linux implementations should be
combined. They are nearly identical.

https://codereview.chromium.org/614363002/diff/160001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_utils.h (right):

https://codereview.chromium.org/614363002/diff/160001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_utils.h:14: namespace
system_tray_delegate_utils {
no namespace for this.

[ygorshenin1, 2014-10-03 17:05:30]

PTAL

https://codereview.chromium.org/614363002/diff/160001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_utils.h (right):

https://codereview.chromium.org/614363002/diff/160001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_utils.h:14: namespace
system_tray_delegate_utils {
On 2014/10/03 16:28:45, sky wrote:
> no namespace for this.

Done.

[ygorshenin1, 2014-10-03 17:06:58]

I've removed system_tray_delegate_{linux|win}.cc and created
system_tray_delegate_common.cc, which contains base class, both implementations
and necessary helper functions.

[sky, 2014-10-03 19:28:05]

https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_common.cc (right):

https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_common.cc:318: class
SystemTrayDelegateWin : public SystemTrayDelegateCommon {
Don't put windows and linux implementations in the same file. What you should do
is move the common code (SystemTrayDelegateCommon) into its own .h/.cc. Then
have subclasses for the Window and Linux implementation. For creation have a
factory function (CreateSystemTrayDelegate) that is declared in the .h, but
implemented in the _win.cc and _linux.cc files and create the right type.

[ygorshenin1, 2014-10-03 20:06:56]

PTAL

https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_common.cc (right):

https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_common.cc:318: class
SystemTrayDelegateWin : public SystemTrayDelegateCommon {
Done, but I'd prefer to keep old CreateLinuxSystemTrayDelegate() and
CreateWindowsSystemTrayDelegate(), because their names are quite explicit and
because it'll not cause changes in other files.

On 2014/10/03 19:28:04, sky wrote:
> Don't put windows and linux implementations in the same file. What you should
do
> is move the common code (SystemTrayDelegateCommon) into its own .h/.cc. Then
> have subclasses for the Window and Linux implementation. For creation have a
> factory function (CreateSystemTrayDelegate) that is declared in the .h, but
> implemented in the _win.cc and _linux.cc files and create the right type.

[sky, 2014-10-03 20:32:25]

https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_common.cc (right):

https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_common.cc:318: class
SystemTrayDelegateWin : public SystemTrayDelegateCommon {
On 2014/10/03 20:06:56, ygorshenin1 wrote:
> Done, but I'd prefer to keep old CreateLinuxSystemTrayDelegate() and
> CreateWindowsSystemTrayDelegate(), because their names are quite explicit and
> because it'll not cause changes in other files.
> 
> On 2014/10/03 19:28:04, sky wrote:
> > Don't put windows and linux implementations in the same file. What you
should
> do
> > is move the common code (SystemTrayDelegateCommon) into its own .h/.cc. Then
> > have subclasses for the Window and Linux implementation. For creation have a
> > factory function (CreateSystemTrayDelegate) that is declared in the .h, but
> > implemented in the _win.cc and _linux.cc files and create the right type.
> 

The common way to do this is as I suggested.

[ygorshenin1, 2014-10-06 09:18:43]

On 2014/10/03 20:32:25, sky wrote:
>
https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
> File chrome/browser/ui/ash/system_tray_delegate_common.cc (right):
> 
>
https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
> chrome/browser/ui/ash/system_tray_delegate_common.cc:318: class
> SystemTrayDelegateWin : public SystemTrayDelegateCommon {
> On 2014/10/03 20:06:56, ygorshenin1 wrote:
> > Done, but I'd prefer to keep old CreateLinuxSystemTrayDelegate() and
> > CreateWindowsSystemTrayDelegate(), because their names are quite explicit
and
> > because it'll not cause changes in other files.
> > 
> > On 2014/10/03 19:28:04, sky wrote:
> > > Don't put windows and linux implementations in the same file. What you
> should
> > do
> > > is move the common code (SystemTrayDelegateCommon) into its own .h/.cc.
Then
> > > have subclasses for the Window and Linux implementation. For creation have
a
> > > factory function (CreateSystemTrayDelegate) that is declared in the .h,
but
> > > implemented in the _win.cc and _linux.cc files and create the right type.
> > 
> 
> The common way to do this is as I suggested.

Am I right that you're proposing the following changes?

In system_tray_delegate_common.h:
SystemTrayDelegate* CreateSystemTrayDelegate();

In system_tray_delegate_linux.cc:

class SystemTrayDelegateLinux : public SystemTrayDelegateCommon { ... }

#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
SystemTrayDelegate* CreateSystemTrayDelegate() { return new
SystemTrayDelegateLinux(); }
#endif

In system_tray_delegate_windows.cc:
class SystemTrayDelegateWindows : public SystemTrayDelegateCommon { ... }

#if defined(OS_WIN)
SystemTrayDelegate* CreateSystemTrayDelegate() { return new
SystemTrayDelegateWindows(); }
#endif

[sky, 2014-10-06 14:13:03]

On Mon, Oct 6, 2014 at 2:18 AM,  <ygorshenin@chromium.org> wrote:
> On 2014/10/03 20:32:25, sky wrote:
>
>
https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
>>
>> File chrome/browser/ui/ash/system_tray_delegate_common.cc (right):
>
>
>
>
https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
>>
>> chrome/browser/ui/ash/system_tray_delegate_common.cc:318: class
>> SystemTrayDelegateWin : public SystemTrayDelegateCommon {
>> On 2014/10/03 20:06:56, ygorshenin1 wrote:
>> > Done, but I'd prefer to keep old CreateLinuxSystemTrayDelegate() and
>> > CreateWindowsSystemTrayDelegate(), because their names are quite
>> > explicit
>
> and
>>
>> > because it'll not cause changes in other files.
>> >
>> > On 2014/10/03 19:28:04, sky wrote:
>> > > Don't put windows and linux implementations in the same file. What you
>> should
>> > do
>> > > is move the common code (SystemTrayDelegateCommon) into its own
>> > > .h/.cc.
>
> Then
>>
>> > > have subclasses for the Window and Linux implementation. For creation
>> > > have
>
> a
>>
>> > > factory function (CreateSystemTrayDelegate) that is declared in the
>> > > .h,
>
> but
>>
>> > > implemented in the _win.cc and _linux.cc files and create the right
>> > > type.
>> >
>
>
>> The common way to do this is as I suggested.
>
>
> Am I right that you're proposing the following changes?
>
> In system_tray_delegate_common.h:
> SystemTrayDelegate* CreateSystemTrayDelegate();
>
> In system_tray_delegate_linux.cc:
>
> class SystemTrayDelegateLinux : public SystemTrayDelegateCommon { ... }
>
> #if defined(OS_LINUX) && !defined(OS_CHROMEOS)

You shouldn't need the OS_LINUX here as the file should only be
compiled on linux.

> SystemTrayDelegate* CreateSystemTrayDelegate() { return new
> SystemTrayDelegateLinux(); }
> #endif
>
> In system_tray_delegate_windows.cc:
> class SystemTrayDelegateWindows : public SystemTrayDelegateCommon { ... }
>
> #if defined(OS_WIN)

And you shouldn't need the OS_WIN here since this file will only be
compile don windows.

But yes, this is roughly what I am suggesting.

  -Scott

> SystemTrayDelegate* CreateSystemTrayDelegate() { return new
> SystemTrayDelegateWindows(); }
> #endif
>
>
> https://codereview.chromium.org/614363002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ygorshenin1, 2014-10-06 14:15:07]

On 2014/10/06 14:13:03, sky wrote:
> On Mon, Oct 6, 2014 at 2:18 AM,  <mailto:ygorshenin@chromium.org> wrote:
> > On 2014/10/03 20:32:25, sky wrote:
> >
> >
>
https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
> >>
> >> File chrome/browser/ui/ash/system_tray_delegate_common.cc (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/614363002/diff/200001/chrome/browser/ui/ash/s...
> >>
> >> chrome/browser/ui/ash/system_tray_delegate_common.cc:318: class
> >> SystemTrayDelegateWin : public SystemTrayDelegateCommon {
> >> On 2014/10/03 20:06:56, ygorshenin1 wrote:
> >> > Done, but I'd prefer to keep old CreateLinuxSystemTrayDelegate() and
> >> > CreateWindowsSystemTrayDelegate(), because their names are quite
> >> > explicit
> >
> > and
> >>
> >> > because it'll not cause changes in other files.
> >> >
> >> > On 2014/10/03 19:28:04, sky wrote:
> >> > > Don't put windows and linux implementations in the same file. What you
> >> should
> >> > do
> >> > > is move the common code (SystemTrayDelegateCommon) into its own
> >> > > .h/.cc.
> >
> > Then
> >>
> >> > > have subclasses for the Window and Linux implementation. For creation
> >> > > have
> >
> > a
> >>
> >> > > factory function (CreateSystemTrayDelegate) that is declared in the
> >> > > .h,
> >
> > but
> >>
> >> > > implemented in the _win.cc and _linux.cc files and create the right
> >> > > type.
> >> >
> >
> >
> >> The common way to do this is as I suggested.
> >
> >
> > Am I right that you're proposing the following changes?
> >
> > In system_tray_delegate_common.h:
> > SystemTrayDelegate* CreateSystemTrayDelegate();
> >
> > In system_tray_delegate_linux.cc:
> >
> > class SystemTrayDelegateLinux : public SystemTrayDelegateCommon { ... }
> >
> > #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
> 
> You shouldn't need the OS_LINUX here as the file should only be
> compiled on linux.
> 
> > SystemTrayDelegate* CreateSystemTrayDelegate() { return new
> > SystemTrayDelegateLinux(); }
> > #endif
> >
> > In system_tray_delegate_windows.cc:
> > class SystemTrayDelegateWindows : public SystemTrayDelegateCommon { ... }
> >
> > #if defined(OS_WIN)
> 
> And you shouldn't need the OS_WIN here since this file will only be
> compile don windows.
> 
> But yes, this is roughly what I am suggesting.
> 
>   -Scott
> 
> > SystemTrayDelegate* CreateSystemTrayDelegate() { return new
> > SystemTrayDelegateWindows(); }
> > #endif
> >
> >
> > https://codereview.chromium.org/614363002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

The problem is that both files are compiled for all platforms:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_brow...

[ygorshenin1, 2014-10-06 14:21:41]

Moreover, Create(Linux|Windows)SystemTrayDelegate are used by
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....

How about to keep both methods as is in _linux.h and _win.h,
and to declare and define CreateSystemTrayDelegate() method in _common.{cc|h},
which will be equivalent to ChromeShellDelegate::CreateSystemTrayDelegate()?

[sky, 2014-10-06 18:10:34]

Why? What's wrong with what I've suggested?

On Mon, Oct 6, 2014 at 7:21 AM,  <ygorshenin@chromium.org> wrote:
> Moreover, Create(Linux|Windows)SystemTrayDelegate are used by
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....
>
> How about to keep both methods as is in _linux.h and _win.h,
> and to declare and define CreateSystemTrayDelegate() method in
> _common.{cc|h},
> which will be equivalent to ChromeShellDelegate::CreateSystemTrayDelegate()?
>
> https://codereview.chromium.org/614363002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ygorshenin1, 2014-10-06 19:09:22]

Patchset #12 (id:220001) has been deleted

[ygorshenin1, 2014-10-06 19:09:35]

Patchset #11 (id:200001) has been deleted

[ygorshenin1, 2014-10-06 19:34:12]

After discussion we decided to move refactoring of SystemTrayDelegateLinux and
SystemTrayDelegateWin to an individual CL.

Scott, could you please take another look?

[sky, 2014-10-06 20:15:42]

LGTM

[ygorshenin1, 2014-10-07 05:47:05]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-10-07 05:47:47]

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

[commit-bot: I haz the power, 2014-10-07 06:47:19]

Committed patchset #11 (id:240001) as 39817d984323cdc319d581ea6efa193f1c3e675b

[commit-bot: I haz the power, 2014-10-07 06:48:08]

Patchset 11 (id:??) landed as
https://crrev.com/9903cf4807ac6604ce3d94e82700115e6f7d8d7c
Cr-Commit-Position: refs/heads/master@{#298408}