Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2825074:
Initial version rendering 2D path for O3D. This will eventually allow O3D app... (Closed)
|
Created:
10 years, 4 months ago by fransiskusx Modified:
9 years, 7 months ago CC:
o3d-review_googlegroups.com Visibility:
Public. |
DescriptionInitial version rendering 2D path for O3D. This will eventually allow O3D apps to fall back to 2D logic on systems without (working) GPUs.
Incorporated 2D library Cairo to O3D.
Currently only support Linux and compiled when renderer = cairo.
TEST= I compiled with renderer = cairo and it worked. Also I compiled with renderer = gl and it worked.
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55008
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 188
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #
Total comments: 54
Patch Set 16 : '' #Patch Set 17 : '' #
Total comments: 18
Patch Set 18 : '' #Patch Set 19 : '' #
Total comments: 9
Patch Set 20 : '' #Patch Set 21 : '' #Patch Set 22 : '' #
Total comments: 6
Patch Set 23 : '' #Patch Set 24 : Initial version rendering 2D path for O3D. This will eventually allow O3D app... #Patch Set 25 : '' #Patch Set 26 : '' #Patch Set 27 : '' #Patch Set 28 : '' #Patch Set 29 : '' #Patch Set 30 : '' #Patch Set 31 : '' #
Messages
Total messages: 23 (0 generated)
Cool. Out of curiosity, have you considered using the Skia library that's already being linked into O3D instead of Cairo? It might give you some size savings.
Overall very good. Lots of minor style issues to correct, but the design is an excellent starting point. http://codereview.chromium.org/2825074/diff/6004/9003 File build/libs.gyp (right): http://codereview.chromium.org/2825074/diff/6004/9003#newcode36 build/libs.gyp:36: [ 'OS=="mac"', We will eventually need Cairo linking flags for Win/Mac. Rather than copying the OpenGL flags here, just leave out the "mac" and "win" conditions and put in "TODO(fransiskusx): Link to Cairo on Win/Mac as a static library" http://codereview.chromium.org/2825074/diff/6004/9004 File core/core.gyp (right): http://codereview.chromium.org/2825074/diff/6004/9004#newcode473 core/core.gyp:473: ['renderer == "2d"', Since there is already a "Texture2D" (as you observed), I don't think we should call the new renderer "2d" because it will lead to some confusion. I think instead we should use, renderer == "cairo", cross/cairo, renderer_cairo.h, etc. http://codereview.chromium.org/2825074/diff/6004/9005 File core/cross/2d/install_check.cc (right): http://codereview.chromium.org/2825074/diff/6004/9005#newcode2 core/cross/2d/install_check.cc:2: * Copyright 2009, Google Inc. Whenever creating/copying/editing files we usually update the copyright year to the current year. http://codereview.chromium.org/2825074/diff/6004/9005#newcode40 core/cross/2d/install_check.cc:40: } // o3d Should have a blank line above this. http://codereview.chromium.org/2825074/diff/6004/9006 File core/cross/2d/renderer_2d.cc (right): http://codereview.chromium.org/2825074/diff/6004/9006#newcode1 core/cross/2d/renderer_2d.cc:1: // Copyright 2010 Google Inc. All Rights Reserved. Actually, it's open-source so it's not "All Rights Reserved". :) Just copy the big comment block used in the existing files. http://codereview.chromium.org/2825074/diff/6004/9006#newcode2 core/cross/2d/renderer_2d.cc:2: // Author: fransiskusx@google.com (Fransiskus Xaverius) In open-source code Google sadly has a policy of not including the name(s) of the author(s), so you should remove this. http://codereview.chromium.org/2825074/diff/6004/9006#newcode12 core/cross/2d/renderer_2d.cc:12: To make this file clearer, I think you should organize the method definitions into three sections: 1) Methods that have been implemented. 2) Methods that haven't been implemented yet and will need to be implemented in the future (or might need to be). At the beginning of this section, put something like "// TODO(fransiskusx): Determine which of these need to be implemented and do so." 3) Methods that haven't been implemented and aren't relevant to 2D rendering, which in the future should be removed from the class entirely when we create the Renderer3D and Renderer2D interfaces. At the beginning of this section, put something like "// TODO(fransiskusx): These aren't applicable to 2D rendering. We should refactor the class hierarchy to remove them entirely." Doing that work may end up being outside the scope of your internship, but it's good to add the comments anyway. Also, for all methods in section 2 or section 3, you should add NOTIMPLEMENTED() to the beginning of their body. That will log a debug message in the event that they are somehow called. You will probably have to #include "base/logging.h" to use that. http://codereview.chromium.org/2825074/diff/6004/9006#newcode18 core/cross/2d/renderer_2d.cc:18: : Renderer(service_locator) { The Google style is: Renderer2D::Renderer2D(ServiceLocator* service_locator) : Renderer(service_locator), <-- 4 spaces of indent kFrameSrcData_(NULL), ... { } Also, you should always initialize all of your class's fields in the constructor, even if you don't have meaningful values for them yet. Otherwise their value will be undefined, which can make debugging harder. Any fields that are instances of classes are initialized automatically, but pointers and integers are not (including pointers to classes), so you need to initialize all of those. For pointers you can just initialize them to NULL and for integers you can just use 0 ("Window" is actually an integer, not a class, so initialize that to 0 too). http://codereview.chromium.org/2825074/diff/6004/9006#newcode39 core/cross/2d/renderer_2d.cc:39: unsigned src_height, int src_pitch) { Google style is to align this with the previous row of arguments. http://codereview.chromium.org/2825074/diff/6004/9006#newcode55 core/cross/2d/renderer_2d.cc:55: // Preparing the image to render Add this: // TODO(fransiskusx): The shared memory may have been unregistered, so our pointer may no longer be valid. We need to listen for shared memory unregistration and unset our pointer when it happens. http://codereview.chromium.org/2825074/diff/6004/9006#newcode61 core/cross/2d/renderer_2d.cc:61: cairo_t *current_drawing = cairo_create(main_surface_); There are two styles for where to put the * and & in C/C++. You can either write "cairo_t *current_drawing" (referred to as "pointer belongs to the variable") or "cairo_t* current_drawing" (referred to as "pointer belongs to the type"). Both ways of writing it do the exact same thing, but for ease of readability you should always be consistent and just use one of the two styles. Most of this file uses "pointer belongs to the type", but here you use "pointer belongs to the variable", so you should change it all to be consistent. It's your choice which style you want to use. http://codereview.chromium.org/2825074/diff/6004/9006#newcode63 core/cross/2d/renderer_2d.cc:63: // Scalling the image Spelling mistake (should be "scaling"). http://codereview.chromium.org/2825074/diff/6004/9006#newcode64 core/cross/2d/renderer_2d.cc:64: double widthScaling = Google C++ style for variable names is to never use capitals like this, always use width_scaling. http://codereview.chromium.org/2825074/diff/6004/9006#newcode65 core/cross/2d/renderer_2d.cc:65: (static_cast<double>(display_width()))/frame_src_width_; Should have spaces before and after "/" http://codereview.chromium.org/2825074/diff/6004/9006#newcode66 core/cross/2d/renderer_2d.cc:66: double heightScaling = Same here. http://codereview.chromium.org/2825074/diff/6004/9006#newcode67 core/cross/2d/renderer_2d.cc:67: (static_cast<double>(display_height()))/frame_src_height_; Same here. http://codereview.chromium.org/2825074/diff/6004/9006#newcode74 core/cross/2d/renderer_2d.cc:74: } You also need to destroy the image surface, or else it will be leaked. http://codereview.chromium.org/2825074/diff/6004/9006#newcode79 core/cross/2d/renderer_2d.cc:79: DefaultVisual(display_, 0), You should use "XDefaultVisual" instead of "DefaultVisual". The X11 functions that omit the "X" at the front are actually not functions, they are a C feature called a macro. Using a macro from a shared library can be unsafe when writing code that will run on lots of different computers and lots of different versions of Linux, so it's better to avoid them. http://codereview.chromium.org/2825074/diff/6004/9006#newcode83 core/cross/2d/renderer_2d.cc:83: void Renderer2D::UninitCommon() {} Whenever you have an implementation of a void method where you don't need to do anything, you should write a comment in it like: // Don't need to do anything. That way the reader knows that that method is correct to do nothing (as opposed to being an unimplemented method that still needs to be written). http://codereview.chromium.org/2825074/diff/6004/9006#newcode90 core/cross/2d/renderer_2d.cc:90: Renderer::InitStatus Renderer2D::InitPlatformSpecific( Should have just one space after ...::InitStatus http://codereview.chromium.org/2825074/diff/6004/9006#newcode91 core/cross/2d/renderer_2d.cc:91: const DisplayWindow& display_window, When you can't fit the first argument onto the previous line, indent it by just 4 spaces, like this: Renderer::InitStatus Renderer2D::InitPlatformSpecific( const DisplayWindow& display_window, ...) { ... } http://codereview.chromium.org/2825074/diff/6004/9006#newcode94 core/cross/2d/renderer_2d.cc:94: static_cast<const DisplayWindowLinux&>(display_window); Also indent by just four spaces. http://codereview.chromium.org/2825074/diff/6004/9006#newcode109 core/cross/2d/renderer_2d.cc:109: cairo_surface_destroy(main_surface_); There is a cairo_xlib_surface_set_size function that you should be able to use instead of destroying and recreating. http://codereview.chromium.org/2825074/diff/6004/9006#newcode117 core/cross/2d/renderer_2d.cc:117: RenderDepthStencilSurface::Ref Renderer2D::CreateDepthStencilSurface(int width, This should go in section 3. http://codereview.chromium.org/2825074/diff/6004/9006#newcode118 core/cross/2d/renderer_2d.cc:118: int height) { Should align with the previous line. http://codereview.chromium.org/2825074/diff/6004/9006#newcode127 core/cross/2d/renderer_2d.cc:127: bool Renderer2D::GoFullscreen(const DisplayWindow& display, Section 2. http://codereview.chromium.org/2825074/diff/6004/9006#newcode139 core/cross/2d/renderer_2d.cc:139: bool Renderer2D::CancelFullscreen(const DisplayWindow& display, Section 2. http://codereview.chromium.org/2825074/diff/6004/9006#newcode145 core/cross/2d/renderer_2d.cc:145: bool Renderer2D::fullscreen() const { Section 2. Also, it should return false for now. http://codereview.chromium.org/2825074/diff/6004/9006#newcode151 core/cross/2d/renderer_2d.cc:151: void Renderer2D::GetDisplayModes(std::vector<DisplayMode> *modes) {} Section 2. Also call modes->clear() so that there aren't old modes in the list potentially. http://codereview.chromium.org/2825074/diff/6004/9006#newcode155 core/cross/2d/renderer_2d.cc:155: bool Renderer2D::GetDisplayMode(int id, DisplayMode *mode) { Section 2. Also, does the plugin still work if you return false here? If so, that would be better. http://codereview.chromium.org/2825074/diff/6004/9006#newcode160 core/cross/2d/renderer_2d.cc:160: StreamBank::Ref Renderer2D::CreateStreamBank() { Next seven methods should go in section 3. http://codereview.chromium.org/2825074/diff/6004/9006#newcode206 core/cross/2d/renderer_2d.cc:206: void Renderer2D::SetBackBufferPlatformSpecific() {} Section 2. http://codereview.chromium.org/2825074/diff/6004/9006#newcode209 core/cross/2d/renderer_2d.cc:209: void Renderer2D::SetRenderSurfacesPlatformSpecific(const RenderSurface* surface, Section 3. http://codereview.chromium.org/2825074/diff/6004/9006#newcode213 core/cross/2d/renderer_2d.cc:213: ParamCache* Renderer2D::CreatePlatformSpecificParamCache() { Section 3. http://codereview.chromium.org/2825074/diff/6004/9006#newcode218 core/cross/2d/renderer_2d.cc:218: Texture2D::Ref Renderer2D::CreatePlatformSpecificTexture2D(int width, This is fully implemented, so section 1. http://codereview.chromium.org/2825074/diff/6004/9006#newcode232 core/cross/2d/renderer_2d.cc:232: TextureCUBE::Ref Renderer2D::CreatePlatformSpecificTextureCUBE(int edge_length, Section 3. http://codereview.chromium.org/2825074/diff/6004/9006#newcode240 core/cross/2d/renderer_2d.cc:240: bool Renderer2D::PlatformSpecificBeginDraw() { Next five methods should go in section 1. http://codereview.chromium.org/2825074/diff/6004/9006#newcode261 core/cross/2d/renderer_2d.cc:261: void Renderer2D::PlatformSpecificClear(const Float4 &color, Not sure about this one. Let's put it in section 2 for now. http://codereview.chromium.org/2825074/diff/6004/9006#newcode269 core/cross/2d/renderer_2d.cc:269: void Renderer2D::ApplyDirtyStates() {} Section 3. http://codereview.chromium.org/2825074/diff/6004/9006#newcode273 core/cross/2d/renderer_2d.cc:273: void Renderer2D::SetViewportInPixels(int left, Not sure about this one either. Section 2 for now. http://codereview.chromium.org/2825074/diff/6004/9006#newcode282 core/cross/2d/renderer_2d.cc:282: void Renderer2D::PushRenderStates(State *state) { Section 3. http://codereview.chromium.org/2825074/diff/6004/9007 File core/cross/2d/renderer_2d.h (right): http://codereview.chromium.org/2825074/diff/6004/9007#newcode1 core/cross/2d/renderer_2d.h:1: // Copyright 2010 Google Inc. All Rights Reserved. Also use standard O3D comment block here. http://codereview.chromium.org/2825074/diff/6004/9007#newcode5 core/cross/2d/renderer_2d.h:5: #ifndef RENDERER_2D_H_ Should start with O3D_CORE_CROSS_2D_ http://codereview.chromium.org/2825074/diff/6004/9007#newcode8 core/cross/2d/renderer_2d.h:8: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9007#newcode15 core/cross/2d/renderer_2d.h:15: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9007#newcode26 core/cross/2d/renderer_2d.h:26: virtual void SetNewFrame(const void* kSrcData, unsigned src_width, This doesn't need to be virtual. http://codereview.chromium.org/2825074/diff/6004/9007#newcode27 core/cross/2d/renderer_2d.h:27: unsigned src_height, int src_pitch); Alignment. http://codereview.chromium.org/2825074/diff/6004/9007#newcode30 core/cross/2d/renderer_2d.h:30: virtual void Paint(); Doesn't need to be virtual. http://codereview.chromium.org/2825074/diff/6004/9007#newcode39 core/cross/2d/renderer_2d.h:39: bool off_screen); Alignment. http://codereview.chromium.org/2825074/diff/6004/9007#newcode44 core/cross/2d/renderer_2d.h:44: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9007#newcode45 core/cross/2d/renderer_2d.h:45: // ///////////////////////////////to IMPLEMENT from Abstract Class Renderer Remove this comment. http://codereview.chromium.org/2825074/diff/6004/9007#newcode52 core/cross/2d/renderer_2d.h:52: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9007#newcode90 core/cross/2d/renderer_2d.h:90: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9007#newcode113 core/cross/2d/renderer_2d.h:113: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9007#newcode119 core/cross/2d/renderer_2d.h:119: // ///////////////////////////////////////////// to IMPLEMENT-- Remove this comment. http://codereview.chromium.org/2825074/diff/6004/9007#newcode123 core/cross/2d/renderer_2d.h:123: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9007#newcode139 core/cross/2d/renderer_2d.h:139: // Keep the constructor protected so only factory methods can create Google style is for all the protected methods to come before any of the protected fields (some O3D files disobey this rule, but you should follow it). http://codereview.chromium.org/2825074/diff/6004/9007#newcode143 core/cross/2d/renderer_2d.h:143: // ///////////////////////////////to IMPLEMENT from Abstract Class Renderer Remove this comment. http://codereview.chromium.org/2825074/diff/6004/9007#newcode156 core/cross/2d/renderer_2d.h:156: int width, Indent by 4 spaces from the start of "virtual". http://codereview.chromium.org/2825074/diff/6004/9007#newcode164 core/cross/2d/renderer_2d.h:164: int edge_length, Indent by 4 spaces from the start of "virtual". http://codereview.chromium.org/2825074/diff/6004/9007#newcode195 core/cross/2d/renderer_2d.h:195: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9007#newcode204 core/cross/2d/renderer_2d.h:204: // ///////////////////////////////////////////// to IMPLEMENT-- Remove this comment. http://codereview.chromium.org/2825074/diff/6004/9007#newcode207 core/cross/2d/renderer_2d.h:207: #endif /* RENDERER_2D_H_ */ We use // for this, and put two spaces after the endif. http://codereview.chromium.org/2825074/diff/6004/9008 File core/cross/2d/texture_2d.cc (right): http://codereview.chromium.org/2825074/diff/6004/9008#newcode3 core/cross/2d/texture_2d.cc:3: * All rights reserved. Also use standard O3D comment block here. http://codereview.chromium.org/2825074/diff/6004/9008#newcode39 core/cross/2d/texture_2d.cc:39: Here you will need to include renderer_2d.h once you remove it from texture_2d.h. http://codereview.chromium.org/2825074/diff/6004/9008#newcode43 core/cross/2d/texture_2d.cc:43: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9008#newcode52 core/cross/2d/texture_2d.cc:52: // NOTE: the Texture2DCairo now owns the texture and will destroy it on exit. Not applicable. http://codereview.chromium.org/2825074/diff/6004/9008#newcode54 core/cross/2d/texture_2d.cc:54: Texture::Format format, Alignment. http://codereview.chromium.org/2825074/diff/6004/9008#newcode73 core/cross/2d/texture_2d.cc:73: Texture::Format format, Alignment. http://codereview.chromium.org/2825074/diff/6004/9008#newcode80 core/cross/2d/texture_2d.cc:80: format, Alignment. http://codereview.chromium.org/2825074/diff/6004/9008#newcode99 core/cross/2d/texture_2d.cc:99: unsigned dst_left, Alignment. http://codereview.chromium.org/2825074/diff/6004/9008#newcode108 core/cross/2d/texture_2d.cc:108: src_width, src_height, Alignment. http://codereview.chromium.org/2825074/diff/6004/9008#newcode114 core/cross/2d/texture_2d.cc:114: bool Texture2DCairo::PlatformSpecificLock( Add NOTIMPLEMENTED() to this and the rest. http://codereview.chromium.org/2825074/diff/6004/9008#newcode134 core/cross/2d/texture_2d.cc:134: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9009 File core/cross/2d/texture_2d.h (right): http://codereview.chromium.org/2825074/diff/6004/9009#newcode3 core/cross/2d/texture_2d.h:3: * All rights reserved. Also use standard O3D comment block here. http://codereview.chromium.org/2825074/diff/6004/9009#newcode39 core/cross/2d/texture_2d.h:39: // Precompiled header comes before everything else. I don't think this comment is applicable here. http://codereview.chromium.org/2825074/diff/6004/9009#newcode41 core/cross/2d/texture_2d.h:41: // Disable compiler warning for openGL calls that require a void* to This stuff is probably not required. http://codereview.chromium.org/2825074/diff/6004/9009#newcode51 core/cross/2d/texture_2d.h:51: #include "core/cross/2d/renderer_2d.h" You don't need this actually, because you already have "class Renderer2D;" below, which is enough for this file because it doesn't rely on any of the details of the Renderer2D class--only the texture_2d.cc file does. http://codereview.chromium.org/2825074/diff/6004/9009#newcode52 core/cross/2d/texture_2d.h:52: Should have just a single blank line here. http://codereview.chromium.org/2825074/diff/6004/9009#newcode60 core/cross/2d/texture_2d.h:60: // Texture2DCairo implements the Texture2D interfacel. Typo, should be "interface". http://codereview.chromium.org/2825074/diff/6004/9009#newcode79 core/cross/2d/texture_2d.h:79: // The created texture takes ownership of the bitmap data. This comment about ownership of the bitmap data doesn't seem to be applicable here. http://codereview.chromium.org/2825074/diff/6004/9009#newcode81 core/cross/2d/texture_2d.h:81: Texture::Format format, Alignment. http://codereview.chromium.org/2825074/diff/6004/9009#newcode107 core/cross/2d/texture_2d.h:107: return reinterpret_cast<void*>(NULL); Move this definition to the .cc file and add NOTIMPLEMENTED(). http://codereview.chromium.org/2825074/diff/6004/9009#newcode109 core/cross/2d/texture_2d.h:109: Should have just a single blank line here. http://codereview.chromium.org/2825074/diff/6004/9009#newcode115 core/cross/2d/texture_2d.h:115: // The texture takes ownership of the bitmap data. Also doesn't seem to be applicable. http://codereview.chromium.org/2825074/diff/6004/9009#newcode124 core/cross/2d/texture_2d.h:124: Should have just one blank line. http://codereview.chromium.org/2825074/diff/6004/9012 File import/cross/collada_conditioner.cc (right): http://codereview.chromium.org/2825074/diff/6004/9012#newcode545 import/cross/collada_conditioner.cc:545: #if !defined(RENDERER_2D) In Google code we always put # stuff (called preprocessor directives) at the start of the line with no indentation, and we don't increase the indentation of the code inside them. http://codereview.chromium.org/2825074/diff/6004/9013 File import/import.gyp (right): http://codereview.chromium.org/2825074/diff/6004/9013#newcode56 import/import.gyp:56: '../build/libs.gyp:cg_libs', Needs two more spaces. http://codereview.chromium.org/2825074/diff/6004/9014 File installer/linux/debian.in/debian.gyp (right): http://codereview.chromium.org/2825074/diff/6004/9014#newcode79 installer/linux/debian.in/debian.gyp:79: Looks like the only difference in this file is a blank line that you added, so you should remove it from the CL by running "gcl change <change name>" and editing the file list. You can also then revert it back to the original version with "svn revert <filename>". http://codereview.chromium.org/2825074/diff/6004/9016 File plugin/linux/main_linux.cc (right): http://codereview.chromium.org/2825074/diff/6004/9016#newcode780 plugin/linux/main_linux.cc:780: Ditto, remove this file from the CL. http://codereview.chromium.org/2825074/diff/6004/9017 File tests/common/linux/testing_common.cc (right): http://codereview.chromium.org/2825074/diff/6004/9017#newcode59 tests/common/linux/testing_common.cc:59: static char kOffScreenRenderer[] = "O3D_D3D9_OFF_SCREEN"; No extra indent here. http://codereview.chromium.org/2825074/diff/6004/9017#newcode114 tests/common/linux/testing_common.cc:114: DefaultScreen(g_display), Looks like a lot of the alignment here got messed up. You should change it back to how it was. http://codereview.chromium.org/2825074/diff/6004/9017#newcode175 tests/common/linux/testing_common.cc:175: return 0; Should be indented by two spaces.
All the comments have been reviewed and made some changes to the files. http://codereview.chromium.org/2825074/diff/6004/9003 File build/libs.gyp (right): http://codereview.chromium.org/2825074/diff/6004/9003#newcode36 build/libs.gyp:36: [ 'OS=="mac"', On 2010/08/03 00:42:31, tschmelcher wrote: > We will eventually need Cairo linking flags for Win/Mac. Rather than copying the > OpenGL flags here, just leave out the "mac" and "win" conditions and put in > "TODO(fransiskusx): Link to Cairo on Win/Mac as a static library" Done. http://codereview.chromium.org/2825074/diff/6004/9004 File core/core.gyp (right): http://codereview.chromium.org/2825074/diff/6004/9004#newcode473 core/core.gyp:473: ['renderer == "2d"', On 2010/08/03 00:42:31, tschmelcher wrote: > Since there is already a "Texture2D" (as you observed), I don't think we should > call the new renderer "2d" because it will lead to some confusion. I think > instead we should use, renderer == "cairo", cross/cairo, renderer_cairo.h, etc. Done. http://codereview.chromium.org/2825074/diff/6004/9005 File core/cross/2d/install_check.cc (right): http://codereview.chromium.org/2825074/diff/6004/9005#newcode2 core/cross/2d/install_check.cc:2: * Copyright 2009, Google Inc. On 2010/08/03 00:42:31, tschmelcher wrote: > Whenever creating/copying/editing files we usually update the copyright year to > the current year. Done. http://codereview.chromium.org/2825074/diff/6004/9005#newcode40 core/cross/2d/install_check.cc:40: } // o3d On 2010/08/03 00:42:31, tschmelcher wrote: > Should have a blank line above this. Done. http://codereview.chromium.org/2825074/diff/6004/9006 File core/cross/2d/renderer_2d.cc (right): http://codereview.chromium.org/2825074/diff/6004/9006#newcode1 core/cross/2d/renderer_2d.cc:1: // Copyright 2010 Google Inc. All Rights Reserved. I copied and pasted the comments from renderer_gl. On 2010/08/03 00:42:31, tschmelcher wrote: > Actually, it's open-source so it's not "All Rights Reserved". :) Just copy the > big comment block used in the existing files. http://codereview.chromium.org/2825074/diff/6004/9006#newcode2 core/cross/2d/renderer_2d.cc:2: // Author: fransiskusx@google.com (Fransiskus Xaverius) On 2010/08/03 00:42:31, tschmelcher wrote: > In open-source code Google sadly has a policy of not including the name(s) of > the author(s), so you should remove this. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode12 core/cross/2d/renderer_2d.cc:12: On 2010/08/03 00:42:31, tschmelcher wrote: > To make this file clearer, I think you should organize the method definitions > into three sections: > > 1) Methods that have been implemented. > > 2) Methods that haven't been implemented yet and will need to be implemented in > the future (or might need to be). At the beginning of this section, put > something like "// TODO(fransiskusx): Determine which of these need to be > implemented and do so." > > 3) Methods that haven't been implemented and aren't relevant to 2D rendering, > which in the future should be removed from the class entirely when we create the > Renderer3D and Renderer2D interfaces. At the beginning of this section, put > something like "// TODO(fransiskusx): These aren't applicable to 2D rendering. > We should refactor the class hierarchy to remove them entirely." > > Doing that work may end up being outside the scope of your internship, but it's > good to add the comments anyway. > > Also, for all methods in section 2 or section 3, you should add NOTIMPLEMENTED() > to the beginning of their body. That will log a debug message in the event that > they are somehow called. You will probably have to #include "base/logging.h" to > use that. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode18 core/cross/2d/renderer_2d.cc:18: : Renderer(service_locator) { On 2010/08/03 00:42:31, tschmelcher wrote: > The Google style is: > > Renderer2D::Renderer2D(ServiceLocator* service_locator) > : Renderer(service_locator), <-- 4 spaces of indent > kFrameSrcData_(NULL), > ... { > } > > Also, you should always initialize all of your class's fields in the > constructor, even if you don't have meaningful values for them yet. Otherwise > their value will be undefined, which can make debugging harder. > > Any fields that are instances of classes are initialized automatically, but > pointers and integers are not (including pointers to classes), so you need to > initialize all of those. For pointers you can just initialize them to NULL and > for integers you can just use 0 ("Window" is actually an integer, not a class, > so initialize that to 0 too). Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode39 core/cross/2d/renderer_2d.cc:39: unsigned src_height, int src_pitch) { On 2010/08/03 00:42:31, tschmelcher wrote: > Google style is to align this with the previous row of arguments. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode55 core/cross/2d/renderer_2d.cc:55: // Preparing the image to render On 2010/08/03 00:42:31, tschmelcher wrote: > Add this: > > // TODO(fransiskusx): The shared memory may have been unregistered, so our > pointer may no longer be valid. We need to listen for shared memory > unregistration and unset our pointer when it happens. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode61 core/cross/2d/renderer_2d.cc:61: cairo_t *current_drawing = cairo_create(main_surface_); On 2010/08/03 00:42:31, tschmelcher wrote: > There are two styles for where to put the * and & in C/C++. You can either write > "cairo_t *current_drawing" (referred to as "pointer belongs to the variable") or > "cairo_t* current_drawing" (referred to as "pointer belongs to the type"). Both > ways of writing it do the exact same thing, but for ease of readability you > should always be consistent and just use one of the two styles. Most of this > file uses "pointer belongs to the type", but here you use "pointer belongs to > the variable", so you should change it all to be consistent. It's your choice > which style you want to use. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode63 core/cross/2d/renderer_2d.cc:63: // Scalling the image On 2010/08/03 00:42:31, tschmelcher wrote: > Spelling mistake (should be "scaling"). Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode64 core/cross/2d/renderer_2d.cc:64: double widthScaling = On 2010/08/03 00:42:31, tschmelcher wrote: > Google C++ style for variable names is to never use capitals like this, always > use width_scaling. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode65 core/cross/2d/renderer_2d.cc:65: (static_cast<double>(display_width()))/frame_src_width_; On 2010/08/03 00:42:31, tschmelcher wrote: > Should have spaces before and after "/" Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode66 core/cross/2d/renderer_2d.cc:66: double heightScaling = On 2010/08/03 00:42:31, tschmelcher wrote: > Same here. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode67 core/cross/2d/renderer_2d.cc:67: (static_cast<double>(display_height()))/frame_src_height_; On 2010/08/03 00:42:31, tschmelcher wrote: > Same here. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode74 core/cross/2d/renderer_2d.cc:74: } On 2010/08/03 00:42:31, tschmelcher wrote: > You also need to destroy the image surface, or else it will be leaked. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode79 core/cross/2d/renderer_2d.cc:79: DefaultVisual(display_, 0), On 2010/08/03 00:42:31, tschmelcher wrote: > You should use "XDefaultVisual" instead of "DefaultVisual". The X11 functions > that omit the "X" at the front are actually not functions, they are a C feature > called a macro. Using a macro from a shared library can be unsafe when writing > code that will run on lots of different computers and lots of different versions > of Linux, so it's better to avoid them. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode83 core/cross/2d/renderer_2d.cc:83: void Renderer2D::UninitCommon() {} On 2010/08/03 00:42:31, tschmelcher wrote: > Whenever you have an implementation of a void method where you don't need to do > anything, you should write a comment in it like: > > // Don't need to do anything. > > That way the reader knows that that method is correct to do nothing (as opposed > to being an unimplemented method that still needs to be written). Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode90 core/cross/2d/renderer_2d.cc:90: Renderer::InitStatus Renderer2D::InitPlatformSpecific( On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one space after ...::InitStatus Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode91 core/cross/2d/renderer_2d.cc:91: const DisplayWindow& display_window, On 2010/08/03 00:42:31, tschmelcher wrote: > When you can't fit the first argument onto the previous line, indent it by just > 4 spaces, like this: > > Renderer::InitStatus Renderer2D::InitPlatformSpecific( > const DisplayWindow& display_window, > ...) { > ... > } Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode94 core/cross/2d/renderer_2d.cc:94: static_cast<const DisplayWindowLinux&>(display_window); On 2010/08/03 00:42:31, tschmelcher wrote: > Also indent by just four spaces. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode109 core/cross/2d/renderer_2d.cc:109: cairo_surface_destroy(main_surface_); On 2010/08/03 00:42:31, tschmelcher wrote: > There is a cairo_xlib_surface_set_size function that you should be able to use > instead of destroying and recreating. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode117 core/cross/2d/renderer_2d.cc:117: RenderDepthStencilSurface::Ref Renderer2D::CreateDepthStencilSurface(int width, On 2010/08/03 00:42:31, tschmelcher wrote: > This should go in section 3. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode118 core/cross/2d/renderer_2d.cc:118: int height) { On 2010/08/03 00:42:31, tschmelcher wrote: > Should align with the previous line. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode127 core/cross/2d/renderer_2d.cc:127: bool Renderer2D::GoFullscreen(const DisplayWindow& display, On 2010/08/03 00:42:31, tschmelcher wrote: > Section 2. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode139 core/cross/2d/renderer_2d.cc:139: bool Renderer2D::CancelFullscreen(const DisplayWindow& display, On 2010/08/03 00:42:31, tschmelcher wrote: > Section 2. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode145 core/cross/2d/renderer_2d.cc:145: bool Renderer2D::fullscreen() const { On 2010/08/03 00:42:31, tschmelcher wrote: > Section 2. Also, it should return false for now. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode151 core/cross/2d/renderer_2d.cc:151: void Renderer2D::GetDisplayModes(std::vector<DisplayMode> *modes) {} On 2010/08/03 00:42:31, tschmelcher wrote: > Section 2. Also call modes->clear() so that there aren't old modes in the list > potentially. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode155 core/cross/2d/renderer_2d.cc:155: bool Renderer2D::GetDisplayMode(int id, DisplayMode *mode) { On 2010/08/03 00:42:31, tschmelcher wrote: > Section 2. > > Also, does the plugin still work if you return false here? If so, that would be > better. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode160 core/cross/2d/renderer_2d.cc:160: StreamBank::Ref Renderer2D::CreateStreamBank() { On 2010/08/03 00:42:31, tschmelcher wrote: > Next seven methods should go in section 3. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode206 core/cross/2d/renderer_2d.cc:206: void Renderer2D::SetBackBufferPlatformSpecific() {} On 2010/08/03 00:42:31, tschmelcher wrote: > Section 2. Isn't it gl specific? http://codereview.chromium.org/2825074/diff/6004/9006#newcode209 core/cross/2d/renderer_2d.cc:209: void Renderer2D::SetRenderSurfacesPlatformSpecific(const RenderSurface* surface, On 2010/08/03 00:42:31, tschmelcher wrote: > Section 3. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode213 core/cross/2d/renderer_2d.cc:213: ParamCache* Renderer2D::CreatePlatformSpecificParamCache() { On 2010/08/03 00:42:31, tschmelcher wrote: > Section 3. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode218 core/cross/2d/renderer_2d.cc:218: Texture2D::Ref Renderer2D::CreatePlatformSpecificTexture2D(int width, On 2010/08/03 00:42:31, tschmelcher wrote: > This is fully implemented, so section 1. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode232 core/cross/2d/renderer_2d.cc:232: TextureCUBE::Ref Renderer2D::CreatePlatformSpecificTextureCUBE(int edge_length, On 2010/08/03 00:42:31, tschmelcher wrote: > Section 3. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode240 core/cross/2d/renderer_2d.cc:240: bool Renderer2D::PlatformSpecificBeginDraw() { On 2010/08/03 00:42:31, tschmelcher wrote: > Next five methods should go in section 1. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode261 core/cross/2d/renderer_2d.cc:261: void Renderer2D::PlatformSpecificClear(const Float4 &color, On 2010/08/03 00:42:31, tschmelcher wrote: > Not sure about this one. Let's put it in section 2 for now. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode269 core/cross/2d/renderer_2d.cc:269: void Renderer2D::ApplyDirtyStates() {} On 2010/08/03 00:42:31, tschmelcher wrote: > Section 3. Done. http://codereview.chromium.org/2825074/diff/6004/9006#newcode273 core/cross/2d/renderer_2d.cc:273: void Renderer2D::SetViewportInPixels(int left, On 2010/08/03 00:42:31, tschmelcher wrote: > Not sure about this one either. Section 2 for now. I put it into section 2 for Now. I think it is better to reconsider it later rather than to leave it and actually it is useful. http://codereview.chromium.org/2825074/diff/6004/9006#newcode282 core/cross/2d/renderer_2d.cc:282: void Renderer2D::PushRenderStates(State *state) { On 2010/08/03 00:42:31, tschmelcher wrote: > Section 3. Done. http://codereview.chromium.org/2825074/diff/6004/9007 File core/cross/2d/renderer_2d.h (right): http://codereview.chromium.org/2825074/diff/6004/9007#newcode1 core/cross/2d/renderer_2d.h:1: // Copyright 2010 Google Inc. All Rights Reserved. On 2010/08/03 00:42:31, tschmelcher wrote: > Also use standard O3D comment block here. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode5 core/cross/2d/renderer_2d.h:5: #ifndef RENDERER_2D_H_ On 2010/08/03 00:42:31, tschmelcher wrote: > Should start with O3D_CORE_CROSS_2D_ Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode8 core/cross/2d/renderer_2d.h:8: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode15 core/cross/2d/renderer_2d.h:15: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode26 core/cross/2d/renderer_2d.h:26: virtual void SetNewFrame(const void* kSrcData, unsigned src_width, On 2010/08/03 00:42:31, tschmelcher wrote: > This doesn't need to be virtual. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode27 core/cross/2d/renderer_2d.h:27: unsigned src_height, int src_pitch); On 2010/08/03 00:42:31, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode30 core/cross/2d/renderer_2d.h:30: virtual void Paint(); On 2010/08/03 00:42:31, tschmelcher wrote: > Doesn't need to be virtual. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode39 core/cross/2d/renderer_2d.h:39: bool off_screen); On 2010/08/03 00:42:31, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode44 core/cross/2d/renderer_2d.h:44: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode45 core/cross/2d/renderer_2d.h:45: // ///////////////////////////////to IMPLEMENT from Abstract Class Renderer On 2010/08/03 00:42:31, tschmelcher wrote: > Remove this comment. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode52 core/cross/2d/renderer_2d.h:52: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode90 core/cross/2d/renderer_2d.h:90: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode113 core/cross/2d/renderer_2d.h:113: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode119 core/cross/2d/renderer_2d.h:119: // ///////////////////////////////////////////// to IMPLEMENT-- On 2010/08/03 00:42:31, tschmelcher wrote: > Remove this comment. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode123 core/cross/2d/renderer_2d.h:123: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode139 core/cross/2d/renderer_2d.h:139: // Keep the constructor protected so only factory methods can create On 2010/08/03 00:42:31, tschmelcher wrote: > Google style is for all the protected methods to come before any of the > protected fields (some O3D files disobey this rule, but you should follow it). Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode143 core/cross/2d/renderer_2d.h:143: // ///////////////////////////////to IMPLEMENT from Abstract Class Renderer On 2010/08/03 00:42:31, tschmelcher wrote: > Remove this comment. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode156 core/cross/2d/renderer_2d.h:156: int width, On 2010/08/03 00:42:31, tschmelcher wrote: > Indent by 4 spaces from the start of "virtual". Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode164 core/cross/2d/renderer_2d.h:164: int edge_length, On 2010/08/03 00:42:31, tschmelcher wrote: > Indent by 4 spaces from the start of "virtual". Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode195 core/cross/2d/renderer_2d.h:195: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode204 core/cross/2d/renderer_2d.h:204: // ///////////////////////////////////////////// to IMPLEMENT-- On 2010/08/03 00:42:31, tschmelcher wrote: > Remove this comment. Done. http://codereview.chromium.org/2825074/diff/6004/9007#newcode207 core/cross/2d/renderer_2d.h:207: #endif /* RENDERER_2D_H_ */ On 2010/08/03 00:42:31, tschmelcher wrote: > We use // for this, and put two spaces after the endif. Done. http://codereview.chromium.org/2825074/diff/6004/9008 File core/cross/2d/texture_2d.cc (right): http://codereview.chromium.org/2825074/diff/6004/9008#newcode3 core/cross/2d/texture_2d.cc:3: * All rights reserved. On 2010/08/03 00:42:31, tschmelcher wrote: > Also use standard O3D comment block here. Done. http://codereview.chromium.org/2825074/diff/6004/9008#newcode39 core/cross/2d/texture_2d.cc:39: On 2010/08/03 00:42:31, tschmelcher wrote: > Here you will need to include renderer_2d.h once you remove it from > texture_2d.h. Done. http://codereview.chromium.org/2825074/diff/6004/9008#newcode43 core/cross/2d/texture_2d.cc:43: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9008#newcode52 core/cross/2d/texture_2d.cc:52: // NOTE: the Texture2DCairo now owns the texture and will destroy it on exit. On 2010/08/03 00:42:31, tschmelcher wrote: > Not applicable. Done. http://codereview.chromium.org/2825074/diff/6004/9008#newcode54 core/cross/2d/texture_2d.cc:54: Texture::Format format, On 2010/08/03 00:42:31, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/6004/9008#newcode73 core/cross/2d/texture_2d.cc:73: Texture::Format format, On 2010/08/03 00:42:31, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/6004/9008#newcode80 core/cross/2d/texture_2d.cc:80: format, On 2010/08/03 00:42:31, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/6004/9008#newcode99 core/cross/2d/texture_2d.cc:99: unsigned dst_left, On 2010/08/03 00:42:31, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/6004/9008#newcode108 core/cross/2d/texture_2d.cc:108: src_width, src_height, On 2010/08/03 00:42:31, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/6004/9008#newcode114 core/cross/2d/texture_2d.cc:114: bool Texture2DCairo::PlatformSpecificLock( On 2010/08/03 00:42:31, tschmelcher wrote: > Add NOTIMPLEMENTED() to this and the rest. Done. http://codereview.chromium.org/2825074/diff/6004/9008#newcode134 core/cross/2d/texture_2d.cc:134: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9009 File core/cross/2d/texture_2d.h (right): http://codereview.chromium.org/2825074/diff/6004/9009#newcode3 core/cross/2d/texture_2d.h:3: * All rights reserved. On 2010/08/03 00:42:31, tschmelcher wrote: > Also use standard O3D comment block here. Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode39 core/cross/2d/texture_2d.h:39: // Precompiled header comes before everything else. On 2010/08/03 00:42:31, tschmelcher wrote: > I don't think this comment is applicable here. Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode41 core/cross/2d/texture_2d.h:41: // Disable compiler warning for openGL calls that require a void* to On 2010/08/03 00:42:31, tschmelcher wrote: > This stuff is probably not required. Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode51 core/cross/2d/texture_2d.h:51: #include "core/cross/2d/renderer_2d.h" On 2010/08/03 00:42:31, tschmelcher wrote: > You don't need this actually, because you already have "class Renderer2D;" > below, which is enough for this file because it doesn't rely on any of the > details of the Renderer2D class--only the texture_2d.cc file does. Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode52 core/cross/2d/texture_2d.h:52: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just a single blank line here. Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode60 core/cross/2d/texture_2d.h:60: // Texture2DCairo implements the Texture2D interfacel. On 2010/08/03 00:42:31, tschmelcher wrote: > Typo, should be "interface". Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode79 core/cross/2d/texture_2d.h:79: // The created texture takes ownership of the bitmap data. On 2010/08/03 00:42:31, tschmelcher wrote: > This comment about ownership of the bitmap data doesn't seem to be applicable > here. Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode81 core/cross/2d/texture_2d.h:81: Texture::Format format, On 2010/08/03 00:42:31, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode107 core/cross/2d/texture_2d.h:107: return reinterpret_cast<void*>(NULL); On 2010/08/03 00:42:31, tschmelcher wrote: > Move this definition to the .cc file and add NOTIMPLEMENTED(). Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode109 core/cross/2d/texture_2d.h:109: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just a single blank line here. Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode115 core/cross/2d/texture_2d.h:115: // The texture takes ownership of the bitmap data. On 2010/08/03 00:42:31, tschmelcher wrote: > Also doesn't seem to be applicable. Done. http://codereview.chromium.org/2825074/diff/6004/9009#newcode124 core/cross/2d/texture_2d.h:124: On 2010/08/03 00:42:31, tschmelcher wrote: > Should have just one blank line. Done. http://codereview.chromium.org/2825074/diff/6004/9012 File import/cross/collada_conditioner.cc (right): http://codereview.chromium.org/2825074/diff/6004/9012#newcode545 import/cross/collada_conditioner.cc:545: #if !defined(RENDERER_2D) On 2010/08/03 00:42:31, tschmelcher wrote: > In Google code we always put # stuff (called preprocessor directives) at the > start of the line with no indentation, and we don't increase the indentation of > the code inside them. Done. http://codereview.chromium.org/2825074/diff/6004/9013 File import/import.gyp (right): http://codereview.chromium.org/2825074/diff/6004/9013#newcode56 import/import.gyp:56: '../build/libs.gyp:cg_libs', On 2010/08/03 00:42:31, tschmelcher wrote: > Needs two more spaces. Done. http://codereview.chromium.org/2825074/diff/6004/9014 File installer/linux/debian.in/debian.gyp (right): http://codereview.chromium.org/2825074/diff/6004/9014#newcode79 installer/linux/debian.in/debian.gyp:79: On 2010/08/03 00:42:31, tschmelcher wrote: > Looks like the only difference in this file is a blank line that you added, so > you should remove it from the CL by running "gcl change <change name>" and > editing the file list. You can also then revert it back to the original version > with "svn revert <filename>". Done. http://codereview.chromium.org/2825074/diff/6004/9016 File plugin/linux/main_linux.cc (right): http://codereview.chromium.org/2825074/diff/6004/9016#newcode780 plugin/linux/main_linux.cc:780: On 2010/08/03 00:42:31, tschmelcher wrote: > Ditto, remove this file from the CL. Done. http://codereview.chromium.org/2825074/diff/6004/9017 File tests/common/linux/testing_common.cc (right): http://codereview.chromium.org/2825074/diff/6004/9017#newcode59 tests/common/linux/testing_common.cc:59: static char kOffScreenRenderer[] = "O3D_D3D9_OFF_SCREEN"; On 2010/08/03 00:42:31, tschmelcher wrote: > No extra indent here. Done. http://codereview.chromium.org/2825074/diff/6004/9017#newcode114 tests/common/linux/testing_common.cc:114: DefaultScreen(g_display), On 2010/08/03 00:42:31, tschmelcher wrote: > Looks like a lot of the alignment here got messed up. You should change it back > to how it was. Done. http://codereview.chromium.org/2825074/diff/6004/9017#newcode175 tests/common/linux/testing_common.cc:175: return 0; On 2010/08/03 00:42:31, tschmelcher wrote: > Should be indented by two spaces. Done.
Getting closer! http://codereview.chromium.org/2825074/diff/6004/9006 File core/cross/2d/renderer_2d.cc (right): http://codereview.chromium.org/2825074/diff/6004/9006#newcode206 core/cross/2d/renderer_2d.cc:206: void Renderer2D::SetBackBufferPlatformSpecific() {} On 2010/08/04 00:05:44, fransiskusx wrote: > On 2010/08/03 00:42:31, tschmelcher wrote: > > Section 2. > > Isn't it gl specific? Not exactly. A back-buffer is a kind of optimization that is used when rendering to a very large window. Instead of rendering directly to the large window, one can first render to a smaller area in memory and then scale up the rendered frame to fit the actual window. If there are lots of things to be drawn then that can improve performance because you only have to scale up once, instead of having to scale up many times. That's a feature that we may want to consider later in order to improve performance (especially in fullscreen mode), so that's why I think section 2 is best for now. http://codereview.chromium.org/2825074/diff/6004/9006#newcode273 core/cross/2d/renderer_2d.cc:273: void Renderer2D::SetViewportInPixels(int left, On 2010/08/04 00:05:44, fransiskusx wrote: > On 2010/08/03 00:42:31, tschmelcher wrote: > > Not sure about this one either. Section 2 for now. > > I put it into section 2 for Now. I think it is better to reconsider it later > rather than to leave it and actually it is useful. My thoughts exactly. :) http://codereview.chromium.org/2825074/diff/25018/64006 File core/cross/cairo/install_check.cc (right): http://codereview.chromium.org/2825074/diff/25018/64006#newcode40 core/cross/cairo/install_check.cc:40: } // o3d Still needs a blank line added above. http://codereview.chromium.org/2825074/diff/25018/64007 File core/cross/cairo/renderer_cairo.cc (right): http://codereview.chromium.org/2825074/diff/25018/64007#newcode51 core/cross/cairo/renderer_cairo.cc:51: } Needs blank line below. http://codereview.chromium.org/2825074/diff/25018/64007#newcode123 core/cross/cairo/renderer_cairo.cc:123: Renderer* Renderer::CreateDefaultRenderer(ServiceLocator* service_locator) { Since this is actually a definition of a method on the Renderer class and not the RendererCairo class, I think it should go at the very top of the file instead of being mixed in with the RendererCairo methods. http://codereview.chromium.org/2825074/diff/25018/64007#newcode131 core/cross/cairo/renderer_cairo.cc:131: static_cast<const DisplayWindowLinux&>(display_window); Still needs indenting to be fixed (should be just 4 spaces more than the "const" on the previous line). http://codereview.chromium.org/2825074/diff/25018/64007#newcode138 core/cross/cairo/renderer_cairo.cc:138: void RendererCairo::SetState(Renderer* renderer, Param* param) { Actually, this should probably be section 3. http://codereview.chromium.org/2825074/diff/25018/64007#newcode163 core/cross/cairo/renderer_cairo.cc:163: int mode_id) { Alignment. http://codereview.chromium.org/2825074/diff/25018/64007#newcode177 core/cross/cairo/renderer_cairo.cc:177: int width, int height) { Alignment. http://codereview.chromium.org/2825074/diff/25018/64007#newcode193 core/cross/cairo/renderer_cairo.cc:193: // Don't need to do anything. We DO need to do something eventually, so you shouldn't put this here. http://codereview.chromium.org/2825074/diff/25018/64007#newcode213 core/cross/cairo/renderer_cairo.cc:213: NOTIMPLEMENTED(); This method is already implemented. You should remove the NOTIMPLEMENTED() and move the method to section 1. http://codereview.chromium.org/2825074/diff/25018/64007#newcode224 core/cross/cairo/renderer_cairo.cc:224: void RendererCairo::PlatformSpecificEndDraw() { The next four methods are done. They should be in section 1 after PlatformSpecificBeginDraw(), and without NOTIMPLEMENTED(). http://codereview.chromium.org/2825074/diff/25018/64007#newcode258 core/cross/cairo/renderer_cairo.cc:258: // Don't need to do anything. We DO need to do something eventually, so you shouldn't put this here. http://codereview.chromium.org/2825074/diff/25018/64007#newcode270 core/cross/cairo/renderer_cairo.cc:270: // Don't need to do anything. We DO need to do something eventually, so you shouldn't put this here. http://codereview.chromium.org/2825074/diff/25018/64007#newcode389 core/cross/cairo/renderer_cairo.cc:389: } Should have blank line below here. http://codereview.chromium.org/2825074/diff/25018/64008 File core/cross/cairo/renderer_cairo.h (right): http://codereview.chromium.org/2825074/diff/25018/64008#newcode48 core/cross/cairo/renderer_cairo.h:48: static RendererCairo* CreateDefault(ServiceLocator* service_locator); Should only be indented by 2 spaces from the start (i.e., 1 space more than "public"). Same for the rest of the methods and fields. http://codereview.chromium.org/2825074/diff/25018/64008#newcode65 core/cross/cairo/renderer_cairo.h:65: void SetNewFrame(const void* kSrcData, unsigned src_width, (I should have noticed this before.) Google style reserves names like kSrcData for global constants, so you should use src_data_ instead. http://codereview.chromium.org/2825074/diff/25018/64008#newcode77 core/cross/cairo/renderer_cairo.h:77: int height); Alignment. http://codereview.chromium.org/2825074/diff/25018/64008#newcode217 core/cross/cairo/renderer_cairo.h:217: const void* kFrameSrcData_; frame_src_data_ http://codereview.chromium.org/2825074/diff/25018/64008#newcode225 core/cross/cairo/renderer_cairo.h:225: } Needs blank line above and below. http://codereview.chromium.org/2825074/diff/25018/64008#newcode227 core/cross/cairo/renderer_cairo.h:227: No trailing blank lines. http://codereview.chromium.org/2825074/diff/25018/64009 File core/cross/cairo/texture_cairo.cc (right): http://codereview.chromium.org/2825074/diff/25018/64009#newcode103 core/cross/cairo/texture_cairo.cc:103: src_width, src_height, Alignment. http://codereview.chromium.org/2825074/diff/25018/64010 File core/cross/cairo/texture_cairo.h (right): http://codereview.chromium.org/2825074/diff/25018/64010#newcode39 core/cross/cairo/texture_cairo.h:39: #if defined(OS_WIN) This Windows stuff probably isn't applicable, so I would just delete it. http://codereview.chromium.org/2825074/diff/25018/64010#newcode44 core/cross/cairo/texture_cairo.h:44: #include <cairo.h> I don't think you need to include any Cairo or X11 .h files here. If texture_cairo.cc needs them then move them to there. http://codereview.chromium.org/2825074/diff/25018/64010#newcode72 core/cross/cairo/texture_cairo.h:72: static TextureCairo* Create(ServiceLocator* service_locator, Usually static functions should be at the start (right after the typedef and before the destructor). http://codereview.chromium.org/2825074/diff/25018/64010#newcode101 core/cross/cairo/texture_cairo.h:101: // Initializes the Texture2D from a preexisting texture handle This comment isn't applicable. http://codereview.chromium.org/2825074/diff/25018/64013 File import/cross/collada_conditioner.cc (right): http://codereview.chromium.org/2825074/diff/25018/64013#newcode549 import/cross/collada_conditioner.cc:549: "technique t {\n" A lot of the indentation here is still messed up. An easy way to get back to the original indentation is to run "svn cat collada_conditioner.cc > tmp.cc". It will write the original version of the file into a new file called tmp.cc. Then you can open tmp.cc and copy-and-paste the parts that you need into your modified collada_conditioner.cc. http://codereview.chromium.org/2825074/diff/25018/64016 File tests/common/linux/testing_common.cc (right): http://codereview.chromium.org/2825074/diff/25018/64016#newcode114 tests/common/linux/testing_common.cc:114: DefaultScreen(g_display), Indenting still messed up here too.
http://codereview.chromium.org/2825074/diff/25018/64006 File core/cross/cairo/install_check.cc (right): http://codereview.chromium.org/2825074/diff/25018/64006#newcode40 core/cross/cairo/install_check.cc:40: } // o3d On 2010/08/04 00:54:44, tschmelcher wrote: > Still needs a blank line added above. Done. http://codereview.chromium.org/2825074/diff/25018/64006#newcode40 core/cross/cairo/install_check.cc:40: } // o3d On 2010/08/04 00:54:44, tschmelcher wrote: > Still needs a blank line added above. Done. http://codereview.chromium.org/2825074/diff/25018/64007 File core/cross/cairo/renderer_cairo.cc (right): http://codereview.chromium.org/2825074/diff/25018/64007#newcode51 core/cross/cairo/renderer_cairo.cc:51: } On 2010/08/04 00:54:44, tschmelcher wrote: > Needs blank line below. Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode123 core/cross/cairo/renderer_cairo.cc:123: Renderer* Renderer::CreateDefaultRenderer(ServiceLocator* service_locator) { On 2010/08/04 00:54:44, tschmelcher wrote: > Since this is actually a definition of a method on the Renderer class and not > the RendererCairo class, I think it should go at the very top of the file > instead of being mixed in with the RendererCairo methods. Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode131 core/cross/cairo/renderer_cairo.cc:131: static_cast<const DisplayWindowLinux&>(display_window); On 2010/08/04 00:54:44, tschmelcher wrote: > Still needs indenting to be fixed (should be just 4 spaces more than the "const" > on the previous line). Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode138 core/cross/cairo/renderer_cairo.cc:138: void RendererCairo::SetState(Renderer* renderer, Param* param) { On 2010/08/04 00:54:44, tschmelcher wrote: > Actually, this should probably be section 3. Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode163 core/cross/cairo/renderer_cairo.cc:163: int mode_id) { On 2010/08/04 00:54:44, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode177 core/cross/cairo/renderer_cairo.cc:177: int width, int height) { On 2010/08/04 00:54:44, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode193 core/cross/cairo/renderer_cairo.cc:193: // Don't need to do anything. On 2010/08/04 00:54:44, tschmelcher wrote: > We DO need to do something eventually, so you shouldn't put this here. Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode213 core/cross/cairo/renderer_cairo.cc:213: NOTIMPLEMENTED(); On 2010/08/04 00:54:44, tschmelcher wrote: > This method is already implemented. You should remove the NOTIMPLEMENTED() and > move the method to section 1. Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode224 core/cross/cairo/renderer_cairo.cc:224: void RendererCairo::PlatformSpecificEndDraw() { On 2010/08/04 00:54:44, tschmelcher wrote: > The next four methods are done. They should be in section 1 after > PlatformSpecificBeginDraw(), and without NOTIMPLEMENTED(). Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode258 core/cross/cairo/renderer_cairo.cc:258: // Don't need to do anything. On 2010/08/04 00:54:44, tschmelcher wrote: > We DO need to do something eventually, so you shouldn't put this here. Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode270 core/cross/cairo/renderer_cairo.cc:270: // Don't need to do anything. On 2010/08/04 00:54:44, tschmelcher wrote: > We DO need to do something eventually, so you shouldn't put this here. Done. http://codereview.chromium.org/2825074/diff/25018/64007#newcode389 core/cross/cairo/renderer_cairo.cc:389: } On 2010/08/04 00:54:44, tschmelcher wrote: > Should have blank line below here. Done. http://codereview.chromium.org/2825074/diff/25018/64008 File core/cross/cairo/renderer_cairo.h (right): http://codereview.chromium.org/2825074/diff/25018/64008#newcode48 core/cross/cairo/renderer_cairo.h:48: static RendererCairo* CreateDefault(ServiceLocator* service_locator); On 2010/08/04 00:54:44, tschmelcher wrote: > Should only be indented by 2 spaces from the start (i.e., 1 space more than > "public"). Same for the rest of the methods and fields. Done. http://codereview.chromium.org/2825074/diff/25018/64008#newcode65 core/cross/cairo/renderer_cairo.h:65: void SetNewFrame(const void* kSrcData, unsigned src_width, On 2010/08/04 00:54:44, tschmelcher wrote: > (I should have noticed this before.) Google style reserves names like kSrcData > for global constants, so you should use src_data_ instead. Done. http://codereview.chromium.org/2825074/diff/25018/64008#newcode77 core/cross/cairo/renderer_cairo.h:77: int height); On 2010/08/04 00:54:44, tschmelcher wrote: > Alignment. Done. http://codereview.chromium.org/2825074/diff/25018/64008#newcode217 core/cross/cairo/renderer_cairo.h:217: const void* kFrameSrcData_; On 2010/08/04 00:54:44, tschmelcher wrote: > frame_src_data_ Done. http://codereview.chromium.org/2825074/diff/25018/64008#newcode225 core/cross/cairo/renderer_cairo.h:225: } On 2010/08/04 00:54:44, tschmelcher wrote: > Needs blank line above and below. Done. http://codereview.chromium.org/2825074/diff/25018/64008#newcode227 core/cross/cairo/renderer_cairo.h:227: On 2010/08/04 00:54:44, tschmelcher wrote: > No trailing blank lines. Done. http://codereview.chromium.org/2825074/diff/25018/64010 File core/cross/cairo/texture_cairo.h (right): http://codereview.chromium.org/2825074/diff/25018/64010#newcode39 core/cross/cairo/texture_cairo.h:39: #if defined(OS_WIN) On 2010/08/04 00:54:44, tschmelcher wrote: > This Windows stuff probably isn't applicable, so I would just delete it. Done. http://codereview.chromium.org/2825074/diff/25018/64010#newcode44 core/cross/cairo/texture_cairo.h:44: #include <cairo.h> On 2010/08/04 00:54:44, tschmelcher wrote: > I don't think you need to include any Cairo or X11 .h files here. If > texture_cairo.cc needs them then move them to there. Done. http://codereview.chromium.org/2825074/diff/25018/64010#newcode72 core/cross/cairo/texture_cairo.h:72: static TextureCairo* Create(ServiceLocator* service_locator, On 2010/08/04 00:54:44, tschmelcher wrote: > Usually static functions should be at the start (right after the typedef and > before the destructor). Done. http://codereview.chromium.org/2825074/diff/25018/64010#newcode101 core/cross/cairo/texture_cairo.h:101: // Initializes the Texture2D from a preexisting texture handle On 2010/08/04 00:54:44, tschmelcher wrote: > This comment isn't applicable. Done. http://codereview.chromium.org/2825074/diff/25018/64013 File import/cross/collada_conditioner.cc (right): http://codereview.chromium.org/2825074/diff/25018/64013#newcode549 import/cross/collada_conditioner.cc:549: "technique t {\n" On 2010/08/04 00:54:44, tschmelcher wrote: > A lot of the indentation here is still messed up. > > An easy way to get back to the original indentation is to run "svn cat > collada_conditioner.cc > tmp.cc". It will write the original version of the file > into a new file called tmp.cc. Then you can open tmp.cc and copy-and-paste the > parts that you need into your modified collada_conditioner.cc. Done. http://codereview.chromium.org/2825074/diff/25018/64016 File tests/common/linux/testing_common.cc (right): http://codereview.chromium.org/2825074/diff/25018/64016#newcode114 tests/common/linux/testing_common.cc:114: DefaultScreen(g_display), On 2010/08/04 00:54:44, tschmelcher wrote: > Indenting still messed up here too. Done.
http://codereview.chromium.org/2825074/diff/25018/64008 File core/cross/cairo/renderer_cairo.h (right): http://codereview.chromium.org/2825074/diff/25018/64008#newcode65 core/cross/cairo/renderer_cairo.h:65: void SetNewFrame(const void* kSrcData, unsigned src_width, On 2010/08/04 17:26:15, fransiskusx wrote: > On 2010/08/04 00:54:44, tschmelcher wrote: > > (I should have noticed this before.) Google style reserves names like kSrcData > > for global constants, so you should use src_data_ instead. > > Done. Still needs to be fixed here. http://codereview.chromium.org/2825074/diff/25018/64008#newcode225 core/cross/cairo/renderer_cairo.h:225: } On 2010/08/04 17:26:15, fransiskusx wrote: > On 2010/08/04 00:54:44, tschmelcher wrote: > > Needs blank line above and below. > > Done. Still needs a blank line below. http://codereview.chromium.org/2825074/diff/12005/28008 File core/cross/cairo/renderer_cairo.cc (right): http://codereview.chromium.org/2825074/diff/12005/28008#newcode74 core/cross/cairo/renderer_cairo.cc:74: void RendererCairo::SetNewFrame(const void* kSrcData, unsigned src_width, Fix here too. http://codereview.chromium.org/2825074/diff/12005/28008#newcode202 core/cross/cairo/renderer_cairo.cc:202: // Don't need to do anything. This comment isn't true, so you should remove it. http://codereview.chromium.org/2825074/diff/12005/28008#newcode214 core/cross/cairo/renderer_cairo.cc:214: // Don't need to do anything. Same here. http://codereview.chromium.org/2825074/diff/12005/28008#newcode226 core/cross/cairo/renderer_cairo.cc:226: // Don't need to do anything. And here. http://codereview.chromium.org/2825074/diff/12005/28010 File core/cross/cairo/texture_cairo.cc (right): http://codereview.chromium.org/2825074/diff/12005/28010#newcode37 core/cross/cairo/texture_cairo.cc:37: #include <cairo.h> Did you try compiling without these includes at all? It looks to me like texture_cairo.cc doesn't directly use any cairo or X11 stuff, it only uses the RendererCairo class, so you shouldn't need to include cairo or X11. http://codereview.chromium.org/2825074/diff/12005/28014 File import/cross/collada_conditioner.cc (right): http://codereview.chromium.org/2825074/diff/12005/28014#newcode580 import/cross/collada_conditioner.cc:580: return true; Hmm, actually, putting "return true" after "return retval" will cause an error with some compilers, so instead you should use #else, e.g.: #else return true; #endif But I actually think you should return false, because in a Cairo build the function will do nothing so it won't have actually succeeded. http://codereview.chromium.org/2825074/diff/12005/28017 File tests/common/linux/testing_common.cc (right): http://codereview.chromium.org/2825074/diff/12005/28017#newcode175 tests/common/linux/testing_common.cc:175: return 0; Same here, and return 1 instead of 0 (by convention, returning 0 from main() means the program ran successfully, and any other number means it failed).
http://codereview.chromium.org/2825074/diff/12005/28008 File core/cross/cairo/renderer_cairo.cc (right): http://codereview.chromium.org/2825074/diff/12005/28008#newcode74 core/cross/cairo/renderer_cairo.cc:74: void RendererCairo::SetNewFrame(const void* kSrcData, unsigned src_width, On 2010/08/04 18:19:31, tschmelcher wrote: > Fix here too. Oops.. My bad. Changed. http://codereview.chromium.org/2825074/diff/12005/28008#newcode202 core/cross/cairo/renderer_cairo.cc:202: // Don't need to do anything. On 2010/08/04 18:19:31, tschmelcher wrote: > This comment isn't true, so you should remove it. Done. http://codereview.chromium.org/2825074/diff/12005/28008#newcode214 core/cross/cairo/renderer_cairo.cc:214: // Don't need to do anything. On 2010/08/04 18:19:31, tschmelcher wrote: > Same here. Done. http://codereview.chromium.org/2825074/diff/12005/28008#newcode226 core/cross/cairo/renderer_cairo.cc:226: // Don't need to do anything. On 2010/08/04 18:19:31, tschmelcher wrote: > And here. Done. http://codereview.chromium.org/2825074/diff/12005/28010 File core/cross/cairo/texture_cairo.cc (right): http://codereview.chromium.org/2825074/diff/12005/28010#newcode37 core/cross/cairo/texture_cairo.cc:37: #include <cairo.h> On 2010/08/04 18:19:31, tschmelcher wrote: > Did you try compiling without these includes at all? It looks to me like > texture_cairo.cc doesn't directly use any cairo or X11 stuff, it only uses the > RendererCairo class, so you shouldn't need to include cairo or X11. I Should have realized. :( Done. http://codereview.chromium.org/2825074/diff/12005/28014 File import/cross/collada_conditioner.cc (right): http://codereview.chromium.org/2825074/diff/12005/28014#newcode580 import/cross/collada_conditioner.cc:580: return true; On 2010/08/04 18:19:31, tschmelcher wrote: > Hmm, actually, putting "return true" after "return retval" will cause an error > with some compilers, so instead you should use #else, e.g.: > > #else > return true; > #endif > > But I actually think you should return false, because in a Cairo build the > function will do nothing so it won't have actually succeeded. Done. http://codereview.chromium.org/2825074/diff/12005/28017 File tests/common/linux/testing_common.cc (right): http://codereview.chromium.org/2825074/diff/12005/28017#newcode175 tests/common/linux/testing_common.cc:175: return 0; On 2010/08/04 18:19:31, tschmelcher wrote: > Same here, and return 1 instead of 0 (by convention, returning 0 from main() > means the program ran successfully, and any other number means it failed). Done.
http://codereview.chromium.org/2825074/diff/12005/28014 File import/cross/collada_conditioner.cc (right): http://codereview.chromium.org/2825074/diff/12005/28014#newcode580 import/cross/collada_conditioner.cc:580: return true; On 2010/08/04 19:00:48, fransiskusx wrote: > On 2010/08/04 18:19:31, tschmelcher wrote: > > Hmm, actually, putting "return true" after "return retval" will cause an error > > with some compilers, so instead you should use #else, e.g.: > > > > #else > > return true; > > #endif > > > > But I actually think you should return false, because in a Cairo build the > > function will do nothing so it won't have actually succeeded. > > Done. Still need to put in the #else. http://codereview.chromium.org/2825074/diff/12005/28017 File tests/common/linux/testing_common.cc (right): http://codereview.chromium.org/2825074/diff/12005/28017#newcode175 tests/common/linux/testing_common.cc:175: return 0; On 2010/08/04 19:00:48, fransiskusx wrote: > On 2010/08/04 18:19:31, tschmelcher wrote: > > Same here, and return 1 instead of 0 (by convention, returning 0 from main() > > means the program ran successfully, and any other number means it failed). > > Done. Still need to put in the #else.
http://codereview.chromium.org/2825074/diff/12005/28014 File import/cross/collada_conditioner.cc (right): http://codereview.chromium.org/2825074/diff/12005/28014#newcode580 import/cross/collada_conditioner.cc:580: return true; On 2010/08/04 19:09:56, tschmelcher wrote: > On 2010/08/04 19:00:48, fransiskusx wrote: > > On 2010/08/04 18:19:31, tschmelcher wrote: > > > Hmm, actually, putting "return true" after "return retval" will cause an > error > > > with some compilers, so instead you should use #else, e.g.: > > > > > > #else > > > return true; > > > #endif > > > > > > But I actually think you should return false, because in a Cairo build the > > > function will do nothing so it won't have actually succeeded. > > > > Done. > > Still need to put in the #else. Done. http://codereview.chromium.org/2825074/diff/12005/28017 File tests/common/linux/testing_common.cc (right): http://codereview.chromium.org/2825074/diff/12005/28017#newcode175 tests/common/linux/testing_common.cc:175: return 0; On 2010/08/04 19:09:56, tschmelcher wrote: > On 2010/08/04 19:00:48, fransiskusx wrote: > > On 2010/08/04 18:19:31, tschmelcher wrote: > > > Same here, and return 1 instead of 0 (by convention, returning 0 from main() > > > means the program ran successfully, and any other number means it failed). > > > > Done. > > Still need to put in the #else. Done.
http://codereview.chromium.org/2825074/diff/10021/65003 File build/libs.gyp (right): http://codereview.chromium.org/2825074/diff/10021/65003#newcode221 build/libs.gyp:221: #TODO(fransiskusx): Link to Cairo on Win/Mac as a static library I just noticed that this is actually in the wrong place. This section is for the Cg libs section, but you don't want to be changing that. It's the cairo_libs above that you should be changing. http://codereview.chromium.org/2825074/diff/10021/65004 File core/core.gyp (right): http://codereview.chromium.org/2825074/diff/10021/65004#newcode480 core/core.gyp:480: 'cross/cairo/install_check.cc', Lists like this should generally be in alphabetical order, so install_check.cc should come first. http://codereview.chromium.org/2825074/diff/10021/65006 File core/cross/cairo/renderer_cairo.cc (right): http://codereview.chromium.org/2825074/diff/10021/65006#newcode134 core/cross/cairo/renderer_cairo.cc:134: static_cast<const DisplayWindowLinux&>(display_window); Should be just 4 spaces of indent, not 6. http://codereview.chromium.org/2825074/diff/10021/65007 File core/cross/cairo/renderer_cairo.h (right): http://codereview.chromium.org/2825074/diff/10021/65007#newcode38 core/cross/cairo/renderer_cairo.h:38: #include <cairo-xlib.h> I think cairo-xlib.h (and only that one) can be moved to the .cc file.
http://codereview.chromium.org/2825074/diff/10021/65003 File build/libs.gyp (right): http://codereview.chromium.org/2825074/diff/10021/65003#newcode221 build/libs.gyp:221: #TODO(fransiskusx): Link to Cairo on Win/Mac as a static library On 2010/08/04 19:36:17, Tristan Schmelcher 2 wrote: > I just noticed that this is actually in the wrong place. This section is for the > Cg libs section, but you don't want to be changing that. It's the cairo_libs > above that you should be changing. Glad that you noticed it. It would be terrible if it is checked in like this. Feeling very bad. Done. http://codereview.chromium.org/2825074/diff/10021/65004 File core/core.gyp (right): http://codereview.chromium.org/2825074/diff/10021/65004#newcode480 core/core.gyp:480: 'cross/cairo/install_check.cc', On 2010/08/04 19:36:17, Tristan Schmelcher 2 wrote: > Lists like this should generally be in alphabetical order, so install_check.cc > should come first. Done. http://codereview.chromium.org/2825074/diff/10021/65006 File core/cross/cairo/renderer_cairo.cc (right): http://codereview.chromium.org/2825074/diff/10021/65006#newcode134 core/cross/cairo/renderer_cairo.cc:134: static_cast<const DisplayWindowLinux&>(display_window); On 2010/08/04 19:36:17, Tristan Schmelcher 2 wrote: > Should be just 4 spaces of indent, not 6. Done. http://codereview.chromium.org/2825074/diff/10021/65007 File core/cross/cairo/renderer_cairo.h (right): http://codereview.chromium.org/2825074/diff/10021/65007#newcode38 core/cross/cairo/renderer_cairo.h:38: #include <cairo-xlib.h> On 2010/08/04 19:36:17, Tristan Schmelcher 2 wrote: > I think cairo-xlib.h (and only that one) can be moved to the .cc file. Done
http://codereview.chromium.org/2825074/diff/10021/65003 File build/libs.gyp (right): http://codereview.chromium.org/2825074/diff/10021/65003#newcode221 build/libs.gyp:221: #TODO(fransiskusx): Link to Cairo on Win/Mac as a static library On 2010/08/04 22:19:10, fransiskusx wrote: > On 2010/08/04 19:36:17, Tristan Schmelcher 2 wrote: > > I just noticed that this is actually in the wrong place. This section is for > the > > Cg libs section, but you don't want to be changing that. It's the cairo_libs > > above that you should be changing. > > Glad that you noticed it. It would be terrible if it is checked in like this. > Feeling very bad. > Done. Don't worry about it. We're going to run the trybots before you submit and they would have caught this anyway. http://codereview.chromium.org/2825074/diff/22018/72006 File core/cross/cairo/install_check.cc (right): http://codereview.chromium.org/2825074/diff/22018/72006#newcode41 core/cross/cairo/install_check.cc:41: } // o3d Should use the full "// namespace o3d" comment, rather than just "// o3d" http://codereview.chromium.org/2825074/diff/22018/72007 File core/cross/cairo/renderer_cairo.cc (right): http://codereview.chromium.org/2825074/diff/22018/72007#newcode384 core/cross/cairo/renderer_cairo.cc:384: } // o3d namespace This should actually be // namespace o3d (i.e., the other way around). http://codereview.chromium.org/2825074/diff/22018/72008 File core/cross/cairo/renderer_cairo.h (right): http://codereview.chromium.org/2825074/diff/22018/72008#newcode226 core/cross/cairo/renderer_cairo.h:226: } Should have blank line below. Also, you should include the "// namespace o3d" comment.
http://codereview.chromium.org/2825074/diff/22018/72006 File core/cross/cairo/install_check.cc (right): http://codereview.chromium.org/2825074/diff/22018/72006#newcode41 core/cross/cairo/install_check.cc:41: } // o3d On 2010/08/04 22:28:08, Tristan Schmelcher 2 wrote: > Should use the full "// namespace o3d" comment, rather than just "// o3d" Done. http://codereview.chromium.org/2825074/diff/22018/72007 File core/cross/cairo/renderer_cairo.cc (right): http://codereview.chromium.org/2825074/diff/22018/72007#newcode384 core/cross/cairo/renderer_cairo.cc:384: } // o3d namespace On 2010/08/04 22:28:08, Tristan Schmelcher 2 wrote: > This should actually be // namespace o3d (i.e., the other way around). Done. http://codereview.chromium.org/2825074/diff/22018/72008 File core/cross/cairo/renderer_cairo.h (right): http://codereview.chromium.org/2825074/diff/22018/72008#newcode226 core/cross/cairo/renderer_cairo.h:226: } On 2010/08/04 22:28:08, Tristan Schmelcher 2 wrote: > Should have blank line below. Also, you should include the "// namespace o3d" > comment. Done.
LGTM
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
