Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1185443005:
cc: Propagate read lock fences constraints to parent compositors. (Closed)
|
Created:
5 years, 6 months ago by Daniele Castagna Modified:
5 years, 6 months ago CC:
cc-bugs_chromium.org, chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Propagate read lock fences constraints to parent compositors.
The root compositor might receive resources that require to be
synchronized on the CPU: they should not be returned to the child
compositor until the GPU is completely done drawing with them.
BUG=386735
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/6158dbf13d7371730b1504ce720a8e51ada59265
Cr-Commit-Position: refs/heads/master@{#334912}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Trying to make MSVC happy. #Patch Set 3 : Add DestroyChild and ContextLost tests. #Patch Set 4 : Fix tests. Returned resourced could be swapped. #
Total comments: 12
Patch Set 5 : Avoid ReadLockFenceHasPassed when not necessary. #
Total comments: 2
Messages
Total messages: 34 (9 generated)
dcastagna@chromium.org changed reviewers: + reveman@chromium.org
reveman@chromium.org changed reviewers: + piman@chromium.org
lgtm % piman's review
LGTM https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... cc/resources/resource_provider.h:111: void EnableReadLockFences(ResourceId id); nit: EnableReadLockFencesForTesting. https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... cc/resources/resource_provider_unittest.cc:1111: } Could you add another test for the lost resource case? If you DestroyChild in the parent before the fence has passed, all resources should be returned as lost.
https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... cc/resources/resource_provider.h:111: void EnableReadLockFences(ResourceId id); On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > nit: EnableReadLockFencesForTesting. We're actually planning to use this method for video resources that are created in media. https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... cc/resources/resource_provider_unittest.cc:1111: } On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > Could you add another test for the lost resource case? If you DestroyChild in the parent before the fence has passed, all resources should be returned as lost. If DestroyChild is called after a context lost Fence::HasPassed is always going to return true: https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... The test wouldn't be testing a lot since it'd need to set TestFence::passed to true to simulate that. Do you think it's still worth adding it?
On Fri, Jun 12, 2015 at 4:36 PM, <dcastagna@chromium.org> wrote: > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > File cc/resources/resource_provider.h (right): > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > cc/resources/resource_provider.h:111: void > EnableReadLockFences(ResourceId id); > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > >> nit: EnableReadLockFencesForTesting. >> > > We're actually planning to use this method for video resources that are > created in media. > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > File cc/resources/resource_provider_unittest.cc (right): > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > cc/resources/resource_provider_unittest.cc:1111: } > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > >> Could you add another test for the lost resource case? If you >> > DestroyChild in the parent before the fence has passed, all resources > should be returned as lost. > > If DestroyChild is called after a context lost Fence::HasPassed is > always going to return true: > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > > The test wouldn't be testing a lot since it'd need to set > TestFence::passed to true to simulate that. > > Do you think it's still worth adding it? > What I'm concerned about is not the lost context case, but the case where a child is removed before the fence has passed. > > https://codereview.chromium.org/1185443005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Jun 12, 2015 at 4:36 PM, <dcastagna@chromium.org> wrote: > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > File cc/resources/resource_provider.h (right): > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > cc/resources/resource_provider.h:111: void > EnableReadLockFences(ResourceId id); > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > >> nit: EnableReadLockFencesForTesting. >> > > We're actually planning to use this method for video resources that are > created in media. Mmh, do you have a pointer to the patch? I don't think "EnableReadLockFences" is a concept that should leak out of ResourceProvider - ideally the video resouce code would go through ScopedWriteLockGpuMemoryBuffer, which sets the flag. > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > File cc/resources/resource_provider_unittest.cc (right): > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > cc/resources/resource_provider_unittest.cc:1111: } > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > >> Could you add another test for the lost resource case? If you >> > DestroyChild in the parent before the fence has passed, all resources > should be returned as lost. > > If DestroyChild is called after a context lost Fence::HasPassed is > always going to return true: > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > > The test wouldn't be testing a lot since it'd need to set > TestFence::passed to true to simulate that. > > Do you think it's still worth adding it? > > https://codereview.chromium.org/1185443005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/06/13 at 00:16:56, piman wrote: > On Fri, Jun 12, 2015 at 4:36 PM, <dcastagna@chromium.org> wrote: > > > > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > File cc/resources/resource_provider.h (right): > > > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > cc/resources/resource_provider.h:111: void > > EnableReadLockFences(ResourceId id); > > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > > > >> nit: EnableReadLockFencesForTesting. > >> > > > > We're actually planning to use this method for video resources that are > > created in media. > > > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > File cc/resources/resource_provider_unittest.cc (right): > > > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > cc/resources/resource_provider_unittest.cc:1111: } > > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > > > >> Could you add another test for the lost resource case? If you > >> > > DestroyChild in the parent before the fence has passed, all resources > > should be returned as lost. > > > > If DestroyChild is called after a context lost Fence::HasPassed is > > always going to return true: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > > > > > > The test wouldn't be testing a lot since it'd need to set > > TestFence::passed to true to simulate that. > > > > Do you think it's still worth adding it? > > > > What I'm concerned about is not the lost context case, but the case where a > child is removed before the fence has passed. > Got it, and at the moment the resource wouldn't be returned. Let me look into that. > > > > > https://codereview.chromium.org/1185443005/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. >
On 2015/06/13 at 00:20:59, piman wrote: > On Fri, Jun 12, 2015 at 4:36 PM, <dcastagna@chromium.org> wrote: > > > > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > File cc/resources/resource_provider.h (right): > > > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > cc/resources/resource_provider.h:111: void > > EnableReadLockFences(ResourceId id); > > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > > > >> nit: EnableReadLockFencesForTesting. > >> > > > > We're actually planning to use this method for video resources that are > > created in media. > > > Mmh, do you have a pointer to the patch? I don't think > "EnableReadLockFences" is a concept that should leak out of > ResourceProvider - ideally the video resouce code would go > through ScopedWriteLockGpuMemoryBuffer, which sets the flag. > I don't have a patch yet. Unfortunately the resources for the video that will need the fences are created in media where we don't have access to the RP there. We then send them via mailboxes to the VideoLayerImpl where we create the resources from the mailboxes. At that point we should be able to set read_lock_fences_enabled flag on the resource. We could add a parameter to CreateResourceFromTextureMailbox (in another patch) and we can have EnableReadLockFencesForTesting in this patch. WDYT? > > > > > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > File cc/resources/resource_provider_unittest.cc (right): > > > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > cc/resources/resource_provider_unittest.cc:1111: } > > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > > > >> Could you add another test for the lost resource case? If you > >> > > DestroyChild in the parent before the fence has passed, all resources > > should be returned as lost. > > > > If DestroyChild is called after a context lost Fence::HasPassed is > > always going to return true: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > > > > The test wouldn't be testing a lot since it'd need to set > > TestFence::passed to true to simulate that. > > > > Do you think it's still worth adding it? > > > > https://codereview.chromium.org/1185443005/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. >
On Fri, Jun 12, 2015 at 6:02 PM, <dcastagna@chromium.org> wrote: > On 2015/06/13 at 00:20:59, piman wrote: > >> On Fri, Jun 12, 2015 at 4:36 PM, <dcastagna@chromium.org> wrote: >> > > > >> > >> > >> > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > >> > File cc/resources/resource_provider.h (right): >> > >> > >> > >> > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > >> > cc/resources/resource_provider.h:111: void >> > EnableReadLockFences(ResourceId id); >> > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: >> > >> >> nit: EnableReadLockFencesForTesting. >> >> >> > >> > We're actually planning to use this method for video resources that are >> > created in media. >> > > > Mmh, do you have a pointer to the patch? I don't think >> "EnableReadLockFences" is a concept that should leak out of >> ResourceProvider - ideally the video resouce code would go >> through ScopedWriteLockGpuMemoryBuffer, which sets the flag. >> > > > I don't have a patch yet. > Unfortunately the resources for the video that will need the fences are > created > in media where we don't have access to the RP there. > We then send them via mailboxes to the VideoLayerImpl where we create the > resources from the mailboxes. At that point we should be able to set > read_lock_fences_enabled flag on the resource. > We could add a parameter to CreateResourceFromTextureMailbox (in another > patch) > and we can have EnableReadLockFencesForTesting in this patch. WDYT? Yeah, I'd prefer we did that. I'm not quite sure what it means to "send them [the fences] via mailboxes". Let's revisit when that is figured out a bit better. > > > > > >> > >> > >> > >> > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > >> > File cc/resources/resource_provider_unittest.cc (right): >> > >> > >> > >> > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > >> > cc/resources/resource_provider_unittest.cc:1111: } >> > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: >> > >> >> Could you add another test for the lost resource case? If you >> >> >> > DestroyChild in the parent before the fence has passed, all resources >> > should be returned as lost. >> > >> > If DestroyChild is called after a context lost Fence::HasPassed is >> > always going to return true: >> > >> > >> > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > >> > >> > The test wouldn't be testing a lot since it'd need to set >> > TestFence::passed to true to simulate that. >> > >> > Do you think it's still worth adding it? >> > >> > https://codereview.chromium.org/1185443005/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email to chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/1185443005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Jun 12, 2015 at 5:49 PM, <dcastagna@chromium.org> wrote: > On 2015/06/13 at 00:16:56, piman wrote: > >> On Fri, Jun 12, 2015 at 4:36 PM, <dcastagna@chromium.org> wrote: >> > > > >> > >> > >> > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > >> > File cc/resources/resource_provider.h (right): >> > >> > >> > >> > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > >> > cc/resources/resource_provider.h:111: void >> > EnableReadLockFences(ResourceId id); >> > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: >> > >> >> nit: EnableReadLockFencesForTesting. >> >> >> > >> > We're actually planning to use this method for video resources that are >> > created in media. >> > >> > >> > >> > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > >> > File cc/resources/resource_provider_unittest.cc (right): >> > >> > >> > >> > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > >> > cc/resources/resource_provider_unittest.cc:1111: } >> > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: >> > >> >> Could you add another test for the lost resource case? If you >> >> >> > DestroyChild in the parent before the fence has passed, all resources >> > should be returned as lost. >> > >> > If DestroyChild is called after a context lost Fence::HasPassed is >> > always going to return true: >> > >> > >> > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > >> > >> > > > > The test wouldn't be testing a lot since it'd need to set >> > TestFence::passed to true to simulate that. >> > >> > Do you think it's still worth adding it? >> > >> > > What I'm concerned about is not the lost context case, but the case where >> a >> child is removed before the fence has passed. >> > > > Got it, and at the moment the resource wouldn't be returned. Let me look > into > that. > I believe with the current patch it would be returned, as lost - this is what the code https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... does. > > > > >> > https://codereview.chromium.org/1185443005/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email to chromium-reviews+unsubscribe@chromium.org. > > > > > https://codereview.chromium.org/1185443005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
On 2015/06/15 at 17:26:16, piman wrote: > On Fri, Jun 12, 2015 at 5:49 PM, <dcastagna@chromium.org> wrote: > > > On 2015/06/13 at 00:16:56, piman wrote: > > > >> On Fri, Jun 12, 2015 at 4:36 PM, <dcastagna@chromium.org> wrote: > >> > > > > > > >> > > >> > > >> > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > > >> > File cc/resources/resource_provider.h (right): > >> > > >> > > >> > > >> > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > > >> > cc/resources/resource_provider.h:111: void > >> > EnableReadLockFences(ResourceId id); > >> > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > >> > > >> >> nit: EnableReadLockFencesForTesting. > >> >> > >> > > >> > We're actually planning to use this method for video resources that are > >> > created in media. > >> > > >> > > >> > > >> > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > > >> > File cc/resources/resource_provider_unittest.cc (right): > >> > > >> > > >> > > >> > > > > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > > > >> > cc/resources/resource_provider_unittest.cc:1111: } > >> > On 2015/06/12 at 01:53:52, piman (Very slow to review) wrote: > >> > > >> >> Could you add another test for the lost resource case? If you > >> >> > >> > DestroyChild in the parent before the fence has passed, all resources > >> > should be returned as lost. > >> > > >> > If DestroyChild is called after a context lost Fence::HasPassed is > >> > always going to return true: > >> > > >> > > >> > > > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > > > >> > > >> > > > > > > > The test wouldn't be testing a lot since it'd need to set > >> > TestFence::passed to true to simulate that. > >> > > >> > Do you think it's still worth adding it? > >> > > >> > > > > What I'm concerned about is not the lost context case, but the case where > >> a > >> child is removed before the fence has passed. > >> > > > > > > Got it, and at the moment the resource wouldn't be returned. Let me look > > into > > that. > > > > I believe with the current patch it would be returned, as lost - this is > what the code > https://codereview.chromium.org/1185443005/diff/1/cc/resources/resource_provi... > does. > > > > > > > > > >> > https://codereview.chromium.org/1185443005/ > >> > > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email to chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > https://codereview.chromium.org/1185443005/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. > Antoine, can you take a look? I had to change the logic in DeleteAndReturnUnusedResourcesToChild so that if a child is being destroyed and the fence has not passed yet we return the resource after marking it as lost.
https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider.cc:1552: bool fence_passed = ReadLockFenceHasPassed(&resource); Checking the fence is an atomic op at best. Should we avoid this when possible? https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider.cc:1561: // We can't postpone the deletion , so we'll have to lose it. I think this comment would be more useful if it explained why. nit: " , " -> ", " https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider.cc:1562: is_lost = true; Is this necessary when "resource.exported_count > 0 || resource.lock_for_read_count > 0" and style != FOR_SHUTDOWN?
https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... File cc/resources/resource_provider.cc (left): https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider.cc:1554: if (resource.exported_count > 0 || resource.lock_for_read_count > 0) { I'm really sorry if our out-of-band discussion was misleading - I'm happy as long as the fence logic and the exported logic are consistent - which you had in PS1. All I wanted was a test that exercises the logic with unpassed fences!
https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... File cc/resources/resource_provider.cc (left): https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider.cc:1554: if (resource.exported_count > 0 || resource.lock_for_read_count > 0) { On 2015/06/16 at 02:02:47, piman (Very slow to review) wrote: > I'm really sorry if our out-of-band discussion was misleading - I'm happy as long as the fence logic and the exported logic are consistent - which you had in PS1. All I wanted was a test that exercises the logic with unpassed fences! I'm not sure the fence logic can be consistent with the exported logic. When a resource is still exported we can delay the deletion since we know that the resource will eventually be returned from the parent, and we'll delete it at that time. Same thing for locked resources, we know that they will be unlocked and we can release it when that happens. For resources that haven't passed the fence while we're destroying the child, this is our last chance to return them. There is no method called referring to that resource after the fence has passed. Marking them as lost should make sure that when the child RP receives them back they're not going to reuse them, since they might be still in use for drawing. Does this make sense?
Ok, I buy that, LGTM+nits (after reveman@'s comments are addressed). https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider_unittest.cc:1084: EXPECT_TRUE(child_resource_provider_->InUseByConsumer(id1)); nit: you could add an EXPECT_TRUE(list[0].read_lock_fences_enabled) https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider_unittest.cc:1167: EXPECT_TRUE(returned_to_child[swapped ? 1 : 0].lost); nit: Maybe something along the lines of: EXPECT_EQ(returned_to_child[0].lost, returned_to_child[0].id == id1); EXPECT_EQ(returned_to_child[1].lost, returned_to_child[1].id == id1); To convey than id1 is the one that should be lost and the other one should not?
The CQ bit was checked by reveman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1185443005/#ps120001 (title: "Avoid ReadLockFenceHasPassed when not necessary.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1185443005/120001
https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider.cc:1552: bool fence_passed = ReadLockFenceHasPassed(&resource); On 2015/06/16 at 01:35:10, reveman wrote: > Checking the fence is an atomic op at best. Should we avoid this when possible? Done. https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider.cc:1561: // We can't postpone the deletion , so we'll have to lose it. On 2015/06/16 at 01:35:10, reveman wrote: > I think this comment would be more useful if it explained why. > > nit: " , " -> ", " Done. https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider.cc:1562: is_lost = true; On 2015/06/16 at 01:35:10, reveman wrote: > Is this necessary when "resource.exported_count > 0 || resource.lock_for_read_count > 0" and style != FOR_SHUTDOWN? Changed this as discussed. https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider_unittest.cc:1084: EXPECT_TRUE(child_resource_provider_->InUseByConsumer(id1)); On 2015/06/16 at 16:46:20, piman (Very slow to review) wrote: > nit: you could add an EXPECT_TRUE(list[0].read_lock_fences_enabled) Done. https://codereview.chromium.org/1185443005/diff/100001/cc/resources/resource_... cc/resources/resource_provider_unittest.cc:1167: EXPECT_TRUE(returned_to_child[swapped ? 1 : 0].lost); On 2015/06/16 at 16:46:20, piman (Very slow to review) wrote: > nit: Maybe something along the lines of: > EXPECT_EQ(returned_to_child[0].lost, returned_to_child[0].id == id1); > EXPECT_EQ(returned_to_child[1].lost, returned_to_child[1].id == id1); > > To convey than id1 is the one that should be lost and the other one should not? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
+palmer for content/common/cc_messages.h
dcastagna@chromium.org changed reviewers: + palmer@chromium.org
+palmer for content/common/cc_messages.h for real this time.
IPC security LGTM % nit. https://codereview.chromium.org/1185443005/diff/120001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1185443005/diff/120001/cc/resources/resource_... cc/resources/resource_provider.cc:1564: // TODO(dcastagna): see if it's possible to use this logic for Is there a bug ID for this TODO?
https://codereview.chromium.org/1185443005/diff/120001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1185443005/diff/120001/cc/resources/resource_... cc/resources/resource_provider.cc:1564: // TODO(dcastagna): see if it's possible to use this logic for On 2015/06/17 at 18:08:51, palmer wrote: > Is there a bug ID for this TODO? There isn't one, but there is a already tentative cl (crrev.com/1188843003) and we're trying to address this before the end of the week.
The CQ bit was checked by dcastagna@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1185443005/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6158dbf13d7371730b1504ce720a8e51ada59265 Cr-Commit-Position: refs/heads/master@{#334912} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
