Use mojo SystemTray interfaces for both mash and classic ash

ash/common/system/tray/system_tray_controller.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/common/system/tray/system_tray_controller.h"
+
+#include "ash/common/system/date/clock_observer.h"
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/i18n/time_formatting.h"
+#include "services/shell/public/cpp/connector.h"
+
+namespace ash {
+
+SystemTrayController::SystemTrayController(shell::Connector* connector)
+    : connector_(connector), hour_clock_type_(base::GetHourClockType()) {
+  // TODO: In general, how do we get initial values?

[sky, 2016/09/29 16:34:05]

This is a similar problem with the keyboard, where you need to know state right
away. If you need state it implies you have to connect in the constructor.

+}
+
+SystemTrayController::~SystemTrayController() {}
+
+base::HourClockType SystemTrayController::GetHourClockType() const {
+  return hour_clock_type_;
+}
+
+void SystemTrayController::ShowDateSettings() {
+  // REVIEWERS: We could also expose ConnectToSystemTrayClient() publicly
+  // and let code in ash directly invoke methods on the mojo interface.
+  // I'm not sure if it's better to keep things centralized, but my instinct
+  // is that we should.
+  ConnectToSystemTrayClient()->ShowDateSettings();
+}
+
+void SystemTrayController::AddClockObserver(ClockObserver* observer) {
+  clock_observers_.AddObserver(observer);
+}
+
+void SystemTrayController::RemoveClockObserver(ClockObserver* observer) {
+  clock_observers_.RemoveObserver(observer);
+}
+
+mojom::SystemTrayClient* SystemTrayController::ConnectToSystemTrayClient() {
+  DCHECK(connector_);
+
+  if (!system_tray_client_.is_bound()) {
+    // Connect (or reconnect) to the interface.
+    connector_->ConnectToInterface("exe:chrome", &system_tray_client_);

[sky, 2016/09/29 16:34:05]

The name of this may need to be exe:content_browser for the classic ash case.
Having to use two different names is rather confusing though. Make we should see
about renaming exe:chrome to exe:content_browser.

[James Cook, 2016/09/29 16:43:33]

Note for msw: We also need to think about reconnections when running ash_shell
on Windows. In that case chrome may not be compiled/available, and if it comes
up it may not provide the SystemTrayClient  interface. This would result in a
connection error. Then this code would try to reconnect. Whoops. :-)

For both of you: I still want to be able to tolerate chrome crashing and coming
back up, though. Maybe we just don't retry on non-chromeos platforms? Or provide
a stub SystemTrayClient impl for Chrome on Windows?

+
+    // Tolerate chrome crashing and coming back up.
+    system_tray_client_.set_connection_error_handler(
+        base::Bind(&SystemTrayController::OnClientConnectionError,
+                   base::Unretained(this)));
+  }
+  return system_tray_client_.get();
+}
+
+void SystemTrayController::OnClientConnectionError() {
+  system_tray_client_.reset();
+}
+
+// mojom::SystemTray:
+
+void SystemTrayController::SetUse24HourClock(bool use_24_hour) {
+  hour_clock_type_ = use_24_hour ? base::k24HourClock : base::k12HourClock;
+  FOR_EACH_OBSERVER(ClockObserver, clock_observers_, OnDateFormatChanged());

[sky, 2016/09/29 16:34:05]

If you're expecting the client to always call this, then it seems entirely
possible the current value matches that from the client. In other words should
this verify hour_clock_type_ actually changed before calling observers?

+}
+
+}  // namespace ash