Change RawDatastore to do less reflection.

Change RawDatastore to do less reflection.

Followup CL will provide the 'magic' reflection-full Datastore interface.

This change makes filters easier to implement, none of the layers of the stack
are required to do reflection, instead of the previous design where all of the
layers were forced to do reflection.

R=dnj@chromium.org, martiniss@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/36ccdaa83237cd89531af120424b945262f0b948

[iannucci, 2015-07-14 17:27:44]

Okie dokie, this one is the next one in the chain. I'll drop some seed comments
in there to give some hints on what's going on.

[iannucci, 2015-07-14 18:15:44]

ptal!

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
File go/src/infra/gae/libs/gae/helper/datastore.go (right):

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/helper/datastore.go:17: func GetPLS(o interface{})
gae.DSPropertyLoadSaver {
To simplify things, I've merged the 'DSStructPLS' interface into
DSPropertyLoadSaver itself. This means that DSPropertyLoadSaver can represent
both data (key -> values) and metadata ($key -> value).

This was done to allow a data blob to communicate with various filters in the
stack (for example: cache control), without needing the RawDatastore interface
to grow new warts.

It also ends up simplifying a lot of stuff since there's now only one interface
to care about.

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
File go/src/infra/gae/libs/gae/helper/datastore_impl.go (right):

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:42: metaIsString   bool
This metaIsString used to be useful... I'll take it out.

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:67: func (p *structPLS)
Load(propMap gae.DSPropertyMap) error {
I changed this interface to only return one error, since Get must pass this
error through. The caller can now determine the type of the error by seeing if
it's a gae.MultiError (which contains *ErrDSFieldMismatch objects). Practically
speaking, this sort of thing only happens on a poorly-planned schema change
(e.g. rename or removal of a struct field and then the calling code doesn't Get
into DSPropertyMap for backwards compatibility).

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:242: func (p *structPLS)
Save(withMeta bool) (gae.DSPropertyMap, error) {
withMeta was added so that the caller can ask for a DSPropertyMap which does or
does not preserve metadata ($-keyed fields). Filters will typically only ever
pass true to this (if they need to call it at all. the mcache filter is the only
one that will really need to call it, since it needs a DSPropertyMap for
serialization purposes). Preserving metadata means that a filter can convert a
structPLS -> DSPropertyMap, and pass the DSPropertyMap to the filter below it,
ensuring that a struct will only ever be converted to a DSPropertyMap once per
Get/GetAll call (since DSPropertyMap.Save(true) just returns the map itself).

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:322: func (p *structPLS)
GetMeta(key string) (interface{}, error) {
"special" was renamed to "meta" to more clearly denote what it was

Additionally, metadata now only can have one value. Consider:

type S struct {
  F int64 `$meta,100`
}

If F has the zero value (in this case ""), and there's a default value (i.e.
",100" in the above example), then GetMeta("meta") will return (int64(100),
nil). If F doesn't have the zero value, it's passed through unscathed.  Only
string and int64, and bool are supported meta values at this time. The code for
converting the default value is in the getStructCodecLocked function below.

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
File go/src/infra/gae/libs/gae/helper/datastore_test.go (left):

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/helper/datastore_test.go:548: convErr       string
this is gone because conversion errors are not multiplexed into the loadErr

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/helper/datastore_test.go:1641: Convey("can transform a
list of things into a list of PLSs", func() {
this functionality was dropped since nothing needs it any more (since
DSPropertyList and DSStructPLS merged).

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
File go/src/infra/gae/libs/gae/helper/datastore_test.go (right):

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/helper/datastore_test.go:27: mpNI = gae.MkDSPropertyNI
I made MkDSProperty and MkDSPropertyNI (no index) real exported functions, since
they seemed to turn up in tests so frequently. They're also good if you
statically know-for-sure that the value you're making a DSProperty around is
actually supported.

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/helper/datastore_test.go:601: want: gae.DSPropertyMap{
I simplified the interface to not require pointer-to-a-map. That was done
previously because it was a []Property in the old-style SDK (which doesn't
permit appends without also changing the slice object itself). It's pointless to
have a pointer-to-a-map since the map itself is already pass-by-reference.

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
File go/src/infra/gae/libs/gae/prod/datastore_key.go (left):

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/prod/datastore_key.go:23: func (k dsKeyImpl) Valid(ns
string, allowSpecial bool) bool {
I removed a bunch of these extraneous methods. Packages should just use the
exported functions in helper, since they're implementation agnostic.

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
File go/src/infra/gae/libs/gae/raw_datastore.go (right):

https://chromiumcodereview.appspot.com/1227183003/diff/40001/go/src/infra/gae...
go/src/infra/gae/libs/gae/raw_datastore.go:57: // helper.Datastore and
RawDatastore.
Coming Soon to a Checkout Near You!

[Vadim Sh., 2015-07-14 18:55:44]

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore_impl.go (right):

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:12: "infra/gae/libs/gae"
nit: move into separate import block

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:249: if err := p.save(ret,
&idxCount, "", false); err != nil {
are you using pointers to return values from a function (&idxCount)? Ugh.. I
see. I was confused why it isn't referenced in Save(...) body itself.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:350: pv := recover()
nooooo!.. Is this the only way to catch errors here?

My concern with recover() (in general): it looks like "catch (...) { }" in C++,
except it also catches memory access errors etc. Though arguably
p.o.Field(idx).Set(reflect.ValueOf(val)) doesn't have a chance to produce any
really unexpected exception.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:554: switch t {
what about boolean?

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/properties.go (right):

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:48: // MkDSProperty makes a new
DSProperty and returns it. If val is an invalid
nit: a new indexed DSProperty

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:50: // normally, use SetValue instead.
nit: use SetValue(..., false) instead.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:61: // handle the error normally, use
SetValue instead.
nit: use SetValue(..., true) instead

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:64: if err := ret.SetValue(val, true);
err != nil {
nit: 'true' to indicate "noindex" is a bit confusing. Maybe use named constants
instead of bool?

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:325: Save(withMeta bool) (DSPropertyMap,
error)
document Save?

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:359: // that property name. It is the
spiritual successor to PropertyList from the
spiritual successor, lol

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:363: // Technically the datastore allows
property names, but all of the SDKs go out
nit: "allows arbitrary property names"? I think you forgot a word there...

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:387: return errors.New("gae: nil
DSPropertyMap")
let it panic? It's caller responsibility to not pass nil.

Actually, it's receiver. So definitely let it panic...

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:401: return pm, nil
copy pm before returning?

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:405: if k[0] == '$' { // remove meta
keys
are you 100% sure k != ""?

[dnj, 2015-07-14 19:34:58]

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore_impl.go (right):

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:350: pv := recover()
On 2015/07/14 18:55:43, Vadim Sh. wrote:
> nooooo!.. Is this the only way to catch errors here?
> 
> My concern with recover() (in general): it looks like "catch (...) { }" in
C++,
> except it also catches memory access errors etc. Though arguably
> p.o.Field(idx).Set(reflect.ValueOf(val)) doesn't have a chance to produce any
> really unexpected exception.

+1. IMO if some app is concerned with a panic in its AppEngine layer, let it do
the recover.

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore_impl.go (right):

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:91: if convFailures != nil {
nit: Since you're using it as an array, I would prefer: len(convFailures) > 0

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/prod/datastore_key.go (right):

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/datastore_key.go:58: // places. Realistically,
everythin should hit the early exit clause above.
"everything" or "everythin'"

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/properties.go (right):

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:401: ret := make(DSPropertyMap, len(pm))
Maybe include the allocation-avoiding optimization:
if len(pm) == 0 {
  return nil, nil
}

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:403: if k[0] == '$' { // remove meta
keys
This will have an issue with an empty key (""). Either strip it or check for it
before indexing '0'.

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:404: continue
Since the action is a single line, avoid the "continue" and just invert the
criteria and append within the "if".

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:438: if pm == nil {
Why do you differentiate between "nil" and empty? I think you should settle on
empty (nil inclusive) being the error.

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/m...
File go/src/infra/gae/libs/meta/eg.go (right):

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/m...
go/src/infra/gae/libs/meta/eg.go:34: ret := int64(0)
Why bother with this assignment when you can just assign it on #36?

[iannucci, 2015-07-14 22:45:49]

fixes!

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore_impl.go (right):

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:12: "infra/gae/libs/gae"
On 2015/07/14 18:55:43, Vadim Sh. wrote:
> nit: move into separate import block

Done.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:249: if err := p.save(ret,
&idxCount, "", false); err != nil {
On 2015/07/14 18:55:43, Vadim Sh. wrote:
> are you using pointers to return values from a function (&idxCount)? Ugh.. I
> see. I was confused why it isn't referenced in Save(...) body itself.

Yeah, I'm sorry... this is C+P from the SDK implementation. I've fixed it though
:)

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:350: pv := recover()
On 2015/07/14 19:34:57, dnj wrote:
> On 2015/07/14 18:55:43, Vadim Sh. wrote:
> > nooooo!.. Is this the only way to catch errors here?
> > 
> > My concern with recover() (in general): it looks like "catch (...) { }" in
> C++,
> > except it also catches memory access errors etc. Though arguably
> > p.o.Field(idx).Set(reflect.ValueOf(val)) doesn't have a chance to produce
any
> > really unexpected exception.
> 
> +1. IMO if some app is concerned with a panic in its AppEngine layer, let it
do
> the recover.

Fixed!

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:554: switch t {
On 2015/07/14 18:55:43, Vadim Sh. wrote:
> what about boolean?

oh, right. I said supported that, didn't I? :D I'm going to just not support
that for now, and add support when there's actually a use for it.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/properties.go (right):

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:48: // MkDSProperty makes a new
DSProperty and returns it. If val is an invalid
On 2015/07/14 18:55:43, Vadim Sh. wrote:
> nit: a new indexed DSProperty

Done.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:50: // normally, use SetValue instead.
On 2015/07/14 18:55:44, Vadim Sh. wrote:
> nit: use SetValue(..., false) instead.

Done.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:61: // handle the error normally, use
SetValue instead.
On 2015/07/14 18:55:43, Vadim Sh. wrote:
> nit: use SetValue(..., true) instead

Done.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:64: if err := ret.SetValue(val, true);
err != nil {
On 2015/07/14 18:55:44, Vadim Sh. wrote:
> nit: 'true' to indicate "noindex" is a bit confusing. Maybe use named
constants
> instead of bool?

Yeah, I think you're right (it was more copy pasta :(). Fixed everywhere.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:325: Save(withMeta bool) (DSPropertyMap,
error)
On 2015/07/14 18:55:44, Vadim Sh. wrote:
> document Save?

Done.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:363: // Technically the datastore allows
property names, but all of the SDKs go out
On 2015/07/14 18:55:43, Vadim Sh. wrote:
> nit: "allows arbitrary property names"? I think you forgot a word there...

yes! good catch! Done.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:387: return errors.New("gae: nil
DSPropertyMap")
On 2015/07/14 18:55:43, Vadim Sh. wrote:
> let it panic? It's caller responsibility to not pass nil.
> 
> Actually, it's receiver. So definitely let it panic...

sgtm, done.

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:401: return pm, nil
On 2015/07/14 18:55:44, Vadim Sh. wrote:
> copy pm before returning?

okay :/ I hate copying stuff... GO, Y U NO CONST :(

https://codereview.chromium.org/1227183003/diff/40001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:405: if k[0] == '$' { // remove meta
keys
On 2015/07/14 18:55:43, Vadim Sh. wrote:
> are you 100% sure k != ""?

good thought, done.

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore_impl.go (right):

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:91: if convFailures != nil {
On 2015/07/14 19:34:57, dnj wrote:
> nit: Since you're using it as an array, I would prefer: len(convFailures) > 0

Done.

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/prod/datastore_key.go (right):

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/datastore_key.go:58: // places. Realistically,
everythin should hit the early exit clause above.
On 2015/07/14 19:34:57, dnj wrote:
> "everything" or "everythin'"

Done.

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/properties.go (right):

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:401: ret := make(DSPropertyMap, len(pm))
On 2015/07/14 19:34:57, dnj wrote:
> Maybe include the allocation-avoiding optimization:
> if len(pm) == 0 {
>   return nil, nil
> }

done though it returns DSPropertyMap{} instead of nil. nil DSPropertyMap's are
panic'able offenses.

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:403: if k[0] == '$' { // remove meta
keys
On 2015/07/14 19:34:57, dnj wrote:
> This will have an issue with an empty key (""). Either strip it or check for
it
> before indexing '0'.

Done.

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:404: continue
On 2015/07/14 19:34:57, dnj wrote:
> Since the action is a single line, avoid the "continue" and just invert the
> criteria and append within the "if".

Done.

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:438: if pm == nil {
On 2015/07/14 19:34:57, dnj wrote:
> Why do you differentiate between "nil" and empty? I think you should settle on
> empty (nil inclusive) being the error.

I just don't check anymore... nil DSPropertyMap is a bozo thing to do anyway, so
I'll let go panic if necessary.

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/m...
File go/src/infra/gae/libs/meta/eg.go (right):

https://codereview.chromium.org/1227183003/diff/60001/go/src/infra/gae/libs/m...
go/src/infra/gae/libs/meta/eg.go:34: ret := int64(0)
On 2015/07/14 19:34:57, dnj wrote:
> Why bother with this assignment when you can just assign it on #36?

Done.

[Vadim Sh., 2015-07-14 22:56:08]

lgtm with nit

https://codereview.chromium.org/1227183003/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/properties.go (right):

https://codereview.chromium.org/1227183003/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/properties.go:43: ShouldIndex IndexSetting = false
nit: ShouldIndex == true? It's feels more "truthy".

[dnj, 2015-07-14 22:58:56]

lgtm

[iannucci, 2015-07-14 23:03:06]

On 2015/07/14 22:56:08, Vadim Sh. wrote:
> lgtm with nit
> 
>
https://codereview.chromium.org/1227183003/diff/80001/go/src/infra/gae/libs/g...
> File go/src/infra/gae/libs/gae/properties.go (right):
> 
>
https://codereview.chromium.org/1227183003/diff/80001/go/src/infra/gae/libs/g...
> go/src/infra/gae/libs/gae/properties.go:43: ShouldIndex IndexSetting = false
> nit: ShouldIndex == true? It's feels more "truthy".

addressed in latest patchset... unfortunately ShouldIndex needs to be the
default value, so it needs to be false :(

[iannucci, 2015-07-14 23:03:14]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-07-14 23:03:34]

The CQ bit was unchecked by iannucci@chromium.org

[commit-bot: I haz the power, 2015-07-14 23:03:41]

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

[iannucci, 2015-07-14 23:04:18]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-07-14 23:04:19]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org,
vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/1227183003/#ps100001
(title: "fix No/ShouldIndex")

[commit-bot: I haz the power, 2015-07-14 23:04:37]

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

[commit-bot: I haz the power, 2015-07-14 23:07:31]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/infra/infra/+/36ccdaa83237cd89531af120424b9...