Mojoify time zone update IPC from browser to renderer

content/browser/time_zone_monitor.cc

Index: content/browser/time_zone_monitor.cc
diff --git a/content/browser/time_zone_monitor.cc b/content/browser/time_zone_monitor.cc
index a9a5a9b6c53689695468bb91294d934281e89c67..3f6573f60e9a1006f361200780ff41f253b23438 100644
--- a/content/browser/time_zone_monitor.cc
+++ b/content/browser/time_zone_monitor.cc
@@ -7,7 +7,6 @@
 #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"
 
@@ -19,9 +18,14 @@ TimeZoneMonitor::TimeZoneMonitor() {
 
 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
@@ -45,12 +49,18 @@ void TimeZoneMonitor::NotifyRenderers() {
   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