Persist 'this device is not managed' information

Persist 'this device is not managed' information

This is necessary backend work for fixing bug 64314 (make login wait only for the first registration check).
It also enables subsequent polling of the DM server at defined intervals (rather than at each login, or never).

BUG=64314
TEST=unit tests: DeviceManagementPolicyProviderTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67530

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67535

[Jakob Kummerow, 2010-11-25 14:03:09]

Currently this CL is based on CL 5198006 patch set 9; I will rebase as
necessary.

[Mattias Nissler (ping if slow), 2010-11-25 16:12:36]

I'm still thinking that creating another file is not the right answer, but I
won't complain if the other reviewers feel it's the right thing to do.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider_unittest.cc:56: void
CreateNewProviderDontRunPending() {
Hm, maybe it's worth to just have one of them and make the caller do the
RunAllPending() call when required?

And if not, I'd rather change the names so this function is called
CreateNewProvider and the other one CreateNewProviderAndRunPending.

[Jakob Kummerow, 2010-11-25 16:28:45]

On 2010/11/25 16:12:36, Mattias Nissler wrote:
> I'm still thinking that creating another file is not the right answer, but I
> won't complain if the other reviewers feel it's the right thing to do.

Let me emphasize that we don't really have "another file", rather we have either
the one or the other. So always having the same file (in terms of filename) but
with alternating content wouldn't make much of a difference IMO.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider_unittest.cc:56: void
CreateNewProviderDontRunPending() {
On 2010/11/25 16:12:36, Mattias Nissler wrote:
> Hm, maybe it's worth to just have one of them and make the caller do the
> RunAllPending() call when required?
> 
> And if not, I'd rather change the names so this function is called
> CreateNewProvider and the other one CreateNewProviderAndRunPending.

Done.
I went for the first option -- only provide one method, which doesn't include
RunAllPending().

[Mattias Nissler (ping if slow), 2010-11-25 16:34:49]

On 2010/11/25 16:28:45, jkummerow wrote:
> On 2010/11/25 16:12:36, Mattias Nissler wrote:
> > I'm still thinking that creating another file is not the right answer, but I
> > won't complain if the other reviewers feel it's the right thing to do.
> 
> Let me emphasize that we don't really have "another file", rather we have
either
> the one or the other. So always having the same file (in terms of filename)
but
> with alternating content wouldn't make much of a difference IMO.

You are right in that their existence is exclusive. What I meant by "another
file" is that we now have code for writing three different files although we're
keeping state for more or less a single component. That just feels wrong to me.
We have three different implementations for
"persist-this-piece-to-a-file-and-read-it-back", where it should be perfectly
possible to at least keep the (admittedtly suboptimal) current situation where
we have only two.

> 
>
http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
> File chrome/browser/policy/device_management_policy_provider_unittest.cc
> (right):
> 
>
http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
> chrome/browser/policy/device_management_policy_provider_unittest.cc:56: void
> CreateNewProviderDontRunPending() {
> On 2010/11/25 16:12:36, Mattias Nissler wrote:
> > Hm, maybe it's worth to just have one of them and make the caller do the
> > RunAllPending() call when required?
> > 
> > And if not, I'd rather change the names so this function is called
> > CreateNewProvider and the other one CreateNewProviderAndRunPending.
> 
> Done.
> I went for the first option -- only provide one method, which doesn't include
> RunAllPending().

[danno, 2010-11-25 16:35:01]

drive by review.... vroooom!

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:37: const char*
kUnmanagedDeviceMarkerFilename = "UnmanagedDevice";
It occurs to me that the information in this file could be nicely put into the
policy cache, and it seems like that is almost a logical place for it. I think
folding it into the the policy boolean PB would save a lot of code here, since
the lifecycles are tied anyway.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:221: if
(policy_request_pending_)
Why reverse this? Seems like an unrelated change.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:273: #if
defined(OS_CHROMEOS)
Do you need this #define here? sending the notification if there are not any
listeners is harmless, and it makes sure this code gets compiled on all
platforms.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:276:
NotifyCloudPolicyUpdate();
Would it be possible to create a specific observer for this even in the same
style as the token fetcher? Given that the notifications are tied to the
profile/provider, I think this would be cleaner, it doesn't require the listener
to know which provider he's expecting the notification from in the "Observe"
method.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.h:135:
unmanaged_device_refresh_rate_ms_ = unmanaged_device_refresh_rate_ms;
Can you use a new private/protected constructor for these that's only called by
the tests? I think I'd like that better.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider_unittest.cc:51: void
CreateNewProvider() {
Probably would be clearer to have:
CreateNewProvider --> just creates provider
CreateNewProviderRunPending --> calls CreateNewProvider and runs all.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/proto/dev...
File chrome/browser/policy/proto/device_management_local.proto (right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/proto/dev...
chrome/browser/policy/proto/device_management_local.proto:31: }
Are you sure you can't fold this into the cache?

[Jakob Kummerow, 2010-11-25 16:54:24]

Quick answers to some of Danno's comments. 

Due to popular demand, I'll give embedding the 'unmanaged' flag into the policy
cache's existing file a shot. I'm not convinced that that'll turn out to be any
more elegant than the current solution, but we'll see.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:221: if
(policy_request_pending_)
On 2010/11/25 16:35:01, danno wrote:
> Why reverse this? Seems like an unrelated change.

It is indeed unrelated and merely a formatting change to reduce indentation. We
recently had the "Boyscout Rule" in the bathroom, remember? ;-)

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:273: #if
defined(OS_CHROMEOS)
On 2010/11/25 16:35:01, danno wrote:
> Do you need this #define here? sending the notification if there are not any
> listeners is harmless, and it makes sure this code gets compiled on all
> platforms.
This is Markus' code, I only moved it. Battle it out with him ;-)

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:276:
NotifyCloudPolicyUpdate();
On 2010/11/25 16:35:01, danno wrote:
> Would it be possible to create a specific observer for this even in the same
> style as the token fetcher? Given that the notifications are tied to the
> profile/provider, I think this would be cleaner, it doesn't require the
listener
> to know which provider he's expecting the notification from in the "Observe"
> method.

Same as above -- not my code. I'm just keeping the interface intact. Apparently
a major restructuring of this particular notification mechanism is on the
horizon anyway, but will probably have to wait until after the branching on
Monday.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider_unittest.cc:51: void
CreateNewProvider() {
On 2010/11/25 16:35:01, danno wrote:
> Probably would be clearer to have:
> CreateNewProvider --> just creates provider
> CreateNewProviderRunPending --> calls CreateNewProvider and runs all.

Done. See Mattias' comment and my reply.

[markusheintz_, 2010-11-25 17:49:44]

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.h:60: // for the first
attempt to fetch policies to complete.
Should we mention that the method also returns true even if the request wasn't
issued yet? This is the case right?

[Jakob Kummerow, 2010-11-26 09:17:23]

Major restructuring: 
• No standalone "UnmanagedDevice" file any more, instead the cache stores this
information. 
• No more accessors to refresh time, instead a private constructor for the
policy provider for use by test cases.

Happy, now, are you?

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:37: const char*
kUnmanagedDeviceMarkerFilename = "UnmanagedDevice";
On 2010/11/25 16:35:01, danno wrote:
> It occurs to me that the information in this file could be nicely put into the
> policy cache, and it seems like that is almost a logical place for it. I think
> folding it into the the policy boolean PB would save a lot of code here, since
> the lifecycles are tied anyway.

Done.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.h:60: // for the first
attempt to fetch policies to complete.
On 2010/11/25 17:49:44, markusheintz_ wrote:
> Should we mention that the method also returns true even if the request wasn't
> issued yet? This is the case right?

Strictly speaking, yes. But the request is issued immediately (that is, as soon
as the IOThread is available). Also, I'd argue that the current formulation
covers that situation. But we can make it more explicit if you wish.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.h:135:
unmanaged_device_refresh_rate_ms_ = unmanaged_device_refresh_rate_ms;
On 2010/11/25 16:35:01, danno wrote:
> Can you use a new private/protected constructor for these that's only called
by
> the tests? I think I'd like that better.

Done.

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/proto/dev...
File chrome/browser/policy/proto/device_management_local.proto (right):

http://codereview.chromium.org/5331008/diff/1/chrome/browser/policy/proto/dev...
chrome/browser/policy/proto/device_management_local.proto:31: }
On 2010/11/25 16:35:01, danno wrote:
> Are you sure you can't fold this into the cache?

Quite the contrary: I'm sure I can if I decide I want to :-)

Done.

[Mattias Nissler (ping if slow), 2010-11-26 10:37:45]

Looks much butter, but I'm not happy enough yet!

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_cache.cc (right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_cache.cc:155:
(is_device_unmanaged ? NULL
Does that mean that when I call SetDeviceUnmanaged(false), we'll overwrite any
existing policy? Is that correct?

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_cache.h (right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_cache.h:47: void
SetDeviceUnmanaged(bool is_device_unmanaged);
Can you swap these? Most classes I have seen have CamelCaseFunction before
under_score_accessors.

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:16: #include
"chrome/browser/policy/proto/device_management_local.pb.h"
do you still need this header?

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:224: }
Why should we check for the file here? Shouldn't we rather ask the cache whether
it has policy available?

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:246: return;
I like newlines after early returns, but that's a matter of taste.

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:332: return
waiting_for_initial_policies_;
would this be equivalent to !cache->IsUnmanaged() && cache->HasPolicy()? That
way, we wouldn't have to track the state here explicitly.

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.h:82: int64
unmanaged_device_refresh_rate_ms);
9 parameters are 5 too many IMHO :) What was wrong about the setters?

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:15: #include
"chrome/browser/policy/proto/device_management_local.pb.h"
What do you need this header for?

[danno, 2010-11-26 10:48:36]

LGTM.

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:332: return
waiting_for_initial_policies_;
I almost gave the same feedback last time, but nope, it also handes the error
case where the policy fetch fails.

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.h:82: int64
unmanaged_device_refresh_rate_ms);
On 2010/11/26 10:37:45, Mattias Nissler wrote:
> 9 parameters are 5 too many IMHO :) What was wrong about the setters?

I disagree, this is testing-specific code that centralizes the
addition/specification of testing-only parameters in one place. It restricts the
usage of them, I find it clearer and more compact than the setters. Plus, there
was already a testing constructor, it seems reasonable to unify the
testing-usage paradigm.

[Jakob Kummerow, 2010-11-26 11:07:42]

Mattias: more to your taste?

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_cache.cc (right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_cache.cc:155:
(is_device_unmanaged ? NULL
On 2010/11/26 10:37:45, Mattias Nissler wrote:
> Does that mean that when I call SetDeviceUnmanaged(false), we'll overwrite any
> existing policy? Is that correct?

Not quite. When the device was unmanaged before, there are no existing policies
we could possibly overwrite. If, however, the device was already managed before
such a call, the method terminates immediately, so nothing is overwritten
either.

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_cache.h (right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_cache.h:47: void
SetDeviceUnmanaged(bool is_device_unmanaged);
On 2010/11/26 10:37:45, Mattias Nissler wrote:
> Can you swap these? Most classes I have seen have CamelCaseFunction before
> under_score_accessors.

Done.

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:16: #include
"chrome/browser/policy/proto/device_management_local.pb.h"
On 2010/11/26 10:37:45, Mattias Nissler wrote:
> do you still need this header?

No, I don't. Good catch. Done.

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:224: }
On 2010/11/26 10:37:45, Mattias Nissler wrote:
> Why should we check for the file here? Shouldn't we rather ask the cache
whether
> it has policy available?

The idea is that the existence of a device token is a surer sign of being a
managed device than the presence of non-null cached policies (e.g. fetching
policies might have failed even if fetching the token succeeded).

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:246: return;
On 2010/11/26 10:37:45, Mattias Nissler wrote:
> I like newlines after early returns, but that's a matter of taste.

Done.

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.h:82: int64
unmanaged_device_refresh_rate_ms);
On 2010/11/26 10:37:45, Mattias Nissler wrote:
> 9 parameters are 5 too many IMHO :) What was wrong about the setters?

(Discussed offline.)
One concrete problem with the setters was that they didn't allow to overwrite a
setting that's already used within the constructor/Initialize(), forcing a very
ugly workaround in one of the test cases.

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/5331008/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:15: #include
"chrome/browser/policy/proto/device_management_local.pb.h"
On 2010/11/26 10:37:45, Mattias Nissler wrote:
> What do you need this header for?

Done.
(Leftover from the earlier ugly workaround mentioned elsewhere.)

[Mattias Nissler (ping if slow), 2010-11-26 14:01:40]

No more complaints :) LGTM.

