Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10537036:
Added rendering benchmark javascript hook (Closed)
|
Created:
8 years, 6 months ago by dmurph Modified:
8 years, 4 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, scheib Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdded rendering benchmark javascript hook
Created json results store for returning results to browser
Includes name filter of rendering benchmarks
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149031
Patch Set 1 #
Total comments: 11
Patch Set 2 : Naming fixes #Patch Set 3 : microbenchmarks moved to content #Patch Set 4 : #Patch Set 5 : renamings + git try fixes #Patch Set 6 : renaming #
Total comments: 34
Patch Set 7 : cleanup, refactoring, git try'd #
Total comments: 7
Patch Set 8 : Patch #
Total comments: 19
Patch Set 9 : nit changes, scoped_vector #Patch Set 10 : Moved results array creation #
Total comments: 2
Patch Set 11 : nit change #Patch Set 12 : added null check for support #
Total comments: 10
Patch Set 13 : nit changes #Patch Set 14 : Refactorings for WebKit changes #
Messages
Total messages: 31 (0 generated)
Here's the javascript hook for the rendering microbenchmarks
webkit sister patch: https://bugs.webkit.org/show_bug.cgi?id=88271
http://codereview.chromium.org/10537036/diff/1/chrome/renderer/benchmarking_e... File chrome/renderer/benchmarking_extension.cc (right): http://codereview.chromium.org/10537036/diff/1/chrome/renderer/benchmarking_e... chrome/renderer/benchmarking_extension.cc:25: // Benchmark results object that populates a json array Comments end with periods. Also, note its not a json array, its a v8 array. :) http://codereview.chromium.org/10537036/diff/1/chrome/renderer/benchmarking_e... chrome/renderer/benchmarking_extension.cc:26: class JsonBenchmarkResults : public WebRenderingBenchmarkResults { V8BenchmarkResults http://codereview.chromium.org/10537036/diff/1/chrome/renderer/benchmarking_e... chrome/renderer/benchmarking_extension.cc:29: : curr_result_index_(0), results_array_(results_array) { } check goog c++ style guide, this feels too compact to me but i cant remember http://codereview.chromium.org/10537036/diff/1/chrome/renderer/benchmarking_e... chrome/renderer/benchmarking_extension.cc:41: unsigned int curr_result_index_; cur_result_index_. Not sure if this is the standard practice... might look around for other instances of v8::array to see if you can just do push? http://codereview.chromium.org/10537036/diff/1/chrome/renderer/benchmarking_e... chrome/renderer/benchmarking_extension.cc:214: JsonBenchmarkResults* results = new JsonBenchmarkResults(resultsArray); This looks good. But, where did we end up with Darin's alternative design where web_view has a method BenchmarkController* getBenchmarkController() where BenchmarkController { // methods for doing different benchmarks, e.g. layout/paintPageToCanvas } http://codereview.chromium.org/10537036/diff/1/skia/skia.gyp File skia/skia.gyp (right): http://codereview.chromium.org/10537036/diff/1/skia/skia.gyp#newcode551 skia/skia.gyp:551: '../third_party/skia/include/utils/SkNullCanvas.h', Can you make this a separate changelist? I'd like to get skia guys feedback on this part of the change. On the new cl, cc the author of that file... I think its brian.
https://chromiumcodereview.appspot.com/10537036/diff/1/chrome/renderer/benchm... File chrome/renderer/benchmarking_extension.cc (right): https://chromiumcodereview.appspot.com/10537036/diff/1/chrome/renderer/benchm... chrome/renderer/benchmarking_extension.cc:25: // Benchmark results object that populates a json array On 2012/06/12 17:42:09, nduca wrote: > Comments end with periods. Also, note its not a json array, its a v8 array. :) Done. https://chromiumcodereview.appspot.com/10537036/diff/1/chrome/renderer/benchm... chrome/renderer/benchmarking_extension.cc:26: class JsonBenchmarkResults : public WebRenderingBenchmarkResults { On 2012/06/12 17:42:09, nduca wrote: > V8BenchmarkResults Done. https://chromiumcodereview.appspot.com/10537036/diff/1/chrome/renderer/benchm... chrome/renderer/benchmarking_extension.cc:41: unsigned int curr_result_index_; On 2012/06/12 17:42:09, nduca wrote: > cur_result_index_. Not sure if this is the standard practice... might look > around for other instances of v8::array to see if you can just do push? Couldn't find a 'push', but instead of having that variable I just used the current array size https://chromiumcodereview.appspot.com/10537036/diff/1/chrome/renderer/benchm... chrome/renderer/benchmarking_extension.cc:214: JsonBenchmarkResults* results = new JsonBenchmarkResults(resultsArray); On 2012/06/12 17:42:09, nduca wrote: > This looks good. But, where did we end up with Darin's alternative design where > > web_view has a method BenchmarkController* getBenchmarkController() > where > BenchmarkController { > // methods for doing different benchmarks, e.g. layout/paintPageToCanvas > } I'm including it now, I just hadn't made it yet for this patch https://chromiumcodereview.appspot.com/10537036/diff/1/skia/skia.gyp File skia/skia.gyp (right): https://chromiumcodereview.appspot.com/10537036/diff/1/skia/skia.gyp#newcode551 skia/skia.gyp:551: '../third_party/skia/include/utils/SkNullCanvas.h', On 2012/06/12 17:42:09, nduca wrote: > Can you make this a separate changelist? I'd like to get skia guys feedback on > this part of the change. On the new cl, cc the author of that file... I think > its brian. I contacted him and he added the files, so SkNullCanvas exists now in the skia source.
ttps://chromiumcodereview.appspot.com/10537036/diff/1/skia/skia.gyp#newcode551 > skia/skia.gyp:551: '../third_party/skia/include/utils/SkNullCanvas.h', > On 2012/06/12 17:42:09, nduca wrote: > > Can you make this a separate changelist? I'd like to get skia guys feedback on > > this part of the change. On the new cl, cc the author of that file... I think > > its brian. > > I contacted him and he added the files, so SkNullCanvas exists now in the skia > source. But it is in utilities? Regardless, this gyp change should be part of a skia review.
This patch moves the microbenchmarks into content/public/renderer I also added a case sensitive name filter to the runRenderingMicrobenchmarks function. Ignore the skia change, that's already in the codebase and it's now redundant.
Move this to the GPu benchmarking extension
On 2012/06/25 18:37:03, nduca wrote: > Move this to the GPu benchmarking extension Moved, removed 'Viewport' benchmarks, all benchmarks now draw 'Everything' (in both software and accelerated). Planning on putting viewport clipped rendering w/accelerated in the next cl
Renamings and git try'd
http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... File content/public/renderer/all_rendering_benchmarks.cc (right): http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.cc:23: class WebCanvasFromFunctor Why not just have every benchmark implement this interface? E.g. CustomPaintBenchmark : public PaintingController? That seems MUCH simpler.
http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... File content/public/renderer/all_rendering_benchmarks.cc (right): http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.cc:2: // Use of this source code is governed by a BSD-style license that can be why is this in public/renderer? It should be in content/renderer http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.cc:117: for (int i = 0; i < benchmark_count_; i++) { Return a vector here with all the benchmarks in it. Have the gpu_benchmarks do the running and setup. http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... File content/public/renderer/all_rendering_benchmarks.h (right): http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.h:10: #include "base/basictypes.h" why? http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.h:11: #include "base/compiler_specific.h" why? http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.h:12: #include "content/common/content_export.h" why? http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.h:16: Include only when you need it. You can predeclare almost everything here. E.g you dont even need rendering_benhchmark results or header. just say class RenderingBenchmark and you'll be all set. http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.h:18: class CONTENT_EXPORT AllRenderingBenchmarks : public RenderingBenchmark { Why oh why another subclass? Why not just return a vector of constructed benchmarks that are in this file? That'd be much much simpler. scoped_vector<RendererBenchmark> getAllRenderingBenchmarks()? http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.h:28: RenderingBenchmark** benchmarks_; Eep! scoped_vector is your friend. But as I pointed out, this whole class should go away. http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.h:29: const int benchmark_count_; You're too obsessive about const. Please, save us the time. Stop using it unless someone tells you to do so in codereview, or on simple accessors. http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... File content/public/renderer/rendering_benchmark.h (right): http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... content/public/renderer/rendering_benchmark.h:6: #define CONTENT_PUBLIC_RENDERER_RENDERING_BENCHMARK_H_ Why is all of this public? Its only used by content, can't it all be in the content/renderer/ folder? http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... content/public/renderer/rendering_benchmark.h:13: #include "third_party/WebKit/Source/WebKit/chromium/public/WebViewBenchmarkSupport.h" Dont include until you use. namespace WebKit { class WebViewBenchmarkSupport; } the rule is to #include only when you actually use a class, e.g. derive from it, use/pass it by value, or access members / deref it. http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... content/public/renderer/rendering_benchmark.h:16: class CONTENT_EXPORT RenderingBenchmark { Dont need CONTENT_EXPORT http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... content/public/renderer/rendering_benchmark.h:23: WebKit::WebViewBenchmarkSupport* benchmarkController) {} naming http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... content/public/renderer/rendering_benchmark.h:33: void RecordResult(const std::string& result_name, Why does this have a member variable that is the results? Why not pass it in on the Run()? http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... File content/public/renderer/rendering_benchmark_results.h (right): http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... content/public/renderer/rendering_benchmark_results.h:13: class CONTENT_EXPORT RenderingBenchmarkResults { I'm not sure this abstraction is needed any more. We can have a benchmark have a public inner struct called Result that has a unit and a value. Run can return that struct. The name of the benchmark and the result name are redundant. Either have benchmarks have a name, or have the result have a name, but not both.
http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... File content/public/renderer/all_rendering_benchmarks.cc (right): http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.cc:2: // Use of this source code is governed by a BSD-style license that can be On 2012/07/18 06:06:17, nduca wrote: > why is this in public/renderer? It should be in content/renderer This stuff had to be public so the old benchmarking extension could access it from chrome. I forgot to move this stuff out of public after the javascript extension location was changed, I'll move all this stuff back. http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.cc:23: class WebCanvasFromFunctor On 2012/07/17 18:54:56, nduca wrote: > Why not just have every benchmark implement this interface? > > E.g. > > CustomPaintBenchmark : public PaintingController? That seems MUCH simpler. Yes, well, it'll have multiple inheritance, but that should be fine, as the PaintClient is basically pure virtual. http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.cc:117: for (int i = 0; i < benchmark_count_; i++) { On 2012/07/18 06:06:17, nduca wrote: > Return a vector here with all the benchmarks in it. Have the gpu_benchmarks do > the running and setup. Ok, just fyi, you had previously requested to have it set up this way :P http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... File content/public/renderer/all_rendering_benchmarks.h (right): http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.h:11: #include "base/compiler_specific.h" On 2012/07/18 06:06:17, nduca wrote: > why? for OVERRIDE http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.h:16: On 2012/07/18 06:06:17, nduca wrote: > Include only when you need it. > > You can predeclare almost everything here. > > E.g you dont even need rendering_benhchmark results or header. just say > > class RenderingBenchmark and you'll be all set. Yeah, I forgot to clean these up, sorry http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/al... content/public/renderer/all_rendering_benchmarks.h:28: RenderingBenchmark** benchmarks_; On 2012/07/18 06:06:17, nduca wrote: > Eep! scoped_vector is your friend. But as I pointed out, this whole class should > go away. Yeah I know http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... File content/public/renderer/rendering_benchmark.h (right): http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... content/public/renderer/rendering_benchmark.h:6: #define CONTENT_PUBLIC_RENDERER_RENDERING_BENCHMARK_H_ On 2012/07/18 06:06:17, nduca wrote: > Why is all of this public? Its only used by content, can't it all be in the > content/renderer/ folder? Addessed in other comment http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... content/public/renderer/rendering_benchmark.h:33: void RecordResult(const std::string& result_name, On 2012/07/18 06:06:17, nduca wrote: > Why does this have a member variable that is the results? Why not pass it in on > the Run()? We talked about this earlier, it's to remove the annoying need to get the benchmark name for recording each result (or messing up the name). You can just call the RecordResults method with all the values from the current test. Plus you get nice encapsulation of the results object. http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... File content/public/renderer/rendering_benchmark_results.h (right): http://codereview.chromium.org/10537036/diff/28002/content/public/renderer/re... content/public/renderer/rendering_benchmark_results.h:13: class CONTENT_EXPORT RenderingBenchmarkResults { On 2012/07/18 06:06:17, nduca wrote: > I'm not sure this abstraction is needed any more. We can have a benchmark have a > public inner struct called Result that has a unit and a value. Run can return > that struct. The name of the benchmark and the result name are redundant. Either > have benchmarks have a name, or have the result have a name, but not both. I agree, as long as you're fine with only one result per benchmark. Think about it - what if you want paint time and canvas count? Wouldn't it be easier to record both results in the same benchmark? Although, we would have to change the naming from 'time_unit/time' to 'value_unit/value', but that would still work.
https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... File content/public/renderer/all_rendering_benchmarks.h (right): https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... content/public/renderer/all_rendering_benchmarks.h:10: #include "base/basictypes.h" On 2012/07/18 06:06:17, nduca wrote: > why? DISALLOW_COPY_AND_ASSIGN, which I'm adding https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... content/public/renderer/all_rendering_benchmarks.h:12: #include "content/common/content_export.h" On 2012/07/18 06:06:17, nduca wrote: > why? Done. https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... content/public/renderer/all_rendering_benchmarks.h:18: class CONTENT_EXPORT AllRenderingBenchmarks : public RenderingBenchmark { On 2012/07/18 06:06:17, nduca wrote: > Why oh why another subclass? Why not just return a vector of constructed > benchmarks that are in this file? That'd be much much simpler. > > scoped_vector<RendererBenchmark> getAllRenderingBenchmarks()? Is it cool if we do getter-style of 'benchmarks()'? The 'all' part is a little redundant, given the class name. https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... content/public/renderer/all_rendering_benchmarks.h:29: const int benchmark_count_; On 2012/07/18 06:06:17, nduca wrote: > You're too obsessive about const. Please, save us the time. Stop using it unless > someone tells you to do so in codereview, or on simple accessors. Done. https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... File content/public/renderer/rendering_benchmark.h (right): https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... content/public/renderer/rendering_benchmark.h:13: #include "third_party/WebKit/Source/WebKit/chromium/public/WebViewBenchmarkSupport.h" On 2012/07/18 06:06:17, nduca wrote: > Dont include until you use. > namespace WebKit { > class WebViewBenchmarkSupport; > } > > the rule is to #include only when you actually use a class, e.g. derive from it, > use/pass it by value, or access members / deref it. Done. https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... content/public/renderer/rendering_benchmark.h:16: class CONTENT_EXPORT RenderingBenchmark { On 2012/07/18 06:06:17, nduca wrote: > Dont need CONTENT_EXPORT Done. https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... content/public/renderer/rendering_benchmark.h:23: WebKit::WebViewBenchmarkSupport* benchmarkController) {} On 2012/07/18 06:06:17, nduca wrote: > naming Done. https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... content/public/renderer/rendering_benchmark.h:33: void RecordResult(const std::string& result_name, On 2012/07/18 16:31:41, dmurph wrote: > On 2012/07/18 06:06:17, nduca wrote: > > Why does this have a member variable that is the results? Why not pass it in > on > > the Run()? > > We talked about this earlier, it's to remove the annoying need to get the > benchmark name for recording each result (or messing up the name). You can just > call the RecordResults method with all the values from the current test. Plus > you get nice encapsulation of the results object. Removing :) https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... File content/public/renderer/rendering_benchmark_results.h (right): https://chromiumcodereview.appspot.com/10537036/diff/28002/content/public/ren... content/public/renderer/rendering_benchmark_results.h:13: class CONTENT_EXPORT RenderingBenchmarkResults { On 2012/07/18 16:31:41, dmurph wrote: > On 2012/07/18 06:06:17, nduca wrote: > > I'm not sure this abstraction is needed any more. We can have a benchmark have > a > > public inner struct called Result that has a unit and a value. Run can return > > that struct. The name of the benchmark and the result name are redundant. > Either > > have benchmarks have a name, or have the result have a name, but not both. > > I agree, as long as you're fine with only one result per benchmark. Think about > it - what if you want paint time and canvas count? Wouldn't it be easier to > record both results in the same benchmark? Although, we would have to change > the naming from 'time_unit/time' to 'value_unit/value', but that would still > work. Agreed on leaving abstraction for allowing multiple results per test
Almost there! I like this. http://codereview.chromium.org/10537036/diff/32002/content/renderer/all_rende... File content/renderer/all_rendering_benchmarks.cc (right): http://codereview.chromium.org/10537036/diff/32002/content/renderer/all_rende... content/renderer/all_rendering_benchmarks.cc:98: benchmarks_.push_back(new CustomPaintBenchmark( Not sure we need this case.... or rather, there's a bit more subtlety in this than you probably realize [software is always opaque, root layer is ... i think opaque, but maybe not..., non-root is transparent until filed]. So, best to just assume transparent always now? ~confused~ http://codereview.chromium.org/10537036/diff/32002/content/renderer/all_rende... content/renderer/all_rendering_benchmarks.cc:99: "PaintEverythingToTransparentBitmap", Pending whether we need this transparent vs opaque case, i'd like to remove that callback-based stuff from the tests. Doesn't seem needed. Also, were this to persist, its cleaner to use subclassing here than method pointers. E.g. leave the create undefined in the base class, and have two subclasses that provide alternate implementations. http://codereview.chromium.org/10537036/diff/32002/content/renderer/all_rende... File content/renderer/all_rendering_benchmarks.h (right): http://codereview.chromium.org/10537036/diff/32002/content/renderer/all_rende... content/renderer/all_rendering_benchmarks.h:22: class AllRenderingBenchmarks { why a class and not a function? What state does this need to retain? http://codereview.chromium.org/10537036/diff/32002/content/renderer/all_rende... content/renderer/all_rendering_benchmarks.h:22: class AllRenderingBenchmarks { two spaces between class and AllRenderingXXX
https://chromiumcodereview.appspot.com/10537036/diff/32002/content/renderer/a... File content/renderer/all_rendering_benchmarks.cc (right): https://chromiumcodereview.appspot.com/10537036/diff/32002/content/renderer/a... content/renderer/all_rendering_benchmarks.cc:98: benchmarks_.push_back(new CustomPaintBenchmark( On 2012/07/19 08:39:08, nduca wrote: > Not sure we need this case.... or rather, there's a bit more subtlety in this > than you probably realize [software is always opaque, root layer is ... i think > opaque, but maybe not..., non-root is transparent until filed]. So, best to just > assume transparent always now? ~confused~ I'll just do the one transparent then. https://chromiumcodereview.appspot.com/10537036/diff/32002/content/renderer/a... content/renderer/all_rendering_benchmarks.cc:99: "PaintEverythingToTransparentBitmap", On 2012/07/19 08:39:08, nduca wrote: > Pending whether we need this transparent vs opaque case, i'd like to remove that > callback-based stuff from the tests. Doesn't seem needed. > > Also, were this to persist, its cleaner to use subclassing here than method > pointers. E.g. leave the create undefined in the base class, and have two > subclasses that provide alternate implementations. Sounds good, done. https://chromiumcodereview.appspot.com/10537036/diff/32002/content/renderer/a... File content/renderer/all_rendering_benchmarks.h (right): https://chromiumcodereview.appspot.com/10537036/diff/32002/content/renderer/a... content/renderer/all_rendering_benchmarks.h:22: class AllRenderingBenchmarks { On 2012/07/19 08:39:08, nduca wrote: > why a class and not a function? What state does this need to retain? Turned into function.
LGTM but nits. @piman for OWNERS and advice. https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/a... File content/renderer/all_rendering_benchmarks.cc (right): https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/a... content/renderer/all_rendering_benchmarks.cc:53: TimeTicks beforeTime; beforeTime_ here and elsewhere, members get _ https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:37: explicit V8BenchmarkResults(v8::Handle<v8::Array> results_array) why not have the array inside here and have an accessor instead? E.g. V8BenchmarkResults() { results_array_ = new ...; } and then an accessor results_array() { return results_array_; } https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:98: return v8::FunctionTemplate::New(RunRenderingBenchmarks); i think there's a blank line after the ifs https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:151: // for our name filter, the argument can be undefined or null to run // For https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:167: V8BenchmarkResults results(resultsArray); initialize the results array as late as you can. E.g. support benchmarks results for loop https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:168: std::string name_filter; how about moving the name_filter stuff up to where you're looking at the args anyway?
https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/a... File content/renderer/all_rendering_benchmarks.h (right): https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/a... content/renderer/all_rendering_benchmarks.h:13: std::vector<RenderingBenchmark*> AllRenderingBenchmarks(); Should this be a ScopedVector? I don't see where the RenderingBenchmark instances are deleted. https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:155: !(args[0]->IsNull() || args[0]->IsUndefined()))) nit: indent (! should be aligned with the one above). https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:186: std::string::npos == name.find(name_filter)) { nit: indent (should be +4)
https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/a... File content/renderer/all_rendering_benchmarks.cc (right): https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/a... content/renderer/all_rendering_benchmarks.cc:53: TimeTicks beforeTime; On 2012/07/20 20:30:09, nduca wrote: > beforeTime_ > > here and elsewhere, members get _ Done. https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:37: explicit V8BenchmarkResults(v8::Handle<v8::Array> results_array) On 2012/07/20 20:30:09, nduca wrote: > why not have the array inside here and have an accessor instead? > > E.g. V8BenchmarkResults() { results_array_ = new ...; } > > and then an accessor results_array() { return results_array_; } The results object doesn't own the array though, the array will persist after the results object is destructed. https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:98: return v8::FunctionTemplate::New(RunRenderingBenchmarks); On 2012/07/20 20:30:09, nduca wrote: > i think there's a blank line after the ifs Done. https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:151: // for our name filter, the argument can be undefined or null to run On 2012/07/20 20:30:09, nduca wrote: > // For Done. https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:167: V8BenchmarkResults results(resultsArray); On 2012/07/20 20:30:09, nduca wrote: > initialize the results array as late as you can. E.g. > > support > benchmarks > results > for loop Done. https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:168: std::string name_filter; On 2012/07/20 20:30:09, nduca wrote: > how about moving the name_filter stuff up to where you're looking at the args > anyway? Done.
https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:37: explicit V8BenchmarkResults(v8::Handle<v8::Array> results_array) On 2012/07/20 20:52:33, dmurph wrote: > On 2012/07/20 20:30:09, nduca wrote: > > why not have the array inside here and have an accessor instead? > > > > E.g. V8BenchmarkResults() { results_array_ = new ...; } > > > > and then an accessor results_array() { return results_array_; } > > The results object doesn't own the array though, the array will persist after > the results object is destructed. After discussion we realized v8 handles destruction with handles, so we can just create the array in the results object and grab it from a setter w/o having to worry about it destructing.
whoops, forgot to mark as done https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/a... File content/renderer/all_rendering_benchmarks.h (right): https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/a... content/renderer/all_rendering_benchmarks.h:13: std::vector<RenderingBenchmark*> AllRenderingBenchmarks(); On 2012/07/20 20:35:35, piman wrote: > Should this be a ScopedVector? I don't see where the RenderingBenchmark > instances are deleted. Done. https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:155: !(args[0]->IsNull() || args[0]->IsUndefined()))) On 2012/07/20 20:35:35, piman wrote: > nit: indent (! should be aligned with the one above). Done. https://chromiumcodereview.appspot.com/10537036/diff/37004/content/renderer/g... content/renderer/gpu/gpu_benchmarking_extension.cc:186: std::string::npos == name.find(name_filter)) { On 2012/07/20 20:35:35, piman wrote: > nit: indent (should be +4) Done.
lgtm https://chromiumcodereview.appspot.com/10537036/diff/41002/content/renderer/a... File content/renderer/all_rendering_benchmarks.h (right): https://chromiumcodereview.appspot.com/10537036/diff/41002/content/renderer/a... content/renderer/all_rendering_benchmarks.h:8: #include <vector> nit: you don't need this anymore.
https://chromiumcodereview.appspot.com/10537036/diff/41002/content/renderer/a... File content/renderer/all_rendering_benchmarks.h (right): https://chromiumcodereview.appspot.com/10537036/diff/41002/content/renderer/a... content/renderer/all_rendering_benchmarks.h:8: #include <vector> On 2012/07/20 21:23:10, piman wrote: > nit: you don't need this anymore. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmurph@chromium.org/10537036/40013
Presubmit check for 10537036-40013 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
If this change requires manual test instructions to QA team, add
TEST=[instructions].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content
Presubmit checks took 1.2s to calculate.
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
Hey John, we need you for an owners lgtm for content/
lgtm with nits http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... File content/renderer/rendering_benchmark.h (right): http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... content/renderer/rendering_benchmark.h:39: #endif // CONTENT_RENDERER_RENDERING_BENCHMARK_H_ nit: blank line above http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... File content/renderer/rendering_benchmark_results.cc (right): http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... content/renderer/rendering_benchmark_results.cc:8: RenderingBenchmarkResults::~RenderingBenchmarkResults() { } nit: you can inline the destructor and get rid of this cc file completely http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... File content/renderer/rendering_benchmark_results.h (right): http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... content/renderer/rendering_benchmark_results.h:13: class RenderingBenchmarkResults { nit: blank line above http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... content/renderer/rendering_benchmark_results.h:17: virtual void addResult(const std::string& benchmark_name, nit: google style is AddResult http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... content/renderer/rendering_benchmark_results.h:23: #endif // CONTENT_RENDERER_RENDERING_BENCHMARK_RESULTS_H_ nit: blank line above
Waiting for changes to the paint client interface before submitting (https://bugs.webkit.org/show_bug.cgi?id=92008) http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... File content/renderer/rendering_benchmark.h (right): http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... content/renderer/rendering_benchmark.h:39: #endif // CONTENT_RENDERER_RENDERING_BENCHMARK_H_ On 2012/07/23 20:45:58, John Abd-El-Malek wrote: > nit: blank line above Done. http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... File content/renderer/rendering_benchmark_results.cc (right): http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... content/renderer/rendering_benchmark_results.cc:8: RenderingBenchmarkResults::~RenderingBenchmarkResults() { } On 2012/07/23 20:45:58, John Abd-El-Malek wrote: > nit: you can inline the destructor and get rid of this cc file completely Done. http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... File content/renderer/rendering_benchmark_results.h (right): http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... content/renderer/rendering_benchmark_results.h:13: class RenderingBenchmarkResults { On 2012/07/23 20:45:58, John Abd-El-Malek wrote: > nit: blank line above Done. http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... content/renderer/rendering_benchmark_results.h:17: virtual void addResult(const std::string& benchmark_name, On 2012/07/23 20:45:58, John Abd-El-Malek wrote: > nit: google style is AddResult Done. http://codereview.chromium.org/10537036/diff/40013/content/renderer/rendering... content/renderer/rendering_benchmark_results.h:23: #endif // CONTENT_RENDERER_RENDERING_BENCHMARK_RESULTS_H_ On 2012/07/23 20:45:58, John Abd-El-Malek wrote: > nit: blank line above Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmurph@chromium.org/10537036/49001
Change committed as 149031 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
