Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
807193005:
cc: cc_perftest was crashing due to lack of GrContext (Closed)
Descriptioncc: cc_perftest was crashing due to lack of GrContext
TileTaskWorkerPoolPerfTests/TileTaskWorkerPoolPerfTest.ScheduleTasks/3 Failed
because GrContext was returning nullptr
BUG=
Committed: https://crrev.com/5d9960e73600c3a8e8486753b7c1fac330d3b002
Cr-Commit-Position: refs/heads/master@{#309330}
Patch Set 1 #
Total comments: 5
Patch Set 2 : review comments #Messages
Total messages: 13 (3 generated)
hendrikw@chromium.org changed reviewers: + vmpstr@chromium.org
PTAL
https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... File cc/resources/tile_task_worker_pool_perftest.cc (right): https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... cc/resources/tile_task_worker_pool_perftest.cc:81: class GrContext* GrContext() override { class GrContext? I bet you don't need 'class' here. https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... cc/resources/tile_task_worker_pool_perftest.cc:85: auto null_interface = skia::AdoptRef(GrGLCreateNullInterface()); s/auto/skia::RefPtr<GrContext> ? https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... cc/resources/tile_task_worker_pool_perftest.cc:103: skia::RefPtr<class GrContext> gr_context_; you don't need 'class' here, right?
lgtm when tfarina is happy with it. https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... File cc/resources/tile_task_worker_pool_perftest.cc (right): https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... cc/resources/tile_task_worker_pool_perftest.cc:85: auto null_interface = skia::AdoptRef(GrGLCreateNullInterface()); On 2014/12/20 00:59:25, tfarina wrote: > s/auto/skia::RefPtr<GrContext> ? FWIW, It's as common knowledge that skia::AdoptRef will return a skia::RefPtr, as it is common knowledge that skia::RefPtr is a smart pointer. On top of it the only use of "null_interface" here is to reinterpret_cast it to something other than GrContext. All in all "skia::RefPtr<GrContext>" adds very little information over "auto" here. But I think the complaining ship has sailed for me already :)
On 2014/12/20 00:59:25, tfarina wrote: > https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... > File cc/resources/tile_task_worker_pool_perftest.cc (right): > > https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... > cc/resources/tile_task_worker_pool_perftest.cc:81: class GrContext* GrContext() > override { > class GrContext? I bet you don't need 'class' here. The function name is GrContext, so the class GrContext is required. > > https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... > cc/resources/tile_task_worker_pool_perftest.cc:85: auto null_interface = > skia::AdoptRef(GrGLCreateNullInterface()); > s/auto/skia::RefPtr<GrContext> ? I followed all the threads on this today. I really think that the full type adds little value here, but I made the change anyway, because I don't care one way or the other. > > https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... > cc/resources/tile_task_worker_pool_perftest.cc:103: skia::RefPtr<class > GrContext> gr_context_; > you don't need 'class' here, right? Again, same reason.
tfarina@chromium.org changed reviewers: + tfarina@chromium.org
lgtm
The CQ bit was checked by hendrikw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/807193005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5d9960e73600c3a8e8486753b7c1fac330d3b002 Cr-Commit-Position: refs/heads/master@{#309330}
Message was sent while issue was closed.
https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... File cc/resources/tile_task_worker_pool_perftest.cc (right): https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... cc/resources/tile_task_worker_pool_perftest.cc:85: auto null_interface = skia::AdoptRef(GrGLCreateNullInterface()); On 2014/12/20 01:17:18, vmpstr wrote: > On 2014/12/20 00:59:25, tfarina wrote: > > s/auto/skia::RefPtr<GrContext> ? > > FWIW, It's as common knowledge that skia::AdoptRef will return a skia::RefPtr, > as it is common knowledge that skia::RefPtr is a smart pointer. On top of it the > only use of "null_interface" here is to reinterpret_cast it to something other > than GrContext. All in all "skia::RefPtr<GrContext>" adds very little > information over "auto" here. > > But I think the complaining ship has sailed for me already :) I think "don't use auto for smart pointers" is a simple rule and avoids having to worry if a case is okay and why. So I'd like if we can stick with that.
Message was sent while issue was closed.
On 2014/12/20 06:45:00, danakj wrote: > https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... > File cc/resources/tile_task_worker_pool_perftest.cc (right): > > https://codereview.chromium.org/807193005/diff/1/cc/resources/tile_task_worke... > cc/resources/tile_task_worker_pool_perftest.cc:85: auto null_interface = > skia::AdoptRef(GrGLCreateNullInterface()); > On 2014/12/20 01:17:18, vmpstr wrote: > > On 2014/12/20 00:59:25, tfarina wrote: > > > s/auto/skia::RefPtr<GrContext> ? > > > > FWIW, It's as common knowledge that skia::AdoptRef will return a skia::RefPtr, > > as it is common knowledge that skia::RefPtr is a smart pointer. On top of it > the > > only use of "null_interface" here is to reinterpret_cast it to something other > > than GrContext. All in all "skia::RefPtr<GrContext>" adds very little > > information over "auto" here. > > > > But I think the complaining ship has sailed for me already :) > > I think "don't use auto for smart pointers" is a simple rule and avoids having > to worry if a case is okay and why. So I'd like if we can stick with that. sgtm |
