Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2190973003:
[Tracing] V8 Tracing Controller - Add args and copy support (Closed)
DescriptionAdd args and copy support to V8 tracing controller.
BUG=v8:4561
Committed: https://crrev.com/9a6a56d21fd606415685d2078d120f6acb9a2027
Cr-Commit-Position: refs/heads/master@{#38255}
Patch Set 1 #Patch Set 2 : Fix deps #Patch Set 3 : More fixes #
Total comments: 6
Patch Set 4 : Remove JSON-escaping #
Total comments: 5
Patch Set 5 : Change to include base/trace_event/common/trace_event_common.h #Patch Set 6 : Fix deps #
Total comments: 4
Patch Set 7 : Fix copy tests #
Total comments: 14
Patch Set 8 : Address comments + Fix trace id output #Patch Set 9 : Ensure NULL strings are output correctly in TraceWriter #
Total comments: 6
Patch Set 10 : Use V8_INLINE static for non-class functions #
Total comments: 1
Messages
Total messages: 79 (52 generated)
The CQ bit was checked by rskang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
The CQ bit was checked by rskang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
The CQ bit was checked by rskang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add args and copy support BUG= ========== to ========== Add args and copy support Note that v8-tracing.h includes v8.h for Isolate, which is required for escaping JSON strings in TraceWriter. This faces the same issue as the TraceConfig JSON parser in https://codereview.chromium.org/2137013006/#ps200001. If we do not want this dependency, we can shift JSONTraceWriter to only be usable by d8. BUG= ==========
rskang@google.com changed reviewers: + fmeawad@google.com, lpy@chromium.org, mattloring@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2190973003/diff/40001/include/libplatform/v8-... File include/libplatform/v8-tracing.h (right): https://codereview.chromium.org/2190973003/diff/40001/include/libplatform/v8-... include/libplatform/v8-tracing.h:12: #include "include/v8.h" Please do not include v8.h here. https://codereview.chromium.org/2190973003/diff/40001/src/libplatform/tracing... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2190973003/diff/40001/src/libplatform/tracing... src/libplatform/tracing/trace-object.cc:9: #include "src/tracing/trace-event.h" I understand you need to use variables in the header file, but libplatform should not depend on V8. I will leave fadi@ to give suggestions. https://codereview.chromium.org/2190973003/diff/40001/src/libplatform/tracing... File src/libplatform/tracing/trace-writer.cc (right): https://codereview.chromium.org/2190973003/diff/40001/src/libplatform/tracing... src/libplatform/tracing/trace-writer.cc:75: EscapeJSONString(isolate_, value.as_string ? value.as_string : "NULL", Why do you think this should be escaped? Can you give use cases using TRACE_EVENT_* macros. Can't we just append the string?
https://codereview.chromium.org/2190973003/diff/40001/src/libplatform/tracing... File src/libplatform/tracing/trace-writer.cc (right): https://codereview.chromium.org/2190973003/diff/40001/src/libplatform/tracing... src/libplatform/tracing/trace-writer.cc:75: EscapeJSONString(isolate_, value.as_string ? value.as_string : "NULL", On 2016/07/28 21:18:20, lpy wrote: > Why do you think this should be escaped? Can you give use cases using > TRACE_EVENT_* macros. > > Can't we just append the string? Example, for single string argument: TRACE_EVENT1("v8", "v8.Test.ll", "ll", "\t\"100\"\n"); Expected output: ... "args":{"ll":"\t\"100\"\n"} ... Actual output: ... "args":{"ll":" "100" "} ...
The CQ bit was checked by rskang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add args and copy support Note that v8-tracing.h includes v8.h for Isolate, which is required for escaping JSON strings in TraceWriter. This faces the same issue as the TraceConfig JSON parser in https://codereview.chromium.org/2137013006/#ps200001. If we do not want this dependency, we can shift JSONTraceWriter to only be usable by d8. BUG= ========== to ========== Add args and copy support BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2190973003/diff/40001/include/libplatform/v8-... File include/libplatform/v8-tracing.h (right): https://codereview.chromium.org/2190973003/diff/40001/include/libplatform/v8-... include/libplatform/v8-tracing.h:12: #include "include/v8.h" On 2016/07/28 21:18:20, lpy wrote: > Please do not include v8.h here. Done.
https://codereview.chromium.org/2190973003/diff/40001/src/libplatform/tracing... File src/libplatform/tracing/trace-writer.cc (right): https://codereview.chromium.org/2190973003/diff/40001/src/libplatform/tracing... src/libplatform/tracing/trace-writer.cc:75: EscapeJSONString(isolate_, value.as_string ? value.as_string : "NULL", On 2016/07/28 21:25:54, rskang wrote: > On 2016/07/28 21:18:20, lpy wrote: > > Why do you think this should be escaped? Can you give use cases using > > TRACE_EVENT_* macros. > > > > Can't we just append the string? > > Example, for single string argument: > TRACE_EVENT1("v8", "v8.Test.ll", "ll", "\t\"100\"\n"); > > Expected output: > ... "args":{"ll":"\t\"100\"\n"} ... > > Actual output: > ... "args":{"ll":" "100" > "} ... I have removed JSON escaping for a later CL.
On 2016/07/28 22:45:36, rskang wrote: > https://codereview.chromium.org/2190973003/diff/40001/src/libplatform/tracing... > File src/libplatform/tracing/trace-writer.cc (right): > > https://codereview.chromium.org/2190973003/diff/40001/src/libplatform/tracing... > src/libplatform/tracing/trace-writer.cc:75: EscapeJSONString(isolate_, > value.as_string ? value.as_string : "NULL", > On 2016/07/28 21:25:54, rskang wrote: > > On 2016/07/28 21:18:20, lpy wrote: > > > Why do you think this should be escaped? Can you give use cases using > > > TRACE_EVENT_* macros. > > > > > > Can't we just append the string? > > > > Example, for single string argument: > > TRACE_EVENT1("v8", "v8.Test.ll", "ll", "\t\"100\"\n"); > > > > Expected output: > > ... "args":{"ll":"\t\"100\"\n"} ... > > > > Actual output: > > ... "args":{"ll":" "100" > > "} ... > > I have removed JSON escaping for a later CL. PTAL
https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... src/libplatform/tracing/trace-object.cc:9: #include "src/tracing/trace-event.h" libplatform should not depend on V8. https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... File src/libplatform/tracing/trace-writer.cc (right): https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... src/libplatform/tracing/trace-writer.cc:10: #include "src/tracing/trace-event.h" same here.
https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... src/libplatform/tracing/trace-object.cc:9: #include "src/tracing/trace-event.h" On 2016/07/28 22:53:54, lpy wrote: > libplatform should not depend on V8. I do need the TRACE_EVENT_FLAG_* and TRACE_EVENT_TYPE_* macros from base/trace_event/common/trace_event_common.h. Should we shift this file to include/libplatform too? This will allow V8 embedders to also have access to the TRACE_EVENT* macros.
On 2016/07/28 23:07:09, rskang wrote: > https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... > File src/libplatform/tracing/trace-object.cc (right): > > https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... > src/libplatform/tracing/trace-object.cc:9: #include "src/tracing/trace-event.h" > On 2016/07/28 22:53:54, lpy wrote: > > libplatform should not depend on V8. > > I do need the TRACE_EVENT_FLAG_* and TRACE_EVENT_TYPE_* macros from > base/trace_event/common/trace_event_common.h. Should we shift this file to > include/libplatform too? This will allow V8 embedders to also have access to the > TRACE_EVENT* macros. No, I think you can include base/trace_event/common/trace_event_common.h.
The CQ bit was checked by rskang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rskang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/28 23:08:24, lpy wrote: > On 2016/07/28 23:07:09, rskang wrote: > > > https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... > > File src/libplatform/tracing/trace-object.cc (right): > > > > > https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... > > src/libplatform/tracing/trace-object.cc:9: #include > "src/tracing/trace-event.h" > > On 2016/07/28 22:53:54, lpy wrote: > > > libplatform should not depend on V8. > > > > I do need the TRACE_EVENT_FLAG_* and TRACE_EVENT_TYPE_* macros from > > base/trace_event/common/trace_event_common.h. Should we shift this file to > > include/libplatform too? This will allow V8 embedders to also have access to > the > > TRACE_EVENT* macros. > > No, I think you can include base/trace_event/common/trace_event_common.h. Changed to include base/trace_event/common/trace_event_common.h. Thanks @lpy for the Chromium BUILD.gn fix.
https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... src/libplatform/tracing/trace-object.cc:9: #include "src/tracing/trace-event.h" On 2016/07/28 23:07:09, rskang wrote: > On 2016/07/28 22:53:54, lpy wrote: > > libplatform should not depend on V8. > > I do need the TRACE_EVENT_FLAG_* and TRACE_EVENT_TYPE_* macros from > base/trace_event/common/trace_event_common.h. Should we shift this file to > include/libplatform too? This will allow V8 embedders to also have access to the > TRACE_EVENT* macros. Changed to include base/trace_event/common/trace_event_common.h. https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... File src/libplatform/tracing/trace-writer.cc (right): https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... src/libplatform/tracing/trace-writer.cc:10: #include "src/tracing/trace-event.h" On 2016/07/28 22:53:55, lpy wrote: > same here. Changed to include base/trace_event/common/trace_event_common.h.
On 2016/07/29 00:04:23, rskang wrote: > https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... > File src/libplatform/tracing/trace-object.cc (right): > > https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... > src/libplatform/tracing/trace-object.cc:9: #include "src/tracing/trace-event.h" > On 2016/07/28 23:07:09, rskang wrote: > > On 2016/07/28 22:53:54, lpy wrote: > > > libplatform should not depend on V8. > > > > I do need the TRACE_EVENT_FLAG_* and TRACE_EVENT_TYPE_* macros from > > base/trace_event/common/trace_event_common.h. Should we shift this file to > > include/libplatform too? This will allow V8 embedders to also have access to > the > > TRACE_EVENT* macros. > > Changed to include base/trace_event/common/trace_event_common.h. > > https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... > File src/libplatform/tracing/trace-writer.cc (right): > > https://codereview.chromium.org/2190973003/diff/60001/src/libplatform/tracing... > src/libplatform/tracing/trace-writer.cc:10: #include "src/tracing/trace-event.h" > On 2016/07/28 22:53:55, lpy wrote: > > same here. > > Changed to include base/trace_event/common/trace_event_common.h. Dependency problems are fixed. PTAL
Description was changed from ========== Add args and copy support BUG= ========== to ========== Add args and copy support to V8 tracing controller. BUG= ==========
https://codereview.chromium.org/2190973003/diff/100001/test/cctest/libplatfor... File test/cctest/libplatform/test-tracing.cc (right): https://codereview.chromium.org/2190973003/diff/100001/test/cctest/libplatfor... test/cctest/libplatform/test-tracing.cc:261: TRACE_EVENT_COPY_INSTANT0(v8.c_str(), mm.c_str(), TRACE_EVENT_SCOPE_THREAD); Could we additionally add a test that modifies the string argument between when the trace event is generated and when it is serialized to ensure the copying was done.
The CQ bit was checked by rskang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2190973003/diff/100001/test/cctest/libplatfor... File test/cctest/libplatform/test-tracing.cc (right): https://codereview.chromium.org/2190973003/diff/100001/test/cctest/libplatfor... test/cctest/libplatform/test-tracing.cc:261: TRACE_EVENT_COPY_INSTANT0(v8.c_str(), mm.c_str(), TRACE_EVENT_SCOPE_THREAD); On 2016/07/29 01:50:50, mattloring wrote: > Could we additionally add a test that modifies the string argument between when > the trace event is generated and when it is serialized to ensure the copying was > done. Yes my bad, the copy test did not test anything previously. I've added a patch for it.
fmeawad@chromium.org changed reviewers: + fmeawad@chromium.org
https://codereview.chromium.org/2190973003/diff/100001/test/cctest/libplatfor... File test/cctest/libplatform/test-tracing.cc (right): https://codereview.chromium.org/2190973003/diff/100001/test/cctest/libplatfor... test/cctest/libplatform/test-tracing.cc:264: TRACE_EVENT_COPY_INSTANT2(v8.c_str(), mm.c_str(), TRACE_EVENT_SCOPE_THREAD, /s/v8.c_str()/"v8"/ Category has to be a constant string Although it is constant here, it gives the impression that it is not. I would hate somebody looking at the test case and thinking they can get away with something similar in the code but dynamic instead. Event name does not have the same strict rules, but in some cases it does, so ditto on that too.
lgtm w/ question https://codereview.chromium.org/2190973003/diff/120001/src/libplatform/tracin... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2190973003/diff/120001/src/libplatform/tracin... src/libplatform/tracing/trace-object.cc:130: arg_names_[i] = NULL; Why are arg names nulled when an object is updated?
fmeawad@chromium.org changed reviewers: - fmeawad@google.com
Loog Good. Some minor comments. https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... File include/libplatform/v8-tracing.h (left): https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... include/libplatform/v8-tracing.h:58: int num_args_; I don't think you should remove num_args_, I think you should keep the value passed in and/or clamp it as you did in the implementation. But leave the member and add a getter. https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... File include/libplatform/v8-tracing.h (right): https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... include/libplatform/v8-tracing.h:20: union TraceValue { We will need to add TracedValue later See: https://cs.chromium.org/chromium/src/cc/debug/traced_value.h?q=Traced&sq=pack... So this name might be confusing, maybe ArgValue? https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... include/libplatform/v8-tracing.h:54: uint64_t bind_id() const { return bind_id_; } missing num_args getter? https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... include/libplatform/v8-tracing.h:55: TraceValue* arg_values() { return arg_values_; } nit: arg_names arg_types arg_values To match the parameters order https://codereview.chromium.org/2190973003/diff/120001/src/libplatform/tracin... File src/libplatform/tracing/trace-writer.cc (right): https://codereview.chromium.org/2190973003/diff/120001/src/libplatform/tracin... src/libplatform/tracing/trace-writer.cc:16: bool IsJSONString(const char* str) { Add a comment on why we need this function. https://codereview.chromium.org/2190973003/diff/120001/src/libplatform/tracin... src/libplatform/tracing/trace-writer.cc:112: for (int i = 0; i < kTraceMaxNumArgs && arg_names[i]; ++i) { Keep num_args_ and use it here.
Looks Good. Some minor comments.
The CQ bit was checked by rskang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Addressed comments. PTAL https://codereview.chromium.org/2190973003/diff/100001/test/cctest/libplatfor... File test/cctest/libplatform/test-tracing.cc (right): https://codereview.chromium.org/2190973003/diff/100001/test/cctest/libplatfor... test/cctest/libplatform/test-tracing.cc:264: TRACE_EVENT_COPY_INSTANT2(v8.c_str(), mm.c_str(), TRACE_EVENT_SCOPE_THREAD, On 2016/07/29 14:32:20, fmeawad wrote: > /s/v8.c_str()/"v8"/ > Category has to be a constant string > Although it is constant here, it gives the impression that it is not. > I would hate somebody looking at the test case and thinking they can get away > with something similar in the code but dynamic instead. > > Event name does not have the same strict rules, but in some cases it does, so > ditto on that too. My bad, the v8.c_str() should not be there. This is fixed in the new patch. https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... File include/libplatform/v8-tracing.h (left): https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... include/libplatform/v8-tracing.h:58: int num_args_; On 2016/07/29 14:57:06, fmeawad wrote: > I don't think you should remove num_args_, I think you should keep the value > passed in and/or clamp it as you did in the implementation. But leave the member > and add a getter. Done. https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... File include/libplatform/v8-tracing.h (right): https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... include/libplatform/v8-tracing.h:20: union TraceValue { On 2016/07/29 14:57:06, fmeawad wrote: > We will need to add TracedValue later > See: > https://cs.chromium.org/chromium/src/cc/debug/traced_value.h?q=Traced&sq=pack... > > So this name might be confusing, maybe ArgValue? Done. https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... include/libplatform/v8-tracing.h:54: uint64_t bind_id() const { return bind_id_; } On 2016/07/29 14:57:06, fmeawad wrote: > missing num_args getter? Done. https://codereview.chromium.org/2190973003/diff/120001/include/libplatform/v8... include/libplatform/v8-tracing.h:55: TraceValue* arg_values() { return arg_values_; } On 2016/07/29 14:57:06, fmeawad wrote: > nit: > arg_names > arg_types > arg_values > To match the parameters order Done. https://codereview.chromium.org/2190973003/diff/120001/src/libplatform/tracin... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2190973003/diff/120001/src/libplatform/tracin... src/libplatform/tracing/trace-object.cc:130: arg_names_[i] = NULL; On 2016/07/29 14:32:42, mattloring wrote: > Why are arg names nulled when an object is updated? This was to indicate the number of arguments. But since we are storing num_args now, this is not needed. https://codereview.chromium.org/2190973003/diff/120001/src/libplatform/tracin... File src/libplatform/tracing/trace-writer.cc (right): https://codereview.chromium.org/2190973003/diff/120001/src/libplatform/tracin... src/libplatform/tracing/trace-writer.cc:16: bool IsJSONString(const char* str) { On 2016/07/29 14:57:06, fmeawad wrote: > Add a comment on why we need this function. Done. https://codereview.chromium.org/2190973003/diff/120001/src/libplatform/tracin... src/libplatform/tracing/trace-writer.cc:112: for (int i = 0; i < kTraceMaxNumArgs && arg_names[i]; ++i) { On 2016/07/29 14:57:06, fmeawad wrote: > Keep num_args_ and use it here. Done.
The CQ bit was checked by rskang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM from my side, but wait for Fadi's review. A few nits. https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... src/libplatform/tracing/trace-object.cc:17: size_t GetAllocLength(const char* str) { return str ? strlen(str) + 1 : 0; } V8_INLINE, maybe also static? I am not sure if static initialization check will complain. https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... File src/libplatform/tracing/trace-writer.cc (right): https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... src/libplatform/tracing/trace-writer.cc:7: #include <cmath> From the coding style I can see we use match.h rather than c-prefix, but I also see old code using c-prefix style, I recommend to use <math.h>. https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... src/libplatform/tracing/trace-writer.cc:19: bool IsJSONString(const char* str) { V8_INLINE, maybe also static? I am not sure if static initialization check will complain.
The CQ bit was checked by rskang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... src/libplatform/tracing/trace-object.cc:17: size_t GetAllocLength(const char* str) { return str ? strlen(str) + 1 : 0; } On 2016/08/02 00:07:09, lpy wrote: > V8_INLINE, maybe also static? I am not sure if static initialization check will > complain. Done. https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... File src/libplatform/tracing/trace-writer.cc (right): https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... src/libplatform/tracing/trace-writer.cc:7: #include <cmath> On 2016/08/02 00:07:09, lpy wrote: > From the coding style I can see we use match.h rather than c-prefix, but I also > see old code using c-prefix style, I recommend to use <math.h>. Acknowledged. https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... src/libplatform/tracing/trace-writer.cc:19: bool IsJSONString(const char* str) { On 2016/08/02 00:07:09, lpy wrote: > V8_INLINE, maybe also static? I am not sure if static initialization check will > complain. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/02 00:46:55, rskang wrote: > https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... > File src/libplatform/tracing/trace-object.cc (right): > > https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... > src/libplatform/tracing/trace-object.cc:17: size_t GetAllocLength(const char* > str) { return str ? strlen(str) + 1 : 0; } > On 2016/08/02 00:07:09, lpy wrote: > > V8_INLINE, maybe also static? I am not sure if static initialization check > will > > complain. > > Done. > > https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... > File src/libplatform/tracing/trace-writer.cc (right): > > https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... > src/libplatform/tracing/trace-writer.cc:7: #include <cmath> > On 2016/08/02 00:07:09, lpy wrote: > > From the coding style I can see we use match.h rather than c-prefix, but I > also > > see old code using c-prefix style, I recommend to use <math.h>. > > Acknowledged. > > https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... > src/libplatform/tracing/trace-writer.cc:19: bool IsJSONString(const char* str) { > On 2016/08/02 00:07:09, lpy wrote: > > V8_INLINE, maybe also static? I am not sure if static initialization check > will > > complain. > > Done. LGTM
On 2016/08/02 11:04:49, fmeawad wrote: > On 2016/08/02 00:46:55, rskang wrote: > > > https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... > > File src/libplatform/tracing/trace-object.cc (right): > > > > > https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... > > src/libplatform/tracing/trace-object.cc:17: size_t GetAllocLength(const char* > > str) { return str ? strlen(str) + 1 : 0; } > > On 2016/08/02 00:07:09, lpy wrote: > > > V8_INLINE, maybe also static? I am not sure if static initialization check > > will > > > complain. > > > > Done. > > > > > https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... > > File src/libplatform/tracing/trace-writer.cc (right): > > > > > https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... > > src/libplatform/tracing/trace-writer.cc:7: #include <cmath> > > On 2016/08/02 00:07:09, lpy wrote: > > > From the coding style I can see we use match.h rather than c-prefix, but I > > also > > > see old code using c-prefix style, I recommend to use <math.h>. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2190973003/diff/160001/src/libplatform/tracin... > > src/libplatform/tracing/trace-writer.cc:19: bool IsJSONString(const char* str) > { > > On 2016/08/02 00:07:09, lpy wrote: > > > V8_INLINE, maybe also static? I am not sure if static initialization check > > will > > > complain. > > > > Done. > > LGTM Please add a BUG # in the description.
fmeawad@chromium.org changed reviewers: + yangguo@chromium.org
fmeawad@chromium.org changed reviewers: + jochen@chromium.org
lgtm
Description was changed from ========== Add args and copy support to V8 tracing controller. BUG= ========== to ========== Add args and copy support to V8 tracing controller. BUG=v8:4561 ==========
The CQ bit was checked by rskang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattloring@google.com, lpy@chromium.org Link to the patchset: https://codereview.chromium.org/2190973003/#ps180001 (title: "Use V8_INLINE static for non-class functions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add args and copy support to V8 tracing controller. BUG=v8:4561 ========== to ========== Add args and copy support to V8 tracing controller. BUG=v8:4561 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add args and copy support to V8 tracing controller. BUG=v8:4561 ========== to ========== Add args and copy support to V8 tracing controller. BUG=v8:4561 Committed: https://crrev.com/9a6a56d21fd606415685d2078d120f6acb9a2027 Cr-Commit-Position: refs/heads/master@{#38255} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/9a6a56d21fd606415685d2078d120f6acb9a2027 Cr-Commit-Position: refs/heads/master@{#38255}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2190973003/diff/180001/src/libplatform/tracin... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2190973003/diff/180001/src/libplatform/tracin... src/libplatform/tracing/trace-object.cc:7: #include "base/trace_event/common/trace_event_common.h" This is no good, things not in chromium's repo must not include base headers. Please fix. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=634087 for this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
