Mojoify time zone update IPC from browser to renderer

Mojoify time zone update IPC from browser to renderer

To start the servicification of //content's time zone monitoring code,
this CL Mojoifies the ViewMsg_TimezoneChange message sent from the browser
to the renderer. To do so, we introduce the TimeZoneMonitor Mojo interface
(living in //device, where the implementation will move to), and change
content::TimeZoneMonitor to implement the interface. Rather than notifying
all RenderProcessHosts on a time zone change, content::TimeZoneMonitor
maintains connections to the clients (RenderThreadImpl in this case)
and notifies them directly on a time zone change. This CL uses the
polling pattern described here:
https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/pSNDDN3gpFo
for consistency with the way that the geolocation and battery monitor
interfaces work (in practice, it's unlikely that time zone change updates
would flood the client, of course).

Followup will decouple content::TimeZoneMonitor from //content and move it to //
device.

BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
     and then click "recheck" to ensure that the renderer picks up the new
     time zone. Don't reload the page, which is likely to give you a new
     renderer process, use the "recheck" link on the page.

Committed: https://crrev.com/c00adf442e48581afa7df022a0e84c507d1ff3e3
Cr-Commit-Position: refs/heads/master@{#419140}

[blundell, 2016-09-02 12:51:47]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 12:52:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 13:10:20]

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

[commit-bot: I haz the power, 2016-09-02 13:10:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[blundell, 2016-09-02 17:06:46]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 17:07:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 17:34:15]

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

[commit-bot: I haz the power, 2016-09-02 17:34:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[blundell, 2016-09-05 10:02:11]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-05 10:02:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-05 10:08:21]

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

[commit-bot: I haz the power, 2016-09-05 10:08:22]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[blundell, 2016-09-06 08:23:19]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 08:23:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 08:29:34]

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

[commit-bot: I haz the power, 2016-09-06 08:29:35]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[blundell, 2016-09-06 08:53:22]

Description was changed from

==========
Mojoify time zone update IPC from browser to renderer

TODO(blundell): Fill in this description.

BUG=TODO(blundell): Fill in.
==========

to

==========
Mojoify time zone update IPC from browser to renderer

To start the servicification of //content's time zone monitoring code,
this CL Mojoifies the ViewMsg_TimezoneChange message sent from the browser
to the renderer. To do so, we introduce the TimeZoneMonitor Mojo interface
(living in //device, where the implementation will move to), and change
content::TimeZoneMonitor to implement the interface. Rather than notifying
all RenderProcessHosts on a time zone change, content::TimeZoneMonitor
maintains connections to the clients (RenderThreadImpl in this case)
and notifies them directly on a time zone change. This CL uses the
polling pattern described here:
https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/pSNDDN3gpFo
for consistency with the way that the geolocation and battery monitor
interfaces work (in practice, it's unlikely that time zone change updates
would flood the client, of course).

A followup CL will rename content::TimeZoneMonitor to its now-correct name of
TimeZoneMonitorImpl. Further followup will decouple that code from //content
and move it to //device.

BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
     and then click "recheck" to ensure that the renderer picks up the new
     time zone. Don't reload the page, which is likely to give you a new
     renderer process, use the "recheck" link on the page.
==========

[blundell, 2016-09-06 09:01:11]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 09:01:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 10:09:54]

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

[commit-bot: I haz the power, 2016-09-06 10:09:55]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-09-06 10:31:58]

blundell@chromium.org changed reviewers:
+ jam@chromium.org, rockot@chromium.org

[blundell, 2016-09-06 10:33:29]

blundell@chromium.org changed reviewers:
+ dcheng@chromium.org

[blundell, 2016-09-06 10:33:29]

+dcheng@ for security review of the new mojom file and the Chrome IPC removal

[jam, 2016-09-06 22:11:18]

lgtm

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
File content/browser/time_zone_monitor.h (right):

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
content/browser/time_zone_monitor.h:33: // TODO(blundell): Update this class to
be named TimeZoneMonitorImpl.
nit: btw no need to do that. the reason interfaces are in the "mojom"
subnamespace is that the impls don't need to end in Impl (that's the reason
given for that mojom intermediate namespace)

