Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1349913002:
Cache gpu suitability in DisplayItemList, remove SetUnsuitable...ForTesting (Closed)
DescriptionCache gpu suitability in DisplayItemList, remove SetUnsuitable...ForTesting
This patch moves the gpu suitability "cache" into display_item_list
so that calls to DisplayItemList::IsSuitableForGpuRasterization are
fast. With the cache moved we no longer need to track suitability in
the recording source and can remove SetUnsuitableForGpuRasterizationForTesting.
BUG=524314
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/5d8f98c33491997b3ffa1c8494fe6014c4b8e4c9
Cr-Commit-Position: refs/heads/master@{#349999}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Address reviewer comments #Patch Set 3 : Cleanup #
Total comments: 7
Patch Set 4 : More reviewer comments #Patch Set 5 : Cleanup tests to not check default suitability #Patch Set 6 : Needs more nullptr #
Total comments: 2
Patch Set 7 : Update LayerTreeHostTestGpuRasterizationDefault #
Total comments: 6
Messages
Total messages: 47 (12 generated)
pdr@chromium.org changed reviewers: + danakj@chromium.org, hendrikw@chromium.org
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349913002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349913002/1
Thanks! A few nitty nits and one question, https://codereview.chromium.org/1349913002/diff/1/cc/layers/picture_layer_uni... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/1349913002/diff/1/cc/layers/picture_layer_uni... cc/layers/picture_layer_unittest.cc:93: FakeDisplayListRecordingSource* recording_source = nit: can you grab this pointer before you Pass it to the layer, then you don't need to cast it. typically i'd do like: scoped_ptr<T> foo_t_owned(new T); T* foo_t = foo_t_owned.get(); Blah(foo_t_owned.Pass()); https://codereview.chromium.org/1349913002/diff/1/cc/playback/display_list_re... File cc/playback/display_list_recording_source.cc (right): https://codereview.chromium.org/1349913002/diff/1/cc/playback/display_list_re... cc/playback/display_list_recording_source.cc:223: return is_suitable_for_gpu_rasterization_; do we need this bool anymore, or should we just ask the display_list_ here? https://codereview.chromium.org/1349913002/diff/1/cc/test/fake_display_list_r... File cc/test/fake_display_list_recording_source.h (right): https://codereview.chromium.org/1349913002/diff/1/cc/test/fake_display_list_r... cc/test/fake_display_list_recording_source.h:98: void SetUnsuitableForGpuRasterization(); nit: i'd make this an inline setter, with style set_the_variable_name() https://codereview.chromium.org/1349913002/diff/1/cc/trees/layer_tree_host_un... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1349913002/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:4509: FakeDisplayListRecordingSource* recording_source = ditto, store the fake source* on the class and do it before Passing to avoid the cast?
Thanks! A few nitty nits and one question,
https://codereview.chromium.org/1349913002/diff/1/cc/layers/picture_layer_uni... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/1349913002/diff/1/cc/layers/picture_layer_uni... cc/layers/picture_layer_unittest.cc:103: EXPECT_FALSE(recording_source->IsSuitableForGpuRasterization()); This part of the test is a bit weird for me, we set force_unsuitable_for_gpu_rasterization_ to true (which only exists in the fake version), then we call IsSuitableForGpuRasterization, which the fake version checks force_unsuitable_for_gpu_rasterization_ and returns false. What I mean is, this test is testing fake code. The old code tested real code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1349913002/diff/1/cc/layers/picture_layer_uni... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/1349913002/diff/1/cc/layers/picture_layer_uni... cc/layers/picture_layer_unittest.cc:103: EXPECT_FALSE(recording_source->IsSuitableForGpuRasterization()); On 2015/09/16 23:15:57, hendrikw wrote: > This part of the test is a bit weird for me, we set > force_unsuitable_for_gpu_rasterization_ to true (which only exists in the fake > version), then we call IsSuitableForGpuRasterization, which the fake version > checks force_unsuitable_for_gpu_rasterization_ and returns false. > > What I mean is, this test is testing fake code. > > The old code tested real code. Ya this EXPECT could go away. This test is basically checking that Layer asks the recording source. I'm not sure how valuable that is, but it doesn't hurt.
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349913002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349913002/40001
PTAL https://codereview.chromium.org/1349913002/diff/1/cc/layers/picture_layer_uni... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/1349913002/diff/1/cc/layers/picture_layer_uni... cc/layers/picture_layer_unittest.cc:93: FakeDisplayListRecordingSource* recording_source = On 2015/09/16 at 23:04:49, danakj wrote: > nit: can you grab this pointer before you Pass it to the layer, then you don't need to cast it. typically i'd do like: > > scoped_ptr<T> foo_t_owned(new T); > T* foo_t = foo_t_owned.get(); > Blah(foo_t_owned.Pass()); Good idea. Done. https://codereview.chromium.org/1349913002/diff/1/cc/playback/display_list_re... File cc/playback/display_list_recording_source.cc (right): https://codereview.chromium.org/1349913002/diff/1/cc/playback/display_list_re... cc/playback/display_list_recording_source.cc:223: return is_suitable_for_gpu_rasterization_; On 2015/09/16 at 23:04:49, danakj wrote: > do we need this bool anymore, or should we just ask the display_list_ here? We don't need it, but we need to cache the value somewhere. I've updated the patch to cache this in display_list_. https://codereview.chromium.org/1349913002/diff/1/cc/test/fake_display_list_r... File cc/test/fake_display_list_recording_source.h (right): https://codereview.chromium.org/1349913002/diff/1/cc/test/fake_display_list_r... cc/test/fake_display_list_recording_source.h:98: void SetUnsuitableForGpuRasterization(); On 2015/09/16 at 23:04:49, danakj wrote: > nit: i'd make this an inline setter, with style set_the_variable_name() Done. https://codereview.chromium.org/1349913002/diff/1/cc/trees/layer_tree_host_un... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1349913002/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:4509: FakeDisplayListRecordingSource* recording_source = On 2015/09/16 at 23:04:49, danakj wrote: > ditto, store the fake source* on the class and do it before Passing to avoid the cast? Hmm, this one doesn't seem worth it in terms of complexity.
https://codereview.chromium.org/1349913002/diff/1/cc/trees/layer_tree_host_un... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1349913002/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:4509: FakeDisplayListRecordingSource* recording_source = On 2015/09/17 20:52:45, pdr wrote: > On 2015/09/16 at 23:04:49, danakj wrote: > > ditto, store the fake source* on the class and do it before Passing to avoid > the cast? > > Hmm, this one doesn't seem worth it in terms of complexity. OK. I just don't like casts if we can avoid it. https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... File cc/playback/display_item_list.cc (right): https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... cc/playback/display_item_list.cc:155: is_suitable_for_gpu_rasterization_ &= why do we do this here too? don't we want to compute all of this at recording time? we use it to decide about how to raster https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... cc/playback/display_item_list.cc:189: picture_->suitableForGpuRasterization(NULL); nullptr https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... File cc/playback/display_list_recording_source.cc (right): https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... cc/playback/display_list_recording_source.cc:220: return !display_list_ || display_list_->IsSuitableForGpuRasterization(); when/how can display_list_ be null?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349913002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349913002/60001
https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... File cc/playback/display_item_list.cc (right): https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... cc/playback/display_item_list.cc:155: is_suitable_for_gpu_rasterization_ &= On 2015/09/17 at 21:11:32, danakj wrote: > why do we do this here too? don't we want to compute all of this at recording time? we use it to decide about how to raster This was a mistake. I had refactored this so ProcessAppendedItems called RasterIntoCanvas but un-factored it and forgot this line. Good catch. https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... cc/playback/display_item_list.cc:189: picture_->suitableForGpuRasterization(NULL); On 2015/09/17 at 21:11:32, danakj wrote: > nullptr We should make a pass through and remove all the NULLs so I can copy&paste with reckless abandon. https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... File cc/playback/display_list_recording_source.cc (right): https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... cc/playback/display_list_recording_source.cc:220: return !display_list_ || display_list_->IsSuitableForGpuRasterization(); On 2015/09/17 at 21:11:32, danakj wrote: > when/how can display_list_ be null? display_list_ can be null if this is called before the display list has been created (through UpdateAndExpandInvalidation). This was hidden previously since is_suitable_for_gpu_rasterization_ would just return false in this case.
https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... File cc/playback/display_list_recording_source.cc (right): https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... cc/playback/display_list_recording_source.cc:220: return !display_list_ || display_list_->IsSuitableForGpuRasterization(); On 2015/09/18 22:40:14, pdr wrote: > On 2015/09/17 at 21:11:32, danakj wrote: > > when/how can display_list_ be null? > > display_list_ can be null if this is called before the display list has been > created (through UpdateAndExpandInvalidation). This was hidden previously since > is_suitable_for_gpu_rasterization_ would just return false in this case. I guess I'm wondering, we ask if it's suitable for raster at raster time. If there's no display list we didn't record yet. Why are we trying to raster a RecordingSource we did not record?
(or did you see this as a crash? do you have a callstack?) On Fri, Sep 18, 2015 at 3:42 PM, <danakj@chromium.org> wrote: > > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... > File cc/playback/display_list_recording_source.cc (right): > > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... > cc/playback/display_list_recording_source.cc:220: return !display_list_ > || display_list_->IsSuitableForGpuRasterization(); > On 2015/09/18 22:40:14, pdr wrote: > >> On 2015/09/17 at 21:11:32, danakj wrote: >> > when/how can display_list_ be null? >> > > display_list_ can be null if this is called before the display list >> > has been > >> created (through UpdateAndExpandInvalidation). This was hidden >> > previously since > >> is_suitable_for_gpu_rasterization_ would just return false in this >> > case. > > I guess I'm wondering, we ask if it's suitable for raster at raster > time. If there's no display list we didn't record yet. Why are we trying > to raster a RecordingSource we did not record? > > https://codereview.chromium.org/1349913002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/09/18 at 22:40:14, pdr wrote: > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... > File cc/playback/display_item_list.cc (right): > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... > cc/playback/display_item_list.cc:155: is_suitable_for_gpu_rasterization_ &= > On 2015/09/17 at 21:11:32, danakj wrote: > > why do we do this here too? don't we want to compute all of this at recording time? we use it to decide about how to raster > > This was a mistake. I had refactored this so ProcessAppendedItems called RasterIntoCanvas but un-factored it and forgot this line. Good catch. > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... > cc/playback/display_item_list.cc:189: picture_->suitableForGpuRasterization(NULL); > On 2015/09/17 at 21:11:32, danakj wrote: > > nullptr > > We should make a pass through and remove all the NULLs so I can copy&paste with reckless abandon. > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... > File cc/playback/display_list_recording_source.cc (right): > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... > cc/playback/display_list_recording_source.cc:220: return !display_list_ || display_list_->IsSuitableForGpuRasterization(); > On 2015/09/17 at 21:11:32, danakj wrote: > > when/how can display_list_ be null? > > display_list_ can be null if this is called before the display list has been created (through UpdateAndExpandInvalidation). This was hidden previously since is_suitable_for_gpu_rasterization_ would just return false in this case. They appear to only be used in tests to verify the default state. For example, this case: // Verify default values. EXPECT_TRUE(root->IsSuitableForGpuRasterization()); EXPECT_TRUE(layer->IsSuitableForGpuRasterization()); EXPECT_TRUE(recording_source->IsSuitableForGpuRasterization()); I think it would be okay to add a dcheck for the display list in IsSuitableForGpuRasterization with a comment, and then remove the checks for default values. Does that sound reasonable to you?
On Fri, Sep 18, 2015 at 4:52 PM, <pdr@chromium.org> wrote: > On 2015/09/18 at 22:40:14, pdr wrote: > > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... > >> File cc/playback/display_item_list.cc (right): >> > > > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... > >> cc/playback/display_item_list.cc:155: is_suitable_for_gpu_rasterization_ >> &= >> On 2015/09/17 at 21:11:32, danakj wrote: >> > why do we do this here too? don't we want to compute all of this at >> > recording time? we use it to decide about how to raster > > This was a mistake. I had refactored this so ProcessAppendedItems called >> > RasterIntoCanvas but un-factored it and forgot this line. Good catch. > > > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_ite... > >> cc/playback/display_item_list.cc:189: >> > picture_->suitableForGpuRasterization(NULL); > >> On 2015/09/17 at 21:11:32, danakj wrote: >> > nullptr >> > > We should make a pass through and remove all the NULLs so I can copy&paste >> > with reckless abandon. > > > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... > >> File cc/playback/display_list_recording_source.cc (right): >> > > > > https://codereview.chromium.org/1349913002/diff/40001/cc/playback/display_lis... > >> cc/playback/display_list_recording_source.cc:220: return !display_list_ || >> > display_list_->IsSuitableForGpuRasterization(); > >> On 2015/09/17 at 21:11:32, danakj wrote: >> > when/how can display_list_ be null? >> > > display_list_ can be null if this is called before the display list has >> been >> > created (through UpdateAndExpandInvalidation). This was hidden previously > since > is_suitable_for_gpu_rasterization_ would just return false in this case. > > They appear to only be used in tests to verify the default state. For > example, > this case: > // Verify default values. > EXPECT_TRUE(root->IsSuitableForGpuRasterization()); > EXPECT_TRUE(layer->IsSuitableForGpuRasterization()); > EXPECT_TRUE(recording_source->IsSuitableForGpuRasterization()); > > I think it would be okay to add a dcheck for the display list in > IsSuitableForGpuRasterization with a comment, and then remove the checks > for > default values. Does that sound reasonable to you? > Yes please. We could instead just have a test that goes through and gets TRUE at the end, and another case that goes through and gets FALSE at the end. That'd make sure we're not always true or sillyness. > > https://codereview.chromium.org/1349913002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> >> On 2015/09/17 at 21:11:32, danakj wrote: > >> > when/how can display_list_ be null? > >> > > > > display_list_ can be null if this is called before the display list has > >> been > >> > > created (through UpdateAndExpandInvalidation). This was hidden previously > > since > > is_suitable_for_gpu_rasterization_ would just return false in this case. > > > > They appear to only be used in tests to verify the default state. For > > example, > > this case: > > // Verify default values. > > EXPECT_TRUE(root->IsSuitableForGpuRasterization()); > > EXPECT_TRUE(layer->IsSuitableForGpuRasterization()); > > EXPECT_TRUE(recording_source->IsSuitableForGpuRasterization()); > > > > I think it would be okay to add a dcheck for the display list in > > IsSuitableForGpuRasterization with a comment, and then remove the checks > > for > > default values. Does that sound reasonable to you? > > > > Yes please. We could instead just have a test that goes through and gets > TRUE at the end, and another case that goes through and gets FALSE at the > end. That'd make sure we're not always true or sillyness. > Done! PTAL In the latest patchset I updated PictureLayerTest::SuitableForGpuRasterization to check for the default suitability, and verify that it can be set. I also updated the layer_tree_host_unittest.cc tests to confirm that the bit works and is sticky. To do this I needed to save off the recording source and picture layer, so I removed the static_casts as well.
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349913002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349913002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Cool! These changes LGTM, but I don't think we have any test that checks IsSuitableForGPURaster() can be true. Can you make a variant of the non-forced test that doesn't set false on the source, and after commit/activation checks that GPU raster is enabled instead of disabled? https://codereview.chromium.org/1349913002/diff/100001/cc/playback/display_li... File cc/playback/display_list_recording_source.cc (right): https://codereview.chromium.org/1349913002/diff/100001/cc/playback/display_li... cc/playback/display_list_recording_source.cc:222: DCHECK(display_list_.get()); nit: no .get() is needed
https://codereview.chromium.org/1349913002/diff/100001/cc/playback/display_li... File cc/playback/display_list_recording_source.cc (right): https://codereview.chromium.org/1349913002/diff/100001/cc/playback/display_li... cc/playback/display_list_recording_source.cc:222: DCHECK(display_list_.get()); On 2015/09/21 at 17:47:34, danakj wrote: > nit: no .get() is needed Fixed, and updated where I copied this from so future pdrs can copy pasta.
On 2015/09/21 at 17:47:34, danakj wrote: > Cool! These changes LGTM, but I don't think we have any test that checks IsSuitableForGPURaster() can be true. Can you make a variant of the non-forced test that doesn't set false on the source, and after commit/activation checks that GPU raster is enabled instead of disabled? Done. I updated the LayerTreeHostTestGpuRasterizationDefault test to check this. PTAL?
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349913002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349913002/120001
Awesome possum. Thanks LGTM
https://codereview.chromium.org/1349913002/diff/120001/cc/layers/picture_laye... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/1349913002/diff/120001/cc/layers/picture_laye... cc/layers/picture_layer_unittest.cc:112: EXPECT_FALSE(recording_source->IsSuitableForGpuRasterization()); You're still testing 100% test code here, but I suppose that's fine, just a bit misleading, it gives the illusion that something real is being tested. https://codereview.chromium.org/1349913002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1349913002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:4469: EXPECT_FALSE(recording_source_->IsSuitableForGpuRasterization()); Same thing here https://codereview.chromium.org/1349913002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:4477: EXPECT_FALSE(recording_source_->IsSuitableForGpuRasterization()); and here. https://codereview.chromium.org/1349913002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:4539: EXPECT_FALSE(recording_source_->IsSuitableForGpuRasterization()); and here https://codereview.chromium.org/1349913002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:4547: EXPECT_FALSE(recording_source_->IsSuitableForGpuRasterization()); and here
https://codereview.chromium.org/1349913002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1349913002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:4469: EXPECT_FALSE(recording_source_->IsSuitableForGpuRasterization()); On 2015/09/21 18:36:46, hendrikw wrote: > Same thing here I think it's ok, it's testing the test which is a practice that I like. If we changed the test harness somehow and this changed, but the layer still reported false, that'd be something we want to know about. It also kinda documents what we're thinking about here. The source is false, and that means the layer is false.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hendrikw, are you okay with danakj's response?
On 2015/09/21 18:43:28, danakj wrote: > https://codereview.chromium.org/1349913002/diff/120001/cc/trees/layer_tree_ho... > File cc/trees/layer_tree_host_unittest.cc (right): > > https://codereview.chromium.org/1349913002/diff/120001/cc/trees/layer_tree_ho... > cc/trees/layer_tree_host_unittest.cc:4469: > EXPECT_FALSE(recording_source_->IsSuitableForGpuRasterization()); > On 2015/09/21 18:36:46, hendrikw wrote: > > Same thing here > > I think it's ok, it's testing the test which is a practice that I like. If we > changed the test harness somehow and this changed, but the layer still reported > false, that'd be something we want to know about. It also kinda documents what > we're thinking about here. The source is false, and that means the layer is > false. To document that source and layer match, EXPECT_EQUAL instead. The code is fakebool = false; EXPECT_FALSE(fakebool), yet looks like it's testing the internals. Misleading, but fine. LGTM.
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349913002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349913002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5d8f98c33491997b3ffa1c8494fe6014c4b8e4c9 Cr-Commit-Position: refs/heads/master@{#349999}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1361663003/ by pdr@chromium.org. The reason for reverting is: Crashes :'( http://crbug.com/534810. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
