Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
176026:
C++ Readability (Closed)
|
Created:
11 years, 3 months ago by rlp Modified:
9 years, 7 months ago Reviewers:
pkeslin CC:
o3d-review_googlegroups.com Base URL:
svn://chrome-svn/chrome/trunk/src/o3d/ Visibility:
Public. |
DescriptionPatch Set 1 #Patch Set 2 : '' #
Total comments: 60
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 10
Patch Set 5 : '' #
Total comments: 2
Patch Set 6 : '' #Patch Set 7 : '' #
Messages
Total messages: 7 (0 generated)
http://codereview.chromium.org/176026/diff/1001/1006 File command_buffer/service/win/d3d9/render_surface_d3d9.cc (right): http://codereview.chromium.org/176026/diff/1001/1006#newcode35 Line 35: Where is the include for render_surface_d3d9.h? It should appear first. http://codereview.chromium.org/176026/diff/1001/1006#newcode50 Line 50: direct3d_surface_(direct3d_surface) { Initializer lists should have a 4 space indent to the colon. Each initializer in the list (in this case) must appear on a separate line indented to the name of the first initialized member variable. http://codereview.chromium.org/176026/diff/1001/1006#newcode58 Line 58: TextureD3D9 *texture) { You must fix the indentation of the parameter list (see render_surface_d3d9.h for more information on how this should be done). http://codereview.chromium.org/176026/diff/1001/1006#newcode59 Line 59: DCHECK_GT(width, 0); You should also validate the mip_level and side parameters. Given the two construction methods, this logic should exist in both here and in the constructor (unless the constructor is private). http://codereview.chromium.org/176026/diff/1001/1006#newcode79 Line 79: : width_(width), height_(height), direct3d_surface_(direct3d_surface) { You must fix the initializer list indentation and variable placement (see above). http://codereview.chromium.org/176026/diff/1001/1006#newcode87 Line 87: DCHECK_GT(height, 0); You should validate gapi as well. Better to get an assert than a NULL ptr dereference. http://codereview.chromium.org/176026/diff/1001/1006#newcode98 Line 98: NULL); I always find it helpful to add a per-line comment that names/describes the parameter as it saves the time required to look these things up when maintaining the code. http://codereview.chromium.org/176026/diff/1001/1006#newcode103 Line 103: new RenderDepthStencilSurfaceD3D9(width, height, direct3d_surface); You use a 2 space indent here for line continuations but use a 4 space indent above. There is no hard and fast rule in this case but you should be consistent within the file/project, with 2 spaces being preferred. http://codereview.chromium.org/176026/diff/1001/1006#newcode132 Line 132: render_surfaces_.Assign(id, render_surface); render_surfaces_? Where is this defined? The style guide prohibits protected/public member variables, requiring accessors/mutators. Ah, now I see. Why are you defining member functions for GAPID3D9 here? They should appear in a separate file. http://codereview.chromium.org/176026/diff/1001/1006#newcode159 Line 159: depth_surfaces_.Assign(id, depth_surface); depth_surfaces_? see render_surfaces_ above. http://codereview.chromium.org/176026/diff/1001/1006#newcode192 Line 192: HR(device->SetRenderTarget(0, d3d_surface)); Just out of curiousity, what does the macro HR do? The name is not terribly descriptive. Looks like it tosses the HRESULT from the d3d9 call. http://codereview.chromium.org/176026/diff/1001/1005 File command_buffer/service/win/d3d9/render_surface_d3d9.h (right): http://codereview.chromium.org/176026/diff/1001/1005#newcode56 Line 56: IDirect3DSurface9* direct3d_surface); Please add comments about each of the public member functions. Include what conditions each of the parameters must satisfy and ownership semantics. http://codereview.chromium.org/176026/diff/1001/1005#newcode57 Line 57: virtual ~RenderSurfaceD3D9() {} Who owns texture_ and how does it get deleted? Looks like a leak to me. http://codereview.chromium.org/176026/diff/1001/1005#newcode65 Line 65: IDirect3DSurface9* GetSurfaceHandle() const { Light weight accessors should use the lower case naming convention. Here you could either use direct3d_surface() or surface_handle(). The former being a bit clearer. http://codereview.chromium.org/176026/diff/1001/1005#newcode70 Line 70: unsigned int width_; The use of unsigned int is frowned upon by the style guide. Given that your arguments are int valued, you should make these int valued as well. http://codereview.chromium.org/176026/diff/1001/1005#newcode73 Line 73: TextureD3D9* texture_; Is there the ability to use scoped_ptrs in your environment. Given the two methods for constructing and the lack of an ownership indicator, I'm assuming that instances of this class own the texture_ object. Using a scoped_ptr would make this clear and handle any cleanup. http://codereview.chromium.org/176026/diff/1001/1005#newcode87 Line 87: int height); Parameter indentation rules are: - Same line as function name if they fit. - First parameter on same line as name, remainder aligned to the first with one parameter decl per line. - First parameter on line following name, indented 4 spaces with subsequent parameters aligned to the first, one per line. http://codereview.chromium.org/176026/diff/1001/1005#newcode88 Line 88: IDirect3DSurface9* GetSurfaceHandle() const { Naming convention. http://codereview.chromium.org/176026/diff/1001/1003 File core/cross/command_buffer/render_surface_cb.cc (right): http://codereview.chromium.org/176026/diff/1001/1003#newcode53 Line 53: DCHECK(texture); You should add checks for the other arguments as well. http://codereview.chromium.org/176026/diff/1001/1002 File core/cross/command_buffer/render_surface_cb.h (right): http://codereview.chromium.org/176026/diff/1001/1002#newcode36 Line 36: #include "core/cross/render_surface.h" Please add file level comments that describe this files contents. http://codereview.chromium.org/176026/diff/1001/1002#newcode46 Line 46: RenderSurfaceCB(ServiceLocator *service_locator, Function comments? Ownership of texture and renderer are really needed here. http://codereview.chromium.org/176026/diff/1001/1002#newcode75 Line 75: RenderDepthStencilSurfaceCB(ServiceLocator *service_locator, Function comments? Ownership of renderer is really needed here. http://codereview.chromium.org/176026/diff/1001/1004 File core/cross/render_surface_test.cc (right): http://codereview.chromium.org/176026/diff/1001/1004#newcode52 Line 52: explicit MockRenderer(Renderer* renderer) Function comments. http://codereview.chromium.org/176026/diff/1001/1004#newcode53 Line 53: : renderer_(renderer) {} Will this fit on the previous line? If so, move it there. If not, fix the indentation. http://codereview.chromium.org/176026/diff/1001/1004#newcode77 Line 77: : object_manager_(g_service_locator) {} Move to previous line. http://codereview.chromium.org/176026/diff/1001/1004#newcode80 Line 80: return service_locator_; These light weight accessors should be const.
Hi, Phil, Thanks for the feedback. I've fixed or replied to all the issues you pointed out. Thanks, Rachel http://codereview.chromium.org/176026/diff/1001/1006 File command_buffer/service/win/d3d9/render_surface_d3d9.cc (right): http://codereview.chromium.org/176026/diff/1001/1006#newcode35 Line 35: On 2009/09/01 17:57:27, pkeslin wrote: > Where is the include for render_surface_d3d9.h? It should appear first. Done. http://codereview.chromium.org/176026/diff/1001/1006#newcode50 Line 50: direct3d_surface_(direct3d_surface) { On 2009/09/01 17:57:27, pkeslin wrote: > Initializer lists should have a 4 space indent to the colon. Each initializer in > the list (in this case) must appear on a separate line indented to the name of > the first initialized member variable. Done. http://codereview.chromium.org/176026/diff/1001/1006#newcode58 Line 58: TextureD3D9 *texture) { On 2009/09/01 17:57:27, pkeslin wrote: > You must fix the indentation of the parameter list (see render_surface_d3d9.h > for more information on how this should be done). Done. http://codereview.chromium.org/176026/diff/1001/1006#newcode59 Line 59: DCHECK_GT(width, 0); On 2009/09/01 17:57:27, pkeslin wrote: > You should also validate the mip_level and side parameters. Given the two > construction methods, this logic should exist in both here and in the > constructor (unless the constructor is private). Done. http://codereview.chromium.org/176026/diff/1001/1006#newcode79 Line 79: : width_(width), height_(height), direct3d_surface_(direct3d_surface) { On 2009/09/01 17:57:27, pkeslin wrote: > You must fix the initializer list indentation and variable placement (see > above). Done. http://codereview.chromium.org/176026/diff/1001/1006#newcode87 Line 87: DCHECK_GT(height, 0); On 2009/09/01 17:57:27, pkeslin wrote: > You should validate gapi as well. Better to get an assert than a NULL ptr > dereference. Done. http://codereview.chromium.org/176026/diff/1001/1006#newcode98 Line 98: NULL); On 2009/09/01 17:57:27, pkeslin wrote: > I always find it helpful to add a per-line comment that names/describes the > parameter as it saves the time required to look these things up when maintaining > the code. Done (for the non-obvious ones). http://codereview.chromium.org/176026/diff/1001/1006#newcode103 Line 103: new RenderDepthStencilSurfaceD3D9(width, height, direct3d_surface); On 2009/09/01 17:57:27, pkeslin wrote: > You use a 2 space indent here for line continuations but use a 4 space indent > above. There is no hard and fast rule in this case but you should be consistent > within the file/project, with 2 spaces being preferred. Thanks. I thought 4 was preferred for line continuation. But I have fixed it to 2 througout. http://codereview.chromium.org/176026/diff/1001/1006#newcode132 Line 132: render_surfaces_.Assign(id, render_surface); On 2009/09/01 17:57:27, pkeslin wrote: > render_surfaces_? Where is this defined? The style guide prohibits > protected/public member variables, requiring accessors/mutators. > > Ah, now I see. Why are you defining member functions for GAPID3D9 here? They > should appear in a separate file. In general for the command buffers, we've kept related functions near each other rather than all in separate gapi files. Partly to make it easier to find these functions (since they're closely related) and partly to reduce the number and length of files. I will move it if it's a dealbreaker, but I would prefer to maintain to same format as the rest of this codebase. http://codereview.chromium.org/176026/diff/1001/1006#newcode159 Line 159: depth_surfaces_.Assign(id, depth_surface); On 2009/09/01 17:57:27, pkeslin wrote: > depth_surfaces_? see render_surfaces_ above. See above. http://codereview.chromium.org/176026/diff/1001/1006#newcode192 Line 192: HR(device->SetRenderTarget(0, d3d_surface)); On 2009/09/01 17:57:27, pkeslin wrote: > Just out of curiousity, what does the macro HR do? The name is not terribly > descriptive. Looks like it tosses the HRESULT from the d3d9 call. Here is the definition for it from http://src.chromium.org/viewvc/chrome/trunk/src/o3d/command_buffer/service/wi...: #ifndef HR #define HR(x) { \ HRESULT hr = x; \ if (FAILED(hr)) { \ LOG(ERROR) << "DirectX error at " << __FILE__ << ":" << __LINE__ \ << " when calling " << #x << ": " << DXGetErrorStringA(hr); \ } \ } #endif So, yes, it does throw it away to some extent, but since this is on the server side it's the best way we have right now for catching the errors to analyze later. http://codereview.chromium.org/176026/diff/1001/1005 File command_buffer/service/win/d3d9/render_surface_d3d9.h (right): http://codereview.chromium.org/176026/diff/1001/1005#newcode56 Line 56: IDirect3DSurface9* direct3d_surface); On 2009/09/01 17:57:27, pkeslin wrote: > Please add comments about each of the public member functions. Include what > conditions each of the parameters must satisfy and ownership semantics. Done. http://codereview.chromium.org/176026/diff/1001/1005#newcode57 Line 57: virtual ~RenderSurfaceD3D9() {} On 2009/09/01 17:57:27, pkeslin wrote: > Who owns texture_ and how does it get deleted? Looks like a leak to me. The texture is owned by the gapi interface where it is stored in the resource map called textures_. The gapi interface handles deleting any of its resources. http://codereview.chromium.org/176026/diff/1001/1005#newcode65 Line 65: IDirect3DSurface9* GetSurfaceHandle() const { On 2009/09/01 17:57:27, pkeslin wrote: > Light weight accessors should use the lower case naming convention. Here you > could either use direct3d_surface() or surface_handle(). The former being a bit > clearer. This was done to mimic a parallel section of the code which uses this same function name. I'll note to fix the rest of the code as well. http://codereview.chromium.org/176026/diff/1001/1005#newcode70 Line 70: unsigned int width_; On 2009/09/01 17:57:27, pkeslin wrote: > The use of unsigned int is frowned upon by the style guide. Given that your > arguments are int valued, you should make these int valued as well. Done. http://codereview.chromium.org/176026/diff/1001/1005#newcode73 Line 73: TextureD3D9* texture_; On 2009/09/01 17:57:27, pkeslin wrote: > Is there the ability to use scoped_ptrs in your environment. Given the two > methods for constructing and the lack of an ownership indicator, I'm assuming > that instances of this class own the texture_ object. Using a scoped_ptr would > make this clear and handle any cleanup. Instances of this class do not own the texture, but simply maintain a reference to it as a render surface must be a associated with a texture to exist. However it is ultimately the d3d texture which defines the render surface, so this reference could be removed if preferred. By similar argument, width and height aren't needed either then. http://codereview.chromium.org/176026/diff/1001/1005#newcode87 Line 87: int height); On 2009/09/01 17:57:27, pkeslin wrote: > Parameter indentation rules are: > - Same line as function name if they fit. > - First parameter on same line as name, remainder aligned to the first with > one parameter decl per line. > - First parameter on line following name, indented 4 spaces with subsequent > parameters aligned to the first, one per line. Done. http://codereview.chromium.org/176026/diff/1001/1005#newcode88 Line 88: IDirect3DSurface9* GetSurfaceHandle() const { On 2009/09/01 17:57:27, pkeslin wrote: > Naming convention. Done. http://codereview.chromium.org/176026/diff/1001/1003 File core/cross/command_buffer/render_surface_cb.cc (right): http://codereview.chromium.org/176026/diff/1001/1003#newcode53 Line 53: DCHECK(texture); On 2009/09/01 17:57:27, pkeslin wrote: > You should add checks for the other arguments as well. Done. http://codereview.chromium.org/176026/diff/1001/1002 File core/cross/command_buffer/render_surface_cb.h (right): http://codereview.chromium.org/176026/diff/1001/1002#newcode36 Line 36: #include "core/cross/render_surface.h" On 2009/09/01 17:57:27, pkeslin wrote: > Please add file level comments that describe this files contents. Done. http://codereview.chromium.org/176026/diff/1001/1002#newcode46 Line 46: RenderSurfaceCB(ServiceLocator *service_locator, On 2009/09/01 17:57:27, pkeslin wrote: > Function comments? Ownership of texture and renderer are really needed here. Done. http://codereview.chromium.org/176026/diff/1001/1002#newcode75 Line 75: RenderDepthStencilSurfaceCB(ServiceLocator *service_locator, On 2009/09/01 17:57:27, pkeslin wrote: > Function comments? Ownership of renderer is really needed here. Done. http://codereview.chromium.org/176026/diff/1001/1004 File core/cross/render_surface_test.cc (right): http://codereview.chromium.org/176026/diff/1001/1004#newcode52 Line 52: explicit MockRenderer(Renderer* renderer) On 2009/09/01 17:57:27, pkeslin wrote: > Function comments. Done for non-obvious functions. http://codereview.chromium.org/176026/diff/1001/1004#newcode53 Line 53: : renderer_(renderer) {} On 2009/09/01 17:57:27, pkeslin wrote: > Will this fit on the previous line? If so, move it there. If not, fix the > indentation. Done. http://codereview.chromium.org/176026/diff/1001/1004#newcode77 Line 77: : object_manager_(g_service_locator) {} On 2009/09/01 17:57:27, pkeslin wrote: > Move to previous line. Done. http://codereview.chromium.org/176026/diff/1001/1004#newcode80 Line 80: return service_locator_; On 2009/09/01 17:57:27, pkeslin wrote: > These light weight accessors should be const. Done.
http://codereview.chromium.org/176026/diff/1001/1006 File command_buffer/service/win/d3d9/render_surface_d3d9.cc (right): http://codereview.chromium.org/176026/diff/1001/1006#newcode103 Line 103: new RenderDepthStencilSurfaceD3D9(width, height, direct3d_surface); 2 space indent it indeed preferred, but 4 is also acceptable. The key is consistency within the project. On 2009/09/01 23:17:15, rlp wrote: > On 2009/09/01 17:57:27, pkeslin wrote: > > You use a 2 space indent here for line continuations but use a 4 space indent > > above. There is no hard and fast rule in this case but you should be > consistent > > within the file/project, with 2 spaces being preferred. > > Thanks. I thought 4 was preferred for line continuation. But I have fixed it to > 2 througout. http://codereview.chromium.org/176026/diff/1001/1006#newcode132 Line 132: render_surfaces_.Assign(id, render_surface); Project consistency is paramount here, so I'll allow it. You should clearly demark this section of the file with a leader comment, however, so that it does not cause confusion. On 2009/09/01 23:17:15, rlp wrote: > On 2009/09/01 17:57:27, pkeslin wrote: > > render_surfaces_? Where is this defined? The style guide prohibits > > protected/public member variables, requiring accessors/mutators. > > > > Ah, now I see. Why are you defining member functions for GAPID3D9 here? They > > should appear in a separate file. > > In general for the command buffers, we've kept related functions near each other > rather than all in separate gapi files. Partly to make it easier to find these > functions (since they're closely related) and partly to reduce the number and > length of files. I will move it if it's a dealbreaker, but I would prefer to > maintain to same format as the rest of this codebase. http://codereview.chromium.org/176026/diff/1001/1002 File core/cross/command_buffer/render_surface_cb.h (right): http://codereview.chromium.org/176026/diff/1001/1002#newcode46 Line 46: RenderSurfaceCB(ServiceLocator *service_locator, Clearly spell out ownership semantics for object arguments (i.e., service_locator, texture and renderer). On 2009/09/01 23:17:15, rlp wrote: > On 2009/09/01 17:57:27, pkeslin wrote: > > Function comments? Ownership of texture and renderer are really needed here. > > Done. http://codereview.chromium.org/176026/diff/1001/1002#newcode75 Line 75: RenderDepthStencilSurfaceCB(ServiceLocator *service_locator, Ownership of service_locator and renderer are needed here. On 2009/09/01 23:17:15, rlp wrote: > On 2009/09/01 17:57:27, pkeslin wrote: > > Function comments? Ownership of renderer is really needed here. > > Done. http://codereview.chromium.org/176026/diff/4007/4012 File command_buffer/service/win/d3d9/render_surface_d3d9.cc (right): http://codereview.chromium.org/176026/diff/4007/4012#newcode113 Line 113: D3DFMT_D24S8, // d3d format of depth stencil surface Either use complete sentences for the comment (with correct punctuation) or simply add the name of the parameter. It is assumed that the reader is somewhat familiar with the interface. You are just adding a helpful reminder to save the cost of a lookup. http://codereview.chromium.org/176026/diff/4007/4011 File command_buffer/service/win/d3d9/render_surface_d3d9.h (right): http://codereview.chromium.org/176026/diff/4007/4011#newcode65 Line 65: // texture - the texture to which this render surface maps. Remove extra blank space between "which" and "this". This problem occurs in other places within the file as well. Please correct those as well. http://codereview.chromium.org/176026/diff/4007/4011#newcode67 Line 67: // rendering surface. Please add a note that the new instance owns the given argument. http://codereview.chromium.org/176026/diff/4007/4011#newcode108 Line 108: TextureD3D9 *texture_; The placement of the '*' for pointers is up to you but should be consistent throughout the file/project. You are using two styles in this file. Please fix. http://codereview.chromium.org/176026/diff/4007/4008 File core/cross/command_buffer/render_surface_cb.h (right): http://codereview.chromium.org/176026/diff/4007/4008#newcode128 Line 128: private: Please add a blank line above access labels.
Hi, Phil, Here's the latest. Thanks, Rachel http://codereview.chromium.org/176026/diff/1001/1006 File command_buffer/service/win/d3d9/render_surface_d3d9.cc (right): http://codereview.chromium.org/176026/diff/1001/1006#newcode103 Line 103: new RenderDepthStencilSurfaceD3D9(width, height, direct3d_surface); On 2009/09/08 18:21:36, pkeslin wrote: > 2 space indent it indeed preferred, but 4 is also acceptable. The key is > consistency within the project. > > On 2009/09/01 23:17:15, rlp wrote: > > On 2009/09/01 17:57:27, pkeslin wrote: > > > You use a 2 space indent here for line continuations but use a 4 space > indent > > > above. There is no hard and fast rule in this case but you should be > > consistent > > > within the file/project, with 2 spaces being preferred. > > > > Thanks. I thought 4 was preferred for line continuation. But I have fixed it > to > > 2 througout. > > Got it. I found some additional inconsistencies in the comments that I adjusted to be 2 spaces as well. http://codereview.chromium.org/176026/diff/1001/1006#newcode132 Line 132: render_surfaces_.Assign(id, render_surface); On 2009/09/08 18:21:36, pkeslin wrote: > Project consistency is paramount here, so I'll allow it. You should clearly > demark this section of the file with a leader comment, however, so that it does > not cause confusion. > > On 2009/09/01 23:17:15, rlp wrote: > > On 2009/09/01 17:57:27, pkeslin wrote: > > > render_surfaces_? Where is this defined? The style guide prohibits > > > protected/public member variables, requiring accessors/mutators. > > > > > > Ah, now I see. Why are you defining member functions for GAPID3D9 here? They > > > should appear in a separate file. > > > > In general for the command buffers, we've kept related functions near each > other > > rather than all in separate gapi files. Partly to make it easier to find these > > functions (since they're closely related) and partly to reduce the number and > > length of files. I will move it if it's a dealbreaker, but I would prefer to > > maintain to same format as the rest of this codebase. > > Done. http://codereview.chromium.org/176026/diff/1001/1002 File core/cross/command_buffer/render_surface_cb.h (right): http://codereview.chromium.org/176026/diff/1001/1002#newcode46 Line 46: RenderSurfaceCB(ServiceLocator *service_locator, On 2009/09/08 18:21:36, pkeslin wrote: > Clearly spell out ownership semantics for object arguments (i.e., > service_locator, texture and renderer). > > On 2009/09/01 23:17:15, rlp wrote: > > On 2009/09/01 17:57:27, pkeslin wrote: > > > Function comments? Ownership of texture and renderer are really needed here. > > > > Done. > > Made more explicit in individual parameter descriptions. http://codereview.chromium.org/176026/diff/1001/1002#newcode75 Line 75: RenderDepthStencilSurfaceCB(ServiceLocator *service_locator, On 2009/09/08 18:21:36, pkeslin wrote: > Ownership of service_locator and renderer are needed here. > > On 2009/09/01 23:17:15, rlp wrote: > > On 2009/09/01 17:57:27, pkeslin wrote: > > > Function comments? Ownership of renderer is really needed here. > > > > Done. > > Made more explicit in individual parameter descriptions. http://codereview.chromium.org/176026/diff/4007/4012 File command_buffer/service/win/d3d9/render_surface_d3d9.cc (right): http://codereview.chromium.org/176026/diff/4007/4012#newcode113 Line 113: D3DFMT_D24S8, // d3d format of depth stencil surface On 2009/09/08 18:21:36, pkeslin wrote: > Either use complete sentences for the comment (with correct punctuation) or > simply add the name of the parameter. It is assumed that the reader is somewhat > familiar with the interface. You are just adding a helpful reminder to save the > cost of a lookup. Done. http://codereview.chromium.org/176026/diff/4007/4011 File command_buffer/service/win/d3d9/render_surface_d3d9.h (right): http://codereview.chromium.org/176026/diff/4007/4011#newcode65 Line 65: // texture - the texture to which this render surface maps. On 2009/09/08 18:21:36, pkeslin wrote: > Remove extra blank space between "which" and "this". This problem occurs in > other places within the file as well. Please correct those as well. Done. Here and in the rest of the file. http://codereview.chromium.org/176026/diff/4007/4011#newcode67 Line 67: // rendering surface. On 2009/09/08 18:21:36, pkeslin wrote: > Please add a note that the new instance owns the given argument. Done here and below. Also clarified ownership of the texture. http://codereview.chromium.org/176026/diff/4007/4011#newcode108 Line 108: TextureD3D9 *texture_; On 2009/09/08 18:21:36, pkeslin wrote: > The placement of the '*' for pointers is up to you but should be consistent > throughout the file/project. You are using two styles in this file. Please fix. Fixed throughout. The '*' should now consistently be next to the variable name except in the case of return types in function declarations. In that case, I've left the '*' with the class type as I think that makes for a clearer function definition. Please let me know if this is still considered inconsistent. http://codereview.chromium.org/176026/diff/4007/4008 File core/cross/command_buffer/render_surface_cb.h (right): http://codereview.chromium.org/176026/diff/4007/4008#newcode128 Line 128: private: On 2009/09/08 18:21:36, pkeslin wrote: > Please add a blank line above access labels. Done.
Just one nit. Other than that, LGTM http://codereview.chromium.org/176026/diff/7002/7007 File command_buffer/service/win/d3d9/render_surface_d3d9.cc (right): http://codereview.chromium.org/176026/diff/7002/7007#newcode118 Line 118: NULL); // This parameter required to be NULL. "required" -> "is required"
Fixed the item below and merged with current version in the repository (mostly in the test file). Thank you! http://codereview.chromium.org/176026/diff/7002/7007 File command_buffer/service/win/d3d9/render_surface_d3d9.cc (right): http://codereview.chromium.org/176026/diff/7002/7007#newcode118 Line 118: NULL); // This parameter required to be NULL. On 2009/09/15 19:52:20, pkeslin wrote: > "required" -> "is required" Done.
And committed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