https://codereview.chromium.org/2304073003/diff/120001/device/time/public/int...
File device/time/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2304073003/diff/120001/device/time/public/int...
device/time/public/interfaces/BUILD.gn:1: # Copyright 2016 The Chromium Authors.
All rights reserved.
nit: add an owner for device/time_zone_montior

https://codereview.chromium.org/2304073003/diff/120001/device/time/public/int...
File device/time/public/interfaces/time_zone_monitor.mojom (right):

https://codereview.chromium.org/2304073003/diff/120001/device/time/public/int...
device/time/public/interfaces/time_zone_monitor.mojom:7: interface
TimeZoneMonitor {
if we don't forsee any other methods in this interface for generic "time"
service, then the code should live in device/time_zone_monitor to make the
directory name clearer

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

https://codereview.chromium.org/2304073003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2304073003/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1243:
BrowserMainLoop::GetInstance()->time_zone_monitor())));
With some hand-waving, I can see how the use of base::Unretained() are safe
(they refer to members or |this|), but I'm not sure how this one is safe. What's
the relationship between RPH and BrowserMainLoop?

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
File content/browser/time_zone_monitor.cc (right):

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
content/browser/time_zone_monitor.cc:21: bindings_.CloseAllBindings();
I thought this would implicitly happen as part of the BindingSet going away. How
come that's not true here?

[blundell, 2016-09-07 13:30:20]

Description was changed from

==========
Mojoify time zone update IPC from browser to renderer

To start the servicification of //content's time zone monitoring code,
this CL Mojoifies the ViewMsg_TimezoneChange message sent from the browser
to the renderer. To do so, we introduce the TimeZoneMonitor Mojo interface
(living in //device, where the implementation will move to), and change
content::TimeZoneMonitor to implement the interface. Rather than notifying
all RenderProcessHosts on a time zone change, content::TimeZoneMonitor
maintains connections to the clients (RenderThreadImpl in this case)
and notifies them directly on a time zone change. This CL uses the
polling pattern described here:
https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/pSNDDN3gpFo
for consistency with the way that the geolocation and battery monitor
interfaces work (in practice, it's unlikely that time zone change updates
would flood the client, of course).

A followup CL will rename content::TimeZoneMonitor to its now-correct name of
TimeZoneMonitorImpl. Further followup will decouple that code from //content
and move it to //device.

BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
     and then click "recheck" to ensure that the renderer picks up the new
     time zone. Don't reload the page, which is likely to give you a new
     renderer process, use the "recheck" link on the page.
==========

to

==========
Mojoify time zone update IPC from browser to renderer

To start the servicification of //content's time zone monitoring code,
this CL Mojoifies the ViewMsg_TimezoneChange message sent from the browser
to the renderer. To do so, we introduce the TimeZoneMonitor Mojo interface
(living in //device, where the implementation will move to), and change
content::TimeZoneMonitor to implement the interface. Rather than notifying
all RenderProcessHosts on a time zone change, content::TimeZoneMonitor
maintains connections to the clients (RenderThreadImpl in this case)
and notifies them directly on a time zone change. This CL uses the
polling pattern described here:
https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/pSNDDN3gpFo
for consistency with the way that the geolocation and battery monitor
interfaces work (in practice, it's unlikely that time zone change updates
would flood the client, of course).

Followup will decouple content::TimeZoneMonitor from //content and move it to //
device.

BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
     and then click "recheck" to ensure that the renderer picks up the new
     time zone. Don't reload the page, which is likely to give you a new
     renderer process, use the "recheck" link on the page.
==========

[blundell, 2016-09-07 14:00:29]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 14:00:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[blundell, 2016-09-07 14:35:11]

Patchset #8 (id:140001) has been deleted

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

Thanks for the reviews!

dcheng@: PTAL.
rockot@: need a review from you as well.

https://codereview.chromium.org/2304073003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2304073003/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1243:
BrowserMainLoop::GetInstance()->time_zone_monitor())));
On 2016/09/06 22:20:38, dcheng wrote:
> With some hand-waving, I can see how the use of base::Unretained() are safe
> (they refer to members or |this|), but I'm not sure how this one is safe.
What's
> the relationship between RPH and BrowserMainLoop?

