Disable easter egg on enterprised enrolled devices

Disable easter egg on enterprised enrolled devices

BUG=462221
Committed: https://crrev.com/7db1c9eae940f6f35f764ad2d107a6b28a71ceda
Cr-Commit-Position: refs/heads/master@{#322167}

[edwardjung, 2015-03-10 14:57:30]

edwardjung@chromium.org changed reviewers:
+ atwilson@chromium.org

[edwardjung, 2015-03-10 14:57:30]

Hi Drew, 

Could you take a look at this CL before I send it out wider.

I've used the flag method you suggested in the bug to get the enrolled status
exposed to the network error page. It works well.

Thanks for your help,

Edward

[Andrew T Wilson (Slow), 2015-03-10 15:31:58]

mostly LG with a few nits/questions - can we add a test?

https://codereview.chromium.org/977323003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/977323003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1416: #else
nit: I prefer a positive check (#if defined(OS_CHROMEOS)) to a negative check.
Any chance that you could swap the order of these two clauses and make this a
#if defined(OS_CHROMEOS))?

https://codereview.chromium.org/977323003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1418: bool isEnterpriseManaged =
g_browser_process->platform_part()->
nit: bool isEnterpriseManaged -> const bool is_enterprise_managed

https://codereview.chromium.org/977323003/diff/1/chrome/renderer/resources/ne...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/977323003/diff/1/chrome/renderer/resources/ne...
chrome/renderer/resources/neterror.js:67: window.Runner = '';
What does this do?

[edwardjung, 2015-03-10 18:12:28]

Thanks Andrew,

With regard to the unit test (I'm fairly new to this) should I be creating one
in chrome_content_renderer_client_unittest.cc 

something like this or is there a better place for a Chrome OS specific test?

#if defined(OS_CHROMEOS)
TEST_F(ChromeContentRendererClientTest, ShouldDisableEasterEgg) {
  const base::CommandLine& browser_command_line =
      *base::CommandLine::ForCurrentProcess();

  EXPECT_TRUE(browser_command_line.HasSwitch(
              switches::kDisableDinosaurEasterEgg));
}
#endif

https://codereview.chromium.org/977323003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/977323003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1416: #else
On 2015/03/10 15:31:57, Andrew T Wilson wrote:
> nit: I prefer a positive check (#if defined(OS_CHROMEOS)) to a negative check.
> Any chance that you could swap the order of these two clauses and make this a
> #if defined(OS_CHROMEOS))?

Done.

https://codereview.chromium.org/977323003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1418: bool isEnterpriseManaged =
g_browser_process->platform_part()->
On 2015/03/10 15:31:58, Andrew T Wilson wrote:
> nit: bool isEnterpriseManaged -> const bool is_enterprise_managed

Done.

https://codereview.chromium.org/977323003/diff/1/chrome/renderer/resources/ne...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/977323003/diff/1/chrome/renderer/resources/ne...
chrome/renderer/resources/neterror.js:67: window.Runner = '';
On 2015/03/10 15:31:58, Andrew T Wilson wrote:
> What does this do?

This removes the reference to the Runner class which means you can't just
instantiate the easter egg from the dev tools console with: new
Runner('.interstitial-wrapper')

[edwardjung, 2015-03-11 19:09:48]

Drew, 

I taken a stab at a test, could you have a look. It's obviously wrong in it's
current state.

How would I go about mocking the isEnterpriseManaged method to return true so
that the flag is set.

Thanks.

https://codereview.chromium.org/977323003/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/977323003/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client_browsertest.cc:118: // const bool
is_enterprise_managed = connector->IsEnterpriseManaged();
This doesn't compile as there is no reference to  g_browser_process. Any ideas
on getting the IsEnterpriseManaged status?

[Andrew T Wilson (Slow), 2015-03-12 15:44:38]

https://codereview.chromium.org/977323003/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/977323003/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client_browsertest.cc:118: // const bool
is_enterprise_managed = connector->IsEnterpriseManaged();
On 2015/03/11 19:09:48, edwardjung wrote:
> This doesn't compile as there is no reference to  g_browser_process. Any ideas
> on getting the IsEnterpriseManaged status?

Have you tried including browser_process.h?

Take a look at preferences_browsertest.cc - there's a call to
SetInstallAttributesForTesting() which is making IsEnterpriseManaged() return
true.

[Andrew T Wilson (Slow), 2015-03-13 10:16:07]

Also, feedback from glen@ on the bug was spot-on -- we should notify the user
when he tries to activate the easter egg that "Fun disabled by administrator".
Otherwise, it just looks like the easter egg is broken. Is this hard to add?

[edwardjung, 2015-03-13 11:17:55]

Thanks for the pointers. IsEnterpriseManaged() is returning true in the test
from the stub. However the disable flag isn't set. I get a bunch of errors when
running the test:

[31080:31080:0313/111354:WARNING:chrome_browser_main_chromeos.cc(289)] Running
as stub user with profile dir: test-user
Xlib:  extension "RANDR" missing on display ":99".
[31080:31110:0313/111355:ERROR:owner_key_util_impl.cc(27)] Could not get size of
/tmp/.org.chromium.Chromium.mqHMoa/d15uQwf/stub_owner.key
[31080:31110:0313/111355:WARNING:local_extension_cache.cc(260)] Extensions will
not be installed from update URLs until /var/cache/external_cache/.initialized
exists.
[31080:31080:0313/111355:WARNING:child_account_service.cc(345)] User instance
wasn't found while setting child account flag.
[31080:31080:0313/111355:ERROR:device_cloud_policy_store_chromeos.cc(185)]
Device policy read on enrolled device yields no DM token! Status: 1.
[31080:31097:0313/111355:WARNING:freezer_cgroup_process_manager.cc(58)] Cgroup
freezer does not exist or is not writable. Unable to freeze renderer processes.

Seems to indicate that I am incorrectly setting the device policy.

In the meantime I'll make the front end changes that Glen suggested.

[Andrew T Wilson (Slow), 2015-03-13 13:44:57]

https://codereview.chromium.org/977323003/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/977323003/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client_browsertest.cc:136:
EXPECT_TRUE(browser_command_line.HasSwitch(
This will fail, because you aren't setting the switch on the browser command
line, but rather on the command line of any renderers being created.

https://codereview.chromium.org/977323003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/977323003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:67: delete Runner;
Put a comment here describing while you're doing this. I'd actually suggest that
you don't actually need to delete the Runner() class, because admins can disable
dev tools if they want to work around this. But I'm fine with deleting it as
well.

[edwardjung, 2015-03-13 16:09:06]

https://codereview.chromium.org/977323003/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/977323003/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client_browsertest.cc:136:
EXPECT_TRUE(browser_command_line.HasSwitch(
On 2015/03/13 13:44:57, Andrew T Wilson wrote:
> This will fail, because you aren't setting the switch on the browser command
> line, but rather on the command line of any renderers being created.

Okay understood, so how do I go about getting the renderer view process specific
command line. All the examples I've seen for checking switches uses
CommandLine::ForCurrentProcess(). 

Apologies that my knowledge is terrible in these respect.

https://codereview.chromium.org/977323003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/977323003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:67: delete Runner;
On 2015/03/13 13:44:57, Andrew T Wilson wrote:
> Put a comment here describing while you're doing this. I'd actually suggest
that
> you don't actually need to delete the Runner() class, because admins can
disable
> dev tools if they want to work around this. But I'm fine with deleting it as
> well.

I'm going to change this since we'll be showing a message.

[Andrew T Wilson (Slow), 2015-03-13 16:52:28]

https://codereview.chromium.org/977323003/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/977323003/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client_browsertest.cc:136:
EXPECT_TRUE(browser_command_line.HasSwitch(
On 2015/03/13 16:09:06, edwardjung wrote:
> On 2015/03/13 13:44:57, Andrew T Wilson wrote:
> > This will fail, because you aren't setting the switch on the browser command
> > line, but rather on the command line of any renderers being created.
> 
> Okay understood, so how do I go about getting the renderer view process
specific
> command line. All the examples I've seen for checking switches uses
> CommandLine::ForCurrentProcess(). 
> 
> Apologies that my knowledge is terrible in these respect.
>    

Great question. I don't write many tests that involve renderer code. The last
test I reviewed that did anything like this was:

https://codereview.chromium.org/964583002/diff/40001/chrome/browser/policy/po...

So, force-open the network error page, then inject some javascript to verify
that the appropriate things happened on the renderer side? Or email chromium-dev
for advice if there's no good way to forcethe network error page to show up?

[edwardjung, 2015-03-16 13:30:59]

Updated with notification 'snackbar' and icon change.

Managed to find tests in errorpage_browsertests that did something similar to
what I wanted. Added two tests one for normal initialisation of the easter egg
and the other CrOS specific for when device is enterprise enrolled.

Thanks.

https://codereview.chromium.org/977323003/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/977323003/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client_browsertest.cc:136:
EXPECT_TRUE(browser_command_line.HasSwitch(
I've moved this test to errorpage_browsertests since they were doing something
similar.

[edwardjung, 2015-03-17 16:22:16]

On 2015/03/16 13:30:59, edwardjung wrote:
> Updated with notification 'snackbar' and icon change.
> 
> Managed to find tests in errorpage_browsertests that did something similar to
> what I wanted. Added two tests one for normal initialisation of the easter egg
> and the other CrOS specific for when device is enterprise enrolled.
> 
> Thanks.
> 
>
https://codereview.chromium.org/977323003/diff/60001/chrome/browser/chrome_co...
> File chrome/browser/chrome_content_browser_client_browsertest.cc (right):
> 
>
https://codereview.chromium.org/977323003/diff/60001/chrome/browser/chrome_co...
> chrome/browser/chrome_content_browser_client_browsertest.cc:136:
> EXPECT_TRUE(browser_command_line.HasSwitch(
> I've moved this test to errorpage_browsertests since they were doing something
> similar.

[edwardjung, 2015-03-18 15:22:04]

edwardjung@chromium.org changed reviewers:
+ arv@chromium.org, sky@chromium.org

[edwardjung, 2015-03-18 15:22:05]

arv:
chrome/browser/renderer/resources/* 

sky:
chrome/browser/*
chrome/common/*
generated_resources.grd

Added a notification when the device is enterprise enrolled. 
Screen recording of the change in the bug:
https://code.google.com/p/chromium/issues/detail?id=462221#c40

Thanks.

[sky, 2015-03-18 20:56:52]

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1411: // Enterprise enrolled
check to disable the easter egg.
Can you do this in chromeos specific startup code?

[edwardjung, 2015-03-19 10:38:49]

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1411: // Enterprise enrolled
check to disable the easter egg.
On 2015/03/18 20:56:52, sky wrote:
> Can you do this in chromeos specific startup code?

I'm not sure, this was the method suggested by atwilson.  Drew, any thoughts on
this, is there a better CrOS specific place to add this flag?

[sky, 2015-03-19 19:26:13]

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1411: // Enterprise enrolled
check to disable the easter egg.
On 2015/03/19 10:38:48, edwardjung wrote:
> On 2015/03/18 20:56:52, sky wrote:
> > Can you do this in chromeos specific startup code?
> 
> I'm not sure, this was the method suggested by atwilson.  Drew, any thoughts
on
> this, is there a better CrOS specific place to add this flag?

I was specifically thinking of something like
browser_process_platform_part_chromeos.

[edwardjung, 2015-03-20 17:36:40]

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1411: // Enterprise enrolled
check to disable the easter egg.
> I was specifically thinking of something like
> browser_process_platform_part_chromeos.

I tried this and although the flag is set on startup. The flag doesn't seem to
filter down when getting the command line for the current process.

[Andrew T Wilson (Slow), 2015-03-20 19:33:04]

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1411: // Enterprise enrolled
check to disable the easter egg.
On 2015/03/20 17:36:40, edwardjung wrote:
> > I was specifically thinking of something like
> > browser_process_platform_part_chromeos.
> 
> I tried this and although the flag is set on startup. The flag doesn't seem to
> filter down when getting the command line for the current process.

You probably have to call CopySwitchesFrom() explicitly in this routine to copy
kDisableDinosaurEasterEgg from browser -> renderer process.

The nice advantage of that is people who want to disable this on desktop can
just pass that flag in (i.e. the commandline flag is supported on all
platforms).

[edwardjung, 2015-03-23 13:17:15]

Moved the check across to browser_process_platform_part_chromeos.

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/977323003/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1411: // Enterprise enrolled
check to disable the easter egg.
Done.

Thanks Drew. 

@sky, moved this check to browser_process_platform_part_chromeos.

[sky, 2015-03-23 15:59:20]

LGTM

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:81: 
remove this line.

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:121: if
(is_enterprise_managed) {
nit: no {}

[Andrew T Wilson (Slow), 2015-03-23 16:38:27]

LGTM with a few nits.

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.h (right):

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:63: // Disable the
offline interstitial easter egg if the device is enterprised
nit: enterprise, not enterprised

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:65: void
DisableDinoEasterEgg();
nit: name this DisableDinoEasterEggIfEnrolled()

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1339: static const char* const
kchromeOSSwitches[] = {
nit: kChromeOSSwitches (capital C)

https://codereview.chromium.org/977323003/diff/120001/chrome/common/localized...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/977323003/diff/120001/chrome/common/localized...
chrome/common/localized_error.cc:580:
error_strings->SetString("disabledEasterEgg",
Maybe add a comment that the presence of this string disables the easter egg
(i.e. it's not just a string, it's also a flag to the UI).

[edwardjung, 2015-03-24 16:07:36]

Nits addressed.

Erik, ready for you to take a look at the resources files.

Thanks,

Edward

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:81: 
On 2015/03/23 15:59:20, sky wrote:
> remove this line.

Done.

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:121: if
(is_enterprise_managed) {
On 2015/03/23 15:59:20, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.h (right):

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:63: // Disable the
offline interstitial easter egg if the device is enterprised
On 2015/03/23 16:38:27, Andrew T Wilson wrote:
> nit: enterprise, not enterprised

Done.

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:65: void
DisableDinoEasterEgg();
On 2015/03/23 16:38:26, Andrew T Wilson wrote:
> nit: name this DisableDinoEasterEggIfEnrolled()

Done.

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/977323003/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1339: static const char* const
kchromeOSSwitches[] = {
On 2015/03/23 16:38:27, Andrew T Wilson wrote:
> nit: kChromeOSSwitches (capital C)

Done.

https://codereview.chromium.org/977323003/diff/120001/chrome/common/localized...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/977323003/diff/120001/chrome/common/localized...
chrome/common/localized_error.cc:580:
error_strings->SetString("disabledEasterEgg",
On 2015/03/23 16:38:27, Andrew T Wilson wrote:
> Maybe add a comment that the presence of this string disables the easter egg
> (i.e. it's not just a string, it's also a flag to the UI).

Done.

[arv (Not doing code reviews), 2015-03-24 16:46:59]

https://codereview.chromium.org/977323003/diff/140001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/977323003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:233: this.containerEl.innerText =
loadTimeData.getValue('disabledEasterEgg');
don't use innerText. You can use textContent here.

https://codereview.chromium.org/977323003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:238: if
(Runner.keycodes.JUMP[String(e.keyCode)]) {
No need for String() here

[edwardjung, 2015-03-24 17:42:36]

All done. Thanks.

https://codereview.chromium.org/977323003/diff/140001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/977323003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:233: this.containerEl.innerText =
loadTimeData.getValue('disabledEasterEgg');
On 2015/03/24 16:46:59, arv wrote:
> don't use innerText. You can use textContent here.

Done.

Need to shake off this habit.

https://codereview.chromium.org/977323003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:238: if
(Runner.keycodes.JUMP[String(e.keyCode)]) {
On 2015/03/24 16:46:59, arv wrote:
> No need for String() here

Done.

[arv (Not doing code reviews), 2015-03-25 09:36:03]

LGTM with nits

https://codereview.chromium.org/977323003/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/977323003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:67: console.log(this.isDisabled());
remove debug log?

https://codereview.chromium.org/977323003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:239: if (Runner.keycodes.JUMP[e.keyCode]) {
wrong indentation

[edwardjung, 2015-03-25 11:05:50]

Thanks Erik.

https://codereview.chromium.org/977323003/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/977323003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:67: console.log(this.isDisabled());
On 2015/03/25 09:36:03, arv wrote:
> remove debug log?

Done.

https://codereview.chromium.org/977323003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:239: if (Runner.keycodes.JUMP[e.keyCode]) {
On 2015/03/25 09:36:03, arv wrote:
> wrong indentation

Done.

[edwardjung, 2015-03-25 15:25:38]

The CQ bit was checked by edwardjung@chromium.org

[edwardjung, 2015-03-25 15:25:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org, sky@chromium.org, arv@chromium.org
Link to the patchset: https://codereview.chromium.org/977323003/#ps180001
(title: "Remove debug, fix indentation in JS")

[commit-bot: I haz the power, 2015-03-25 15:25:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/977323003/180001

[commit-bot: I haz the power, 2015-03-25 15:44:50]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-03-25 15:45:43]

Patchset 10 (id:??) landed as
https://crrev.com/7db1c9eae940f6f35f764ad2d107a6b28a71ceda
Cr-Commit-Position: refs/heads/master@{#322167}