[strong] Stricter check for referring to other classes inside methods.

[strong] Stricter check for referring to other classes inside methods.

Add the restriction that both classes must be declared inside the same
consectutive class declaration batch.

Dependency analysis not implemented yet.

BUG=v8:3956
LOG=N

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

[marja, 2015-04-20 08:32:44]

marja@chromium.org changed reviewers:
+ rossberg@chromium.org

[marja, 2015-04-20 08:32:45]

Here's the next step. rossberg@, ptal.

The "class inside a computed property name" case is a mess.. there's no new
FunctionState created, but we can still detect that the classes are not
consecutive because they're in different scopes.

[marja, 2015-04-20 09:41:18]

Hmm, another possibility would be to put the "batch start position" thing to
Scope instead of FunctionState... not sure if that would work better.

[marja, 2015-04-20 11:13:18]

Alright, I uploaded an alternative Scope-based (instead of FunctionState-based)
solution, patch set 8. I think that's cleaner.. ptal.

[rossberg, 2015-04-20 11:15:26]

There may be a kind of off-by-one-declaration bug in there, if I understand the
code correctly.

To ease terminology, I suggest saying "declaration group" instead of
"consecutive declaration batch". That is terminology used in the context of some
other languages with similar mechanisms (e.g. Haskell).

https://codereview.chromium.org/1060913005/diff/90001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1060913005/diff/90001/src/ast.h#newcode577
src/ast.h:577: int consecutive_declaration_batch_start() const {
Nit: Let's call this class_declaration_group_start (we probably will want to use
a separate one for function groups).