OK, I had to dig into this to respond to your question and validate my intuition
that this logically "should be" safe. It is unfortunately subtle:

BrowserMainLoop::ShutdownThreadsAndCleanUp() deletes its
mojo::edk::ScopedIPCSupport instance. The header comment on the latter says that
it "shuts down Mojo on destruction". So after the BrowserMainLoop is destroyed
(which is when its TimeZoneMonitor instance is deleted), there should not be any
incoming Mojo connection requests by design. Thoughts?

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
File content/browser/time_zone_monitor.cc (right):

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
content/browser/time_zone_monitor.cc:21: bindings_.CloseAllBindings();
On 2016/09/06 22:20:38, dcheng wrote:
> I thought this would implicitly happen as part of the BindingSet going away.
How
> come that's not true here?

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.

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
File content/browser/time_zone_monitor.h (right):

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
content/browser/time_zone_monitor.h:33: // TODO(blundell): Update this class to
be named TimeZoneMonitorImpl.
On 2016/09/06 22:11:18, jam wrote:
> nit: btw no need to do that. the reason interfaces are in the "mojom"
> subnamespace is that the impls don't need to end in Impl (that's the reason
> given for that mojom intermediate namespace)

Ah convenient.

https://codereview.chromium.org/2304073003/diff/120001/device/time/public/int...
File device/time/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2304073003/diff/120001/device/time/public/int...
device/time/public/interfaces/BUILD.gn:1: # Copyright 2016 The Chromium Authors.
All rights reserved.
On 2016/09/06 22:11:18, jam wrote:
> nit: add an owner for device/time_zone_montior

Will ask mark@ once he's back in the office next week.

https://codereview.chromium.org/2304073003/diff/120001/device/time/public/int...
File device/time/public/interfaces/time_zone_monitor.mojom (right):

https://codereview.chromium.org/2304073003/diff/120001/device/time/public/int...
device/time/public/interfaces/time_zone_monitor.mojom:7: interface
TimeZoneMonitor {
On 2016/09/06 22:11:18, jam wrote:
> if we don't forsee any other methods in this interface for generic "time"
> service, then the code should live in device/time_zone_monitor to make the
> directory name clearer

I think that sooner or later we'll need the POSIX-specific method to get
localtime() (as we discussed in the design doc). I'm not sure when I'll get to
that though, so I'm happy to rename to what makes sense now and it's easy enough
to change later if/when appropriate.

[blundell, 2016-09-07 14:40:08]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 14:40:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-09-07 14:54:40]

LGTM with optional mojom change

https://codereview.chromium.org/2304073003/diff/160001/device/time_zone_monit...
File device/time_zone_monitor/public/interfaces/time_zone_monitor.mojom (right):

https://codereview.chromium.org/2304073003/diff/160001/device/time_zone_monit...
device/time_zone_monitor/public/interfaces/time_zone_monitor.mojom:7: interface
TimeZoneMonitor {
Optional nit: I don't think the concerns which originally motivated this pattern
are valid anymore, so it might be better to use a simple push interface:

  interface TimeZoneMonitor {
    AddClient(TimeZoneMonitorClient);
  };

  interface TimeZoneMonitorClient {
    OnTimeZoneChange(string tz_info);
  };

My rationale:

1. If a specific interface client is misbehaving and not reading messages off
its message pipe, it doesn't matter. The unread messages still are queued on the
receiver's side, not the sender's side, so the worst that can happen is the
client runs itself OOM.

2. If a client is very badly misbehaving and preventing its local EDK from even
reading messages off the underlying system IPC pipe from the browser, then it
CAN cause buffering to build up on the browser side, but that can happen no
matter how we design our mojom interfaces, and we can develop ways to detect
that and kill bad processes in that case.

3. I'm pretty sure everyone I've spoken with about it agrees that the polling
pattern is awkward to build and use, so given #1 and #2 we should just avoid it
moving forward.

WDYT?

[commit-bot: I haz the power, 2016-09-07 15:55:30]

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

[commit-bot: I haz the power, 2016-09-07 15:55:31]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-09-07 16:33:27]

