Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(304)

Issue 12699047: add TRACE_EVENT support to ANGLE

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by scottmg
Modified:
10 years, 7 months ago
CC:
angleproject-review_googlegroups.com
Base URL:
https://chromium.googlesource.com/external/angle.git@master
Visibility:
Public.

Description

add TRACE_EVENT support to ANGLE TRACE_EVENT header from Chromium/WebKit, with support functions to allow plumbing back to an embedder. Also adds some TRACE_EVENTs to startup flow. R=apatrick@google.com, shannonwoods@google.com BUG=270179

Patch Set 1 #

Total comments: 2

Patch Set 2 : review fixes #

Patch Set 3 : fix style #

Patch Set 4 : add to CONTRIBUTORS #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+1007 lines, -54 lines) Patch
M CONTRIBUTORS View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M src/build_angle.gypi View 1 1 chunk +6 lines, -0 lines 0 comments Download
A src/common/event_tracer.h View 1 2 1 chunk +26 lines, -0 lines 0 comments Download
A src/common/event_tracer.cpp View 1 2 1 chunk +37 lines, -0 lines 1 comment Download
M src/common/version.h View 1 1 chunk +1 line, -1 line 0 comments Download
M src/libGLESv2/libGLESv2.vcxproj View 1 2 4 chunks +10 lines, -2 lines 0 comments Download
M src/libGLESv2/libGLESv2.vcxproj.filters View 1 2 3 chunks +12 lines, -0 lines 0 comments Download
M src/libGLESv2/renderer/Renderer.cpp View 1 3 chunks +3 lines, -1 line 0 comments Download
M src/libGLESv2/renderer/Renderer9.cpp View 1 2 3 9 chunks +85 lines, -50 lines 0 comments Download
A src/third_party/trace_event/trace_event.h View 1 1 chunk +826 lines, -0 lines 0 comments Download

Messages

Total messages: 19
scottmg
10 years, 7 months ago (2013-08-09 20:02:52 UTC) #1
scottmg
+shannonwoods
10 years, 7 months ago (2013-08-09 20:28:37 UTC) #2
shannonwoods1
In addition to the inline comments-- this'll break the build for anyone using the build ...
10 years, 7 months ago (2013-08-09 20:55:18 UTC) #3
shannonwoods1
Also-- in src/common/version.h, the build revision will need to be updated before checkin.
10 years, 7 months ago (2013-08-09 20:56:12 UTC) #4
scottmg
On 2013/08/09 20:55:18, shannonwoods1 wrote: > In addition to the inline comments-- this'll break the ...
10 years, 7 months ago (2013-08-09 21:38:12 UTC) #5
scottmg
On 2013/08/09 20:56:12, shannonwoods1 wrote: > Also-- in src/common/version.h, the build revision will need to ...
10 years, 7 months ago (2013-08-09 21:38:25 UTC) #6
scottmg
https://codereview.appspot.com/12699047/diff/1/src/libGLESv2/renderer/Renderer9.cpp File src/libGLESv2/renderer/Renderer9.cpp (right): https://codereview.appspot.com/12699047/diff/1/src/libGLESv2/renderer/Renderer9.cpp#newcode237 src/libGLESv2/renderer/Renderer9.cpp:237: { TRACE_EVENT0("gpu", "GetDeviceCaps"); On 2013/08/09 20:55:18, shannonwoods1 wrote: > ...
10 years, 7 months ago (2013-08-09 21:38:44 UTC) #7
shannonwoods1
On 2013/08/09 21:38:12, scottmg wrote: > On 2013/08/09 20:55:18, shannonwoods1 wrote: > > In addition ...
10 years, 7 months ago (2013-08-09 22:05:10 UTC) #8
shannonwoods1
On 2013/08/09 21:38:25, scottmg wrote: > On 2013/08/09 20:56:12, shannonwoods1 wrote: > > Also-- in ...
10 years, 7 months ago (2013-08-09 22:14:07 UTC) #9
scottmg
Thanks! On 2013/08/09 22:05:10, shannonwoods1 wrote: > On 2013/08/09 21:38:12, scottmg wrote: > > On ...
10 years, 7 months ago (2013-08-09 22:28:49 UTC) #10
shannonwoods1
On 2013/08/09 22:28:49, scottmg wrote: > Thanks! > > On 2013/08/09 22:05:10, shannonwoods1 wrote: > ...
10 years, 7 months ago (2013-08-09 22:51:24 UTC) #11
scottmg
On 2013/08/09 22:51:24, shannonwoods1 wrote: > On 2013/08/09 22:28:49, scottmg wrote: > > Thanks! > ...
10 years, 7 months ago (2013-08-09 22:59:05 UTC) #12
shannonwoods1
> The goal (not sure if achieved or not, but works for me locally) is ...
10 years, 7 months ago (2013-08-09 23:02:47 UTC) #13
scottmg
On 2013/08/09 23:02:47, shannonwoods1 wrote: > > The goal (not sure if achieved or not, ...
10 years, 7 months ago (2013-08-09 23:07:52 UTC) #14
apatrick1
LGTM. I'll land this if it's not already underway?
10 years, 7 months ago (2013-08-12 19:32:05 UTC) #15
shannonwoods1
On 2013/08/12 19:32:05, apatrick1 wrote: > LGTM. I'll land this if it's not already underway? ...
10 years, 7 months ago (2013-08-12 19:38:20 UTC) #16
scottmg
https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cpp File src/common/event_tracer.cpp (right): https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cpp#newcode26 src/common/event_tracer.cpp:26: return 0; Sorry, it seems 0 is the wrong ...
10 years, 7 months ago (2013-08-12 21:43:16 UTC) #17
scottmg
On 2013/08/12 21:43:16, scottmg wrote: > https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cpp > File src/common/event_tracer.cpp (right): > > https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cpp#newcode26 > ...
10 years, 7 months ago (2013-08-13 16:43:29 UTC) #18
shannonwoods1
10 years, 7 months ago (2013-08-13 17:42:03 UTC) #19
On 2013/08/13 16:43:29, scottmg wrote:
> On 2013/08/12 21:43:16, scottmg wrote:
> >
https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cpp
> > File src/common/event_tracer.cpp (right):
> > 
> >
>
https://codereview.appspot.com/12699047/diff/19001/src/common/event_tracer.cp...
> > src/common/event_tracer.cpp:26: return 0;
> > Sorry, it seems 0 is the wrong default here. The tracing macros want a
pointer
> > to an unsigned char containing 0, not NULL themselves.
> > 
> > I've uploaded a fix here: https://codereview.appspot.com/12806043/
> > 
> > Or feel free to revert if you prefer. Sorry for the hassle.
> 
> Hmm, actually, semi-conveniently, it looks like the file adds got missed in
> landing this CL originally
>
https://code.google.com/p/angleproject/source/detail?r=889f9d7185fdceaa72b5e4...
> 
> So at least there's no crash checked in! :) (the project files are broken at
> HEAD though).
> 
> I also need to change the dll import/export setup for
> https://codereview.chromium.org/22744005/ to work, so maybe I'll pull the file
> adds, the crash fix and the dll stuff into single fixup CL to make things less
> confusing.

Ah-- I think the file adds must have gone missing somewhere during my struggle
with cygwin's endline behavior. Apologies.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b