Mojoify time zone update IPC from browser to renderer

content/browser/time_zone_monitor.cc

 // Copyright 2014 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 "content/browser/time_zone_monitor.h"
 
 #include "base/logging.h"
 #include "build/build_config.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/render_process_host.h"
 #include "third_party/icu/source/common/unicode/unistr.h"
 #include "third_party/icu/source/i18n/unicode/timezone.h"
 
 namespace content {
 
 TimeZoneMonitor::TimeZoneMonitor() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 }
 
 TimeZoneMonitor::~TimeZoneMonitor() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  bindings_.CloseAllBindings();

[dcheng, 2016/09/06 22:20:38]

I thought this would implicitly happen as part of the BindingSet going away. How
come that's not true here?

[blundell, 2016/09/07 14:40:05]

Ah yes, thanks for making me revisit this! I had misinterpreted the root cause
of the problem. The problem is that if |on_time_zone_change_tasks_| is destroyed
while the connections are still active, DCHECK failures go off because the
callbacks destroyed are unfilled while the connection is still valid. By tearing
down the connections first, the callbacks are happy to be destroyed unfilled.

Another solution to this problem would be to switch the order of the ivars in
the header. I'm inclined to leave the CloseAllBindings() call as it's less
fragile. Added a comment. If you think the other solution is better I don't mind
changing.

[dcheng, 2016/09/07 20:59:27]

I kind of feel like it would be nice to have explicit calls to this be
relatively rare (it's only called 4 times atm, while BindingSet is used in 85
places). Also, having an explicit call like this probably means the compiler
emits the code for cleaning up twice: once for the call to CloseAllBindings(),
and another for the actual dtor of BindingSet.

[blundell, 2016/09/12 14:30:33]

Noted for the future, reasoning works for me. After switching to the push model,
this concern is obsolete here in any case and I removed this call.

 }
 
-void TimeZoneMonitor::NotifyRenderers() {
+void TimeZoneMonitor::Bind(device::mojom::TimeZoneMonitorRequest request) {
+  bindings_.AddBinding(this, std::move(request));
+}
+
+void TimeZoneMonitor::NotifyClients() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 #if defined(OS_CHROMEOS)
   // On CrOS, ICU's default tz is already set to a new zone. No
   // need to redetect it with detectHostTimeZone().
   std::unique_ptr<icu::TimeZone> new_zone(icu::TimeZone::createDefault());
 #else
   icu::TimeZone* new_zone = icu::TimeZone::detectHostTimeZone();
 #if defined(OS_LINUX)
   // We get here multiple times on Linux per a single tz change, but
   // want to update the ICU default zone and notify renderer only once.
   std::unique_ptr<icu::TimeZone> current_zone(icu::TimeZone::createDefault());
   if (*current_zone == *new_zone) {
     VLOG(1) << "timezone already updated";
     delete new_zone;
     return;
   }
 #endif
   icu::TimeZone::adoptDefault(new_zone);
 #endif
   icu::UnicodeString zone_id;
   std::string zone_id_str;
   new_zone->getID(zone_id).toUTF8String(zone_id_str);
   VLOG(1) << "timezone reset to " << zone_id_str;
-  for (RenderProcessHost::iterator iterator =
-           RenderProcessHost::AllHostsIterator();
-       !iterator.IsAtEnd();
-       iterator.Advance()) {
-    iterator.GetCurrentValue()->NotifyTimezoneChange(zone_id_str);
-  }
+
+  std::vector<device::mojom::TimeZoneMonitor::QueryNextTimeZoneChangeCallback>
+      tasks;
+  on_time_zone_change_tasks_.swap(tasks);
+  for (auto& task : tasks)
+    task.Run(zone_id_str);
+}
+
+void TimeZoneMonitor::QueryNextTimeZoneChange(
+    const device::mojom::TimeZoneMonitor::QueryNextTimeZoneChangeCallback&
+        callback) {
+  on_time_zone_change_tasks_.push_back(callback);
 }
 
 }  // namespace content