On 2016/09/07 14:54:40, Ken Rockot wrote:
> LGTM with optional mojom change
> 
>
https://codereview.chromium.org/2304073003/diff/160001/device/time_zone_monit...
> File device/time_zone_monitor/public/interfaces/time_zone_monitor.mojom
(right):
> 
>
https://codereview.chromium.org/2304073003/diff/160001/device/time_zone_monit...
> device/time_zone_monitor/public/interfaces/time_zone_monitor.mojom:7:
interface
> TimeZoneMonitor {
> Optional nit: I don't think the concerns which originally motivated this
pattern
> are valid anymore, so it might be better to use a simple push interface:
> 
>   interface TimeZoneMonitor {
>     AddClient(TimeZoneMonitorClient);
>   };
> 
>   interface TimeZoneMonitorClient {
>     OnTimeZoneChange(string tz_info);
>   };
> 
> My rationale:
> 
> 1. If a specific interface client is misbehaving and not reading messages off
> its message pipe, it doesn't matter. The unread messages still are queued on
the
> receiver's side, not the sender's side, so the worst that can happen is the
> client runs itself OOM.
> 
> 2. If a client is very badly misbehaving and preventing its local EDK from
even
> reading messages off the underlying system IPC pipe from the browser, then it
> CAN cause buffering to build up on the browser side, but that can happen no
> matter how we design our mojom interfaces, and we can develop ways to detect
> that and kill bad processes in that case.
> 
> 3. I'm pretty sure everyone I've spoken with about it agrees that the polling
> pattern is awkward to build and use, so given #1 and #2 we should just avoid
it
> moving forward.
> 
> WDYT?

Do you mind raising this issue on chromium-mojo@? I think it would be good to
cast a wider net for making this decision as a default policy, and landing this
CL isn't time-critical in any case. Personally I definitely find the polling
pattern harder to read as it's basically an inversion of the flow you expect.
I'm not really sure about your (1) as an argument though; I would think that
there could be legitimate circumstances where the producer generates messages at
a significantly faster rate than the client can produce them.

[dcheng, 2016-09-07 20:59:28]

LGTM, though I'd personally prefer to just see the fields re-ordered

(Having dependencies on field ordering is not /that/ surprising or fragile IMO;
for example, we do this for WeakPtrFactory)

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
File content/browser/time_zone_monitor.cc (right):

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
content/browser/time_zone_monitor.cc:21: bindings_.CloseAllBindings();
On 2016/09/07 14:40:05, blundell wrote:
> On 2016/09/06 22:20:38, dcheng wrote:
> > I thought this would implicitly happen as part of the BindingSet going away.
> How
> > come that's not true here?
> 
> 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.

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 13:17:33]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 13:17:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 14:22:53]

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

[commit-bot: I haz the power, 2016-09-12 14:22:55]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

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

Hey Ken,

I put the CL in the push model but as a result time_zone_monitor.cc has become
IMO clunky with the addition of the management of the clients. I feel like
there's a more idiomatic way to implement this that I'm missing. Thoughts?

Thanks,

Colin

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
File content/browser/time_zone_monitor.cc (right):

https://codereview.chromium.org/2304073003/diff/120001/content/browser/time_z...
content/browser/time_zone_monitor.cc:21: bindings_.CloseAllBindings();
On 2016/09/07 20:59:27, dcheng wrote:
> On 2016/09/07 14:40:05, blundell wrote:
> > On 2016/09/06 22:20:38, dcheng wrote:
> > > I thought this would implicitly happen as part of the BindingSet going
away.
> > How
> > > come that's not true here?
> > 
> > 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.
> 
> 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.

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.

[Ken Rockot(use gerrit already), 2016-09-12 16:30:34]

