Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2162083005:
Use surface copy requests for layout tests. (Closed)
|
Created:
4 years, 5 months ago by jbroman Modified:
4 years, 4 months ago CC:
cc-bugs_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse surface copy requests for layout tests.
This replaces the CopyOutputRequest on the root layer in
RenderWidgetCompositor::compositeAndReadbackAsync with one on the cc::Surface.
This allows Blink to remain ignorant of copy requests, even in
Slimming Paint v2, when it builds its own property trees (where copy requests
are passed today).
BUG=630691
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel
Committed: https://crrev.com/6ccbc7d47b41c05863cce3c0762d21dd406316c7
Cr-Commit-Position: refs/heads/master@{#408043}
Patch Set 1 #Patch Set 2 : \ #Patch Set 3 : fix for popups, debugging logging #Patch Set 4 : merge with origin/master #Patch Set 5 : trim out old code that makes msvc sad #Patch Set 6 : things seem to work; check before cleanup #Patch Set 7 : remove logging #Patch Set 8 : remove DCHECK left over from debugging #
Total comments: 14
Patch Set 9 : danakj nits, inc. using raw ptr #
Total comments: 6
Patch Set 10 : danakj nits #
Total comments: 3
Patch Set 11 : piman comment #Messages
Total messages: 66 (52 generated)
Description was changed from ========== WIP: Use surface copy requests for layout tests. This removes the copy request on the layer tree, and uses a copy request provided to the SurfaceFactory used in layout tests instead. TODO(jbroman): Better description. BUG= ========== to ========== WIP: Use surface copy requests for layout tests. This removes the copy request on the layer tree, and uses a copy request provided to the SurfaceFactory used in layout tests instead. TODO(jbroman): Better description. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jbroman@chromium.org
Description was changed from ========== WIP: Use surface copy requests for layout tests. This removes the copy request on the layer tree, and uses a copy request provided to the SurfaceFactory used in layout tests instead. TODO(jbroman): Better description. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== WIP: Use surface copy requests for layout tests. This removes the copy request on the layer tree, and uses a copy request provided to the SurfaceFactory used in layout tests instead. TODO(jbroman): Better description. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel ==========
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP: Use surface copy requests for layout tests. This removes the copy request on the layer tree, and uses a copy request provided to the SurfaceFactory used in layout tests instead. TODO(jbroman): Better description. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel ========== to ========== WIP: Use surface copy requests for layout tests. This removes the copy request on the layer tree, and uses a copy request provided to the SurfaceFactory used in layout tests instead. TODO(jbroman): Better description. BUG=630691 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel ==========
Description was changed from ========== WIP: Use surface copy requests for layout tests. This removes the copy request on the layer tree, and uses a copy request provided to the SurfaceFactory used in layout tests instead. TODO(jbroman): Better description. BUG=630691 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel ========== to ========== Use surface copy requests for layout tests. This replaces the CopyOutputRequest on the root layer in RenderWidgetCompositor::compositeAndReadbackAsync with one on the cc::Surface. This allows Blink to remain ignorant of copy requests, even in Slimming Paint v2, when it builds its own property trees (where copy requests are passed today). BUG=630691 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel ==========
jbroman@chromium.org changed reviewers: + danakj@chromium.org
If this LGTY, I'll ask a content/ owner (probably piman) to review as well. The pattern so far doesn't seem to include unit tests for the layout test support code, but there is test coverage insofar as the layout tests need this to work, and they pass. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This approach looks great! A few questions, and I think I suggested the WeakPtr thing, but looking at it I wondered if it's actually needed in practice for layout tests, please LMK. cc/ changes LGTM https://codereview.chromium.org/2162083005/diff/140001/content/renderer/layou... File content/renderer/layout_test_dependencies.h (right): https://codereview.chromium.org/2162083005/diff/140001/content/renderer/layou... content/renderer/layout_test_dependencies.h:38: // Returns a SwapPromise which should be queued for the appropriate frame. appropriate = the next compositor frame? https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... File content/test/layouttest_support.cc (right): https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:188: using OutputSurfaceCallback = FindOutputSurfaceCallback? https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:195: // cc::SwapPromise "cc::SwapPromise implementation." https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:199: if (output_surface_) Is this ever going to be null in a layout test? They don't exactly lose their context.. well maybe some webgl ones do, but not the compositor context. If we can dcheck instead that'd be nice. Or make the bound function return a raw pointer instead of a weakptr. https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:205: copy_request_->SendEmptyResult(); Does this happen outside of test flakiness? https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:274: // last output surface to be requested was. This takes a raw pointer to "wait until OnCommit to find the currently active OutputSurface for the given RenderWidget (via the routing_id)." or something? https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:275: // LayoutTestDependenciesImpl, so it must not be destroyed before commit. "so the dependencies must not" ("it" is ambiguous) Maybe rather than saying "must not" you can explain why it will not. Like, the fact that it's tied to the lifetime of the RenderThreadImpl which is not destroyed before closing any RenderWidgets' compositors? Or so I believe..
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Seems fine locally, assuming the bots agree. PTAL and check that I've addressed your concerns adequately. https://codereview.chromium.org/2162083005/diff/140001/content/renderer/layou... File content/renderer/layout_test_dependencies.h (right): https://codereview.chromium.org/2162083005/diff/140001/content/renderer/layou... content/renderer/layout_test_dependencies.h:38: // Returns a SwapPromise which should be queued for the appropriate frame. On 2016/07/22 at 20:50:26, danakj wrote: > appropriate = the next compositor frame? Technically no (you could hold on to this and wait until the frame you want to take a screenshot), but in practice yes, so I'll change the comment as you suggested. https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... File content/test/layouttest_support.cc (right): https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:188: using OutputSurfaceCallback = On 2016/07/22 at 20:50:26, danakj wrote: > FindOutputSurfaceCallback? Done. https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:195: // cc::SwapPromise On 2016/07/22 at 20:50:27, danakj wrote: > "cc::SwapPromise implementation." Done. https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:199: if (output_surface_) On 2016/07/22 at 20:50:27, danakj wrote: > Is this ever going to be null in a layout test? They don't exactly lose their context.. well maybe some webgl ones do, but not the compositor context. If we can dcheck instead that'd be nice. Or make the bound function return a raw pointer instead of a weakptr. Fair enough. I don't think it can happen (and if it did, the layout test would get an empty pixel result which would fail anyhow), so OK. If there were more general users of WebLayerTreeView::compositeAndReadbackAsync it'd be a bigger deal to fail gracefully, but there aren't, so I'll simplify as you suggest. https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:205: copy_request_->SendEmptyResult(); On 2016/07/22 at 20:50:27, danakj wrote: > Does this happen outside of test flakiness? I don't think so, aside from the bug I've already fixed. In a general use it might be reasonable to send an empty result, but for layout tests it basically changes where the failure occurs, not whether one does. https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:274: // last output surface to be requested was. This takes a raw pointer to On 2016/07/22 at 20:50:26, danakj wrote: > "wait until OnCommit to find the currently active OutputSurface for the given RenderWidget (via the routing_id)." or something? Done. https://codereview.chromium.org/2162083005/diff/140001/content/test/layouttes... content/test/layouttest_support.cc:275: // LayoutTestDependenciesImpl, so it must not be destroyed before commit. On 2016/07/22 at 20:50:27, danakj wrote: > "so the dependencies must not" ("it" is ambiguous) > > Maybe rather than saying "must not" you can explain why it will not. Like, the fact that it's tied to the lifetime of the RenderThreadImpl which is not destroyed before closing any RenderWidgets' compositors? Or so I believe.. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2162083005/diff/160001/content/test/layouttes... File content/test/layouttest_support.cc (right): https://codereview.chromium.org/2162083005/diff/160001/content/test/layouttes... content/test/layouttest_support.cc:208: DCHECK(false) << "did not swap for reason " << r; nit: DCHECK(false) == NOTREACHED() (except on chromeos lol but whatever) https://codereview.chromium.org/2162083005/diff/160001/content/test/layouttes... content/test/layouttest_support.cc:215: cc::TestDelegatingOutputSurface* output_surface_ = nullptr; nit: output_surface_from_commit_ would document the lifetime/expectations of this var a bit https://codereview.chromium.org/2162083005/diff/160001/content/test/layouttes... content/test/layouttest_support.cc:283: [](const LayoutTestDependenciesImpl* self, int32_t routing_id) { nit: I think I marginally prefer this be a private method on the class instead of a lambda. You'll need to Unretained(this), and it'd be nice to move the comment about LTDepsImpl being value onto the use of that unretained.
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jbroman@chromium.org changed reviewers: + piman@chromium.org
+piman for content/ https://codereview.chromium.org/2162083005/diff/160001/content/test/layouttes... File content/test/layouttest_support.cc (right): https://codereview.chromium.org/2162083005/diff/160001/content/test/layouttes... content/test/layouttest_support.cc:208: DCHECK(false) << "did not swap for reason " << r; On 2016/07/25 at 21:16:28, danakj wrote: > nit: DCHECK(false) == NOTREACHED() (except on chromeos lol but whatever) Done. https://codereview.chromium.org/2162083005/diff/160001/content/test/layouttes... content/test/layouttest_support.cc:215: cc::TestDelegatingOutputSurface* output_surface_ = nullptr; On 2016/07/25 at 21:16:28, danakj wrote: > nit: output_surface_from_commit_ would document the lifetime/expectations of this var a bit OK. This class is small enough that it's hard to miss, but fair enough. https://codereview.chromium.org/2162083005/diff/160001/content/test/layouttes... content/test/layouttest_support.cc:283: [](const LayoutTestDependenciesImpl* self, int32_t routing_id) { On 2016/07/25 at 21:16:28, danakj wrote: > nit: I think I marginally prefer this be a private method on the class instead of a lambda. You'll need to Unretained(this), and it'd be nice to move the comment about LTDepsImpl being value onto the use of that unretained. As you command.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2162083005/diff/180001/content/test/layouttes... File content/test/layouttest_support.cc (right): https://codereview.chromium.org/2162083005/diff/180001/content/test/layouttes... content/test/layouttest_support.cc:268: output_surfaces_[routing_id] = output_surface.get(); Where is the entry removed? Also, what guarantees the entry outlives the output surface (otherwise we have a stale pointer)?
https://codereview.chromium.org/2162083005/diff/180001/content/test/layouttes... File content/test/layouttest_support.cc (right): https://codereview.chromium.org/2162083005/diff/180001/content/test/layouttes... content/test/layouttest_support.cc:268: output_surfaces_[routing_id] = output_surface.get(); On 2016/07/26 at 06:56:32, piman - slow reviews until 8-8 wrote: > Where is the entry removed? It isn't, though this object is only used in layout tests, so while it can grow unbounded over a single content_shell runner's lifetime, it cannot occur in production. > Also, what guarantees the entry outlives the output surface (otherwise we have a stale pointer)? The entry has the lifetime of the LayoutTestDependenciesImpl, which in turn has the lifetime of the RenderThreadImpl (which should outlive all renderer output surfaces, IIUC). I'd originally used WeakPtr here and intended to have the surfaces remove themselves from the map on destruction, but danakj suggested that was overkill for layout test support code. I'm happy either way.
lgtm https://codereview.chromium.org/2162083005/diff/180001/content/test/layouttes... File content/test/layouttest_support.cc (right): https://codereview.chromium.org/2162083005/diff/180001/content/test/layouttes... content/test/layouttest_support.cc:268: output_surfaces_[routing_id] = output_surface.get(); On 2016/07/26 13:28:42, jbroman wrote: > On 2016/07/26 at 06:56:32, piman - slow reviews until 8-8 wrote: > > Where is the entry removed? > > It isn't, though this object is only used in layout tests, so while it can grow > unbounded over a single content_shell runner's lifetime, it cannot occur in > production. > > > Also, what guarantees the entry outlives the output surface (otherwise we have > a stale pointer)? > > The entry has the lifetime of the LayoutTestDependenciesImpl, which in turn has > the lifetime of the RenderThreadImpl (which should outlive all renderer output > surfaces, IIUC). > > I'd originally used WeakPtr here and intended to have the surfaces remove > themselves from the map on destruction, but danakj suggested that was overkill > for layout test support code. I'm happy either way. Ok, can you leave a comment though? I wouldn't want something to start using an arbitrary entry, and then cause UAF and other hard to track issues.
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2162083005/#ps200001 (title: "piman comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Drive-by "thank you for cleaning up the temporary copy output request mess that I left" lgtm!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/75816)
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Use surface copy requests for layout tests. This replaces the CopyOutputRequest on the root layer in RenderWidgetCompositor::compositeAndReadbackAsync with one on the cc::Surface. This allows Blink to remain ignorant of copy requests, even in Slimming Paint v2, when it builds its own property trees (where copy requests are passed today). BUG=630691 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel ========== to ========== Use surface copy requests for layout tests. This replaces the CopyOutputRequest on the root layer in RenderWidgetCompositor::compositeAndReadbackAsync with one on the cc::Surface. This allows Blink to remain ignorant of copy requests, even in Slimming Paint v2, when it builds its own property trees (where copy requests are passed today). BUG=630691 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel Committed: https://crrev.com/6ccbc7d47b41c05863cce3c0762d21dd406316c7 Cr-Commit-Position: refs/heads/master@{#408043} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6ccbc7d47b41c05863cce3c0762d21dd406316c7 Cr-Commit-Position: refs/heads/master@{#408043} |