https://codereview.chromium.org/1060913005/diff/90001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1060913005/diff/90001/src/parser.cc#newcode1320
src/parser.cc:1320: if (peek() != Token::CLASS) {
Hm, I'm not sure I fully understand the logic. You seem to resetting the pos
_unless_ it is a class declaration. But wouldn't that mean that

  let A = class { m(){B} }
  class B {}

would be allowed, because pos(a) would be taken as the start of the declaration
group?

What I would have expected is that the start pos is invalid if we're not inside
a respective group, is set when we see a respective declaration and was invalid
before, and is left alone when we see a suitable declaration and already have a
valid start. Any other declaration would reset it to invalid.

https://codereview.chromium.org/1060913005/diff/90001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1060913005/diff/90001/src/scopes.cc#newcode1164
src/scopes.cc:1164: // This is needed because a class normally creates two
scopes (outer and
Hm, a class only introduces a single scope. Do you mean it creates two bindings?

https://codereview.chromium.org/1060913005/diff/90001/src/scopes.cc#newcode1166
src/scopes.cc:1166: // same consectutive class declaration batch are declared in
the outer
Nit: "consectutive class declaration batch" -> "declaration group" (here and
elsewhere)

https://codereview.chromium.org/1060913005/diff/90001/src/scopes.cc#newcode1173
src/scopes.cc:1173: if (maybe_outer_class_var) {
This may be too liberal. I suppose you somehow need to verify that this var is
in fact a class variable, for the same class. Otherwise you might incorrectly
capture

  let A = class C { ... }
  let C = class D { ... }

Unfortunately, we don't distinguish that in the declaration. Maybe some trick
with positions would work? If not, a TODO is fine for this CL.

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mut...
File test/mjsunit/strong/mutually-recursive-classes.js (right):

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mut...
test/mjsunit/strong/mutually-recursive-classes.js:16: function
assertThrowsEarlyErrors(code) {
How about assertLateErrorsBecomeEarly?

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mut...
test/mjsunit/strong/mutually-recursive-classes.js:33: function
assertThrowsAddedErrors(code) {
assertNonErrorsBecomeEarly?

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mut...
test/mjsunit/strong/mutually-recursive-classes.js:151: // Note that this is also
an error:
I think the comment only makes sense if A refers to B1. Otherwise it's just a
plain scoping error unrelated to class recursion -- obviously, you cannot access
local variables outside their scope, forwards or backwards.

[rossberg, 2015-04-20 11:18:01]

On 2015/04/20 11:13:18, marja wrote:
> Alright, I uploaded an alternative Scope-based (instead of
FunctionState-based)
> solution, patch set 8. I think that's cleaner.. ptal.

Sounds good, should be mostly orthogonal to my comments.

[marja, 2015-04-20 12:37:30]

omg, turns out the counterexample

let A = class { m(){B} }
class B {}

was working (as in, we were producing an error), but it was working by accident:
when looking for a class variable for m() we didn't find A. In fact we didn't
find anything.

Idk how to fix that though... it just looks like an empty scope (the class
scope) inside another scope, and one of the variables in the outer scope happens
to be "our" class variable but we don't know which one. Omg.

(But the group position logic you suggested is clearer and more correct than the
current one.)

[rossberg, 2015-04-20 13:52:45]

On 2015/04/20 12:37:30, marja wrote:
> omg, turns out the counterexample
> 
> let A = class { m(){B} }
> class B {}
> 
> was working (as in, we were producing an error), but it was working by
accident:
> when looking for a class variable for m() we didn't find A. In fact we didn't
> find anything.
> 
> Idk how to fix that though... it just looks like an empty scope (the class
> scope) inside another scope, and one of the variables in the outer scope
happens
> to be "our" class variable but we don't know which one. Omg.

But why does this case matter? It can only occur for class expressions, which
never participate in a class declaration group. So you can always skip over this
one.

Btw, I think there is another problem with the current logic: in
CheckStrongModeDeclaration you assume that there is only one level of nesting.
But in general, classes can be nested arbitrarily deep. This should work:

  class A {
    m() { class C { m(){ B } } }
  }
  class B {}

Or even more interestingly:

  class A {
    m() { class C extends B {} }
  }
  class B {}

So you need to loop over outer scopes until you find the right one.

> (But the group position logic you suggested is clearer and more correct than
the
> current one.)

[marja, 2015-04-20 13:59:46]

Yup, I'm still unsure whether the inability to find a class variable in the
following cases causes any practical problems:

let A = class { ... }
let A = class B { ... }

I don't think this should work though:

class A {
  m() { class C extends B {} }
}
class B {}

since we should allow init-time references only to already declared things.

(The other example probably should work, yes, and doesn't currently work.)

[rossberg, 2015-04-20 14:13:26]

On 2015/04/20 13:59:46, marja wrote:
> I don't think this should work though:
> 
> class A {
>   m() { class C extends B {} }
> }
> class B {}
> 
> since we should allow init-time references only to already declared things.

No, that should definitely work. Any example of the form

class A {
  m() { ...B... }
}
class B {}

is supposed to work, no matter what "..." is. Failing that would be violating
compositionality.

[marja, 2015-04-20 14:21:13]

Oh my, this is exponentially more complicated than I thought.

Then this should be forbidden:

class A {
  static sm() { class C extends B { } ... }
}

class B {
  [A.sm()]() { ... }
}

but the cycle detection logic I proposed earlier doesn't cover it.

[rossberg, 2015-04-20 14:33:15]

On 2015/04/20 14:21:13, marja wrote:
> Oh my, this is exponentially more complicated than I thought.

I would think it's actually simpler. It certainly is more regular, which means
no special hacks.

All we need is a generic method that, given two scopes S1 and S2, finds the
binding in S1 that contains S2. (And can tell whether that is a class binding.)
Then S1 is the scope you find the variable in, and S2 the current scope. If B is
the binding in question owning S2, then it has to be a class and in the same
group as the variable.

> Then this should be forbidden:
> 
> class A {
>   static sm() { class C extends B { } ... }
> }
> 
> class B {
>   [A.sm()]() { ... }
> }

Yes, but like before, that is forbidden regardless of _how_ B is used inside sm.
B can't be used there, period.

> but the cycle detection logic I proposed earlier doesn't cover it.

Why not?

[marja, 2015-04-20 14:35:46]

> > but the cycle detection logic I proposed earlier doesn't cover it.
> 
> Why not?

Meh, it does, I was just confused. B has an init-time dependency to A, so
doesn't matter which type of dependency A has to B.

[marja, 2015-04-20 14:36:31]

On 2015/04/20 14:35:46, marja wrote:
> > > but the cycle detection logic I proposed earlier doesn't cover it.
> > 
> > Why not?
> 
> Meh, it does, I was just confused. B has an init-time dependency to A, so
> doesn't matter which type of dependency A has to B.

... it just means we need to collect the references for all classes in the
chain, so, now both C and A have a reference to B.

[marja, 2015-04-20 15:58:23]

Alright, I tried to fix all the cases you pointed out. ptal.

I'm adding extra stuff to Variable though and that's sad :(

https://codereview.chromium.org/1060913005/diff/90001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1060913005/diff/90001/src/ast.h#newcode577
src/ast.h:577: int consecutive_declaration_batch_start() const {
On 2015/04/20 11:15:25, rossberg wrote:
> Nit: Let's call this class_declaration_group_start (we probably will want to
use
> a separate one for function groups).

batch -> group done

But I don't need a separate one for class here afaics, I was intending to use
the same one! If the VariableDeclaration is for a function, it'll be the
function group start position, if the VariableDeclaration is a class, it'll be
the class group start position.

(Other places need to distinguish between them though, and they have _class_ in
the name.)

https://codereview.chromium.org/1060913005/diff/90001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1060913005/diff/90001/src/parser.cc#newcode1320
src/parser.cc:1320: if (peek() != Token::CLASS) {
On 2015/04/20 11:15:25, rossberg wrote:
> Hm, I'm not sure I fully understand the logic. You seem to resetting the pos
> _unless_ it is a class declaration. But wouldn't that mean that
> 
>   let A = class { m(){B} }
>   class B {}
> 
> would be allowed, because pos(a) would be taken as the start of the
declaration
> group?
> 
> What I would have expected is that the start pos is invalid if we're not
inside
> a respective group, is set when we see a respective declaration and was
invalid
> before, and is left alone when we see a suitable declaration and already have
a
> valid start. Any other declaration would reset it to invalid.

Done + added a test for the counterexample (which was working by accident
though).

https://codereview.chromium.org/1060913005/diff/90001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1060913005/diff/90001/src/scopes.cc#newcode1164
src/scopes.cc:1164: // This is needed because a class normally creates two
scopes (outer and
On 2015/04/20 11:15:25, rossberg wrote:
> Hm, a class only introduces a single scope. Do you mean it creates two
bindings?

Yes, s/two scopes/two bindings/ was what I meant; fixed.

https://codereview.chromium.org/1060913005/diff/90001/src/scopes.cc#newcode1166
src/scopes.cc:1166: // same consectutive class declaration batch are declared in
the outer
On 2015/04/20 11:15:25, rossberg wrote:
> Nit: "consectutive class declaration batch" -> "declaration group" (here and
> elsewhere)

Done.

https://codereview.chromium.org/1060913005/diff/90001/src/scopes.cc#newcode1173
src/scopes.cc:1173: if (maybe_outer_class_var) {
Ahh, true. The breaking case is this:

let A = class B {
  m() { C; }
}
;;;;
class C {}
class B {}

(Should produce an error, doesn't.)

Fixed that and added a test.

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mut...
File test/mjsunit/strong/mutually-recursive-classes.js (right):

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mut...
test/mjsunit/strong/mutually-recursive-classes.js:16: function
assertThrowsEarlyErrors(code) {
On 2015/04/20 11:15:25, rossberg wrote:
> How about assertLateErrorsBecomeEarly?

Done.

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mut...
test/mjsunit/strong/mutually-recursive-classes.js:33: function
assertThrowsAddedErrors(code) {
On 2015/04/20 11:15:25, rossberg wrote:
> assertNonErrorsBecomeEarly?

Done.

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mut...
test/mjsunit/strong/mutually-recursive-classes.js:151: // Note that this is also
an error:
On 2015/04/20 11:15:25, rossberg wrote:
> I think the comment only makes sense if A refers to B1. Otherwise it's just a
> plain scoping error unrelated to class recursion -- obviously, you cannot
access
> local variables outside their scope, forwards or backwards.

Yes, the comment "B2 will be unbound" tried to explain that. But maybe having it
here is more confusing than clarifying?

[marja, 2015-04-20 15:58:28]

Alright, I tried to fix all the cases you pointed out. ptal.

I'm adding extra stuff to Variable though and that's sad :(

[rossberg, 2015-04-21 13:30:02]

https://codereview.chromium.org/1060913005/diff/190001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1060913005/diff/190001/src/ast.h#newcode577
src/ast.h:577: int consecutive_declaration_group_start() const {
Nit: also drop the "consecutive" (here and elsewhere)

https://codereview.chromium.org/1060913005/diff/190001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1060913005/diff/190001/src/parser.cc#newcode1317
src/parser.cc:1317: if (scope_->consecutive_class_declaration_group_start() >= 0
&&
Isn't this extra condition redundant?

https://codereview.chromium.org/1060913005/diff/190001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1060913005/diff/190001/src/scopes.cc#newcode1153
src/scopes.cc:1153: while (scope) {
A couple of observations:
- I shouldn't need to loop further than var->scope().
- You only need to look for the class variable if you encounter a class scope.
- You don't need to check the group start at each level, only once you've
reached the right scope.
- I think you can skip this whole part if var is not part of a declaration
group.

What we want to find is an outer_class_var that is bound in var->scope(), and
that corresponds to a class_var in an inner class scope surrounding us. So I
think something like this should work:

if (var->class_declaration_group_start() != -1) {
  Variable* class_var = nullptr;
  for (Scope* scope = this; scope != var->scope(); scope = scope->outer_scope())
{
    if (scope->is_class_scope()) {
      if (Variable* inner_class_var = scope->ClassVariable()) {
        class_var = inner_var->corresponding_outer_class_variable();
      }
    }
  }

  if (var->class_declaration_group_start() ==
class_var->class_declaration_group_start()) {
    return true;
  }
}

where Scope::ClassVariable can be factored out of the current
GetClassVariableForMethod.

https://codereview.chromium.org/1060913005/diff/190001/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/1060913005/diff/190001/src/variables.h#newcod...
src/variables.h:193: Variable* corresponding_outer_class_variable_;
As discussed offline, maybe we want to introduce a subclass ClassVariable for
this.

[rossberg, 2015-04-21 13:33:28]

PS: Might also try to store the link to the outer var in class scopes.

(And don't we need to serialize it?)

[marja, 2015-04-23 09:52:37]

Alright, here's a new try.

As mentioned (offline?), it's a bug (also in bleeding_edge) that the variable
Kinds and initializer positions are not stored correctly in the ScopeInfo.

I was wondering why it doesn't cause visible problems, and turns out it's
because e.g., the initializer position for functions are not stored either, so
we don't do the initializer order check :P

I modified this CL to keep that "working by accident" the same way bleeding_edge
is; we allow references to variables whose initializer positions we have not
stored. This way the ScopeInfo storing can be fixed separately.

We'd at least need to store the variable kind (that's easy since it's a couple
of bits); storing the initializer position will add an integer (so it's a
non-trivial overhead?), and storing the corresponding outer scope variables...
idk how that would work.

I also added TODOs into this CL in places where we have hacks around the
ScopeInfo not storing all the needed data.

https://codereview.chromium.org/1060913005/diff/190001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1060913005/diff/190001/src/ast.h#newcode577
src/ast.h:577: int consecutive_declaration_group_start() const {
On 2015/04/21 13:30:02, rossberg wrote:
> Nit: also drop the "consecutive" (here and elsewhere)

Done.

https://codereview.chromium.org/1060913005/diff/190001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1060913005/diff/190001/src/parser.cc#newcode1317
src/parser.cc:1317: if (scope_->consecutive_class_declaration_group_start() >= 0
&&
On 2015/04/21 13:30:02, rossberg wrote:
> Isn't this extra condition redundant?

Done.

https://codereview.chromium.org/1060913005/diff/190001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1060913005/diff/190001/src/scopes.cc#newcode1153
src/scopes.cc:1153: while (scope) {
I rewrote the code so that it's closer to what you suggested, but not quite
(some details, like, 1) we're not necessarily in a method even if scope is a
class scope; we want this check to only apply to variables inside methods, 2)
there might not be an outer class var).

https://codereview.chromium.org/1060913005/diff/190001/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/1060913005/diff/190001/src/variables.h#newcod...
src/variables.h:193: Variable* corresponding_outer_class_variable_;
On 2015/04/21 13:30:02, rossberg wrote:
> As discussed offline, maybe we want to introduce a subclass ClassVariable for
> this.

Done.

[rossberg, 2015-04-23 12:20:54]

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mut...
File test/mjsunit/strong/mutually-recursive-classes.js (right):

https://codereview.chromium.org/1060913005/diff/90001/test/mjsunit/strong/mut...
test/mjsunit/strong/mutually-recursive-classes.js:151: // Note that this is also
an error:
On 2015/04/20 15:58:23, marja wrote:
> On 2015/04/20 11:15:25, rossberg wrote:
> > I think the comment only makes sense if A refers to B1. Otherwise it's just
a
> > plain scoping error unrelated to class recursion -- obviously, you cannot
> access
> > local variables outside their scope, forwards or backwards.
> 
> Yes, the comment "B2 will be unbound" tried to explain that. But maybe having
it
> here is more confusing than clarifying?

Yeah, I would just remove it. It certainly confused me, although that might not
mean much. :)

https://codereview.chromium.org/1060913005/diff/230001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1060913005/diff/230001/src/scopes.cc#newcode1207
src/scopes.cc:1207: if (class_var->class_declaration_group_start() ==
Can't class_var be null at this point? E.g., when you are in a class expression
with a local class variable but no corresponding outer one?

https://codereview.chromium.org/1060913005/diff/230001/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/1060913005/diff/230001/src/variables.h#newcod...
src/variables.h:198: int class_declaration_group_start() const {
Nit: since this is now in ClassVariable, the class_ prefix is kind of redundant.

https://codereview.chromium.org/1060913005/diff/230001/test/mjsunit/strong/mu...
File test/mjsunit/strong/mutually-recursive-classes.js (right):

https://codereview.chromium.org/1060913005/diff/230001/test/mjsunit/strong/mu...
test/mjsunit/strong/mutually-recursive-classes.js:1: // Copyright 2015 the V8
project authors. All rights reserved.
Perhaps add a few sanity tests with class expressions (with or without inner
class variable), to make sure that e.g. we don't crash (I believe we currently
do, see other comment).

[rossberg, 2015-04-23 12:24:59]

On 2015/04/23 09:52:37, marja wrote:
> As mentioned (offline?), it's a bug (also in bleeding_edge) that the variable
> Kinds and initializer positions are not stored correctly in the ScopeInfo.
> 
> I was wondering why it doesn't cause visible problems, and turns out it's
> because e.g., the initializer position for functions are not stored either, so
> we don't do the initializer order check :P

Oh crap.

> I modified this CL to keep that "working by accident" the same way
bleeding_edge
> is; we allow references to variables whose initializer positions we have not
> stored. This way the ScopeInfo storing can be fixed separately.
> 
> We'd at least need to store the variable kind (that's easy since it's a couple
> of bits); storing the initializer position will add an integer (so it's a
> non-trivial overhead?), and storing the corresponding outer scope variables...
> idk how that would work.

We only need to store these for class decls, so the overhead should be minimal.
Just a bit of extra special-casing.

> I also added TODOs into this CL in places where we have hacks around the
> ScopeInfo not storing all the needed data.

Great, thanks!

[marja, 2015-04-23 12:38:25]

Afaics, we need to store the initializer positions for all variables, since we
can e.g., parse a lazy function and want to check its variables against the
outer scopes, no?

(Anyway, that's being worked on in a separate CL.)

[rossberg, 2015-04-23 12:58:37]

On 2015/04/23 12:38:25, marja wrote:
> Afaics, we need to store the initializer positions for all variables, since we
> can e.g., parse a lazy function and want to check its variables against the
> outer scopes, no?

You're right, I was thinking of the declaration group start.

[marja, 2015-04-23 13:12:43]

https://codereview.chromium.org/1060913005/diff/230001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1060913005/diff/230001/src/scopes.cc#newcode1207
src/scopes.cc:1207: if (class_var->class_declaration_group_start() ==
On 2015/04/23 12:20:53, rossberg wrote:
> Can't class_var be null at this point? E.g., when you are in a class
expression
> with a local class variable but no corresponding outer one?

As discussed online; it can't; added more test cases for this.

https://codereview.chromium.org/1060913005/diff/230001/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/1060913005/diff/230001/src/variables.h#newcod...
src/variables.h:198: int class_declaration_group_start() const {
On 2015/04/23 12:20:53, rossberg wrote:
> Nit: since this is now in ClassVariable, the class_ prefix is kind of
redundant.

Done.

https://codereview.chromium.org/1060913005/diff/230001/test/mjsunit/strong/mu...
File test/mjsunit/strong/mutually-recursive-classes.js (right):

https://codereview.chromium.org/1060913005/diff/230001/test/mjsunit/strong/mu...
test/mjsunit/strong/mutually-recursive-classes.js:1: // Copyright 2015 the V8
project authors. All rights reserved.
On 2015/04/23 12:20:53, rossberg wrote:
> Perhaps add a few sanity tests with class expressions (with or without inner
> class variable), to make sure that e.g. we don't crash (I believe we currently
> do, see other comment).

Done.

[rossberg, 2015-04-23 13:17:21]

lgtm

https://codereview.chromium.org/1060913005/diff/250001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1060913005/diff/250001/src/scopes.cc#newcode1191
src/scopes.cc:1191: class_var = class_var->corresponding_outer_class_variable();
Nit: you could switch the two lines and avoid the duplication:

class_var = class_var>corresponding_outer_class_variable();
if (class_var) {

[marja, 2015-04-23 13:40:23]

thanks for review!

https://codereview.chromium.org/1060913005/diff/250001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/1060913005/diff/250001/src/scopes.cc#newcode1191
src/scopes.cc:1191: class_var = class_var->corresponding_outer_class_variable();
On 2015/04/23 13:17:20, rossberg wrote:
> Nit: you could switch the two lines and avoid the duplication:
> 
> class_var = class_var>corresponding_outer_class_variable();
> if (class_var) {

Done.

[marja, 2015-04-23 13:40:27]

The CQ bit was checked by marja@chromium.org

[marja, 2015-04-23 13:40:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1060913005/#ps270001
(title: "code review (rossberg@)")

[commit-bot: I haz the power, 2015-04-23 13:40:36]

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

[commit-bot: I haz the power, 2015-04-23 14:05:09]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2015-04-23 14:05:26]

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