I see. Seems reasonable that we would add something like an InterfacePtrSet for
this sort of usage. I know I've wanted something like it in the past. I'll
upload a CL in a bit.

[blundell, 2016-09-13 09:48:01]

Patchset #9 (id:180001) has been deleted

[blundell, 2016-09-13 09:50:00]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 09:50:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 11:08:54]

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

[commit-bot: I haz the power, 2016-09-13 11:08:56]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-09-13 11:48:36]

On 2016/09/12 16:30:34, Ken Rockot wrote:
> I see. Seems reasonable that we would add something like an InterfacePtrSet
for
> this sort of usage. I know I've wanted something like it in the past. I'll
> upload a CL in a bit.

Whoops, InterfacePtrSet totally already exists! CL is fully updated now to use
the push or pull model, making it a significantly different CL.

jam@, rockot@: Could you do full re-reviews?
dcheng@: Could you do a security review again as the mojom file is now
different?

[blundell, 2016-09-13 11:49:18]

On 2016/09/13 11:48:36, blundell wrote:
> On 2016/09/12 16:30:34, Ken Rockot wrote:
> > I see. Seems reasonable that we would add something like an InterfacePtrSet
> for
> > this sort of usage. I know I've wanted something like it in the past. I'll
> > upload a CL in a bit.
> 
> Whoops, InterfacePtrSet totally already exists! CL is fully updated now to use
> the push or pull model, making it a significantly different CL.

heh, that was supposed to be "push instead of pull model", although the "push or
pull model" sounds appealing.

[blundell, 2016-09-13 11:50:38]

blundell@chromium.org changed reviewers:
+ mark@chromium.org

[blundell, 2016-09-13 11:50:39]

Mark, this CL is preparing the split of time zone monitor code out of //content
into //device/time_zone_monitor. You seem like the best OWNER for the latter
directory. Could you review //device/time_zone_monitor/OWNERS? Thanks!

[Ken Rockot(use gerrit already), 2016-09-13 15:27:03]

lgtm

https://codereview.chromium.org/2304073003/diff/200001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2304073003/diff/200001/content/renderer/rende...
content/renderer/render_thread_impl.cc:860:
GetRemoteInterfaces()->GetInterface(mojo::GetProxy(&time_zone_monitor_));
nit: No need for time_zone_monitor_ to be owned by RenderThreadImpl or persisted
beyond the scope of this call.

[Mark Mentovai, 2016-09-13 15:33:55]

LGTM

https://codereview.chromium.org/2304073003/diff/200001/device/time_zone_monit...
File device/time_zone_monitor/OWNERS (right):

https://codereview.chromium.org/2304073003/diff/200001/device/time_zone_monit...
device/time_zone_monitor/OWNERS:1: mark@chromium.org
One-OWNER files aren’t great. Care to add yourself?

[blundell, 2016-09-14 09:35:14]

Thanks!

jam@, dcheng@: ping :).

https://codereview.chromium.org/2304073003/diff/200001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2304073003/diff/200001/content/renderer/rende...
content/renderer/render_thread_impl.cc:860:
GetRemoteInterfaces()->GetInterface(mojo::GetProxy(&time_zone_monitor_));
On 2016/09/13 15:27:03, Ken Rockot wrote:
> nit: No need for time_zone_monitor_ to be owned by RenderThreadImpl or
persisted
> beyond the scope of this call.

Done.

https://codereview.chromium.org/2304073003/diff/200001/device/time_zone_monit...
File device/time_zone_monitor/OWNERS (right):

https://codereview.chromium.org/2304073003/diff/200001/device/time_zone_monit...
device/time_zone_monitor/OWNERS:1: mark@chromium.org
On 2016/09/13 15:33:55, Mark Mentovai wrote:
> One-OWNER files aren’t great. Care to add yourself?

Done.

[jam, 2016-09-15 16:12:11]

https://codereview.chromium.org/2304073003/diff/220001/device/time_zone_monit...
File device/time_zone_monitor/public/interfaces/time_zone_monitor.mojom (right):

