Commits · d95511c57ffb9f61dec8013dfb444c3affa375fe · Linshizhi / V8

22 Dec, 2015 22 commits

PPC: [runtime] Rewrite Function.prototype.toString in C++. · d95511c5

mbrandy authored Dec 22, 2015

Port 4acca53e

Original commit message:
    There's actually no point trying to do Function.prototype.toString in
    JavaScript, as it always calls into C++ at least once, so it only
    complicates things (esp. once we start optimizing bound functions).

    Drive-by-fix: Rename FunctionApply and FunctionCall builtins to also
    reflect the fact that these are builtins in the Function.prototype and
    not on Function itself.

R=bmeurer@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com
BUG=chromium:535408
LOG=n

Review URL: https://codereview.chromium.org/1544833002

Cr-Commit-Position: refs/heads/master@{#33014}

d95511c5

[proxies] Expose proxies in the API · 88b58591

cbruni authored Dec 22, 2015

Add API-accessors for [[ProxyTarget]], [[ProxyHandler]]. Additionally
create new proxies and revoke proxies via the API.

BUG=v8:1543
LOG=n

Review URL: https://codereview.chromium.org/1542943002

Cr-Commit-Position: refs/heads/master@{#33013}

88b58591

[runtime] Also migrate the Function and GeneratorFunction constructors to C++. · d1d4fa20

bmeurer authored Dec 22, 2015

These constructors always go through C++ at least twice anyway, so
there's not really a point in trying to implement them in JavaScript.

R=yangguo@chromium.org
BUG=chromium:535408
LOG=n

Review URL: https://codereview.chromium.org/1548623002

Cr-Commit-Position: refs/heads/master@{#33012}

d1d4fa20

[turbofan] Exhaustive switches for MachineRepresentation. · 739c0187

jarin authored Dec 22, 2015

Review URL: https://codereview.chromium.org/1542093002

Cr-Commit-Position: refs/heads/master@{#33011}

739c0187

[proxies] Better print for proxies in d8 · b7ff2bd5

cbruni authored Dec 22, 2015

Function proxies would not be printed so far since they ended up in Function.prototype.toString which only works with Function as a receiver but no Proxy. Additionally added support for more gracefully dealing with recursive __proto__ structures introduced by proxies.

drive-by-fix: use IS_PROXY if possible in .js files.

BUG=v8:1543
LOG=n

Committed: https://crrev.com/8bfb7189a3472bc9d0820a1bd4534eaaf78ff847
Cr-Commit-Position: refs/heads/master@{#32985}

Review URL: https://codereview.chromium.org/1530293004

Cr-Commit-Position: refs/heads/master@{#33010}

b7ff2bd5

[debugger] step on every assignment in destructuring bind. · b00d9e25

yangguo authored Dec 22, 2015

R=caitpotter88@gmail.com, littledan@chromium.org

Review URL: https://codereview.chromium.org/1542813003

Cr-Commit-Position: refs/heads/master@{#33009}

b00d9e25

Prevent using 0 as random seed. · a2cc715b

yangguo authored Dec 22, 2015

R=bmeurer@chromium.org

Review URL: https://codereview.chromium.org/1541143002

Cr-Commit-Position: refs/heads/master@{#33008}

a2cc715b

[proxies] Limit recursive proxy prototype lookups to 100'000 · e10fdbed

cbruni authored Dec 22, 2015

Creating proto-recursive proxies might lead to instanceof while-looping
endlessly in Object::HasInPrototypeChain (For traps we already have stack
guards in place to prevent stack overflows). We prevent this by limiting
the number of proxies we visit in PrototypeIterator to a magic large number.

LOG=n
BUG=v8:1534

Review URL: https://codereview.chromium.org/1542583003

Cr-Commit-Position: refs/heads/master@{#33007}

e10fdbed

[runtime] Migrate GlobalEval to C++. · a878dcfd

bmeurer authored Dec 22, 2015

The GlobalEval JavaScript function was just a small driver for stuff
implemented in C++ anyway, so there's no point in having it around at
all. The next step will be to move the Function constructor to C++ as
well, which is the other user of %CompileString.

R=yangguo@chromium.org
BUG=chromium:535408
LOG=n

Review URL: https://codereview.chromium.org/1540893004

Cr-Commit-Position: refs/heads/master@{#33006}

a878dcfd

[Interpreter] Allocates new temporary register outside the reservation for consecutive registers. · e7373f42

mythria authored Dec 22, 2015

Consecutive registers are allocated in two passes. First we "reserve"
a set of registers and these get allocated when we actually use them.
If we request for a temporary register before we use all the consecutive
registers, the earlier implementation does not gaurantee that it allocates
outside the reservation for consecutive registers. This could cause problems
for example, in call_func(a, b++, c). This cl fixes
TemporaryRegisterScope::NewRegister, to return a new temporary register
outside the reservation for consecutive registers.

BUG=v8:4280
LOG=N

Review URL: https://codereview.chromium.org/1531273002

Cr-Commit-Position: refs/heads/master@{#33005}

e7373f42

[Interpreter] Adds support for CreateArguments to BytecodeGraphBuilder. · 5dd3122c

mythria authored Dec 22, 2015

Adds implementation and tests for CreateMappedArguments and
CreateUnmappedArguments to bytecode graph builder.

BUG=v8:4280
LOG=N

Review URL: https://codereview.chromium.org/1490283003

Cr-Commit-Position: refs/heads/master@{#33004}

5dd3122c

Reland of [es6] ship regexp sticky flag. (patchset #1 id:1 of... · 155cbad5

littledan authored Dec 22, 2015

Reland of [es6] ship regexp sticky flag. (patchset #1 id:1 of https://codereview.chromium.org/1531043002/ )

Reason for revert:
This can be relanded since the web compatibility fix in https://codereview.chromium.org/1543723002 has been submitted.

Original issue's description:
> Revert of [es6] ship regexp sticky flag. (patchset #2 id:20001 of https://codereview.chromium.org/1509733010/ )
>
> Reason for revert:
> This causes compatibility issues, as documented in https://bugs.chromium.org/p/v8/issues/detail?id=4617#c9
>
> Original issue's description:
> > [es6] ship regexp sticky flag.
> >
> > R=littledan@chromium.org
> > BUG=v8:4342
> > LOG=Y
> >
> > Committed: https://crrev.com/86c2dd4042dc9ce293e004234eb094f2b51d9640
> > Cr-Commit-Position: refs/heads/master@{#32826}
>
> TBR=yangguo@chromium.org
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=v8:4342

TBR=yangguo@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:4342

Review URL: https://codereview.chromium.org/1546563002

Cr-Commit-Position: refs/heads/master@{#33003}

155cbad5

Reland of Add web compat workarounds for ES2015 RegExp semantics (patchset #3... · 424ef009

littledan authored Dec 22, 2015

Reland of Add web compat workarounds for ES2015 RegExp semantics (patchset #3 id:40001 of https://codereview.chromium.org/1543723002/ )

Unexpectedly, websites depend on doing feature testing with
RegExp.prototype.sticky and browser testing with RegExp.prototype.toString().
ES2015 newly throws exceptions for both of these. In order to enable shipping
new ES2015 semantics, this patch puts in narrow workarounds for those two
cases, keeping their old behavior. UseCounters are added for how often
those particular cases come up, so we can see if it can be deprecated.

This reland replaces problematic legacy const usage with var, to
avoid issues with nosnap builds.

R=yangguo
CC=bmeurer
BUG=v8:4637,v8:4617
LOG=Y
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1545633002

Cr-Commit-Position: refs/heads/master@{#33002}

424ef009

Revert of [proxies] Better print for proxies in d8 (patchset #6 id:100001 of... · f4cd91c7

cbruni authored Dec 22, 2015

Revert of [proxies] Better print for proxies in d8 (patchset #6 id:100001 of https://codereview.chromium.org/1530293004/ )

Reason for revert:
needs --allow-natives-syntax

Original issue's description:
> [proxies] Better print for proxies in d8
>
> Function proxies would not be printed so far since they ended up in Function.prototype.toString which only works with Function as a receiver but no Proxy. Additionally added support for more gracefully dealing with recursive __proto__ structures introduced by proxies.
>
> BUG=v8:1543
> LOG=n
>
> Committed: https://crrev.com/8bfb7189a3472bc9d0820a1bd4534eaaf78ff847
> Cr-Commit-Position: refs/heads/master@{#32985}

TBR=neis@chromium.org,yangguo@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:1543

Review URL: https://codereview.chromium.org/1543803002

Cr-Commit-Position: refs/heads/master@{#33001}

f4cd91c7

[debugger] step on every assignment in a destructuring assignment. · 831b7ee3

yangguo authored Dec 22, 2015

R=caitpotter88@gmail.com, littledan@chromium.org
BUG=v8:811
LOG=N

Review URL: https://codereview.chromium.org/1533313002

Cr-Commit-Position: refs/heads/master@{#33000}

831b7ee3

Revert of Add web compat workarounds for ES2015 RegExp semantics (patchset #3... · 08a1d1a2

bmeurer authored Dec 22, 2015

Revert of Add web compat workarounds for ES2015 RegExp semantics (patchset #3 id:40001 of https://codereview.chromium.org/1543723002/ )

Reason for revert:
Breaks nosnap: http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap/builds/5883

Original issue's description:
> Add web compat workarounds for ES2015 RegExp semantics
>
> Unexpectedly, websites depend on doing feature testing with
> RegExp.prototype.sticky and browser testing with RegExp.prototype.toString().
> ES2015 newly throws exceptions for both of these. In order to enable shipping
> new ES2015 semantics, this patch puts in narrow workarounds for those two
> cases, keeping their old behavior. UseCounters are added for how often
> those particular cases come up, so we can see if it can be deprecated.
>
> R=yangguo
> BUG=v8:4637,v8:4617
> LOG=Y
> CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
>
> Committed: https://crrev.com/98f819c3e0c92d54a306cdacadda73cf96d21b52
> Cr-Commit-Position: refs/heads/master@{#32997}

TBR=yangguo@google.com,yangguo@chromium.org,littledan@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:4637,v8:4617

Review URL: https://codereview.chromium.org/1546493003

Cr-Commit-Position: refs/heads/master@{#32999}

08a1d1a2

[turbofan] Remove some dead code in move optimizer. · d33b8f20

mtrofin authored Dec 22, 2015

There was no use for the second temp vector, and the one we
are using is scoped to certain methods, and that scope can be
further restricted.

I'm curious if there is really any value in having the temp
vector instead of allocating a function-scoped local. Will
verify that separately.

Review URL: https://codereview.chromium.org/1533423003

Cr-Commit-Position: refs/heads/master@{#32998}

d33b8f20

Add web compat workarounds for ES2015 RegExp semantics · 98f819c3

littledan authored Dec 22, 2015

Unexpectedly, websites depend on doing feature testing with
RegExp.prototype.sticky and browser testing with RegExp.prototype.toString().
ES2015 newly throws exceptions for both of these. In order to enable shipping
new ES2015 semantics, this patch puts in narrow workarounds for those two
cases, keeping their old behavior. UseCounters are added for how often
those particular cases come up, so we can see if it can be deprecated.

R=yangguo
BUG=v8:4637,v8:4617
LOG=Y
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1543723002

Cr-Commit-Position: refs/heads/master@{#32997}

98f819c3

[runtime] Rewrite Function.prototype.toString in C++. · 4acca53e

bmeurer authored Dec 22, 2015

There's actually no point trying to do Function.prototype.toString in
JavaScript, as it always calls into C++ at least once, so it only
complicates things (esp. once we start optimizing bound functions).

Drive-by-fix: Rename FunctionApply and FunctionCall builtins to also
reflect the fact that these are builtins in the Function.prototype and
not on Function itself.

TBR=hpayer@chromium.org
R=yangguo@chromium.org
BUG=chromium:535408
LOG=n

Review URL: https://codereview.chromium.org/1540953004

Cr-Commit-Position: refs/heads/master@{#32996}

4acca53e

Update V8 DEPS. · 0589e842

v8-autoroll authored Dec 22, 2015

Rolling v8/tools/clang to 693bc799b83f3a0bb2dc9b035a2ba3118398a582

TBR=machenbach@chromium.org,vogelheim@chromium.org,hablich@chromium.org

Review URL: https://codereview.chromium.org/1546553002

Cr-Commit-Position: refs/heads/master@{#32995}

0589e842

test262 roll, as of 17-12-2015 · 492d93a0

littledan authored Dec 22, 2015

Review URL: https://codereview.chromium.org/1539843003

Cr-Commit-Position: refs/heads/master@{#32994}

492d93a0

postmortem: Remove Context::GLOBAL_OBJECT_INDEX · 638e20da

ofrobots authored Dec 22, 2015

Context::GLOBAL_OBJECT_INDEX has been replaced by NATIVE_CONTEXT_INDEX in
https://codereview.chromium.org/1480003002. Update the postmortem data generator
to reflect this change.

R=bmeurer@chromium.org,yangguo@chromium.org
BUG=

Review URL: https://codereview.chromium.org/1542753002

Cr-Commit-Position: refs/heads/master@{#32993}

638e20da

21 Dec, 2015 12 commits

[ES6] Stage sloppy function block scoping · 4fb5a9ff

hablich authored Dec 21, 2015

The Inbox problem got resolved so staging is ok.

BUG=v8:3305
LOG=Y
R=adamk@chromium.org, littledan@chromium.org,rossberg@chromium.org

Review URL: https://codereview.chromium.org/1538243002

Cr-Commit-Position: refs/heads/master@{#32992}

4fb5a9ff

[es6] use correct --harmony-destructuring-assignment flag when preparsing · 76f6d2a3

caitpotter88 authored Dec 21, 2015

BUG=v8:811, v8:4636
LOG=N
R=adamk@chromium.org, yangguo@chromium.org

Review URL: https://codereview.chromium.org/1544483002

Cr-Commit-Position: refs/heads/master@{#32991}

76f6d2a3

Reland of "MIPS64: Fix trunc_l_[s,d] in simulator." · e93b8306

akos.palfi authored Dec 21, 2015

The trunc_l_[s,d] instructions incorrectly returns success when the input is INT64_MAX.

TEST=test-run-machops/RunTryTruncateFloat32ToInt64WithCheck,test-run-machops/RunTryTruncateFloat64ToInt64WithCheck
BUG=

Review URL: https://codereview.chromium.org/1542673002

Cr-Commit-Position: refs/heads/master@{#32990}

e93b8306

Revert of MIPS64: Fix trunc_l_[s,d] in simulator. (patchset #1 id:1 of... · 3e27f6d5

akos.palfi authored Dec 21, 2015

Revert of MIPS64: Fix trunc_l_[s,d] in simulator. (patchset #1 id:1 of https://codereview.chromium.org/1539763003/ )

Reason for revert:
Causes failures in r6 mode.

Original issue's description:
> MIPS64: Fix trunc_l_[s,d] in simulator.
>
> The trunc_l_[s,d] instructions incorrectly returns success when the input is INT64_MAX.
>
> TEST=test-run-machops/RunTryTruncateFloat32ToInt64WithCheck,test-run-machops/RunTryTruncateFloat64ToInt64WithCheck
> BUG=
>
> Committed: https://crrev.com/53a0cc846661c65b4b1711f67642677d8da69119
> Cr-Commit-Position: refs/heads/master@{#32968}

TBR=paul.lind@imgtec.com,balazs.kilvady@imgtec.com,ahaas@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=

Review URL: https://codereview.chromium.org/1543613002

Cr-Commit-Position: refs/heads/master@{#32989}

3e27f6d5

X87: Change the test case for X87 RunFloat64Add and RunFloat64Sub · dd31b080

zhengxing.li authored Dec 21, 2015

The CL #32908 (https://codereview.chromium.org/1526293002) updated the Float64 test data and cause the RunFloat64Add and RunFloat64Sub test cases failed.

The reason is same as the CL #31808 (issue 1430943002, X87: Change the test case for X87 float operations), please refer: https://codereview.chromium.org/1430943002/

Here is the key comments from CL #31808
Some new test cases use CheckFloatEq(...) and CheckDoubleEq(...) function for result check. When GCC compiling the CheckFloatEq() and CheckDoubleEq() function,
those inlined functions has different behavior comparing with GCC ia32 build and x87 build.
The major difference is sse float register still has single precision rounding semantic. While X87 register has no such rounding precsion semantic when directly use register value.
The V8 turbofan JITTed has exactly same result in both X87 and IA32 port.

So we add the following sentence to do type case to keep the same precision for RunFloat64Add and RunFloat64Sub.
Such as: volatile double expect = *i +/- *j; // *i +/- *j, etc.

BUG=

Review URL: https://codereview.chromium.org/1533593003

Cr-Commit-Position: refs/heads/master@{#32988}

dd31b080

[harmony] stage regexp lookbehind assertion. · cd3054bf

yangguo authored Dec 21, 2015

R=hablich@chromium.org
BUG=v8:4545
LOG=N

Review URL: https://codereview.chromium.org/1537273004

Cr-Commit-Position: refs/heads/master@{#32987}

cd3054bf

[Interpreter] Implement ForIn in bytecode graph builder. · 4926be6e

oth authored Dec 21, 2015

A pre-requisite for this change was changing the interpreter to use
Runtime::ForInStep to bring the interpreter implementation closer
to the turbofan implementation. Also required to flatten out the
cache parameters into the interpreter frame for de-opt.

BUG=v8:4280
LOG=N

Review URL: https://codereview.chromium.org/1531693002

Cr-Commit-Position: refs/heads/master@{#32986}

4926be6e

[proxies] Better print for proxies in d8 · 8bfb7189

cbruni authored Dec 21, 2015

BUG=v8:1543
LOG=n

Review URL: https://codereview.chromium.org/1530293004

Cr-Commit-Position: refs/heads/master@{#32985}

8bfb7189

[wasm] Fixed float-to-int32 conversion to match the spec. · dcac1f14

ahaas authored Dec 21, 2015

The new implementation detects if the input value is outside i32 range
and traps it that case.

The range check is done as follows:
The input value is converted to int32 and then back to float. If the
result is the same as the truncated input value, then the input value
is within int32 range.

R=bmeurer@chromium.org

Review URL: https://codereview.chromium.org/1537393003

Cr-Commit-Position: refs/heads/master@{#32984}

dcac1f14

MIPS: Fix uninitialized upper word bits for Cvt_d_uw macro. · 108d5264

ivica.bogosavljevic authored Dec 21, 2015

After Cvt_d_uw macro, upper 32 bits of the output remain
unitnitialized which caused flaky failures on some tests on
MIPS32R6

TEST=cctest/test-assembler-mips/MIPS13,mjsunit/asm/int32-umod
BUG=

Review URL: https://codereview.chromium.org/1537973002

Cr-Commit-Position: refs/heads/master@{#32983}

108d5264

[turbofan] Pass type information of arguments to EmitPrepareArguments. · c0c8c756

ahaas authored Dec 21, 2015

On ia32 the code which pushes parameters on the stack depends on the
types of the parameters which are to be pushed. I provide this type
information now by not only passing parameter nodes to
EmitPrepareArguments, but also the index in the call descriptor which
belongs to the parameter nodes.

This type information will also be necessary if we want to use the
PokePair instruction on arm64 again.

R=bradnelson@chromium.org, bmeurer@chromium.org

Review URL: https://codereview.chromium.org/1534593004

Cr-Commit-Position: refs/heads/master@{#32982}

c0c8c756

X87: [Interpreter] Add basic deoptimization support from TurboFan to Ignition. · bf8c5160

zhengxing.li authored Dec 21, 2015

  port b10d24ff(r32971)

  original commit message:
  Adds support for generating deoptimization translations for interpreter
  stack frames, and building interpreter frames for these translations
  when a function deopts. Also adds builtins for
  InterpreterNotifyDeoptimized which resume the function's continuation at
  the correct point in the interpreter after deopt.

  MIPS patch contributed by balazs.kilvady@igmtec.com

BUG=

Review URL: https://codereview.chromium.org/1543433002

Cr-Commit-Position: refs/heads/master@{#32981}

bf8c5160

20 Dec, 2015 1 commit

Mark all APIs without callers in Blink as deprecated · 0018ca5e

jochen authored Dec 20, 2015

R=vogelheim@chromium.org
BUG=none
LOG=y

Review URL: https://codereview.chromium.org/1526643002

Cr-Commit-Position: refs/heads/master@{#32980}

0018ca5e

19 Dec, 2015 2 commits

Update V8 DEPS. · 8e60df2a

v8-autoroll authored Dec 19, 2015

Rolling v8/tools/clang to 0983acc03554d2e1be1eb3b37ec7905474f541dc

TBR=machenbach@chromium.org,vogelheim@chromium.org,hablich@chromium.org

Review URL: https://codereview.chromium.org/1533083005

Cr-Commit-Position: refs/heads/master@{#32979}

8e60df2a

Revert of [arm64] Improve some new builtins. (patchset #1 id:1 of... · 826afb30

machenbach authored Dec 19, 2015

Revert of [arm64] Improve some new builtins. (patchset #1 id:1 of https://codereview.chromium.org/1537903004/ )

Reason for revert:
[Sheriff] Breaks arm64 nosnap debug:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20sim%20-%20nosnap%20-%20debug/builds/753

Original issue's description:
> [arm64] Improve some new builtins.
>
> - Simplify the variable-length pop sequence on entry. (It now uses
>   smaller code with no branches.)
> - Use conditional compare to merge branches where appropriate.
> - Make use of Ldrsw + UntagSmiFieldMemOperand to load smis more
>   efficiently.
> - Only load 'undefined' and 'null' once per builtin.
> - A few other small improvements.
>
> BUG=
>
> Committed: https://crrev.com/efc641a7be54978cd85a4dcdf1950cd97725917d
> Cr-Commit-Position: refs/heads/master@{#32975}

TBR=bmeurer@chromium.org,yangguo@chromium.org,jacob.bramley@arm.com
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=

Review URL: https://codereview.chromium.org/1539073002

Cr-Commit-Position: refs/heads/master@{#32978}

826afb30

18 Dec, 2015 3 commits

PPC: [Interpreter] Add basic deoptimization support from TurboFan to Ignition. · f5261a6a

mbrandy authored Dec 18, 2015

Port b10d24ff

Original commit message:
    Adds support for generating deoptimization translations for interpreter
    stack frames, and building interpreter frames for these translations
    when a function deopts. Also adds builtins for
    InterpreterNotifyDeoptimized which resume the function's continuation at
    the correct point in the interpreter after deopt.

R=rmcilroy@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com
BUG=v8:4280
LOG=N

Review URL: https://codereview.chromium.org/1541483002

Cr-Commit-Position: refs/heads/master@{#32977}

f5261a6a

PPC: Fix simulator overflow detection for float -> integer conversions. · 5fa2a112

mbrandy authored Dec 18, 2015

R=joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com
BUG=

Review URL: https://codereview.chromium.org/1538943003

Cr-Commit-Position: refs/heads/master@{#32976}

5fa2a112

[arm64] Improve some new builtins. · efc641a7

jacob.bramley authored Dec 18, 2015

- Simplify the variable-length pop sequence on entry. (It now uses
  smaller code with no branches.)
- Use conditional compare to merge branches where appropriate.
- Make use of Ldrsw + UntagSmiFieldMemOperand to load smis more
  efficiently.
- Only load 'undefined' and 'null' once per builtin.
- A few other small improvements.

BUG=

Review URL: https://codereview.chromium.org/1537903004

Cr-Commit-Position: refs/heads/master@{#32975}

efc641a7