[markusheintz_, 2010-11-26 14:04:27]

LGTM

[Mattias Nissler (ping if slow), 2010-11-26 15:07:30]

fixup all style violations and you're good to go!

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_cache.cc (right):

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_cache.cc:30: : path_(path),
fix indentation.

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_cache.cc:33:
is_device_unmanaged_(is_device_unmanaged) {
You may move the closing brace to the end of this line.

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:201: 
whitespace error?

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:249: return;
remove tab.

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:43:
loop_.RunAllPending();
indentation?

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:57: 
whitespace.

[Jakob Kummerow, 2010-11-26 16:04:02]

Whitespace fixed. 
As discussed offline, I also removed the "#ifdef defined(OS_CHROMEOS)" checks,
so that all of the code is now compiled on all platforms.

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_cache.cc (right):

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_cache.cc:30: : path_(path),
On 2010/11/26 15:07:30, Mattias Nissler wrote:
> fix indentation.

Done.

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_cache.cc:33:
is_device_unmanaged_(is_device_unmanaged) {
On 2010/11/26 15:07:30, Mattias Nissler wrote:
> You may move the closing brace to the end of this line.

Done.

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:201: 
On 2010/11/26 15:07:30, Mattias Nissler wrote:
> whitespace error?

Done.

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:249: return;
On 2010/11/26 15:07:30, Mattias Nissler wrote:
> remove tab.

Done.

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:43:
loop_.RunAllPending();
On 2010/11/26 15:07:30, Mattias Nissler wrote:
> indentation?

Done.

http://codereview.chromium.org/5331008/diff/25001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:57: 
On 2010/11/26 15:07:30, Mattias Nissler wrote:
> whitespace.

Done.

[markusheintz_, 2010-11-26 16:17:02]

still LGTM

[Jakob Kummerow, 2010-11-29 15:10:23]

I had to revert this because it didn't compile on Windows. I fixed that, the
Windows try job is already at browser_tests (Mac and Linux ran without problems
anyway). 

Please give me the thumbs-up to land this again (of course I'll wait until the
try job is completely finished).

[danno, 2010-11-29 15:17:07]

LGTM (pending bots) :-)