https://codereview.chromium.org/2304073003/diff/220001/device/time_zone_monit...
device/time_zone_monitor/public/interfaces/time_zone_monitor.mojom:11:
AddClient(TimeZoneMonitorClient client);
nit: do we really need two interfaces? Why not just have just this one, i.e.

RegisterForTimeZoneChanges() => (string tz_info)

[jam, 2016-09-16 00:19:44]

lgtm

On 2016/09/15 16:12:11, jam wrote:
>
https://codereview.chromium.org/2304073003/diff/220001/device/time_zone_monit...
> File device/time_zone_monitor/public/interfaces/time_zone_monitor.mojom
(right):
> 
>
https://codereview.chromium.org/2304073003/diff/220001/device/time_zone_monit...
> device/time_zone_monitor/public/interfaces/time_zone_monitor.mojom:11:
> AddClient(TimeZoneMonitorClient client);
> nit: do we really need two interfaces? Why not just have just this one, i.e.
> 
> RegisterForTimeZoneChanges() => (string tz_info)

Ken reminded me that the callbacks for message calls can only be called once.

[dcheng, 2016-09-16 04:42:16]

lgtm

[blundell, 2016-09-16 08:13:17]

The CQ bit was checked by blundell@chromium.org

[blundell, 2016-09-16 08:13:18]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2304073003/#ps220001
(title: "Response to reviews")

[commit-bot: I haz the power, 2016-09-16 08:13:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 08:15:45]

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

[commit-bot: I haz the power, 2016-09-16 08:15:46]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[blundell, 2016-09-16 09:55:23]

The CQ bit was checked by blundell@chromium.org

[blundell, 2016-09-16 09:55:24]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
rockot@chromium.org, jam@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2304073003/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-16 09:55:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 10:44:02]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2016-09-16 10:45:53]

Description was changed from

==========
Mojoify time zone update IPC from browser to renderer

To start the servicification of //content's time zone monitoring code,
this CL Mojoifies the ViewMsg_TimezoneChange message sent from the browser
to the renderer. To do so, we introduce the TimeZoneMonitor Mojo interface
(living in //device, where the implementation will move to), and change
content::TimeZoneMonitor to implement the interface. Rather than notifying
all RenderProcessHosts on a time zone change, content::TimeZoneMonitor
maintains connections to the clients (RenderThreadImpl in this case)
and notifies them directly on a time zone change. This CL uses the
polling pattern described here:
https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/pSNDDN3gpFo
for consistency with the way that the geolocation and battery monitor
interfaces work (in practice, it's unlikely that time zone change updates
would flood the client, of course).

Followup will decouple content::TimeZoneMonitor from //content and move it to //
device.

BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
     and then click "recheck" to ensure that the renderer picks up the new
     time zone. Don't reload the page, which is likely to give you a new
     renderer process, use the "recheck" link on the page.
==========

to

==========
Mojoify time zone update IPC from browser to renderer

To start the servicification of //content's time zone monitoring code,
this CL Mojoifies the ViewMsg_TimezoneChange message sent from the browser
to the renderer. To do so, we introduce the TimeZoneMonitor Mojo interface
(living in //device, where the implementation will move to), and change
content::TimeZoneMonitor to implement the interface. Rather than notifying
all RenderProcessHosts on a time zone change, content::TimeZoneMonitor
maintains connections to the clients (RenderThreadImpl in this case)
and notifies them directly on a time zone change. This CL uses the
polling pattern described here:
https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/pSNDDN3gpFo
for consistency with the way that the geolocation and battery monitor
interfaces work (in practice, it's unlikely that time zone change updates
would flood the client, of course).

Followup will decouple content::TimeZoneMonitor from //content and move it to //
device.

BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
     and then click "recheck" to ensure that the renderer picks up the new
     time zone. Don't reload the page, which is likely to give you a new
     renderer process, use the "recheck" link on the page.

Committed: https://crrev.com/c00adf442e48581afa7df022a0e84c507d1ff3e3
Cr-Commit-Position: refs/heads/master@{#419140}
==========

[commit-bot: I haz the power, 2016-09-16 10:45:54]

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