Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
892953004:
Simplify cpu-profiler test code with help of wrappers. (Closed)
DescriptionSimplify cpu-profiler test code with help of wrappers.
BTW: a few fixes for string comparison
BUG=none
LOG=n
Committed: https://crrev.com/fb73392bfd41e84bdb029c4fded78bd2df010040
Cr-Commit-Position: refs/heads/master@{#26495}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : comments addressed #Messages
Total messages: 16 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
loislo@chromium.org changed reviewers: + alph@chromium.org, mstarzinger@chromium.org, svenpanne@chromium.org
PTAL The patch is mostly cosmetic and has a few fixes in string comparison.
lgtm https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:186: CHECK_EQ(0, strcmp(aaa_str, aaa->name())); why's that?
LGTM with comments. https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:53: static v8::Handle<v8::Script> Compile(const char* utf8_source) { Can we use v8_compile for this? https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:58: static v8::Local<v8::Function> GetFunction(LocalContext& env, Can we drop this one and always use the one below? IIUC it only requires one additional '*' character. https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:561: Compile(cpu_profiler_test_source)->Run(); Can we use CompileRun() here and for the other occurences below?
New patchsets have been uploaded after l-g-t-m from alph@chromium.org,mstarzinger@chromium.org
https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:53: static v8::Handle<v8::Script> Compile(const char* utf8_source) { On 2015/02/06 14:26:40, Michael Starzinger wrote: > Can we use v8_compile for this? Done. https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:58: static v8::Local<v8::Function> GetFunction(LocalContext& env, On 2015/02/06 14:26:40, Michael Starzinger wrote: > Can we drop this one and always use the one below? IIUC it only requires one > additional '*' character. Done. https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:186: CHECK_EQ(0, strcmp(aaa_str, aaa->name())); On 2015/02/06 13:18:07, alph wrote: > why's that? Direct comparison doesn't work for raw strings. https://codereview.chromium.org/892953004/diff/60001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:561: Compile(cpu_profiler_test_source)->Run(); On 2015/02/06 14:26:40, Michael Starzinger wrote: > Can we use CompileRun() here and for the other occurences below? Done.
The CQ bit was checked by loislo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/892953004/80001
The CQ bit was unchecked by loislo@chromium.org
The CQ bit was checked by loislo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/892953004/80001
Message was sent while issue was closed.
Committed patchset #2 (id:80001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/fb73392bfd41e84bdb029c4fded78bd2df010040 Cr-Commit-Position: refs/heads/master@{#26495} |
